New keyboard API for Linux #1932

maroider · 2021-05-07T15:38:20Z

Tested on all platforms changed
Compilation warnings were addressed
cargo fmt has been run on this branch
cargo doc builds successfully
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

This implements #753 for Linux (I know that issue number by heart now, lol). Much like #1888, this PR is lacking in some areas, but unlike the aforementioned PR, this PR should be usable enough to get some useful feedback and testing done.

TODOs

cc @garasubo @jamadazi

ArturKovacs

I skimmed through the X11 implementation. Here are the things that caught my eye.

src/platform_impl/linux/common/xkb_state.rs

src/platform_impl/linux/common/keymap.rs

src/platform_impl/linux/common/xkb_state.rs

src/platform_impl/linux/x11/event_processor.rs

src/platform_impl/linux/x11/mod.rs

src/platform_impl/linux/x11/util/input.rs

ArturKovacs · 2021-05-10T17:47:53Z

For the record: I updated xkbcommon-dl which allows testing on Ubuntu 20.04

inodentry · 2021-05-12T16:38:52Z

How could I test this, as an outsider that isn't familiar with winit and its APIs?

Is there something ready-made I can run (example or something) or something I could code up easily, to play around with this and see if I encounter bugs or other feedback to give?

maroider · 2021-05-12T16:45:30Z

You can use this program I made in order to test any of the keyboard rework PRs.

pickfire · 2021-05-20T10:15:36Z

@maroider Can you also test it out with multi-pointer x (I believe wayland also supports this) while working on this? In the demo, can also add a "seat" details there. https://wiki.archlinux.org/title/Multi-pointer_X#xinput_utility provides some instructions on how to do it.

garasubo

Sorry for my late comment.

I tested in my local x11 env, but couldn't get ReceivedImeText event.

src/platform_impl/linux/x11/event_processor.rs

maroider · 2021-06-16T03:53:53Z

Can you also test it out with multi-pointer x (I believe wayland also supports this) while working on this? In the demo, can also add a "seat" details there. https://wiki.archlinux.org/title/Multi-pointer_X#xinput_utility provides some instructions on how to do it.

Is there a particular reason you can't test this yourself, @pickfire? I have no experience with "multi-pointer X", and the wiki page you linked doesn't list i3 (my X11 WM) as compatible with it.

pickfire · 2021-06-16T04:36:00Z

Is there a particular reason you can't test this yourself, @pickfire? I have no experience with "multi-pointer X", and the wiki page you linked doesn't list i3 (my X11 WM) as compatible with it.

Oh, I forgot to mention that multi-pointer X is only useful for window manager. I don't think it will be useful for application, I don't think I am aware of any rust window manager that I can try out with this patch, it needs to have support from both the window manager and xinit for it to work.

maroider · 2021-08-04T22:18:42Z

Changing the keyboard layout should now work under native X11, but the details aren't pretty.
I haven't tested it under XWayland, as my Wayland config is currently borked.
Wayland might actually handle layout changes right now, but I don't recall.

In any case, I think I'll be able to push through with the other TODOs now that this sort of works on X11, as it has been the biggest mental roadblock to getting anything done on this PR. I don't recall if I thanked you on the discord call, @ArturKovacs, so here you go: Thank you! You helped a bunch.

src/platform_impl/linux/x11/event_processor.rs

ArturKovacs · 2021-08-22T13:53:09Z

I went through the TODOs in your list and I believe that the only item that's blocking this from getting merged is

Ensure control characters only occur when we expect them. This is mostly implemented, but there seems to be a couple edge cases which I'm not inclined to fix today.

Can you elaborate on what those edge cases are and how can one reproduce them?

And the other thing that I believe is blocking this is the missing implementation for KeyCodeExtScancode.

maroider · 2021-08-22T15:11:27Z

I went through the TODOs in your list and I believe that the only item that's blocking this from getting merged is

Ensure control characters only occur when we expect them. This is mostly implemented, but there seems to be a couple edge cases which I'm not inclined to fix today.

I have forgotten, and I'm annoyed at myself for not writing it down. I recall writing that with the intention of re-visiting it within a couple of days. Needless to say, that didn't happen 😅.

As for when I can get on with wrapping this PR up: I've admittedly been distracted by other PRs a bit (as well as the start of a new school year, as well as moving to a new place for said school), but I have some measure of confidence that I'll get around to doing the things you mentioned either today or next week.

I also have an implementation for Window::reset_dead_keys sitting around, but it's kind of hacky in how it passes data around (it uses lazy_statics since I didn't feel motivated enough to figure out a proper way to do it at the time).

ArturKovacs · 2021-08-22T15:33:02Z

but I have some measure of confidence that I'll get around to doing the things you mentioned either today or next week.

That sounds great Markus! Let me know if you need help with anything. I'm eager to get these improvements out to the public.

maroider · 2021-08-24T22:50:29Z

Ok, I have done the things now.

maroider · 2021-09-09T15:45:09Z

I've had another brief look over things, and I believe the PR should be in a state where nothing should change too much from further review (other than that one horrible hack that we don't talk about, shhh).

I'd like those who know how to use an IME to check if this PR introduces any regressions at this point.

I'm also waiting on a response from @garasubo.

Turns out I was an idiot when initially working on this. Don't listen to XkbMapNotify This event appears to be subsumed by XkbNewKeyboardNotify when we start listening to XkbNewKeyboardNotify, although the documentation feels a little unclear on the matter.

This commit retains the old code-path for doing this tracking in order to test for regressions introduced by this change. It appears that we manage to avoid any visible regressions due to having already removed the long since deprecated `modifiers` field from keyboard events, which hides the fact that using XkbStateNotify makes ModifiersChanged events arrive _after_ the WindowEvent for the keypress instead of between the DeviceEvent and the WindowEvent.

In particular, this removes the X11_EVPROC_NEXT_COMPOSE hack now that our IME event code-path seems to ignore compose sequences.

maroider · 2022-07-07T08:28:56Z

Ok, I'm fairly certain the current keyboard layout change handling should work for most people now (and without the horrible hack that was previously used). I also went ahead and moved us over to using XkbStateNotify to track modifier keys, and I couldn't observe any regressions in my own (admittedly brief) testing.

So as of this point, I don't think there's anything I'd consider a "hard blocker" to merging this (after some review, of course).

notgull

Looks good to me!

notgull · 2022-07-24T16:58:14Z

src/platform_impl/linux/common/keymap.rs

+/// X11-style keycodes are offset by 8 from the keycodes the Linux kernel uses.
+pub fn raw_keycode_to_keycode(keycode: u32) -> KeyCode {
+    let rawkey = keycode - 8;
+    // The keycode values are taken from linux/include/uapi/linux/input-event-codes.h, as


According to this, in most cases it should map 1/1 in most cases.

So that means it'll (hopefully) work just fine on FreeBSD?

hch12907 · 2022-08-15T00:43:32Z

Hi! I chanced upon this PR when I was trying to figure out why my application wasn't receiving DeviceEvent::Key events when it was running on Wayland. Since I found the behaviour rather surprising... (other platforms that my application supported received said event)

One thing that should be decided on before merging is whether we should "emulate" device events for keyboard input on Wayland.

I'm supportive of this, but if it is ultimately not implemented, I suggest adding a line or two to the docs for clarification.

rdrpenguin04 · 2022-08-27T06:05:18Z

This got a LGTM about a month ago; could a maintainer merge this? This may affect one of my projects.

maroider · 2022-08-27T14:34:29Z

While I did ask @notgull to have a look over this PR (and am glad he did), I still want a review from @kchibisov (which I've asked for, but other things have taken priority), since he's our Wayland maintainer. Don't get me wrong, I'd love nothing more than to merge this, but I can't exactly merge major work on a backend without sign-off from the maintainer of said backend.

kchibisov

I haven't looked that much into semantics of XKB handling and X11 backend.

src/platform_impl/linux/common/xkb_state.rs

kchibisov · 2021-09-09T16:16:57Z

src/platform_impl/linux/common/xkb_state.rs

+        );
+
+        if keymap.is_null() {
+            panic!("Received invalid keymap from compositor.");


Do we really need to crash here? Though I guess we can't do much at this point...

I chose to panic here because it was less work than bubbling the error up to the user somehow, although it's not obvious to me how we'd do that and still let the user use their window(s) in this degraded state. Another option would've been to log the error and move on with keyboard events remaining non-functional, but that makes the error one you have to dig through logs to find. I also don't think this will come up often in practice, but that's just a guess.

kchibisov · 2022-09-01T03:03:32Z

src/platform_impl/linux/common/keymap.rs

@@ -0,0 +1,882 @@
+//! Convert Wayland keys to winit keys.


Not sure why Wayland is here given that it's a common thing?

I'm not sure why that's like that either (it's been a while since I wrote this, haha). "X11-style" is what I'd go with (since it existed first), but as you mention, it's the same both on X11 and Wayland.

kchibisov · 2022-09-01T03:03:46Z

src/platform_impl/linux/common/keymap.rs

+/// Map the raw X11-style keycode to the `KeyCode` enum.
+///
+/// X11-style keycodes are offset by 8 from the keycodes the Linux kernel uses.


And here we talk about x11?

kchibisov · 2022-09-01T03:05:43Z

src/platform_impl/linux/common/xkb_state.rs

+
+pub(crate) struct KbState {
+    #[cfg(feature = "x11")]
+    xcb_connection: *mut xcb_connection_t,


Do we need to use xcb given that winit is xlib based? I'm pretty sure there's an xkb X11 extension.

libxkbcommon wants a *mut xcb_connection_t in some of the functions we want to use, and the *mut xcb_connection_t we use is derived from the xlib connection.

kchibisov · 2022-09-01T03:16:29Z

src/platform_impl/linux/common/xkb_state.rs

+    /// libxkbcommon is not available
+    XKBNotFound,
+    /// Provided RMLVO specified a keymap that would not be loaded
+    #[cfg(feature = "wayland")]


I don't think that feature gating enum variants is a good idea.

kchibisov · 2022-09-01T03:21:48Z

src/platform_impl/linux/wayland/seat/keyboard/handlers.rs

+    fn register(
+        &mut self,
+        poll: &mut calloop::Poll,
+        token_factory: &mut calloop::TokenFactory,
+    ) -> std::io::Result<()> {
+        self.timer.register(poll, token_factory)
+    }
+
+    fn reregister(
+        &mut self,
+        poll: &mut calloop::Poll,
+        token_factory: &mut calloop::TokenFactory,
+    ) -> std::io::Result<()> {
+        self.timer.reregister(poll, token_factory)
+    }
+
+    fn unregister(&mut self, poll: &mut calloop::Poll) -> std::io::Result<()> {
+        self.timer.unregister(poll)
+    }


Could you recheck latest SCTK release wrt that code, it was changed iirc given that calloop updated, but maybe that's a new version of it?

This is all based off of a 5 months old version of Winit, so maybe that'll be an issue once we rebase.

kchibisov · 2022-09-01T03:23:53Z

src/platform_impl/linux/wayland/seat/keyboard/handlers.rs

+        {
+            if let Some(ref mut repeat) = self.repeat {
+                repeat.stop_all_repeat();
+            }
+        }


You don't need extra scope for that.

kchibisov · 2022-09-01T03:25:42Z

src/window.rs

+    #[inline]
+    pub fn transparent(&self) -> bool {
+        self.window.transparent
+    }


Seems like a strange rebase.

kchibisov · 2022-09-01T03:26:18Z

src/platform_impl/linux/x11/mod.rs

@@ -202,6 +201,27 @@ impl<T: 'static> EventLoop<T> {
            ext
        };

+        let xkbext = {


Could you put unsafe here?

kchibisov · 2022-11-18T14:10:19Z

@maroider given that you really want to work on such thing. Could we have a branch against latest winit with the following structure.

1 commit - Core API
2-5 commits - one commit per backend.

So at least we'd have a reviewable thing and the thing that could actually be used?

Right now I find it insanely hard to review any of those branches since I have 0 clue what is going on, which API is the final one and so on. So having everything in a single branch with suggest commit structure could help me a lot to move this forward given that I don't have time to do that myself at all.

The single branch against master could help me port something like alacritty and test it, since it's a client using all sorts of keyboard input in the current winit.

kchibisov · 2023-05-28T18:00:00Z

closing as we have mono branch.

maroider marked this pull request as draft May 7, 2021 15:45

maroider added DS - wayland DS - x11 C - in progress Implementation is proceeding smoothly C - waiting on maintainer A maintainer must review this code C - waiting on testing Does this PR work? labels May 7, 2021

maroider mentioned this pull request May 7, 2021

Meta Issue: Keyboard input #1806

Closed

maroider added this to the Keyboard Events Overhaul milestone May 7, 2021

mockersf mentioned this pull request May 7, 2021

Improve keyboard input with Input<ScanCode> bevyengine/bevy#2052

Closed

ArturKovacs mentioned this pull request May 9, 2021

Fix for shared object errors on Ubuntu 20.04 maroider/xkbcommon-dl#1

Merged

ArturKovacs reviewed May 9, 2021

View reviewed changes

Kethku mentioned this pull request May 13, 2021

Non Qwerty\En US Input neovide/neovide#445

Closed

ArturKovacs mentioned this pull request May 16, 2021

Add new mappings for numlock, numpadenter and numpadcomma on X11 #1937

Merged

8 tasks

garasubo reviewed May 21, 2021

View reviewed changes

src/platform_impl/linux/x11/event_processor.rs Show resolved Hide resolved

garasubo reviewed May 21, 2021

View reviewed changes

src/platform_impl/linux/x11/event_processor.rs Outdated Show resolved Hide resolved

maroider mentioned this pull request Jun 14, 2021

[X11] Missing key release event #1965

Closed

Kethku reviewed Aug 4, 2021

View reviewed changes

src/platform_impl/linux/x11/event_processor.rs Outdated Show resolved Hide resolved

maroider marked this pull request as ready for review September 9, 2021 15:45

maroider force-pushed the new-keyboard-linux branch 3 times, most recently from 7d59987 to ba8023a Compare June 15, 2022 04:01

barklan mentioned this pull request Jun 18, 2022

Neovide does not start with kwin 5.25 in wayland session (wayland-client error) neovide/neovide#1356

Closed

maroider force-pushed the new-keyboard-linux branch 3 times, most recently from b37656e to 0676c39 Compare July 6, 2022 12:39

maroider added 3 commits July 7, 2022 09:45

Rip out the old code-path for modifier state tracking on X11

6c7820e

maroider force-pushed the new-keyboard-linux branch from 0676c39 to 6c7820e Compare July 7, 2022 07:46

maroider added 2 commits July 7, 2022 10:08

Rip out some vestigial parts

51975ef

In particular, this removes the X11_EVPROC_NEXT_COMPOSE hack now that our IME event code-path seems to ignore compose sequences.

Deal with various TODOs

9d077ab

MultisampledNight mentioned this pull request Jul 10, 2022

neovide doesn't launch on kwin_wayland neovide/neovide#1394

Closed

notgull approved these changes Jul 24, 2022

View reviewed changes

MultisampledNight mentioned this pull request Aug 6, 2022

Issue overwiew/prioritization as of 08/2022 neovide/neovide#1463

Open

14 tasks

kchibisov requested changes Sep 1, 2022

View reviewed changes

Deduplicate code in ModifiersState::update_with

29c6762

notgull mentioned this pull request Jan 1, 2023

internal: Add libxcb and replace some Xlib functions #2614

Closed

5 tasks

maroider mentioned this pull request Jan 31, 2023

Keyboard Input 2.0 #2662

Merged

5 tasks

kchibisov mentioned this pull request Feb 11, 2023

KeyCode::Plus corresponds to = input on North American keyboard #2682

Closed

TimJentzsch mentioned this pull request Mar 2, 2023

Add support for scancode keyboard input Leafwing-Studios/leafwing-input-manager#312

Merged

2 tasks

kchibisov mentioned this pull request May 5, 2023

[FOLLOW UP] Keyboard Input 2.0 #2789

Closed

kchibisov closed this May 28, 2023

New keyboard API for Linux #1932

New keyboard API for Linux #1932

Conversation

maroider commented May 7, 2021 • edited Loading

TODOs

ArturKovacs left a comment

Choose a reason for hiding this comment

ArturKovacs commented May 10, 2021

inodentry commented May 12, 2021

maroider commented May 12, 2021

pickfire commented May 20, 2021 • edited Loading

garasubo left a comment

Choose a reason for hiding this comment

maroider commented Jun 16, 2021

pickfire commented Jun 16, 2021 • edited Loading

maroider commented Aug 4, 2021 • edited Loading

ArturKovacs commented Aug 22, 2021

maroider commented Aug 22, 2021 • edited Loading

ArturKovacs commented Aug 22, 2021

maroider commented Aug 24, 2021

maroider commented Sep 9, 2021

maroider commented Jul 7, 2022 • edited Loading

notgull left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hch12907 commented Aug 15, 2022

rdrpenguin04 commented Aug 27, 2022

maroider commented Aug 27, 2022 • edited Loading

kchibisov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maroider Nov 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maroider Nov 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kchibisov commented Nov 18, 2022

kchibisov commented May 28, 2023

maroider commented May 7, 2021 •

edited

Loading

pickfire commented May 20, 2021 •

edited

Loading

pickfire commented Jun 16, 2021 •

edited

Loading

maroider commented Aug 4, 2021 •

edited

Loading

maroider commented Aug 22, 2021 •

edited

Loading

maroider commented Jul 7, 2022 •

edited

Loading

maroider commented Aug 27, 2022 •

edited

Loading

maroider Nov 18, 2022 •

edited

Loading

maroider Nov 18, 2022 •

edited

Loading