Merge branch 'remove-unused-key-bindings'

Closes #614

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

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++) {
+            remove_action_from_key_bindings_list(bindings, action, pipe_argv);
-                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;
-            }
-
             free(pipe_argv);
             return true;
         }
@@ -1715,21 +1742,7 @@ parse_key_binding_section(
             return false;
         }
 
-        /* Remove existing bindings for this action+pipe */
+        remove_action_from_key_bindings_list(bindings, action, pipe_argv);
-        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;
-            }
-        }
 
         /* 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);

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);
     }
 }