Fix X11 KeyboardInput events sending with ibus input method. #2182

ashdnazg · 2022-02-04T22:17:14Z

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

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: glfw/glfw@9a3664b

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: glfw/glfw@9a3664b

ArturKovacs · 2022-02-14T14:26:13Z

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 · 2022-02-25T11:16:36Z

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

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

ashdnazg · 2022-04-20T19:30:03Z

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

maroider · 2022-04-20T19:51:54Z

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

ashdnazg · 2022-04-20T21:49:29Z

The perils of open source...

rib · 2022-05-29T06:43:57Z

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 · 2022-05-29T08:01:05Z

src/platform_impl/linux/x11/event_processor.rs

+            let signed_time = xkev.time as i64;
+            let time_since_previous_event =
+                signed_time.wrapping_sub(key_event_times[keycode as usize] as i64);
+


this look suspicious - casting the new time and the time that it's being compared with into i64 values will defeat the purpose of using wrapping_sub to account for the 32bit timestamp wrapping around I think?

rib · 2022-05-29T08:06:11Z

src/platform_impl/linux/x11/event_processor.rs

+            // An event is a replay of a previous event if it:
+            // 1) Isn't filtered by the IM.
+            // 2) Isn't the first time that an event with this keycode arrives.
+            // 3) Has a timestamp earlier or equal to the latest non-replayed event with this keycode.


could maybe also be good for future readers to have some comments about why it's necessary to filter out these repeat events. This is kind of a hacky workaround really, so it feels like that should be really clearly sign posted in the code.

rib · 2022-05-29T08:11:03Z

src/platform_impl/linux/x11/event_processor.rs

+                && time_since_previous_event <= 0;
+
+            if !replayed_event {
+                key_event_times[keycode as usize] = xkev.time;


considering the wrapping_sub arithmetic earlier, I guess this should be more careful to preserve the 32bit size of the X event timestamp. I guess ffi::Time is the same a time_t which I think can be 8 bytes on 64bit platforms. Could be cast back again later but still, just seems like it's important to be clear about the size if using wrapping arithmetic.

rib · 2022-05-29T08:15:17Z

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:
#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 #310 and PistonDevelopers/piston#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 · 2022-05-29T19:24:56Z

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

rib · 2022-05-30T07:37:33Z

@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 · 2022-06-04T10:15:20Z

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 · 2022-06-04T15:46:55Z

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

geirsagberg · 2022-09-07T21:03:11Z

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

madsmtm · 2022-09-08T00:53:44Z

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

zesterer

Thanks for opening this PR, I'm really excited to see progress made on it: we're had quite a number of users (including myself) bitten by this problem, and it seems like recent changes to the GNOME stack make it much more likely to happen. For these users, this isn't just a minor inconvenience: it makes playing Veloren mutually exclusive with the use of repeat keys, and most afflicted users never even discover that disabling repeat keys 'solves' the problem.

Given that this solution has precedence in GLFW, I feel like this PR is a relatively safe solution, but I've not interacted with the internals of winit much so I realise my perspective has little weight.

zesterer · 2023-03-18T10:34:05Z

src/platform_impl/linux/x11/event_processor.rs

@@ -37,6 +37,7 @@ pub(super) struct EventProcessor<T: 'static> {
    pub(super) first_touch: Option<u64>,
    // Currently focused window belonging to this process
    pub(super) active_window: Option<ffi::Window>,
+    pub(super) key_event_times: [ffi::Time; 256],


Perhaps 256 - KEYCODE_OFFSET given that we know the last 8 entries will never be used?

zesterer · 2023-03-18T10:36:39Z

src/platform_impl/linux/x11/event_processor.rs

+        if state == Pressed && !filtered_by_im {
+            let written = if let Some(ic) = wt.ime.borrow().get_context(window) {
+                wt.xconn.lookup_utf8(ic, xkev)
+            } else {
+                return;
+            };
+
+            for chr in written.chars() {
+                let event = Event::WindowEvent {
+                    window_id,
+                    event: WindowEvent::ReceivedCharacter(chr),
+                };
+                callback(event);
+            }
+        }


This early return feels a little easy to break in a future refactor. Perhaps something a bit less brittle like the following?

if state == Pressed && !filtered_by_im { if let Some(ic) = wt.ime.borrow().get_context(window) { let written = wt.xconn.lookup_utf8(ic, xkev); for chr in written.chars() { let event = Event::WindowEvent { window_id, event: WindowEvent::ReceivedCharacter(chr), }; callback(event); } } }

ashdnazg · 2023-03-19T21:23:48Z

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

kchibisov · 2023-03-19T21:25:38Z

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

zesterer · 2023-03-19T21:28:07Z

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 · 2023-03-19T21:30:49Z

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 · 2023-05-28T18:11:58Z

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

maroider added the DS - x11 label Feb 4, 2022

ashdnazg added 2 commits February 5, 2022 11:55

extract update_modifiers to a function.

3026356

maroider added the C - waiting on maintainer A maintainer must review this code label Feb 25, 2022

rib reviewed May 29, 2022

View reviewed changes

rib mentioned this pull request May 29, 2022

Add integration tests #2088

Open

rib mentioned this pull request May 30, 2022

android: Hold NativeWindow lock until after notifying the user with Event::Suspended #2307

Merged

5 tasks

geirsagberg mentioned this pull request Sep 7, 2022

Inputs are not processed correctly if key repeat rate is faster than FPS bevyengine/bevy#5909

Closed

zesterer reviewed Mar 18, 2023

View reviewed changes

kchibisov closed this Oct 20, 2023

kchibisov removed the C - waiting on maintainer A maintainer must review this code label Oct 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix X11 KeyboardInput events sending with ibus input method. #2182

Fix X11 KeyboardInput events sending with ibus input method. #2182

ashdnazg commented Feb 4, 2022

ArturKovacs commented Feb 14, 2022

ashdnazg commented Feb 25, 2022

ashdnazg commented Apr 20, 2022

maroider commented Apr 20, 2022

ashdnazg commented Apr 20, 2022

rib commented May 29, 2022

rib May 29, 2022

rib May 29, 2022

rib May 29, 2022

rib commented May 29, 2022

ArturKovacs commented May 29, 2022

rib commented May 30, 2022

ArturKovacs commented Jun 4, 2022 •

edited

Loading

kchibisov commented Jun 4, 2022

geirsagberg commented Sep 7, 2022

madsmtm commented Sep 8, 2022

zesterer left a comment •

edited

Loading

zesterer Mar 18, 2023

zesterer Mar 18, 2023

ashdnazg commented Mar 19, 2023

kchibisov commented Mar 19, 2023

zesterer commented Mar 19, 2023

kchibisov commented Mar 19, 2023

kchibisov commented May 28, 2023

Fix X11 KeyboardInput events sending with ibus input method. #2182

Fix X11 KeyboardInput events sending with ibus input method. #2182

Conversation

ashdnazg commented Feb 4, 2022

ArturKovacs commented Feb 14, 2022

ashdnazg commented Feb 25, 2022

ashdnazg commented Apr 20, 2022

maroider commented Apr 20, 2022

ashdnazg commented Apr 20, 2022

rib commented May 29, 2022

rib May 29, 2022

Choose a reason for hiding this comment

rib May 29, 2022

Choose a reason for hiding this comment

rib May 29, 2022

Choose a reason for hiding this comment

rib commented May 29, 2022

ArturKovacs commented May 29, 2022

rib commented May 30, 2022

ArturKovacs commented Jun 4, 2022 • edited Loading

kchibisov commented Jun 4, 2022

geirsagberg commented Sep 7, 2022

madsmtm commented Sep 8, 2022

zesterer left a comment • edited Loading

Choose a reason for hiding this comment

zesterer Mar 18, 2023

Choose a reason for hiding this comment

zesterer Mar 18, 2023

Choose a reason for hiding this comment

ashdnazg commented Mar 19, 2023

kchibisov commented Mar 19, 2023

zesterer commented Mar 19, 2023

kchibisov commented Mar 19, 2023

kchibisov commented May 28, 2023

ArturKovacs commented Jun 4, 2022 •

edited

Loading

zesterer left a comment •

edited

Loading