keyboard: break up handle_compositor_keybindings()

This function has grown quite large over time. Breaking out various
smaller functions makes the logic easier to follow.

No functional change intended, but there is a minor logical change:

- Due to factoring out match_keybinding(), each keypress can only
  match a single keybinding now. Previously, it was theoretically
  possible for a single keypress to map to multiple keysyms which could
  each match a different keybinding.

src/input/keyboard.c

@@ -13,6 +13,19 @@
 #include "view.h"
 #include "workspaces.h"
 
+struct keysyms {
+	const xkb_keysym_t *syms;
+	int nr_syms;
+};
+
+struct keyinfo {
+	xkb_keycode_t xkb_keycode;
+	struct keysyms translated;
+	struct keysyms raw;
+	uint32_t modifiers;
+	bool is_modifier;
+};
+
 static bool should_cancel_cycling_on_next_key_release;
 
 static void
@@ -81,11 +94,10 @@ keyboard_modifiers_notify(struct wl_listener *listener, void *data)
 	wlr_seat_keyboard_notify_modifiers(seat->seat, &wlr_keyboard->modifiers);
 }
 
-static bool
+static struct keybind *
-handle_keybinding(struct server *server, uint32_t modifiers, xkb_keysym_t sym, xkb_keycode_t code)
+match_keybinding_for_sym(struct server *server, uint32_t modifiers,
+		xkb_keysym_t sym, xkb_keycode_t xkb_keycode)
 {
-	uint32_t evdev_scancode = code - 8;
-
 	struct keybind *keybind;
 	wl_list_for_each(keybind, &rc.keybinds, link) {
 		if (modifiers ^ keybind->modifiers) {
@@ -100,30 +112,79 @@ handle_keybinding(struct server *server, uint32_t modifiers, xkb_keysym_t sym, x
 		if (sym == XKB_KEY_NoSymbol) {
 			/* Use keycodes */
 			for (size_t i = 0; i < keybind->keycodes_len; i++) {
-				/*
+				if (keybind->keycodes[i] == xkb_keycode) {
-				 * Update key-state before action_run() because
+					return keybind;
-				 * the action might lead to seat_focus() in
-				 * which case we pass the 'pressed-sent' keys to
-				 * the new surface.
-				 */
-				if (keybind->keycodes[i] == code) {
-					key_state_store_pressed_key_as_bound(evdev_scancode);
-					actions_run(NULL, server, &keybind->actions, 0);
-					return true;
 				}
 			}
 		} else {
 			/* 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_key_as_bound(evdev_scancode);
+					return keybind;
-					actions_run(NULL, server, &keybind->actions, 0);
-					return true;
 				}
 			}
 		}
 	}
-	return false;
+	return NULL;
+}
+
+/*
+ * When matching against keybinds, we process the input keys in the
+ * following order of precedence:
+ *   a. Keycodes (of physical keys) (not if keybind is layoutDependent)
+ *   b. Translated keysyms (taking into account modifiers, so if Shift+1
+ *      were pressed on a us keyboard, the keysym would be '!')
+ *   c. Raw keysyms (ignoring modifiers such as shift, so in the above
+ *      example the keysym would just be '1')
+ *
+ * The reasons for this approach are:
+ *   1. To make keybinds keyboard-layout agnostic (by checking keycodes
+ *      before keysyms). This means that in a multi-layout situation,
+ *      keybinds work regardless of which layout is active at the time
+ *      of the key-press.
+ *   2. To support keybinds relating to keysyms that are only available
+ *      in a particular layout, for example å, ä and ö.
+ *   3. To support keybinds that are only valid with a modifier, for
+ *      example the numpad keys with NumLock enabled: KP_x. These would
+ *      only be matched by the translated keysyms.
+ *   4. To support keybinds such as `S-1` (by checking raw keysyms).
+ *
+ * Reason 4 will also be satisfied by matching the keycodes. However,
+ * when a keybind is configured to be layoutDependent we still need
+ * the raw keysym fallback.
+ */
+static struct keybind *
+match_keybinding(struct server *server, struct keyinfo *keyinfo)
+{
+	/* First try keycodes */
+	struct keybind *keybind = match_keybinding_for_sym(server,
+		keyinfo->modifiers, XKB_KEY_NoSymbol, keyinfo->xkb_keycode);
+	if (keybind) {
+		wlr_log(WLR_DEBUG, "keycode matched");
+		return keybind;
+	}
+
+	/* Then fall back to keysyms */
+	for (int i = 0; i < keyinfo->translated.nr_syms; i++) {
+		keybind = match_keybinding_for_sym(server, keyinfo->modifiers,
+			keyinfo->translated.syms[i], keyinfo->xkb_keycode);
+		if (keybind) {
+			wlr_log(WLR_DEBUG, "translated keysym matched");
+			return keybind;
+		}
+	}
+
+	/* And finally test for keysyms without modifier */
+	for (int i = 0; i < keyinfo->raw.nr_syms; i++) {
+		keybind = match_keybinding_for_sym(server, keyinfo->modifiers,
+			keyinfo->raw.syms[i], keyinfo->xkb_keycode);
+		if (keybind) {
+			wlr_log(WLR_DEBUG, "raw keysym matched");
+			return keybind;
+		}
+	}
+
+	return NULL;
 }
 
 static bool
@@ -145,10 +206,104 @@ is_modifier_key(xkb_keysym_t sym)
 	}
 }
 
-struct keysyms {
+static struct keyinfo
-	const xkb_keysym_t *syms;
+get_keyinfo(struct wlr_keyboard *wlr_keyboard, uint32_t evdev_keycode)
-	int nr_syms;
+{
-};
+	struct keyinfo keyinfo = {0};
+
+	/* Translate evdev/libinput keycode -> xkb */
+	keyinfo.xkb_keycode = evdev_keycode + 8;
+
+	/* Get a list of keysyms based on the keymap for this keyboard */
+	keyinfo.translated.nr_syms = xkb_state_key_get_syms(
+		wlr_keyboard->xkb_state, keyinfo.xkb_keycode,
+		&keyinfo.translated.syms);
+
+	/*
+	 * Get keysyms from the keyboard as if there was no modifier
+	 * translations. For example, get Shift+1 rather than Shift+!
+	 * (with US keyboard layout).
+	 */
+	xkb_layout_index_t layout_index = xkb_state_key_get_layout(
+		wlr_keyboard->xkb_state, keyinfo.xkb_keycode);
+	keyinfo.raw.nr_syms = xkb_keymap_key_get_syms_by_level(
+		wlr_keyboard->keymap, keyinfo.xkb_keycode, layout_index, 0,
+		&keyinfo.raw.syms);
+
+	/*
+	 * keyboard_key_notify() is called before keyboard_key_modifier(),
+	 * so 'modifiers' refers to modifiers that were pressed before the
+	 * key event in hand. Consequently, we use is_modifier_key() to
+	 * find out if the key event being processed is a modifier.
+	 *
+	 * Sway solves this differently by saving the 'modifiers' state
+	 * and checking if it has changed each time we get to the equivalent
+	 * of this function. If it has changed, it concludes that the last
+	 * key was a modifier and then deletes it from the buffer of pressed
+	 * keycodes. For us the equivalent would look something like this:
+	 *
+	 * static uint32_t last_modifiers;
+	 * bool last_key_was_a_modifier = last_modifiers != modifiers;
+	 * last_modifiers = modifiers;
+	 * if (last_key_was_a_modifier) {
+	 *	key_state_remove_last_pressed_key(last_pressed_keycode);
+	 * }
+	 */
+	keyinfo.modifiers = wlr_keyboard_get_modifiers(wlr_keyboard);
+	keyinfo.is_modifier = false;
+	for (int i = 0; i < keyinfo.translated.nr_syms; i++) {
+		keyinfo.is_modifier |=
+			is_modifier_key(keyinfo.translated.syms[i]);
+	}
+
+	return keyinfo;
+}
+
+static bool
+handle_key_release(struct server *server, uint32_t evdev_keycode)
+{
+	/*
+	 * 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(evdev_keycode)) {
+		return false;
+	}
+
+	/*
+	 * 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(evdev_keycode);
+	return true;
+}
+
+static bool
+handle_change_vt_key(struct server *server, struct keysyms *translated)
+{
+	for (int i = 0; i < translated->nr_syms; i++) {
+		unsigned int vt =
+			translated->syms[i] - XKB_KEY_XF86Switch_VT_1 + 1;
+		if (vt >= 1 && vt <= 12) {
+			change_vt(server, vt);
+			return true;
+		}
+	}
+	return false;
+}
 
 static void
 handle_menu_keys(struct server *server, struct keysyms *syms)
@@ -186,6 +341,30 @@ handle_menu_keys(struct server *server, struct keysyms *syms)
 	}
 }
 
+static void
+handle_cycle_view_key(struct server *server, struct keyinfo *keyinfo)
+{
+	for (int i = 0; i < keyinfo->translated.nr_syms; i++) {
+		if (keyinfo->translated.syms[i] == XKB_KEY_Escape) {
+			/* cancel view-cycle */
+			osd_preview_restore(server);
+			osd_finish(server);
+			return;
+		}
+	}
+
+	/* cycle to next */
+	bool backwards = keyinfo->modifiers & WLR_MODIFIER_SHIFT;
+	if (!keyinfo->is_modifier) {
+		enum lab_cycle_dir dir = backwards
+			? LAB_CYCLE_DIR_BACKWARD
+			: LAB_CYCLE_DIR_FORWARD;
+		server->osd_state.cycle_view = desktop_cycle_view(server,
+			server->osd_state.cycle_view, dir);
+		osd_update(server);
+	}
+}
+
 static bool
 handle_compositor_keybindings(struct keyboard *keyboard,
 		struct wlr_keyboard_key_event *event)
@@ -193,109 +372,20 @@ handle_compositor_keybindings(struct keyboard *keyboard,
 	struct seat *seat = keyboard->base.seat;
 	struct server *server = seat->server;
 	struct wlr_keyboard *wlr_keyboard = keyboard->wlr_keyboard;
-
+	struct keyinfo keyinfo = get_keyinfo(wlr_keyboard, event->keycode);
-	/* Translate libinput keycode -> xkbcommon */
-	uint32_t keycode = event->keycode + 8;
-
-	/* Get a list of keysyms based on the keymap for this keyboard */
-	struct keysyms translated = { 0 };
-	translated.nr_syms = xkb_state_key_get_syms(wlr_keyboard->xkb_state,
-		keycode, &translated.syms);
-
-	/*
-	 * Get keysyms from the keyboard as if there was no modifier
-	 * translations. For example, get Shift+1 rather than Shift+! (with US
-	 * keyboard layout).
-	 */
-	struct keysyms raw = { 0 };
-	xkb_layout_index_t layout_index =
-		xkb_state_key_get_layout(wlr_keyboard->xkb_state, keycode);
-	raw.nr_syms = xkb_keymap_key_get_syms_by_level(wlr_keyboard->keymap,
-		keycode, layout_index, 0, &raw.syms);
-
-	/*
-	 * keyboard_key_notify() is called before keyboard_key_modifier(), so
-	 * 'modifiers' refers to modifiers that were pressed before the key
-	 * event in hand. Consequently, we use is_modifier_key() to find out if
-	 * the key event being processed is a modifier.
-	 *
-	 * Sway solves this differently by saving the 'modifiers' state and
-	 * checking if it has changed each time we get to the equivalent of this
-	 * function. If it has changed, it concludes that the last key was a
-	 * modifier and then deletes it from the buffer of pressed keycodes.
-	 * For us the equivalent would look something like this:
-	 *
-	 * static uint32_t last_modifiers;
-	 * bool last_key_was_a_modifier = last_modifiers != modifiers;
-	 * last_modifiers = modifiers;
-	 * if (last_key_was_a_modifier) {
-	 *	key_state_remove_last_pressed_key(last_pressed_keycode);
-	 * }
-	 */
-
-	bool is_modifier = false;
-	uint32_t modifiers = wlr_keyboard_get_modifiers(wlr_keyboard);
-
-	for (int i = 0; i < translated.nr_syms; i++) {
-		is_modifier |= is_modifier_key(translated.syms[i]);
-	}
 
 	key_state_set_pressed(event->keycode,
-		event->state == WL_KEYBOARD_KEY_STATE_PRESSED, is_modifier);
+		event->state == WL_KEYBOARD_KEY_STATE_PRESSED,
+		keyinfo.is_modifier);
 
 	if (event->state == WL_KEYBOARD_KEY_STATE_RELEASED) {
-		/*
+		return handle_key_release(server, event->keycode);
-		 * 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 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) {
+	if (handle_change_vt_key(server, &keyinfo.translated)) {
-		for (int i = 0; i < translated.nr_syms; i++) {
+		key_state_store_pressed_key_as_bound(event->keycode);
-			unsigned int vt = translated.syms[i] - XKB_KEY_XF86Switch_VT_1 + 1;
+		return true;
-			if (vt >= 1 && vt <= 12) {
-				change_vt(server, vt);
-				/*
-				 * Don't send any key events to clients when
-				 * changing tty
-				 */
-				handled = true;
-				goto out;
-			}
-		}
 	}
 
 	/*
@@ -311,109 +401,33 @@ handle_compositor_keybindings(struct keyboard *keyboard,
 	}
 
 	if (server->input_mode == LAB_INPUT_STATE_MENU) {
-		if (event->state == WL_KEYBOARD_KEY_STATE_PRESSED) {
+		key_state_store_pressed_key_as_bound(event->keycode);
-			key_state_store_pressed_key_as_bound(event->keycode);
+		handle_menu_keys(server, &keyinfo.translated);
-			handle_menu_keys(server, &translated);
-		}
 		return true;
 	}
 
 	if (server->osd_state.cycle_view) {
-		if (event->state == WL_KEYBOARD_KEY_STATE_PRESSED) {
+		key_state_store_pressed_key_as_bound(event->keycode);
-			for (int i = 0; i < translated.nr_syms; i++) {
+		handle_cycle_view_key(server, &keyinfo);
-				if (translated.syms[i] == XKB_KEY_Escape) {
+		return true;
-					/*
-					 * Cancel view-cycle
-					 *
-					 * osd_finish() additionally resets
-					 * cycle_view to NULL
-					 */
-					osd_preview_restore(server);
-					osd_finish(server);
-
-					handled = true;
-					goto out;
-				}
-			}
-
-			/* cycle to next */
-			bool backwards = modifiers & WLR_MODIFIER_SHIFT;
-			if (!is_modifier) {
-				enum lab_cycle_dir dir = backwards
-					? LAB_CYCLE_DIR_BACKWARD
-					: LAB_CYCLE_DIR_FORWARD;
-				server->osd_state.cycle_view = desktop_cycle_view(server,
-					server->osd_state.cycle_view, dir);
-				osd_update(server);
-			}
-		}
-		/* don't send any key events to clients when osd onscreen */
-		handled = true;
-		goto out;
 	}
 
 	/*
 	 * Handle compositor keybinds
-	 *
-	 * When matching against keybinds, we process the input keys in the
-	 * following order of precedence:
-	 *   a. Keycodes (of physical keys) (not if keybind is layoutDependent)
-	 *   b. Translated keysyms (taking into account modifiers, so if Shift+1
-	 *      were pressed on a us keyboard, the keysym would be '!')
-	 *   c. Raw keysyms (ignoring modifiers such as shift, so in the above
-	 *      example the keysym would just be '1')
-	 *
-	 * The reasons for this approach are:
-	 *   1. To make keybinds keyboard-layout agnostic (by checking keycodes
-	 *      before keysyms). This means that in a multi-layout situation,
-	 *      keybinds work regardless of which layout is active at the time
-	 *      of the key-press.
-	 *   2. To support keybinds relating to keysyms that are only available
-	 *      in a particular layout, for example å, ä and ö.
-	 *   3. To support keybinds that are only valid with a modifier, for
-	 *      example the numpad keys with NumLock enabled: KP_x. These would
-	 *      only be matched by the translated keysyms.
-	 *   4. To support keybinds such as `S-1` (by checking raw keysyms).
-	 *
-	 * Reason 4 will also be satisfied by matching the keycodes. However,
-	 * when a keybind is configured to be layoutDependent we still need
-	 * the raw keysym fallback.
 	 */
-
+	struct keybind *keybind = match_keybinding(server, &keyinfo);
-	if (event->state == WL_KEYBOARD_KEY_STATE_PRESSED) {
+	if (keybind) {
-		/* First try keycodes */
+		/*
-		handled |= handle_keybinding(server, modifiers, XKB_KEY_NoSymbol, keycode);
+		 * Update key-state before action_run() because the action
-		if (handled) {
+		 * might lead to seat_focus() in which case we pass the
-			wlr_log(WLR_DEBUG, "keycodes matched");
+		 * 'pressed-sent' keys to the new surface.
-			return true;
+		 */
-		}
-
-		/* Then fall back to keysyms */
-		for (int i = 0; i < translated.nr_syms; i++) {
-			handled |= handle_keybinding(server, modifiers,
-				translated.syms[i], keycode);
-		}
-		if (handled) {
-			wlr_log(WLR_DEBUG, "translated keysyms matched");
-			return true;
-		}
-
-		/* And finally test for keysyms without modifier */
-		for (int i = 0; i < raw.nr_syms; i++) {
-			handled |= handle_keybinding(server, modifiers, raw.syms[i], keycode);
-		}
-		if (handled) {
-			wlr_log(WLR_DEBUG, "raw keysyms matched");
-			return true;
-		}
-	}
-
-out:
-	if (handled) {
 		key_state_store_pressed_key_as_bound(event->keycode);
+		actions_run(NULL, server, &keybind->actions, 0);
+		return true;
 	}
 
-	return handled;
+	return false;
 }
 
 static int