From 0469716fd6cbe22916b1f7a0cc8193f6025e8405 Mon Sep 17 00:00:00 2001 From: Michael Stapelberg Date: Mon, 12 Sep 2016 12:05:38 +0200 Subject: [PATCH] Bugfix: compare all resolved modifier masks Before this commit, i3 only compared the user-specified modifiers and incorrectly ignored the resolved modifiers (such as the numlock fallback). While at it, also fix the testcase which treated numlock as a momentary modifier, whereas it really is a latched modifier. fixes #2418 --- src/bindings.c | 59 +++++++++++++++++------------- testcases/t/264-keypress-numlock.t | 46 +++++++++++++++++++---- 2 files changed, 71 insertions(+), 34 deletions(-) diff --git a/src/bindings.c b/src/bindings.c index 9bac68d6..db88aee3 100644 --- a/src/bindings.c +++ b/src/bindings.c @@ -188,6 +188,17 @@ void regrab_all_buttons(xcb_connection_t *conn) { xcb_ungrab_server(conn); } +static bool modifiers_match(const uint32_t modifiers_mask, const uint32_t modifiers_state) { + /* modifiers_mask is a special case: a value of 0 does not mean “match + * all”, but rather “match exactly when no modifiers are present”. */ + if (modifiers_mask == 0) { + /* Verify no modifiers are pressed. A bitwise AND would lead to + * false positives, see issue #2002. */ + return (modifiers_state == 0); + } + return ((modifiers_state & modifiers_mask) == modifiers_mask); +} + /* * Returns a pointer to the Binding with the specified modifiers and * keycode or NULL if no such binding exists. @@ -210,34 +221,15 @@ static Binding *get_binding(i3_event_state_mask_t state_filtered, bool is_releas const uint32_t xkb_group_state = (state_filtered & 0xFFFF0000); const uint32_t modifiers_state = (state_filtered & 0x0000FFFF); TAILQ_FOREACH(bind, bindings, bindings) { - const uint32_t xkb_group_mask = (bind->event_state_mask & 0xFFFF0000); - /* modifiers_mask is a special case: a value of 0 does not mean “match all”, - * but rather “match exactly when no modifiers are present”. */ - const uint32_t modifiers_mask = (bind->event_state_mask & 0x0000FFFF); - const bool groups_match = ((xkb_group_state & xkb_group_mask) == xkb_group_mask); - bool mods_match; - if (modifiers_mask == 0) { - /* Verify no modifiers are pressed. A bitwise AND would lead to - * false positives, see issue #2002. */ - mods_match = (modifiers_state == 0); - } else { - mods_match = ((modifiers_state & modifiers_mask) == modifiers_mask); - } - const bool state_matches = (groups_match && mods_match); - - DLOG("binding groups_match = %s, mods_match = %s, state_matches = %s\n", - (groups_match ? "yes" : "no"), - (mods_match ? "yes" : "no"), - (state_matches ? "yes" : "no")); - /* First compare the state_filtered (unless this is a - * B_UPON_KEYRELEASE_IGNORE_MODS binding and this is a KeyRelease - * event) */ if (bind->input_type != input_type) continue; - if (!state_matches && - (bind->release != B_UPON_KEYRELEASE_IGNORE_MODS || - !is_release)) + + const uint32_t xkb_group_mask = (bind->event_state_mask & 0xFFFF0000); + const bool groups_match = ((xkb_group_state & xkb_group_mask) == xkb_group_mask); + if (!groups_match) { + DLOG("skipping binding %p because XKB groups do not match\n", bind); continue; + } /* For keyboard bindings where a symbol was specified by the user, we * need to look in the array of translated keycodes for the event’s @@ -247,7 +239,11 @@ static Binding *get_binding(i3_event_state_mask_t state_filtered, bool is_releas bool found_keycode = false; struct Binding_Keycode *binding_keycode; TAILQ_FOREACH(binding_keycode, &(bind->keycodes_head), keycodes) { - if (binding_keycode->keycode == input_keycode) { + const uint32_t modifiers_mask = (binding_keycode->modifiers & 0x0000FFFF); + const bool mods_match = modifiers_match(modifiers_mask, modifiers_state); + DLOG("binding_keycode->modifiers = %d, modifiers_mask = %d, modifiers_state = %d, mods_match = %s\n", + binding_keycode->modifiers, modifiers_mask, modifiers_state, (mods_match ? "yes" : "no")); + if (binding_keycode->keycode == input_keycode && mods_match) { found_keycode = true; break; } @@ -255,6 +251,17 @@ static Binding *get_binding(i3_event_state_mask_t state_filtered, bool is_releas if (!found_keycode) continue; } else { + const uint32_t modifiers_mask = (bind->event_state_mask & 0x0000FFFF); + const bool mods_match = modifiers_match(modifiers_mask, modifiers_state); + DLOG("binding mods_match = %s\n", (mods_match ? "yes" : "no")); + /* First compare the state_filtered (unless this is a + * B_UPON_KEYRELEASE_IGNORE_MODS binding and this is a KeyRelease + * event) */ + if (!mods_match && + (bind->release != B_UPON_KEYRELEASE_IGNORE_MODS || + !is_release)) + continue; + /* This case is easier: The user specified a keycode */ if (bind->keycode != input_code) continue; diff --git a/testcases/t/264-keypress-numlock.t b/testcases/t/264-keypress-numlock.t index 84db0e3c..4f255b78 100644 --- a/testcases/t/264-keypress-numlock.t +++ b/testcases/t/264-keypress-numlock.t @@ -35,6 +35,9 @@ bindsym KP_End nop KP_End # Binding which should work with numlock and without. bindsym Mod4+a nop a + +# Binding which should work with numlock and without, see issue #2418. +bindsym Escape nop Escape EOT my $pid = launch_with_config($config); @@ -52,10 +55,12 @@ is(listen_for_binding( is(listen_for_binding( sub { - xtest_key_press(77); # Num_Lock + xtest_key_press(77); # enable Num_Lock + xtest_key_release(77); # enable Num_Lock xtest_key_press(87); # KP_1 xtest_key_release(87); # KP_1 - xtest_key_release(77); # Num_Lock + xtest_key_press(77); # disable Num_Lock + xtest_key_release(77); # disable Num_Lock }, ), 'KP_1', @@ -74,20 +79,43 @@ is(listen_for_binding( is(listen_for_binding( sub { - xtest_key_press(77); # Num_Lock + xtest_key_press(77); # enable Num_Lock + xtest_key_release(77); # enable Num_Lock xtest_key_press(133); # Super_L xtest_key_press(38); # a xtest_key_release(38); # a xtest_key_release(133); # Super_L - xtest_key_release(77); # Num_Lock + xtest_key_press(77); # disable Num_Lock + xtest_key_release(77); # disable Num_Lock }, ), 'a', 'triggered the "a" keybinding'); +is(listen_for_binding( + sub { + xtest_key_press(9); # Escape + xtest_key_release(9); # Escape + }, + ), + 'Escape', + 'triggered the "Escape" keybinding'); + +is(listen_for_binding( + sub { + xtest_key_press(77); # enable Num_Lock + xtest_key_release(77); # enable Num_Lock + xtest_key_press(9); # Escape + xtest_key_release(9); # Escape + xtest_key_press(77); # disable Num_Lock + xtest_key_release(77); # disable Num_Lock + }, + ), + 'Escape', + 'triggered the "Escape" keybinding'); sync_with_i3; -is(scalar @i3test::XTEST::binding_events, 4, 'Received exactly 4 binding events'); +is(scalar @i3test::XTEST::binding_events, 6, 'Received exactly 4 binding events'); exit_gracefully($pid); @@ -117,10 +145,12 @@ is(listen_for_binding( is(listen_for_binding( sub { - xtest_key_press(77); # Num_Lock + xtest_key_press(77); # enable Num_Lock + xtest_key_release(77); # enable Num_Lock xtest_key_press(87); # KP_1 xtest_key_release(87); # KP_1 - xtest_key_release(77); # Num_Lock + xtest_key_press(77); # disable Num_Lock + xtest_key_release(77); # disable Num_Lock }, ), 'timeout', @@ -129,7 +159,7 @@ is(listen_for_binding( # TODO: This test does not verify that i3 does _NOT_ grab keycode 87 with Mod2. sync_with_i3; -is(scalar @i3test::XTEST::binding_events, 5, 'Received exactly 5 binding events'); +is(scalar @i3test::XTEST::binding_events, 7, 'Received exactly 5 binding events'); exit_gracefully($pid);