GNOME Bugzilla – Bug 786408
Avoid overlapping keybinding when multiple layouts are configured
Last modified: 2017-08-21 13:27:19 UTC
The keysym -> keycodes lookup iterated through all of the layout groups of a keymap to fetch all the keycodes a keybinding would trigger. If the same keymap has both qwerty and dvorak, this will break various bindings. For example, what made me discover this was that 'switch-monitors' (bound to Super+p) and 'lock-screensaver' (bound to Super+l) overlapped, causing Super+l to trigger 'switch-monitors' instead of 'lock-screensaver'. This happened because the Super+p (XKB_KEY_p) was resolved to keycode 33 (KEY_P+8) on qwerty, but also 27 (KEY_R+8) on dvorak, while Super+l (XKB_KEY_l) was resolved to keycode 46 (KEY_L+8) on qwerty and 33 (KEY_P+8) on dvorak. Both bindings had both keycodes added to the lookup table, causing the overlap. In my case, the 'switch-monitors' took precedence in the key_bindings_index table. In other words, when the 'lock-screensaver' keybinding was pressed (Super+l) when the dvorak layout was enabled, the modifier super and the keycode KEY_P+8 was used to look up the keybinding, which fetched the Super+p keybinding ('switch-monitors'). These patches avoids this issue by keeping track of the current locked layout group (thus the layout used) and only resolves keycodes on that layout.
Created attachment 357778 [details] [review] clutter/evdev: Add API to get current layout index We set the layout index when changing keyboard layout, but have no way to get it back would so be needed. Add that.
Created attachment 357779 [details] [review] backend: Add API to get layout group Add API to get the layout group (layout index) currently active. In the native backend this is done by fetching the state directly from the evdev backend; on X11 this works by listening for XkbStateNotify events, caching the layout group value.
Created attachment 357780 [details] [review] backend/x11: Notify whenever the layout group changes Will be used to trigger keyboard binding rebuild.
Created attachment 357781 [details] [review] keybindings: Only resolve keysyms for the current layout group When resolving what keycodes a key binding resolves to, only look up key codes from the current layout group. Without this, unwanted overlaps may occur. For example when a keymap has both a dvorak and a qwerty layout on different layout groups, one keybinding may be bound on multiple keys, arbitrarily "shadowing" another.
Review of attachment 357781 [details] [review]: looks fine ::: src/core/keybindings.c @@ +4235,3 @@ G_CALLBACK (on_keymap_changed), display); + g_signal_connect (backend, "keymap-layout-group-changed", + G_CALLBACK (on_keymap_layout_group_changed), display); nit: could connect_swapped to the same handler
Review of attachment 357780 [details] [review]: lgtm ::: src/backends/x11/meta-backend-x11.c @@ +301,3 @@ + if (layout_group_changed) + meta_backend_notify_keymap_layout_group_changed (backend, + layout_group); the native backend always emits the signal even if the layout index is the same, perhaps it should be changed too
Review of attachment 357779 [details] [review]: sure ::: src/backends/x11/meta-backend-x11.c @@ +293,3 @@ break; + case XkbStateNotify: + layout_group = xkb_ev->state.locked_group; could further check if (xkb_ev->state.changed & XkbGroupLockMask) but not a real difference here
Review of attachment 357778 [details] [review]: ++
Created attachment 358050 [details] [review] monitor-manager-xrandr: Use G_DECLARE_FINAL_TYPE macro Declare the MetaMonitorManagerXrandr type using the helper macro, instead of manually.
Created attachment 358051 [details] [review] x11: Open backend X11 connection ourself Don't wait for clutter to initialize for connecting to X11; do it when constructing the backend instance. This way we can later depend on having an X11 connection earlier during initialization.
Those two patches are required, but were part of bug 785381 but I split them out and put them here, so that I can potentially land this before bug 785381.
(In reply to Rui Matos from comment #7) > Review of attachment 357779 [details] [review] [review]: > > sure > > ::: src/backends/x11/meta-backend-x11.c > @@ +293,3 @@ > break; > + case XkbStateNotify: > + layout_group = xkb_ev->state.locked_group; > > could further check if (xkb_ev->state.changed & XkbGroupLockMask) but not a > real difference here Seems like a good idea none the less. (In reply to Rui Matos from comment #6) > Review of attachment 357780 [details] [review] [review]: > > lgtm > > ::: src/backends/x11/meta-backend-x11.c > @@ +301,3 @@ > + if (layout_group_changed) > + meta_backend_notify_keymap_layout_group_changed (backend, > + > layout_group); > > the native backend always emits the signal even if the layout index is the > same, perhaps it should be changed too Makes more sense to stop emitting the event on non-changes for the native backend, now that we have a getter for that state. I put a patch doing exactly that after clutter API introduction.
Review of attachment 358050 [details] [review]: ++
Review of attachment 358051 [details] [review]: looks fine
Attachment 357778 [details] pushed as 5685449 - clutter/evdev: Add API to get current layout index Attachment 357779 [details] pushed as 33f1706 - backend: Add API to get layout group Attachment 357780 [details] pushed as a81d4ae - backend/x11: Notify whenever the layout group changes Attachment 357781 [details] pushed as 92a53f0 - keybindings: Only resolve keysyms for the current layout group Attachment 358050 [details] pushed as f950380 - monitor-manager-xrandr: Use G_DECLARE_FINAL_TYPE macro Attachment 358051 [details] pushed as a6d67b1 - x11: Open backend X11 connection ourself