Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Keyboard Input 2.0 #2662

Merged
merged 1 commit into from
May 28, 2023
Merged

Keyboard Input 2.0 #2662

merged 1 commit into from
May 28, 2023

Conversation

maroider
Copy link
Member

@maroider maroider commented Jan 31, 2023

  • Tested on all platforms changed
  • 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
  • Created or updated an example program if it would help users understand this functionality
  • Updated feature matrix, if new features were added or implemented

Sorry again for taking so long to get to this point, but @ArturKovacs and I are finally done rewriting the git history and integrating all our separate branches into one branch with one commit per backend, and rebasing the resulting branch on top of master. I'll make sure to keep this continuously rebased on master until it is merged.

While this branch should largely work as-is, there are a couple of things I think are worth addressing:

  1. The meaning of the Meta/Super enum variants may not be consistent across backends.
  2. I'm no longer all that happy with Key::Character(&'static str) (i.e. leaking key strings and stuffing them in a HashMap). I intend to summarize the benefits and drawbacks of various alternatives shortly (so please don't immediately dive into bike-shedding this), but I'd like to get this PR before I fall asleep.
  3. The X11 backend could potentially be ill-behaved WRT keyboard shortcuts provided by desktop environments. I haven't tested this myself, but it's a potential issue I seem to recall encountering when trudging through documentation.
  4. The Android backend pulls a somewhat icky trick with pointer casts. It would be nice if android_activity could deref KeyEvent to the ndk struct it wraps on the NativeActivity backend, similar to how the struct works on the GameActivity backend.
  5. Wiring up Window::reset_dead_keys on the Linux backend without using a static AtomicBool would be a nice-to-have.
  6. xkbcommon-dl needs a new release with changes we depend on.
  7. Between when the main implementation work was done and now, we've gained a Redox backend. As such, we ought to get the Redox backend working on this branch.

Closes #1888, #1890, and #1932.

@maroider

This comment was marked as outdated.

Copy link
Member

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay, you two are wonderful and we love you!

Points 1 to 5

In the spirit of wanting to get this merged as soon as possible so that this giant change doesn't go out of sync again, I propose that all of these are non-important, and can be resolved in future PRs instead.

  1. xkbcommon-dl needs a new release with changes we depend on.

The git dependency is fine until we want to do a release.

  1. Between when the main implementation work was done and now, we've gained a Redox backend. As such, we ought to get the Redox backend working on this branch.

Same philosophy, I don't think we necessarily need to block this PR on that (sorry @jackpot51, but Redox is lower on users priority list than proper keyboard handling. Still something we'd need to resolve before the next release of course!)


I reviewed the macOS implementation, with mostly a focus on the unsafe stuff, I kinda lost track of things inside create_key_event (since I'm not nearly as qualified as y'all in determining the correct behaviour). But it looks pretty good in that regard, only a few minor things that I noted down for reference, but can be resolved later. @ArturKovacs I can make my nitpick changes myself if you'd like?

src/platform_impl/macos/ffi.rs Outdated Show resolved Hide resolved
src/platform_impl/macos/ffi.rs Outdated Show resolved Hide resolved
src/platform_impl/macos/event.rs Outdated Show resolved Hide resolved
src/platform_impl/macos/util/mod.rs Outdated Show resolved Hide resolved
src/platform_impl/macos/view.rs Outdated Show resolved Hide resolved
src/event.rs Outdated Show resolved Hide resolved
0xa => VirtualKeyCode::Caret,
_ => return None,
})
pub fn code_to_location(code: KeyCode) -> KeyLocation {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: This seems like it should be a pub(crate) fn on KeyLocation in src/keyboard.rs instead (since it's also used e.g. in the Wayland backend).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Be careful that if you add this change, it should go in a separate commmit that's a fixup for the API changes commit.

Copy link
Member Author

@maroider maroider Feb 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(since it's also used e.g. in the Wayland backend).

There's only really a similar function in the Android backend (with the same name to boot). The X11 and Wayland backends do share a function called keysym_to_location, however.

Copy link
Contributor

@ArturKovacs ArturKovacs Feb 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be public regardless, because it might be a functionality that winit users want to use. I think it would probably be best to add this as a method of KeyCode so people can write code.get_location()

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@madsmtm @maroider is it okay if I add this get_location function as a public method on KeyCode? I mean something like:

impl KeyCode {
   pub fn get_location(&self) -> KeyLocation {
       match self {
           // ...
       }
   }
}

// This function was called form the flagsChanged event, which is triggered
// when the user presses/releases a modifier even if the same kind of modifier
// has already been pressed
if is_flags_changed_event {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I'd much rather have this inside flagsChanged: instead of it being merged with the other callers of update_modifiers (since then it's easier to verify that e.g. code_to_location doesn't hit the unreachable! case)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't actually see why that would be better, but I trust you on this one.

src/platform_impl/macos/event.rs Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
Copy link
Member

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently on Web there is a bug that e.g. if you press left and right shift at the same time, but only release right, it will report that no modifier keys are now active, even though you are still holding down the left shift key.
This happens with any modifier key.

I will provide a fix tomorrow!

@kchibisov
Copy link
Member

kchibisov commented May 26, 2023

Currently on Web there is a bug that e.g. if you press left and right shift at the same time, but only release right, it will report that no modifier keys are now active, even though you are still holding down the left shift key.
This happens with any modifier key.

I have a feeling that it's the issue with your system given that you have the same issue on Web and X11. On X11 we get the modifiers out of the event we don't track any of the keys.

On my system I can't repro the issue you're talking about. Maybe you looked at the event logs wrong though, lifting one modifier shouldn't result in modifiers changed event.

@daxpedda
Copy link
Member

I have a feeling that it's the issue with your system given that you have the same issue on Web and X11. On X11 we get the modifiers out of the event we don't track any of the keys.

On my system I can't repro the issue you're talking about. Maybe you looked at the event logs wrong though, lifting one modifier shouldn't result in modifiers changed event.

We cleared this up on Matrix, it was a misunderstanding. This is not a bug.

Overhaul the keyboard API in winit to mimic the W3C specification
to achieve better crossplatform parity. The `KeyboardInput` event
is now uses `KeyEvent` which consists of:

  - `physical_key` - a cross platform way to refer to scancodes;
  - `logical_key`  - keysym value, which shows your key respecting the
                     layout;
  - `text`         - the text produced by this keypress;
  - `location`     - the location of the key on the keyboard;
  - `repeat`       - whether the key was produced by the repeat.

And also a `platform_specific` field which encapsulates extra
information on desktop platforms, like key without modifiers
and text with all modifiers.

The `Modifiers` were also slightly reworked as in, the information
whether the left or right modifier is pressed is now also exposed
on platforms where it could be queried reliably. The support was
also added for the web and orbital platforms finishing the API
change.

This change made the `OptionAsAlt` API on macOS redundant thus it
was removed all together.

Co-Authored-By: Artúr Kovács <[email protected]>
Co-Authored-By: Kirill Chibisov <[email protected]>
Co-Authored-By: daxpedda <[email protected]>
Fixes: #2631.
Fixes: #2055.
Fixes: #2032.
Fixes: #1904.
Fixes: #1810.
Fixes: #1700.
Fixes: #1443.
Fixes: #1343.
Fixes: #1208.
Fixes: #1151.
Fixes: #812.
Fixes: #600.
Fixes: #361.
Fixes: #343.
@kchibisov kchibisov merged commit 9184309 into master May 28, 2023
@kchibisov kchibisov deleted the new-keyboard-v3 branch May 28, 2023 18:03
@kchibisov
Copy link
Member

Thanks to everyone involved into this work.

@ArturKovacs
Copy link
Contributor

Working on this changeset has been a major part of my life in 2020 and in 2021 - it has brought joy, frustration, excitement and everything in-between. This getting mered is almost like a dream coming true to me. I'd like to thank everybody that put effort into moving this forward.

There were many who contributed, but in particular I would like to highlight @maroider who joined me at the beggining in implementing the API and put a huge amount of effort into X11, Wayland, and the Web implementations. I would also like to give a big thanks to @kchibisov who picked up our work, took care of the rough edges and pushed this over the finish line.

fredizzimo added a commit to neovide/neovide that referenced this pull request Jun 14, 2023
…glutin 0.30.7 (#1789)

The exact version of winit is f7a400ddf6a7822a1ac0cf80ca805dcf3298872e and includes the big keyboard API refactoring, a very big effort by the winit team - rust-windowing/winit#2662. Additionally the actual Winit version is several versions newer than the previous one (0.24.0), so a lot of other things have also been fixed.

The biggest impact fix for Neovide is working Wayland support #1356
The winit update also fixes unhanded key up events #1631

There are probably also a lot of still unconfirmed fixes, and maybe also some new regression as with all big updates.

---------

Co-authored-by: Fred Sundvik <[email protected]>
Co-authored-by: rkuklik <[email protected]>
Co-authored-by: MultisampledNight <[email protected]>
Co-authored-by: Serhii Tereshchenko <[email protected]>
MarijnS95 added a commit that referenced this pull request Jul 31, 2023
 #2662 renamed `VirtualKeyCode` to `Key` yet references to the former
type still exist in `src/platform_impl/linux/x11/events.rs`.  As it
turns out the `mod events;` in `x11/mod.rs` was removed in the same PR,
but the file accidentally stuck around without being referenced anywhere
else.
kchibisov pushed a commit that referenced this pull request Jul 31, 2023
#2662 renamed `VirtualKeyCode` to `Key` yet references to the former
type still exist in `src/platform_impl/linux/x11/events.rs`.  As it
turns out the `mod events;` in `x11/mod.rs` was removed in the same PR,
but the file accidentally stuck around without being referenced anywhere
else.
kchibisov pushed a commit to kchibisov/winit that referenced this pull request Aug 14, 2023
rust-windowing#2662 renamed `VirtualKeyCode` to `Key` yet references to the former
type still exist in `src/platform_impl/linux/x11/events.rs`.  As it
turns out the `mod events;` in `x11/mod.rs` was removed in the same PR,
but the file accidentally stuck around without being referenced anywhere
else.
kchibisov pushed a commit that referenced this pull request Aug 15, 2023
#2662 renamed `VirtualKeyCode` to `Key` yet references to the former
type still exist in `src/platform_impl/linux/x11/events.rs`.  As it
turns out the `mod events;` in `x11/mod.rs` was removed in the same PR,
but the file accidentally stuck around without being referenced anywhere
else.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.