From 9a5a2d9957d8421b815d8ab07e867e73112b9591 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Daniel=20Ekl=C3=B6f?= <daniel@ekloef.se>
Date: Mon, 27 Feb 2023 17:56:03 +0100
Subject: [PATCH] key-binding: sort binding lists
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Sort bindings such that bindings with the same symbol are sorted with
the binding having the most modifiers comes first.

This fixes an issue where the “wrong” key binding are triggered when
used with “consumed” modifiers.

For example: if Control+BackSpace is bound before
Control+Shift+BackSpace, then the latter binding is never triggered.

Why? Because Shift is a consumed modifier. This means
Control+BackSpace is “the same” as Control+Shift+BackSpace.

By sorting bindings with more modifiers first, we work around the
problem. But note that it is *just* a workaround, and I’m not
confident there aren’t cases where it doesn’t work.

Closes #1280
---
 CHANGELOG.md  |  3 +++
 key-binding.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 4fc80821..40072182 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -126,6 +126,8 @@
 * Wayland protocol violation when ack:ing a configure event for an
   unmapped surface ([#1249][1249]).
 * `xdg\_toplevel::set_min_size()` not being called.
+* Key bindings with consumed modifiers masking other key bindings
+  ([#1280][1280]).
 
 [1173]: https://codeberg.org/dnkl/foot/issues/1173
 [1190]: https://codeberg.org/dnkl/foot/issues/1190
@@ -135,6 +137,7 @@
 [1259]: https://codeberg.org/dnkl/foot/issues/1259
 [1256]: https://codeberg.org/dnkl/foot/issues/1256
 [1249]: https://codeberg.org/dnkl/foot/issues/1249
+[1280]: https://codeberg.org/dnkl/foot/issues/1280
 
 
 ### Security
diff --git a/key-binding.c b/key-binding.c
index 1876a885..1dffd3ee 100644
--- a/key-binding.c
+++ b/key-binding.c
@@ -350,6 +350,60 @@ maybe_repair_key_combo(const struct seat *seat,
     return sym;
 }
 
+static int
+key_cmp(struct key_binding a, struct key_binding b)
+{
+    xassert(a.type == b.type);
+
+    /*
+     * Sort bindings such that bindings with the same symbol are
+     * sorted with the binding having the most modifiers comes first.
+     *
+     * This fixes an issue where the “wrong” key binding are triggered
+     * when used with “consumed” modifiers.
+     *
+     * For example: if Control+BackSpace is bound before
+     * Control+Shift+BackSpace, then the latter binding is never
+     * triggered.
+     *
+     * Why? Because Shift is a consumed modifier. This means
+     * Control+BackSpace is “the same” as Control+Shift+BackSpace.
+     *
+     * By sorting bindings with more modifiers first, we work around
+     * the problem. But note that it is *just* a workaround, and I’m
+     * not confident there aren’t cases where it doesn’t work.
+     *
+     * See https://codeberg.org/dnkl/foot/issues/1280
+     */
+
+    const int a_mod_count = __builtin_popcount(a.mods);
+    const int b_mod_count = __builtin_popcount(b.mods);
+
+    switch (a.type) {
+    case KEY_BINDING:
+        if (a.k.sym != b.k.sym)
+            return b.k.sym - a.k.sym;
+        return b_mod_count - a_mod_count;
+
+    case MOUSE_BINDING: {
+        if (a.m.button != b.m.button)
+            return b.m.button - a.m.button;
+        if (a_mod_count != b_mod_count)
+            return b_mod_count - a_mod_count;
+        return b.m.count - a.m.count;
+    }
+    }
+
+    BUG("invalid key binding type");
+    return 0;
+}
+
+static void NOINLINE
+sort_binding_list(key_binding_list_t *list)
+{
+    tll_sort(*list, key_cmp);
+}
+
 static void NOINLINE
 convert_key_binding(struct key_set *set,
                     const struct config_key_binding *conf_binding,
@@ -371,6 +425,7 @@ convert_key_binding(struct key_set *set,
         },
     };
     tll_push_back(*bindings, binding);
+    sort_binding_list(bindings);
 }
 
 static void
@@ -421,6 +476,7 @@ convert_mouse_binding(struct key_set *set,
         },
     };
     tll_push_back(set->public.mouse, binding);
+    sort_binding_list(&set->public.mouse);
 }
 
 static void