Revise Key, KeyCode enums by dhardy · Pull Request #3143 · rust-windowing/winit

dhardy · 2023-10-14T18:32:12Z

Tested on all platforms changed:
- Wayland
- Web
- X11
- Windows
- MacOS
- Android
- Orbital
Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior

Implement desired changes from #2995:

Split enum KeyCode into PhysicalKey, KeyCode
Split enum Key into Key, Action

This commit is not supposed to be merged, but reviewed by someone familiar with the Windows API.

src/keyboard.rs

kchibisov · 2023-10-15T19:54:56Z

src/keyboard.rs

+impl PartialEq<NativeKey> for NativeKeyCode {
+    #[allow(clippy::cmp_owned)] // uses less code than direct match; target is stack allocated
+    #[inline]
+    fn eq(&self, rhs: &NativeKey) -> bool {
+        NativeKey::from(*self) == *rhs
+    }
+}
+
+impl PartialEq<NativeKeyCode> for NativeKey {
+    #[inline]
+    fn eq(&self, rhs: &NativeKeyCode) -> bool {
+        rhs == self
+    }
+}


do we really need impls like that? Can't regular derive just work?

No we can't derive: these are cross-type comparisons. (We don't need them either, but I consider them slightly useful and not harmful.)

I mean, do we really need them? Given that we can from and then compare. Cross type compare is a rare in rust.

I've found this type of comparison useful before and added one or two token usages in the examples... but I think the only common usage in Rust is comparing things like String to &str or String to Cow<str> (both Deref to the same type but Deref alone won't make the comparison work).

At any rate, I don't think these comparisons are dangerous (mis-usable). But I can remove if you don't want them?

I'd prefer to remove them just for forward compat and because we have into.

I mean that in the future we could end in a state where we can't really compare them anymore.

I mean do we care about that future? We can always remove them if it becomes incomparable, we aren't at v1 yet.

We could implement as if it matters.

Do you mean Deref? I think that also would be a nice addition, then comparisons could be made without this implementation as well, if it all works out that is.

Not Deref, but as_physical, but it doesn't make any sense, since the type is Copy.

#3143 (comment) just saw that Deref was already discussed/mentioned here, sorry.

Yeah, that wouldn't really make sense to me either. I don't really see the downside of adding these PartialEq implementations, so I'm still in favor.

I don't think Deref can be implemented for any of these.

I mean, we could have them as is, but my main point is that it's not common for rust code to have things like that. The string case is more of exception than the rule. We could keep as is.

kchibisov · 2023-10-15T19:55:57Z

src/keyboard.rs

+impl PartialEq<KeyCode> for PhysicalKey {
+    #[inline]
+    fn eq(&self, rhs: &KeyCode) -> bool {
+        match self {
+            PhysicalKey::Code(ref code) => code == rhs,
+            _ => false,
+        }
+    }
+}
+
+impl PartialEq<PhysicalKey> for KeyCode {
+    #[inline]
+    fn eq(&self, rhs: &PhysicalKey) -> bool {
+        rhs == self
+    }
+}
+
+impl PartialEq<NativeKeyCode> for PhysicalKey {
+    #[inline]
+    fn eq(&self, rhs: &NativeKeyCode) -> bool {
+        match self {
+            PhysicalKey::Unidentified(ref code) => code == rhs,
+            _ => false,
+        }
+    }
+}
+
+impl PartialEq<PhysicalKey> for NativeKeyCode {
+    #[inline]
+    fn eq(&self, rhs: &PhysicalKey) -> bool {
+        rhs == self
+    }
+}


Ditto about these PartialEq, I think we can just derive?

src/platform_impl/windows/keyboard_layout.rs

tests/serde_objects.rs

dhardy · 2023-10-16T08:38:30Z

src/platform_impl/windows/keyboard_layout.rs

            let keycode = match PhysicalKey::from_scancode(scancode) {
                PhysicalKey::Code(code) => code,
+                // TODO: validate that we can skip on unidentified keys (probably never occurs?)
                _ => continue,
            };


@notgull could you check this please? (Also 52 lines below.)

dhardy · 2023-10-17T13:50:57Z

There is a certain lack of symmetry in the new naming:

pub struct KeyEvent {
    pub physical_key: PhysicalKey,
    pub logical_key: Key,
    // ...
}

pub enum PhysicalKey {
    Code(KeyCode),
    Unidentified(NativeKeyCode),
}

pub enum KeyCode {
    // 194 variants
}

pub enum Key<Str = SmolStr> {
    Named(NamedKey),
    Character(Str),
    Unidentified(NativeKey),
    Dead(Option<char>),
}

pub enum NamedKey {
    // 306 variants
}

kchibisov · 2023-10-17T14:52:05Z

There is a certain lack of symmetry in the new naming:

That's expected though. Since you have more named keys than keycodes. It was always been like that iirc.

kchibisov

Looks fine other than present TODO comments. I'll update CHANGELOG myself though when I'll be doing a release.

# Objective - Update winit dependency to 0.29 ## Changelog ### KeyCode changes - Removed `ScanCode`, as it was [replaced by KeyCode](https://github.com/rust-windowing/winit/blob/master/CHANGELOG.md#0292). - `ReceivedCharacter.char` is now a `SmolStr`, [relevant doc](https://docs.rs/winit/latest/winit/event/struct.KeyEvent.html#structfield.text). - Changed most `KeyCode` values, and added more. KeyCode has changed meaning. With this PR, it refers to physical position on keyboard rather than the printed letter on keyboard keys. In practice this means: - On QWERTY keyboard layouts, nothing changes - On any other keyboard layout, `KeyCode` no longer reflects the label on key. - This is "good". In bevy 0.12, when you used WASD for movement, users with non-QWERTY keyboards couldn't play your game! This was especially bad for non-latin keyboards. Now, WASD represents the physical keys. A French player will press the ZQSD keys, which are near each other, Kyrgyz players will use "Цфыв". - This is "bad" as well. You can't know in advance what the label of the key for input is. Your UI says "press WASD to move", even if in reality, they should be pressing "ZQSD" or "Цфыв". You also no longer can use `KeyCode` for text inputs. In any case, it was a pretty bad API for text input. You should use `ReceivedCharacter` now instead. ### Other changes - Use `web-time` rather than `instant` crate. (rust-windowing/winit#2836) - winit did split `run_return` in `run_onDemand` and `pump_events`, I did the same change in bevy_winit and used `pump_events`. - Removed `return_from_run` from `WinitSettings` as `winit::run` now returns on supported platforms. - I left the example "return_after_run" as I think it's still useful. - This winit change is done partly to allow to create a new window after quitting all windows: emilk/egui#1918 ; this PR doesn't address. - added `width` and `height` properties in the `canvas` from wasm example (#10702 (comment)) ## Known regressions (important follow ups?) - Provide an API for reacting when a specific key from current layout was released. - possible solutions: use winit::Key from winit::KeyEvent ; mapping between KeyCode and Key ; or . - We don't receive characters through alt+numpad (e.g. alt + 151 = "ù") anymore ; reproduced on winit example "ime". maybe related to rust-windowing/winit#2945 - (windows) Window content doesn't refresh at all when resizing. By reading rust-windowing/winit#2900 ; I suspect we should just fire a `window.request_redraw();` from `AboutToWait`, and handle actual redrawing within `RedrawRequested`. I'm not sure how to move all that code so I'd appreciate it to be a follow up. - (windows) unreleased winit fix for using set_control_flow in AboutToWait rust-windowing/winit#3215 ; ⚠️ I'm not sure what the implications are, but that feels bad 🤔 ## Follow up I'd like to avoid bloating this PR, here are a few follow up tasks worthy of a separate PR, or new issue to track them once this PR is closed, as they would either complicate reviews, or at risk of being controversial: - remove CanvasParentResizePlugin (#10702 (comment)) - avoid mentionning explicitly winit in docs from bevy_window ? - NamedKey integration on bevy_input: rust-windowing/winit#3143 introduced a new NamedKey variant. I implemented it only on the converters but we'd benefit making the same changes to bevy_input. - Add more info in KeyboardInput #10702 (review) - #9905 added a workaround on a bug allegedly fixed by winit 0.29. We should check if it's still necessary. - update to raw_window_handle 0.6 - blocked by wgpu - Rename `KeyCode` to `PhysicalKeyCode` #10702 (comment) - remove `instant` dependency, [replaced by](rust-windowing/winit#2836) `web_time`), we'd need to update to : - fastrand >= 2.0 - [`async-executor`](https://github.com/smol-rs/async-executor) >= 1.7 - [`futures-lite`](https://github.com/smol-rs/futures-lite) >= 2.0 - Verify license, see [discussion](#8745 (comment)) - we might be missing a short notice or description of changes made - Consider using https://github.com/rust-windowing/cursor-icon directly rather than vendoring it in bevy. - investigate [this unwrap](#8745 (comment)) (`winit_window.canvas().unwrap();`) - Use more good things about winit's update - #10689 (comment) ## Migration Guide This PR should have one.

# Objective - Update winit dependency to 0.29 ## Changelog ### KeyCode changes - Removed `ScanCode`, as it was [replaced by KeyCode](https://github.com/rust-windowing/winit/blob/master/CHANGELOG.md#0292). - `ReceivedCharacter.char` is now a `SmolStr`, [relevant doc](https://docs.rs/winit/latest/winit/event/struct.KeyEvent.html#structfield.text). - Changed most `KeyCode` values, and added more. KeyCode has changed meaning. With this PR, it refers to physical position on keyboard rather than the printed letter on keyboard keys. In practice this means: - On QWERTY keyboard layouts, nothing changes - On any other keyboard layout, `KeyCode` no longer reflects the label on key. - This is "good". In bevy 0.12, when you used WASD for movement, users with non-QWERTY keyboards couldn't play your game! This was especially bad for non-latin keyboards. Now, WASD represents the physical keys. A French player will press the ZQSD keys, which are near each other, Kyrgyz players will use "Цфыв". - This is "bad" as well. You can't know in advance what the label of the key for input is. Your UI says "press WASD to move", even if in reality, they should be pressing "ZQSD" or "Цфыв". You also no longer can use `KeyCode` for text inputs. In any case, it was a pretty bad API for text input. You should use `ReceivedCharacter` now instead. ### Other changes - Use `web-time` rather than `instant` crate. (rust-windowing/winit#2836) - winit did split `run_return` in `run_onDemand` and `pump_events`, I did the same change in bevy_winit and used `pump_events`. - Removed `return_from_run` from `WinitSettings` as `winit::run` now returns on supported platforms. - I left the example "return_after_run" as I think it's still useful. - This winit change is done partly to allow to create a new window after quitting all windows: emilk/egui#1918 ; this PR doesn't address. - added `width` and `height` properties in the `canvas` from wasm example (bevyengine/bevy#10702 (comment)) ## Known regressions (important follow ups?) - Provide an API for reacting when a specific key from current layout was released. - possible solutions: use winit::Key from winit::KeyEvent ; mapping between KeyCode and Key ; or . - We don't receive characters through alt+numpad (e.g. alt + 151 = "ù") anymore ; reproduced on winit example "ime". maybe related to rust-windowing/winit#2945 - (windows) Window content doesn't refresh at all when resizing. By reading rust-windowing/winit#2900 ; I suspect we should just fire a `window.request_redraw();` from `AboutToWait`, and handle actual redrawing within `RedrawRequested`. I'm not sure how to move all that code so I'd appreciate it to be a follow up. - (windows) unreleased winit fix for using set_control_flow in AboutToWait rust-windowing/winit#3215 ; ⚠️ I'm not sure what the implications are, but that feels bad 🤔 ## Follow up I'd like to avoid bloating this PR, here are a few follow up tasks worthy of a separate PR, or new issue to track them once this PR is closed, as they would either complicate reviews, or at risk of being controversial: - remove CanvasParentResizePlugin (bevyengine/bevy#10702 (comment)) - avoid mentionning explicitly winit in docs from bevy_window ? - NamedKey integration on bevy_input: rust-windowing/winit#3143 introduced a new NamedKey variant. I implemented it only on the converters but we'd benefit making the same changes to bevy_input. - Add more info in KeyboardInput bevyengine/bevy#10702 (review) - bevyengine/bevy#9905 added a workaround on a bug allegedly fixed by winit 0.29. We should check if it's still necessary. - update to raw_window_handle 0.6 - blocked by wgpu - Rename `KeyCode` to `PhysicalKeyCode` bevyengine/bevy#10702 (comment) - remove `instant` dependency, [replaced by](rust-windowing/winit#2836) `web_time`), we'd need to update to : - fastrand >= 2.0 - [`async-executor`](https://github.com/smol-rs/async-executor) >= 1.7 - [`futures-lite`](https://github.com/smol-rs/futures-lite) >= 2.0 - Verify license, see [discussion](bevyengine/bevy#8745 (comment)) - we might be missing a short notice or description of changes made - Consider using https://github.com/rust-windowing/cursor-icon directly rather than vendoring it in bevy. - investigate [this unwrap](bevyengine/bevy#8745 (comment)) (`winit_window.canvas().unwrap();`) - Use more good things about winit's update - bevyengine/bevy#10689 (comment) ## Migration Guide This PR should have one.

# Objective - Update winit dependency to 0.29 ## Changelog ### KeyCode changes - Removed `ScanCode`, as it was [replaced by KeyCode](https://github.com/rust-windowing/winit/blob/master/CHANGELOG.md#0292). - `ReceivedCharacter.char` is now a `SmolStr`, [relevant doc](https://docs.rs/winit/latest/winit/event/struct.KeyEvent.html#structfield.text). - Changed most `KeyCode` values, and added more. KeyCode has changed meaning. With this PR, it refers to physical position on keyboard rather than the printed letter on keyboard keys. In practice this means: - On QWERTY keyboard layouts, nothing changes - On any other keyboard layout, `KeyCode` no longer reflects the label on key. - This is "good". In bevy 0.12, when you used WASD for movement, users with non-QWERTY keyboards couldn't play your game! This was especially bad for non-latin keyboards. Now, WASD represents the physical keys. A French player will press the ZQSD keys, which are near each other, Kyrgyz players will use "Цфыв". - This is "bad" as well. You can't know in advance what the label of the key for input is. Your UI says "press WASD to move", even if in reality, they should be pressing "ZQSD" or "Цфыв". You also no longer can use `KeyCode` for text inputs. In any case, it was a pretty bad API for text input. You should use `ReceivedCharacter` now instead. ### Other changes - Use `web-time` rather than `instant` crate. (rust-windowing/winit#2836) - winit did split `run_return` in `run_onDemand` and `pump_events`, I did the same change in bevy_winit and used `pump_events`. - Removed `return_from_run` from `WinitSettings` as `winit::run` now returns on supported platforms. - I left the example "return_after_run" as I think it's still useful. - This winit change is done partly to allow to create a new window after quitting all windows: emilk/egui#1918 ; this PR doesn't address. - added `width` and `height` properties in the `canvas` from wasm example (bevyengine#10702 (comment)) ## Known regressions (important follow ups?) - Provide an API for reacting when a specific key from current layout was released. - possible solutions: use winit::Key from winit::KeyEvent ; mapping between KeyCode and Key ; or . - We don't receive characters through alt+numpad (e.g. alt + 151 = "ù") anymore ; reproduced on winit example "ime". maybe related to rust-windowing/winit#2945 - (windows) Window content doesn't refresh at all when resizing. By reading rust-windowing/winit#2900 ; I suspect we should just fire a `window.request_redraw();` from `AboutToWait`, and handle actual redrawing within `RedrawRequested`. I'm not sure how to move all that code so I'd appreciate it to be a follow up. - (windows) unreleased winit fix for using set_control_flow in AboutToWait rust-windowing/winit#3215 ; ⚠️ I'm not sure what the implications are, but that feels bad 🤔 ## Follow up I'd like to avoid bloating this PR, here are a few follow up tasks worthy of a separate PR, or new issue to track them once this PR is closed, as they would either complicate reviews, or at risk of being controversial: - remove CanvasParentResizePlugin (bevyengine#10702 (comment)) - avoid mentionning explicitly winit in docs from bevy_window ? - NamedKey integration on bevy_input: rust-windowing/winit#3143 introduced a new NamedKey variant. I implemented it only on the converters but we'd benefit making the same changes to bevy_input. - Add more info in KeyboardInput bevyengine#10702 (review) - bevyengine#9905 added a workaround on a bug allegedly fixed by winit 0.29. We should check if it's still necessary. - update to raw_window_handle 0.6 - blocked by wgpu - Rename `KeyCode` to `PhysicalKeyCode` bevyengine#10702 (comment) - remove `instant` dependency, [replaced by](rust-windowing/winit#2836) `web_time`), we'd need to update to : - fastrand >= 2.0 - [`async-executor`](https://github.com/smol-rs/async-executor) >= 1.7 - [`futures-lite`](https://github.com/smol-rs/futures-lite) >= 2.0 - Verify license, see [discussion](bevyengine#8745 (comment)) - we might be missing a short notice or description of changes made - Consider using https://github.com/rust-windowing/cursor-icon directly rather than vendoring it in bevy. - investigate [this unwrap](bevyengine#8745 (comment)) (`winit_window.canvas().unwrap();`) - Use more good things about winit's update - bevyengine#10689 (comment) ## Migration Guide This PR should have one.

dhardy added 4 commits October 14, 2023 14:30

examples/ime, web: prefer use of logical_key

19a3e4d

Split enum KeyCode into PhysicalKey, KeyCode

c5691ed

Add validation notes on previous commit

1968f61

Add some From impls for NativeKey, PhysicalKey, Key

3e87fe2

dhardy force-pushed the key branch from f99c4ba to 21bb990 Compare October 15, 2023 08:07

dhardy added 3 commits October 15, 2023 09:30

Fixes for KeyCode / PhysicalKey split

a4077a9

Add some PartialEq impls for NativeKey, Key, etc.

44832ea

Split enum Key into Key, Action

8610913

dhardy force-pushed the key branch from 21bb990 to 8610913 Compare October 15, 2023 09:19

dhardy marked this pull request as ready for review October 15, 2023 09:25

dhardy requested review from daxpedda, jackpot51, kchibisov, madsmtm and msiglreith as code owners October 15, 2023 09:25

dhardy mentioned this pull request Oct 15, 2023

0.29.0 Release planning #2686

Closed

2 tasks

kchibisov requested changes Oct 15, 2023

View reviewed changes

dhardy commented Oct 16, 2023

View reviewed changes

kchibisov mentioned this pull request Oct 17, 2023

Winit version 0.29.2 #3160

Merged

3 tasks

Rename Key::Action -> Key::Named, enum Action -> NamedKey

c01a81e

tests/serde_objects.rs: add NamedKey

9e07381

dhardy force-pushed the key branch from c251ab5 to 9e07381 Compare October 17, 2023 15:47

kchibisov approved these changes Oct 19, 2023

View reviewed changes

dhardy and others added 2 commits October 19, 2023 15:05

Remove TODO (unnecessary validation)

4fe1b1d

Merge branch 'master' into key

4a7fb76

kchibisov merged commit acfeff5 into rust-windowing:master Oct 19, 2023

dhardy deleted the key branch October 20, 2023 10:23

ThierryBerger mentioned this pull request Oct 24, 2023

Update winit dependency to 0.29 bevyengine/bevy#8745

Closed

11 tasks

ThierryBerger mentioned this pull request Nov 22, 2023

Update winit dependency to 0.29 bevyengine/bevy#10702

Merged

ThierryBerger mentioned this pull request Dec 21, 2023

Winit 0.29 follow up bevyengine/bevy#11052

Closed

23 tasks

Jasper-Bekkers mentioned this pull request Jan 6, 2024

Winit API redesign #3367

Closed

madsmtm mentioned this pull request Dec 1, 2024

Separate Key into Character and NamedKey rust-windowing/keyboard-types#50

Merged

Conversation

dhardy commented Oct 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhardy commented Oct 17, 2023

Uh oh!

kchibisov commented Oct 17, 2023

Uh oh!

kchibisov left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

dhardy commented Oct 14, 2023 •

edited

Loading