New keyboard API for Linux

[maroider]

• [ ] Tested on all platforms changed
• [x] Compilation warnings were addressed
• [x] cargo fmt has been run on this branch
• [x] 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
• [x] ~~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

• [x] Make IMEs work as expected. I have 0 experience with IMEs, so this part may be completely incorrect.
• [x] Do another pass on keymap.rs, as there are things there I suspect can be mapped, and some things that may have been mapped incorrectly.
• [x] Handle keyboard layout changes
• [x] Should Ctrl override any attempts to compose a character (with dead keys)? gnome-terminal agrees, but konsole disagrees. Should it be configurable instead?
• [x] 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.
• [ ] Make sure it runs fine under multiseat configurations
  • [ ] Wayland
  • [ ] X11
• [x] Various TODO comments in the code.
  • [x] src/platform_impl/linux/common/keymap.rs
  • [ ] src/platform_impl/linux/common/xkb_state.rs
  • [x] src/platform_impl/linux/x11/event_processor.rs

cc @garasubo @jamadazi

[ArturKovacs]

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

[inodentry]

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]

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

[pickfire]

@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.

[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.

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]

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]

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.

[ArturKovacs]

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]

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]

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]

Ok, I have done the things now.

[maroider]

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.

[Kethku]

Just want to highlight https://github.com/maroider/winit/pull/1 which I think addresses an issue we've been seeing upstream in neovide using these branches

[maroider]

Well, I'll be damned. That apparently slipped my notice. I think the issue is that GitHub just doesn't automatically put me on "watch all activity" for my own forks.

I'll check out the PR tomorrow, but I was already thinking of using the modifiers exposed to us through the XKB extension rather than the ones we get in the "regular" protocol (which is why I wrote this comment).

[maroider]

Just a quick status update for those of you who are (rather helpfully) sending me PRs as well as those who are just lurking: I'm currently a bit swamped by school (and that's mostly my fault), so I can't in good conscience do work on this PR when I should be focusing on my studies. That's not to say that I'll stop working on this PR (and on winit in general), nor does it mean I'll focus only on my studies when I should, as working on winit is at times a way for me to procrastinate. However, I do not at the moment plan on working on winit this week.

If all goes to plan, I'll get back to work on this next week (but don't be surprised if it takes yet another week).

[Kethku]

I can't help but feel somewhat responsible for the added pressure ;)

Thank you so much for working on this. Its a crucial part of the rust ecosystem and these keyboard reworks are a big deal. But also school is important! This can wait :)

[maroider]

Another update: I've gotten to a point where I'm a lot less swamped, so I believe I should be able to resume work on this PR (fingers crossed) some time next week.

[maroider]

I'm more swamped than I thought I'd be at this point (thanks in part to me being dumb about time allocation). Thankfully, I'll get a lecture-free week in two weeks (or rather, in the week that begins after the one that begins tomorrow). I hope I'll be able to look at this PR again by that weekend, but I can't promise anything.

[maroider]

I've been struck by some motivation to work on this right now, so I'll go ahead and look into the PRs (maroider/winit#1, maroider/winit#2, and maroider/winit#3) you folks have sent my way :)

PS: I'll stop with the memes now

[tailhook]

Can someone take a look at this issue: https://github.com/neovide/neovide/issues/958 ? It uses a branch from @Kethku although that is based on this branch as much as I can see. The issue is: neovide captures keystrokes from the different window in some window managers. And this is quite unfortunate.

[tailhook]

I'm confirming that bug neovide/neovide#958 is reproducing using winit_keyboard_tester built with the tip of this branch ea59172d

[tailhook]

Also got this error when tried to strace winit_keyboard_tester:

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Os { code: 4, kind: Interrupted, message: "Interrupted system call" }', /source/src/platform_impl/linux/x11/mod.rs:365:54

[tailhook]

Notes before I've forgot the details:

1. winit captures all the keyboard events (which I'm not very comfortable with, but..)
2. winit_keyboard_tester (and perhaps neovide too) uses keybindings only when window is focused

Now with focus, something strange happens. If I have window focused and mouse pointer on it, when I press window manager shortcut that changes workspace, I have the following events (using xev):

FocusOut event, serial 21, synthetic NO, window 0x3000001,
    mode NotifyGrab, detail NotifyAncestor

FocusOut event, serial 21, synthetic NO, window 0x3000001,
    mode NotifyWhileGrabbed, detail NotifyNonlinear

FocusIn event, serial 21, synthetic NO, window 0x3000001,
    mode NotifyWhileGrabbed, detail NotifyPointer

Which leads program thinking that the window is focused while it's hidden. NotifyGrab happens when I'm pressing window manager shortcut which is 'grabbed' (i.e. overriden so goes to window manager rather than focused window). In this case we never see ungrab events as windows becomes hidden (so does not receive further events).

So the bug is: window still thinks that it is focused and needs to process keystrokes while it's hidden.

If window manager shortcuts does not lead to hiding window (and neither to focus change), the sequence looks like this:

FocusOut event, serial 18, synthetic NO, window 0x3000001,
    mode NotifyGrab, detail NotifyAncestor

FocusIn event, serial 18, synthetic NO, window 0x3000001,
    mode NotifyUngrab, detail NotifyAncestor

So I think bug should be fixed by carefully processing focus events. I think filtering out NotifyWhileGrabbed events should be fine. Although I can't find good documentation on modes here, so maybe I'm wrong.

Also noting that when there is no pointer on the window, application behaves correctly. So maybe filtering out detail NotifyPointer is also an option. Just in case you're wondering: when changing focus by mouse, window manager acually does that happen, and normal focus events are received:

FocusIn event, serial 18, synthetic NO, window 0x3000001,
    mode NotifyNormal, detail NotifyNonlinear

---

With all that said, can someone comment why winit listens keystrokes on the level where all keystrokes are received by the application? I.e. if I use xev on the application it receives keypresses only when window is physically focused (and not hidden), so It looks like this library is reimplementing something that already implemented in X server.

[mahkoh]

Also got this error when tried to strace winit_keyboard_tester:

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Os { code: 4, kind: Interrupted, message: "Interrupted system call" }', /source/src/platform_impl/linux/x11/mod.rs:365:54

Fixed in https://github.com/rust-windowing/winit/pull/2031

[mahkoh]

Can someone take a look at this issue: neovide/neovide#958 ? It uses a branch from @Kethku although that is based on this branch as much as I can see. The issue is: neovide captures keystrokes from the different window in some window managers. And this is quite unfortunate.

Fixed in https://github.com/maroider/winit/pull/5

[mahkoh]

Fyi I'm currently working on pushing this over the line. I've already completely ported all input over to xinput2/xkb and validated that ad-hoc keyboard layout changes, multiple seats, etc. work on i3 and gnome (X11). What remains to be done is to re-implement IME support because the IME client in Xlib is incompatible with xinput2. I will amend https://github.com/maroider/winit/pull/5 in the coming days with the result.

After that I will look at Wayland but maybe this should be done in a separate PR.

[maroider]

I'm incredibly glad that you're doing this work, @mahkoh.

As I've mentioned previously, I have had a (mostly self-inflicted) hard time with schoolwork this autumn, so it has been harder to mentally justify spending time on winit (and this PR in particular). I feel like a broken record at this point, but I can't really say when I'll be able to do more work (i.e. review and merge PRs against this branch) on this outside of random bursts of motivation.

[mahkoh]

Quick update: While implementing the IM client I got the idea that it would be good to completely port the X11 backend from Xlib and friends to libxcb to improve error handling and robustness. To do this I wrote https://github.com/mahkoh/xcb-dl and a utility library that makes using raw xcb more convenient (currently unpublished). I have so far completed the port and removed all xlib dependencies. As I next step I plan to write a suite of integration tests that validates the behavior of the X11 backend (think a tiny window manager that launches a winit application, simulates input and checks the events generated by winit). Once this is done I will return to implementing the IM client.

To summarize:

1. Complete the port of input handling to xinput/xkb
2. Remove the old, dysfunctional IM client
3. Port the X11 backend to xcb
4. Create integration tests <-- We are here
5. Write a new IM client (partially done)

Unfortunately I've injured one of my hands last week so I haven't been getting much programming done. I will report back next week unless I have something to report before then.

[maroider]

I love your enthusiasm, but I'd like to voice a couple of concerns about porting the entire X11 backend to xcb:

• This feels like it should be its own PR against master (or a new branch)
• This would represent a rather large change, and we already take our sweet time reviewing PRs and getting them merged.
• The implications for application which assume that they get an Xlib handle from their winit windows are unclear to me.
• Adding an xcb backend was discussed in #5, but what you're doing is replacing the Xlib backend, so I suppose the concerns raised there about maintaining yet another backend don't apply here.

We already depend on Xlib being implemented on top of XCB, so is it at all possible to "splice out" the XCB-based IM client, or is that just too cumbersome?

[mahkoh]

• This feels like it should be its own PR against master (or a new branch)

That is possible. The port as it is right now is based on the previous work on this branch.

• This would represent a rather large change, and we already take our sweet time reviewing PRs and getting them merged.

That's why I'm working on a comprehensive test suite. Currently there seem to be no regression tests whatsoever. With a regression test suite it will become much easier to merge large changes.

• The implications for application which assume that they get an Xlib handle from their winit windows are unclear to me.

I'm not sure how much of a concern that is but it is true that the extension method that previously returned the Xlib handle returns None in my port. A possible solution would be to (optionally) create an Xlib handle and derive the XCB handle from that as we currently already do for xkbcommon. The Xlib handle would only exist to be returned from that extension method. I haven't looked into this too much so there might be other problems (the event-filtering mechanism comes to mind). Hopefully this is not necessary.

We already depend on Xlib being implemented on top of XCB, so is it at all possible to "splice out" the XCB-based IM client, or is that just too cumbersome?

It would be complicated unless the event loop runs on XCB, i.e., events are retrieved via xcb_poll_for_event. At that point it would be easier to write an Xlib based client (which I have no enthusiasm for.)