Add integration tests #2088
Add integration tests #2088
Conversation
We must not report virtual keyboard events for keys that were grabbed by other applications (XGrabKey, etc.). Since grabs only affect master devices, we must consume virtual events from master devices only.
…aroider fix: meta mod key on focus handling for gnome/x11
X11: Only fetch virtual keyboard events from master devices
This patch completes the port of the X11 backend from core input handling to XInput/XKB input handling. In this context the word 'core' refers to the core X11 protocol in contrast to protocol extensions such as XInput and XKB. XInput and XKB are very large protocols that extend X11 with features expected from modern desktop environments such as - Support for a rich set of input devices such as touchpads. - Support for multiple attached keyboards, mice, touchpads, tablets, etc. - Support for rich and interactive keyboard layouts. # Breaking Changes - This patch removes all processing of core input events in favor of XInput events. The legacy XIM input method protocol is based on filtering and injecting core input events. Therefore, this patch also removes support for XIM input methods. Applications are encouraged to switch to more modern IM protocols such as [IBus]. These protocols can be implemented in application space outside of winit. Note that modern toolskits such as QT5 and chromium do not support XIM. [IBus]: https://en.wikipedia.org/wiki/Intelligent_Input_Bus - This patch removes support for synthetic keyboard events. This feature cannot be implemented correctly: - XKB is a complex state machine where key presses and releases can perform a rich set of actions. For example: - Switching modifiers on and off. - Switching between keyboard layouts. - Moving the mouse cursor. These actions depend on the order the key are pressed and released. For example, if a key that switches layouts is released before a regular key, then the release of the regular key will produce different events than it would otherwise. - The winit API does not permit synthetic `ModifierChanged` events. As such, an application cannot distinguish between the user deliberately changing the active modifiers and synthetic changes. For example, consider an application that performs a drag-and-drop operation as long as the Shift modifier is active. Applications are encouraged to track the state of keys manually in a way that is suitable for their application. # New and Changed Features - Winit no longer tracks keyboard events if no winit window has the focus except that: - Raw keyboard events are still being tracked. A future patch might make this behavior optional. See rust-windowing#1634. - Changes to the keyboard layout are being tracked at all times. - The backend now has complete support for multiple seats. For each seat it tracks the modifier state and the focused window. In the case of `KeyboardInput` events, applications can distinguish multiple seats by tracking the value of the `device_id` field. In the case of `ModifierChanged` events, applications cannot distinguish different seats. A future patch might add a `device_id` field to `ModifierChanged` events. The following sequence of events is possible: 1. Key Press: Seat 1, Left Shift 2. Modifiers Changed: Shift 3. Key Press: Seat 2, Left Ctrl 4. Modifiers Changed: Ctrl 5. Key Press: Seat 1, KeyA, Text: "A" (due to active Shift) 6. Key Release: Seat 1, Left Shift 7. Modifiers Changed: None 8. Key Release: Seat 2, Left Ctrl 9. Modifiers Changed: None - Keyboard state and window events are now completely independent of device events. Applications can disable device events by modifying the winit source code (or in the future with a supported toggle) without incurring regressions in other areas. - Key release events no longer contain a value in the `text` and `text_with_all_modifiers` fields. - Key presses that are part of a compose sequence no longer contain a value in the `text` and `text_with_all_modifiers`. Applications that simply want to handle text input can therefore listen for key events and append the values of the `text` field to the input buffer without having to track any state. - The `logical_key` field of key events is no longer affected by compose sequences. This is in line with how browsers handle compose sequences. - Aborted compose sequences no longer produce any `text`. An aborted compose sequence is a sequence that was not completed correctly. For example, consider the following sequence of keysyms: 1. Multi_key 2. ( 3. c 4. ( `(` is not a valid continuation of the compose sequence starting with `[Multi_key, (, c]`. Therefore it aborts the sequence and no `text` is produced (not even for the final `(`). This is in line with existing practice on linux. - The `Dead` `Key` is now used exclusively for those keysyms that have `_dead_` in their name. This appears to be in line with how browsers handle dead keys. - The value of a `Dead` `Key` is in one of three categories: - If the dead key does not correspond to any particular diacritical mark, the value is `None`. For example, `dead_greek` (used to input Greek characters on a Latin keyboard). - If the dead key has a freestanding variant in unicode, the value is `Some(c)` with `c` being the freestanding character. For example, `dead_circumflex` has the value `Some('^')`. - Otherwise the value is `None`. For example, `dead_belowdot`. - `key_without_modifiers` now respects the effective XKB group. It only discards the state of modifiers. This is essential to correctly handle keyboard layouts in the GNOME desktop environment which uses XKB groups to switch between layouts. # Implementation Details - `EventProcessor` no longer uses any interior mutability. In cases where there were conflicting borrows, the code has been rewritten to use freestanding functions. - Keyboard state is now tracked exclusively by xkbcommon. The code that manually tracked some of this state has been removed. - The `xkb_state` module has been significantly simplified. The `process_key_event` function now computes all effects produced by a key press/release ahead of time. - Almost all XInput events also carry the current XKB state of its seat. We use this to track the state of modifiers eagerly and independently of keyboard events.
This patch implements reset_dead_keys for X11. The previous implementation was incorrect in that it only affected a single instance of KbState and that it relied on global state which could interfere with other event loops. This patch also removes the previous implementation for Wayland. The wayland backend is architecturally broken because it processes key events as soon as they are received and not immediately before they are dispatched. As such a call to reset_dead_keys would possibly not even affect the following key event because it has already been computed. Before reset_dead_keys can be implemented, the backend must be refactored to take this into account.
As of the previous patch, this code no longer has any effect. A future patch *might* restore XIM support.
# Background Clients communicate with the X server by sending and receiving size-prefixed binary messages. The format of these messages is described in the form of XML files at [freedesktop]. All of these messages can be described in the form of `repr(C)` structs with potentially trailing variably-sized data. [freedesktop]: https://gitlab.freedesktop.org/xorg/proto/xorgproto XCB is a C library that provides a way to send and receive these messages. It consists of - A small number of hand-written functions that implement a send and receive queue and a way to pair requests and replies via sequence numbers. - A large number of functions that are auto generated from the aforementioned XML files. These functions perform serialization and deserialization of requests before passing them on. Each message passed from XCB to the application consists of a single allocated buffer that can later be freed with `free`. XCB has many desirable properties: - The API is uniform. With the exception of the hand-written functions, the behavior of a function can be determined purely by looking at its name and signature. - Since received messages consist of a single allocation, memory management is easy and can be automated. - Since strings in the X protocol are not null terminated, it is not necessary to convert rust strings to C strings. - Functions that generate a reply are able the return an error. That is, they act as if they returned `Result<Reply, Error>` and it is easy to write rust wrappers with this return value. - It is completely thread safe. Xlib is a C library that nowadays uses XCB as a transport layer. On top of this it implements a C API with the following properties: - Strings are 0 terminated. - Returned values can contain nested allocations and must be freed with appropriate functions. - It uses global state. - It is not thread-safe by default. - Errors are handled via a callback function and cannot easily be matched with the functions that generated them. It gets worse when the application uses Xlib via multiple threads or with multiple connections. - By default all errors cause the application to terminate. - IO errors always cause the application to terminate. (In 2019 an API was introduced to modify this behavior.) - It provides some utility functions that make certain tasks easier. This includes an XIM client. - While each X protocol message is composed of 8, 16, and 32 bit units, Xlib exposes these as `char`, `short`, and `long`. It internally converts between 32 bit and `long` units. # Backwards Compatibility Some applications require winit to provide an Xlib connection to them. Notably, the GLX API is specified to work on top of Xlib and cannot be used without an Xlib connection. This patch allows applications to opt into using an Xlib connection by enabling the `xlib` feature. In this case they can retrieve the Xlib connection by calling the `xlib_display` function. Even if this feature is enabled, applications and users are able to opt out of using an Xlib connection by setting the `WINIT_DISABLE_XLIB` environment variable. This can be useful if a dependency enables the `xlib` feature but it is not necessary for the functionality of the application. # Breaking Changes - The `x11` module is no longer exported. This module was previously public but `doc(hidden)`. - The `xlib_xconnection` function has been removed. This function was previously `doc(hidden)`. - The `xlib_window` function has been renamed to `x11_window` and now returns `u32` instead of `c_long`. - The `xlib_screen_id` function has been renamed to `x11_screen_id` and now returns `u32` instead of `c_long`. - `WindowBuilder` now accepts a `u32` screen id instead of `c_long`. - `WindowBuilder` previously accepted an untyped pointer specifying an Xlib visual and unsoundly converted it internally to an Xlib visual pointer. It now accepts a new type that allows the user to specify the desired visual id and depth. - The backend now has a hard dependency on libxcb and associated libraries. libxcb is always available as it is a dependency of libx11 but some distributions (notably Debian) split associated libraries such as libxcb-xinput into separate packages that the user might have to install. # New and Changed Features - If the `xlib` feature is enabled, the new `xlib_display` function returns the Xlib connection unless this connection was disabled via the `WINIT_DISABLE_XLIB` environment variable. - The new `xcb_connection` function returns the XCB connection. - The backend now supports multiple screens throughout. It was already possible for the user to specify a screen to use via `WindowBuilder` but in many places the backend always used the default screen. - Many places that previously panicked now simply log an error. - Error messages are much improved.
This patch adds an extension method that allows the user to retrieve the XInput device id.
This patch fixes unminimizing via `set_minimized`. Previously it would ask the window manager to give focus to our window. But the window manager is free to ignore this message and to not even map our window. The documented way to exit minimized state is to simply map the window.
|
The tricky thing with porting from xlib to xcb is that xlib does more than just wrap the wire protocol, it also includes various utilities, with the most notable here maybe being the XFilterEvent mechanism that supports IME integration. It doesn't seem ideal that this series ends up removing IME support. Since xlib is based on xcb and an xcb connection can be got from xlib I would hope that xcb usage could be introduced incrementally where it's particularly beneficial, e.g. to help avoid synchronous round trips. |
|
It looks like there's loads of really good work buried in this series - though it's a bit surprising how long of a series of changes this is stacked on top of. The first commit in this series is apparently two years old :/ I was looking at some small changes in the x11 backend and now I'm worried about how much of a huge backlog of x11 changes there are for winit. |
IME is broken on X11 with xlib anyway. And the only way to solve it is to move to xcb. For more see https://gitlab.freedesktop.org/xorg/lib/libx11/-/merge_requests/127 . I'm not sure there's a way to get it MR any further, so moving away from xlib is the only solution for this particular problem. If we want to solve IME problem with xcb there's https://crates.io/crates/xcb-imdkit and upstream C library is maintained by fctix developers, so it's kind of battle tested. |
Like I said in the OP, for this PR only the last two commits are relevant. The remaining commits are part of the linked PR. |
Yeah, although I'd seen that this issue was only focused on the final patches, I still initially interpreted "This patch is based on my work in #1932" to mean that this series was a super set of #1932, so when I looked over the series of commits here I was under the impression it was the same series under #1932. Taking another look at the commits for #1932 I do see now that some things like XInput2 enabling are now being done by @maroider so I probably got the wrong impression about what work is outstanding from looking at the commits here. Is there another issue where you've maybe discussed more about upstreaming the XCB port - I couldn't find anything, so this was the only place I'd seen that work. I just found this old issue which didn't have any link to your work: #5 |
Yeah, this makes sense. Tbh, when I was first skimming the patch series it just stood out that the series was removing IME support and I hadn't initially seen much discussion of that so I was concerned this change might potentially be for the wrong reasons (just to remove xlib for the sake of it even if it regresses features). I'd also recently come across PR #2182 which gave me an initial impression their might be users actively using XIM based IMEs with winit considering going to the effort to fix issues with it. Actually though it looks like it might be more the opposite - they are trying to workaround the way in which key repeats are synthesized with im=ibus, and arguably that problem would also be solved by removing the XIM support from the current backend. |
|
@mahkoh Do you have plans to rebase this work on the new XCB-based |
mahkoh
requested review from
msiglreith,
kchibisov and
notgull
as code owners
Note: This patch is based on my work in #1932 and cannot currently be merged. Please ignore all but the
lasttwo last commit. That is, ignore everything outside theitdirectory.This patch adds integration tests than run as a github action. Currently only the X11 backend is being tested. It should not be hard to add support for the wayland backend.
You can see them in action here: https://github.com/mahkoh/winit/runs/4417323203