keyboard: avoid stuck keys due to keybindings (alternate approach)

Before commit e77330bc3fe7, there were issues with keys becoming "stuck"
if other keys were pressed at the time a keybinding was matched, because
those other keys were included in the "bound" set and the release events
were incorrectly eaten by labwc.

Commit e77330bc3fe7 solved that issue with the "big hammer" approach of
preventing keybindings from working at all if other keys were pressed:

        if (key_state_nr_pressed_keys() > 1) {
                return false;
        }

This is an alternate approach to solving the original problem, by (1)
not including those other keys in the "bound" set and (2) making sure we
always forward release events for un-bound keys to clients (even if a
menu or OSD is displayed).

Details:

- Since we only ever want to store the single matched keycode as bound,
  key_state_store_pressed_keys_as_bound() doesn't really make sense in
  the plural, so rename it to key_state_store_pressed_key_as_bound() and
  pass in the keycode.

- The calls to key_state_store_pressed_keys_as_bound() within
  handle_keybinding() appear to be redundant since it is also called
  from the parent function (handle_compositor_keybindings()). So remove
  these calls.

- Finally, rework the logic for handling key-release events so that we
  always forward release events for keys not in the "bound" set.

This PR does not remove the "key_state_nr_pressed_keys() > 1" check, and
because of that should not result in any functional change. It should
however make it possible to relax or remove that check in future.

diff --git a/include/input/key-state.h b/include/input/key-state.h
index 71a3eb63f7481f1d96e0c7a6d128d51c61d62d3a..1fd83523d4a3a70b9c2da9204957f7009b44190f 100644 (file)
--- a/include/input/key-state.h
+++ b/include/input/key-state.h
@@ -2,6 +2,9 @@
 #ifndef LABWC_KEY_STATE_H
 #define LABWC_KEY_STATE_H
 
+#include <stdbool.h>
+#include <stdint.h>
+
 /*
  * All keycodes in these functions are (Linux) libinput evdev scancodes which is
  * what 'wlr_keyboard' uses (e.g. 'seat->keyboard_group->keyboard->keycodes').
@@ -17,7 +20,7 @@ uint32_t *key_state_pressed_sent_keycodes(void);
 int key_state_nr_pressed_sent_keycodes(void);
 
 void key_state_set_pressed(uint32_t keycode, bool ispressed);
-void key_state_store_pressed_keys_as_bound(void);
+void key_state_store_pressed_key_as_bound(uint32_t keycode);
 bool key_state_corresponding_press_event_was_bound(uint32_t keycode);
 void key_state_bound_key_remove(uint32_t keycode);
 int key_state_nr_bound_keys(void);

diff --git a/src/input/key-state.c b/src/input/key-state.c
index fad8e3b1c5900e2d712c8a1656c948dfc83fd508..193022a7eb27052147e0e22100eb7595aa7b218c 100644 (file)
--- a/src/input/key-state.c
+++ b/src/input/key-state.c
@@ -79,10 +79,9 @@ key_state_set_pressed(uint32_t keycode, bool ispressed)
 }
 
 void
-key_state_store_pressed_keys_as_bound(void)
+key_state_store_pressed_key_as_bound(uint32_t keycode)
 {
-       memcpy(bound.keys, pressed.keys, MAX_PRESSED_KEYS * sizeof(uint32_t));
-       bound.nr_keys = pressed.nr_keys;
+       add_key(&bound, keycode);
 }
 
 bool

diff --git a/src/input/keyboard.c b/src/input/keyboard.c
index 69e5fa4a0e6cc0ca03fb8e75ee6d69fbe5ef0d86..b99555d66e28793602a5cf279e7552e3f504d358 100644 (file)
--- a/src/input/keyboard.c
+++ b/src/input/keyboard.c
@@ -105,7 +105,6 @@ handle_keybinding(struct server *server, uint32_t modifiers, xkb_keysym_t sym, x
                        /* Use keycodes */
                        for (size_t i = 0; i < keybind->keycodes_len; i++) {
                                if (keybind->keycodes[i] == code) {
-                                       key_state_store_pressed_keys_as_bound();
                                        actions_run(NULL, server, &keybind->actions, 0);
                                        return true;
                                }
@@ -114,7 +113,6 @@ handle_keybinding(struct server *server, uint32_t modifiers, xkb_keysym_t sym, x
                        /* Use syms */
                        for (size_t i = 0; i < keybind->keysyms_len; i++) {
                                if (xkb_keysym_to_lower(sym) == keybind->keysyms[i]) {
-                                       key_state_store_pressed_keys_as_bound();
                                        actions_run(NULL, server, &keybind->actions, 0);
                                        return true;
                                }
@@ -211,8 +209,6 @@ handle_compositor_keybindings(struct keyboard *keyboard,
        raw.nr_syms = xkb_keymap_key_get_syms_by_level(wlr_keyboard->keymap,
                keycode, layout_index, 0, &raw.syms);
 
-       bool handled = false;
-
        /*
         * keyboard_key_notify() is called before keyboard_key_modifier(), so
         * 'modifiers' refers to modifiers that were pressed before the key
@@ -254,31 +250,45 @@ handle_compositor_keybindings(struct keyboard *keyboard,
                key_state_set_pressed(event->keycode, true);
        }
 
-       /*
-        * If a user lets go of the modifier (e.g. alt) before the 'normal' key
-        * (e.g. tab) when window-cycling, we do not end the cycling until both
-        * keys have been released.  If we end the window-cycling on release of
-        * the modifier only, some XWayland clients such as hexchat realise that
-        * tab is pressed (even though we did not forward the event) and because
-        * we absorb the equivalent release event it gets stuck on repeat.
-        */
-       if (should_cancel_cycling_on_next_key_release
-                       && event->state == WL_KEYBOARD_KEY_STATE_RELEASED) {
-               end_cycling(server);
-               handled = true;
-               goto out;
-       }
+       if (event->state == WL_KEYBOARD_KEY_STATE_RELEASED) {
+               /*
+                * Release events for keys that were not bound should
+                * always be forwarded to clients to avoid stuck keys.
+                */
+               if (!key_state_corresponding_press_event_was_bound(
+                               event->keycode)) {
+                       return false;
+               }
 
-       /*
-        * If a press event was handled by a compositor binding, then do not
-        * forward the corresponding release event to clients
-        */
-       if (key_state_corresponding_press_event_was_bound(event->keycode)
-                       && event->state == WL_KEYBOARD_KEY_STATE_RELEASED) {
+               /*
+                * If a user lets go of the modifier (e.g. alt) before
+                * the 'normal' key (e.g. tab) when window-cycling, we
+                * do not end the cycling until both keys have been
+                * released. If we end the window-cycling on release of
+                * the modifier only, some XWayland clients such as
+                * hexchat realise that tab is pressed (even though we
+                * did not forward the event) and because we absorb the
+                * equivalent release event it gets stuck on repeat.
+                */
+               if (should_cancel_cycling_on_next_key_release) {
+                       end_cycling(server);
+               }
+
+               /*
+                * If a press event was handled by a compositor binding,
+                * then do not forward the corresponding release event
+                * to clients.
+                */
                key_state_bound_key_remove(event->keycode);
                return true;
        }
 
+       /*
+        * Note: checks of event->state == WL_KEYBOARD_KEY_STATE_PRESSED
+        * below are redundant, but are left in place for now.
+        */
+       bool handled = false;
+
        /* Catch C-A-F1 to C-A-F12 to change tty */
        if (event->state == WL_KEYBOARD_KEY_STATE_PRESSED) {
                for (int i = 0; i < translated.nr_syms; i++) {
@@ -308,10 +318,6 @@ handle_compositor_keybindings(struct keyboard *keyboard,
        }
 
        if (server->input_mode == LAB_INPUT_STATE_MENU) {
-               /*
-                * Usually, release events are already caught via _press_event_was_bound().
-                * But to be on the safe side we will simply ignore them here as well.
-                */
                if (event->state == WL_KEYBOARD_KEY_STATE_PRESSED) {
                        handle_menu_keys(server, &translated);
                }
@@ -423,7 +429,7 @@ handle_compositor_keybindings(struct keyboard *keyboard,
 
 out:
        if (handled) {
-               key_state_store_pressed_keys_as_bound();
+               key_state_store_pressed_key_as_bound(event->keycode);
        }
 
        return handled;