From 149c52bd4496eb4dd13eaa53bec690c60499502a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20Ekl=C3=B6f?= Date: Tue, 29 Jun 2021 18:08:15 +0200 Subject: [PATCH] =?UTF-8?q?config:=20remove=20replaced/removed=20key=20bin?= =?UTF-8?q?dings,=20instead=20of=20marking=20as=20=E2=80=98unused=E2=80=99?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Instead of keeping removed/replaced key bindings in the key binding array (marked as ‘unused’), remove them, by compacting the array. The invariant is thus that there should be *no* entries in the key binding list with the `BIND_ACTION_NONE` for action. Add code to debug builds that verifies this, plus a unit test. Closes #614 --- CHANGELOG.md | 2 + config.c | 158 ++++++++++++++++++++++++++++++++++++++++++--------- input.c | 6 -- 3 files changed, 134 insertions(+), 32 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a44d8105..78016bae 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,6 +43,8 @@ ### Fixed * Grapheme cluster state being reset between codepoints. +* Regression: custom URL key bindings not working + (https://codeberg.org/dnkl/foot/issues/614). ### Security diff --git a/config.c b/config.c index c417fb19..2fa9100e 100644 --- a/config.c +++ b/config.c @@ -1662,6 +1662,43 @@ pipe_argv_from_string(const char *value, char ***argv, return remove_len; } +static void +remove_action_from_key_bindings_list(struct config_key_binding_list *bindings, + int action, char **pipe_argv) +{ + size_t remove_first_idx = 0; + size_t remove_count = 0; + + for (size_t i = 0; i < bindings->count; i++) { + struct config_key_binding *binding = &bindings->arr[i]; + + if (binding->action == action && + ((binding->pipe.argv.args == NULL && pipe_argv == NULL) || + (binding->pipe.argv.args != NULL && pipe_argv != NULL && + argv_compare(binding->pipe.argv.args, pipe_argv) == 0))) + { + if (remove_count++ == 0) + remove_first_idx = i; + + xassert(remove_first_idx + remove_count - 1 == i); + + if (binding->pipe.master_copy) + free_argv(&binding->pipe.argv); + } + } + + if (remove_count == 0) + return; + + size_t move_count = bindings->count - (remove_first_idx + remove_count); + + memmove( + &bindings->arr[remove_first_idx], + &bindings->arr[remove_first_idx + remove_count], + move_count * sizeof(bindings->arr[0])); + bindings->count -= remove_count; +} + static bool NOINLINE parse_key_binding_section( const char *section, const char *key, const char *value, @@ -1688,17 +1725,7 @@ parse_key_binding_section( /* Unset binding */ if (strcasecmp(value, "none") == 0) { - for (size_t i = 0; i < bindings->count; i++) { - struct config_key_binding *binding = &bindings->arr[i]; - - if (binding->action != action) - continue; - - if (binding->pipe.master_copy) - free_argv(&binding->pipe.argv); - binding->action = BIND_ACTION_NONE; - } - + remove_action_from_key_bindings_list(bindings, action, pipe_argv); free(pipe_argv); return true; } @@ -1715,21 +1742,7 @@ parse_key_binding_section( return false; } - /* Remove existing bindings for this action+pipe */ - for (size_t i = 0; i < bindings->count; i++) { - struct config_key_binding *binding = &bindings->arr[i]; - - if (binding->action == action && - ((binding->pipe.argv.args == NULL && pipe_argv == NULL) || - (binding->pipe.argv.args != NULL && pipe_argv != NULL && - argv_compare(binding->pipe.argv.args, pipe_argv) == 0))) - { - - if (binding->pipe.master_copy) - free_argv(&binding->pipe.argv); - binding->action = BIND_ACTION_NONE; - } - } + remove_action_from_key_bindings_list(bindings, action, pipe_argv); /* Emit key bindings */ size_t ofs = bindings->count; @@ -1767,6 +1780,90 @@ parse_key_binding_section( return false; } +UNITTEST +{ + enum test_actions { + TEST_ACTION_NONE, + TEST_ACTION_FOO, + TEST_ACTION_BAR, + TEST_ACTION_COUNT, + }; + + const char *const map[] = { + [TEST_ACTION_NONE] = NULL, + [TEST_ACTION_FOO] = "foo", + [TEST_ACTION_BAR] = "bar", + }; + + struct config conf = {0}; + struct config_key_binding_list bindings = {0}; + + /* + * ADD foo=Escape + * + * This verifies we can bind a single key combo to an action. + */ + xassert(parse_key_binding_section( + "", "foo", "Escape", ALEN(map), map, &bindings, &conf, "", 0)); + xassert(bindings.count == 1); + xassert(bindings.arr[0].action == TEST_ACTION_FOO); + xassert(bindings.arr[0].sym == XKB_KEY_Escape); + + /* + * ADD bar=Control+g Control+Shift+x + * + * This verifies we can bind multiple key combos to an action. + */ + xassert(parse_key_binding_section( + "", "bar", "Control+g Control+Shift+x", ALEN(map), map, + &bindings, &conf, "", 0)); + xassert(bindings.count == 3); + xassert(bindings.arr[0].action == TEST_ACTION_FOO); + xassert(bindings.arr[1].action == TEST_ACTION_BAR); + xassert(bindings.arr[1].sym == XKB_KEY_g); + xassert(bindings.arr[1].modifiers.ctrl); + xassert(bindings.arr[2].action == TEST_ACTION_BAR); + xassert(bindings.arr[2].sym == XKB_KEY_x); + xassert(bindings.arr[2].modifiers.ctrl && bindings.arr[2].modifiers.shift); + + /* + * REPLACE foo with foo=Mod+v Shift+q + * + * This verifies we can update a single-combo action with multiple + * key combos. + */ + xassert(parse_key_binding_section( + "", "foo", "Mod1+v Shift+q", ALEN(map), map, + &bindings, &conf, "", 0)); + xassert(bindings.count == 4); + xassert(bindings.arr[0].action == TEST_ACTION_BAR); + xassert(bindings.arr[1].action == TEST_ACTION_BAR); + xassert(bindings.arr[2].action == TEST_ACTION_FOO); + xassert(bindings.arr[2].sym == XKB_KEY_v); + xassert(bindings.arr[2].modifiers.alt); + xassert(bindings.arr[3].action == TEST_ACTION_FOO); + xassert(bindings.arr[3].sym == XKB_KEY_q); + xassert(bindings.arr[3].modifiers.shift); + + /* + * REMOVE bar + */ + xassert(parse_key_binding_section( + "", "bar", "none", ALEN(map), map, &bindings, &conf, "", 0)); + xassert(bindings.count == 2); + xassert(bindings.arr[0].action == TEST_ACTION_FOO); + xassert(bindings.arr[1].action == TEST_ACTION_FOO); + + /* + * REMOVE foo + */ + xassert(parse_key_binding_section( + "", "foo", "none", ALEN(map), map, &bindings, &conf, "", 0)); + xassert(bindings.count == 0); + + free(bindings.arr); +} + static bool parse_section_key_bindings( const char *key, const char *value, struct config *conf, @@ -2852,6 +2949,15 @@ out: } } +#if defined(_DEBUG) + for (size_t i = 0; i < conf->bindings.key.count; i++) + xassert(conf->bindings.key.arr[i].action != BIND_ACTION_NONE); + for (size_t i = 0; i < conf->bindings.search.count; i++) + xassert(conf->bindings.search.arr[i].action != BIND_ACTION_SEARCH_NONE); + for (size_t i = 0; i < conf->bindings.url.count; i++) + xassert(conf->bindings.url.arr[i].action != BIND_ACTION_URL_NONE); +#endif + free(conf_file.path); if (conf_file.fd >= 0) close(conf_file.fd); diff --git a/input.c b/input.c index 7db3ca85..9cf18b53 100644 --- a/input.c +++ b/input.c @@ -529,8 +529,6 @@ convert_key_bindings(const struct config *conf, struct seat *seat) { for (size_t i = 0; i < conf->bindings.key.count; i++) { const struct config_key_binding *binding = &conf->bindings.key.arr[i]; - if (binding->action == BIND_ACTION_NONE) - continue; convert_key_binding(seat, binding, &seat->kbd.bindings.key); } } @@ -540,8 +538,6 @@ convert_search_bindings(const struct config *conf, struct seat *seat) { for (size_t i = 0; i < conf->bindings.search.count; i++) { const struct config_key_binding *binding = &conf->bindings.search.arr[i]; - if (binding->action == BIND_ACTION_SEARCH_NONE) - continue; convert_key_binding(seat, binding, &seat->kbd.bindings.search); } } @@ -551,8 +547,6 @@ convert_url_bindings(const struct config *conf, struct seat *seat) { for (size_t i = 0; i < conf->bindings.url.count; i++) { const struct config_key_binding *binding = &conf->bindings.url.arr[i]; - if (binding->action == BIND_ACTION_URL_NONE) - continue; convert_key_binding(seat, binding, &seat->kbd.bindings.url); } }