Fix X11 KeyboardInput events sending with ibus input method.

[ashdnazg]

• [x] Tested on all platforms changed
• [x] Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
• [x] 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
• [x] Updated feature matrix, if new features were added or implemented

Previously, KeyboardInput events were sent when they weren't filtered by the IM. In cases where the IM first filtered an event and later replayed it, the replayed event was the one to be sent. This caused issues under the ibus input method and repeated keys. Ibus replays the repeated key events one by one, which depending on frame rate, can be long after the key was released. The solution is to keep track of the time of arrival of the last event of each keycode. An event is a repeat if its time is not later than the previous event for the same keycode. This solution is in use in glfw as well: https://github.com/glfw/glfw/commit/9a3664b

[ArturKovacs]

To be honest, I don't completely understand what the original issue was. Can you tell us how to reproduce this? Also please describe the expected list of KeyboardInput and ReceivedCharacter events and the actual events you get when reproducing the issue.

[ashdnazg]

@ArturKovacs you can see the effects in this bug report in Veloren: https://gitlab.com/veloren/veloren/-/issues/877 in particular this comment: https://gitlab.com/veloren/veloren/-/issues/877#note_476757314 You can easily reproduce this on linux/X11:

1. Set your X11 input method to ibus export XMODIFIERS=@im=ibus
2. Turn Repeat Keys on
3. Run the window example from this branch: https://github.com/Imberflur/winit/tree/event-test
4. Hold any key down for 2 seconds
5. Release the key
6. See how you keep getting key presses.

[ashdnazg]

Sorry for pinging, but is there a way I could assist in reviewing this PR?

[maroider]

I think the issue is that we lack people who are both experienced with and interested in maintaining the X11 backend.

[ashdnazg]

The perils of open source...

[rib]

Reviewing some of the outstanding PRs / issues for x11 I also couldn't help but notice this mammoth series which includes full integration of XInput2 and libxkbcommon (definitely a good direction for the x11 backend) as well as a full port to xcb.

What's notable with respect to this issue though is that the above series effectively removes IME support via the XIM protocol, arguing that IMEs could be better supported either higher up in the stack, or via direct ibus support.

On the one hand it doesn't seem ideal that removing XIM integration has the effect of removing at least crude IME support from the x11 backend, but on the other hand this issue effectively demonstrates how XIM is a poor interface for IME integration, and it looks like removing XIM integration (use of XFilterEvents) entirely would potentially be another solution for this bug?

[rib]

At least in terms of the description of the issue given, and the proposed fix this seems like a reasonable workaround. I mean it's kinda terrible to have this kind of arms race with the XIM filtering, but x11 is kinda long in the tooth, and it's not surprising :)

I suppose a question to ask here though is who's responsible for key repeat synthesis. Does winit document that it does key repeat synthesis, or set the expectation that it's left to downstream users to handle key repeat?

Looking around, it seems like this is still a bit of an unanswered question: https://github.com/rust-windowing/winit/issues/753 so winit doesn't currently seem to have a clear opinion on how key repeats are exposed to apps, and doesn't promise that key repeats aren't delivered by the OS.

see also https://github.com/rust-windowing/winit/issues/310 and https://github.com/PistonDevelopers/piston/issues/1220

I'm wondering about that because arguably the input module is configured by the user to generate key repeats, and the behaviour seen looks like it's technically inline with what the user had configured (except very laggy), even though that's not what was expected / desired within Veloren.

Looking at the patch itself, it looks ok to me besides some questions around the wrapping arithmetic and maybe extra comments about why the filtering is needed. It would be slightly easier to review the changes if handle_key_events were split into a function in a separate patch, since there's a decent amount of code that moves in the patch with inline changes too, but I think I followed it. Essentially it uses a heuristic to recognise key repeats that come from an IM filter (based on their timestamp) and discards them - based on the solution in glfw.

Besides the question around how winit is supposed to handle key repeats generally, I do wonder about the other (simpler) option raised by the patch series I linked above, which is that winit could drop it's use of XFilterEvents entirely and consider that mechanism to be out-dated and inadequate?

[ArturKovacs]

Is this still relevant, given that #2243 has been merged? @kchibisov might be able to help in that case.

[rib]

@ArturKovacs it looks like that work was essentially to integrate more deeply with XIM so that pre-edit events from the XIM server could be translated into platform-independent Winit events.

[ArturKovacs]

On my local repo, I merged master into this, but after the merge there was a WindowEvent::KeyboardInput sent for the first IME input (this shouldn't happen). I'm guessing that ImeEvent::Start is received too late from ime_event_receiver, but I don't know why that would happen or what's the right way to fix it.

(I tested on Ubuntu 20.04 LTS with GNOME 3.36.8)

@kchibisov please help, this defeated me.

[kchibisov]

@ArturKovacs I'll try to take a look once I have time for that particular issue.

[geirsagberg]

I'm facing a similar issue on macOS in Bevy when key repeat rate is set faster than FPS (15ms key repeat, 16.7ms fps), see https://github.com/bevyengine/bevy/issues/5909

[madsmtm]

I'm facing a similar issue on macOS in Bevy when key repeat rate is set faster than FPS (15ms key repeat, 16.7ms fps), see bevyengine/bevy#5909

Please open a new issue for that, it'll get lost in this one

[ashdnazg]

@zesterer thanks! I'll have time to address your comments in a few days.

[kchibisov]

Be aware that keyboard patches won't be applied due to https://github.com/rust-windowing/winit/pull/2662 , so you should test on that branch your issues instead.

[zesterer]

Be aware that keyboard patches won't be applied due to #2662 , so you should test on that branch your issues instead.

Do you know whether that branch is likely to fix this problem incidentally, or will this fix likely need porting over?

[kchibisov]

Do you know whether that branch is likely to fix this problem incidentally, or will this fix likely need porting over?

That branch has entirely different input model, so it's hard to say for me.

[kchibisov]

Could this issue be rechecked with the latest master, given that all the keyboard input was reworked on X11 to use xkb?