keybinds: fix keys getting stuck + minor refactor & optimizations to keybind handling (#4304)

3 files changed, 142 insertions, 73 deletions

diff --git a/src/config/ConfigManager.cpp b/src/config/ConfigManager.cpp
index ee198ded..28959128 100644
--- a/src/config/ConfigManager.cpp
+++ b/src/config/ConfigManager.cpp
@@ -935,7 +935,7 @@ void CConfigManager::handleBind(const std::string& command, const std::string& v
             g_pKeybindManager->addKeybind(
                 SKeybind{"", std::stoi(KEY.substr(5)), MOD, HANDLER, COMMAND, locked, m_szCurrentSubmap, release, repeat, mouse, nonConsuming, transparent, ignoreMods});
         else
-            g_pKeybindManager->addKeybind(SKeybind{KEY, -1, MOD, HANDLER, COMMAND, locked, m_szCurrentSubmap, release, repeat, mouse, nonConsuming, transparent, ignoreMods});
+            g_pKeybindManager->addKeybind(SKeybind{KEY, 0, MOD, HANDLER, COMMAND, locked, m_szCurrentSubmap, release, repeat, mouse, nonConsuming, transparent, ignoreMods});
     }
 }
 
diff --git a/src/managers/KeybindManager.cpp b/src/managers/KeybindManager.cpp
index 899003d8..d379a408 100644
--- a/src/managers/KeybindManager.cpp
+++ b/src/managers/KeybindManager.cpp
@@ -7,6 +7,7 @@
 
 #include <sys/ioctl.h>
 #include <fcntl.h>
+#include <vector>
 #if defined(__linux__)
 #include <linux/vt.h>
 #elif defined(__NetBSD__) || defined(__OpenBSD__)
@@ -95,7 +96,7 @@ void CKeybindManager::addKeybind(SKeybind kb) {
 void CKeybindManager::removeKeybind(uint32_t mod, const std::string& key) {
     for (auto it = m_lKeybinds.begin(); it != m_lKeybinds.end(); ++it) {
         if (isNumber(key) && std::stoi(key) > 9) {
-            const auto KEYNUM = std::stoi(key);
+            const uint32_t KEYNUM = std::stoi(key);
 
             if (it->modmask == mod && it->keycode == KEYNUM) {
                 it = m_lKeybinds.erase(it);
@@ -137,6 +138,22 @@ uint32_t CKeybindManager::stringToModMask(std::string mods) {
     return modMask;
 }
 
+uint32_t CKeybindManager::keycodeToModifier(xkb_keycode_t keycode) {
+    switch (keycode - 8) {
+        case KEY_LEFTMETA: return WLR_MODIFIER_LOGO;
+        case KEY_RIGHTMETA: return WLR_MODIFIER_LOGO;
+        case KEY_LEFTSHIFT: return WLR_MODIFIER_SHIFT;
+        case KEY_RIGHTSHIFT: return WLR_MODIFIER_SHIFT;
+        case KEY_LEFTCTRL: return WLR_MODIFIER_CTRL;
+        case KEY_RIGHTCTRL: return WLR_MODIFIER_CTRL;
+        case KEY_LEFTALT: return WLR_MODIFIER_ALT;
+        case KEY_RIGHTALT: return WLR_MODIFIER_ALT;
+        case KEY_CAPSLOCK: return WLR_MODIFIER_CAPS;
+        case KEY_NUMLOCK: return WLR_MODIFIER_MOD2;
+        default: return 0;
+    }
+}
+
 void CKeybindManager::updateXKBTranslationState() {
     if (m_pXKBTranslationState) {
         xkb_keymap_unref(xkb_state_get_keymap(m_pXKBTranslationState));
@@ -261,8 +278,7 @@ void CKeybindManager::switchToWindow(CWindow* PWINDOWTOCHANGETO) {
 
 bool CKeybindManager::onKeyEvent(wlr_keyboard_key_event* e, SKeyboard* pKeyboard) {
     if (!g_pCompositor->m_bSessionActive || g_pCompositor->m_bUnsafeState) {
-        m_dPressedKeycodes.clear();
-        m_dPressedKeysyms.clear();
+        m_dPressedKeys.clear();
         return true;
     }
 
@@ -291,7 +307,13 @@ bool CKeybindManager::onKeyEvent(wlr_keyboard_key_event* e, SKeyboard* pKeyboard
     m_uLastCode      = KEYCODE;
     m_uLastMouseCode = 0;
 
-    bool mouseBindWasActive = ensureMouseBindState();
+    bool       mouseBindWasActive = ensureMouseBindState();
+
+    const auto KEY = SPressedKeyWithMods{
+        .keysym             = keysym,
+        .keycode            = KEYCODE,
+        .modmaskAtPressTime = MODS,
+    };
 
     bool found = false;
     if (e->state == WL_KEYBOARD_KEY_STATE_PRESSED) {
@@ -302,16 +324,13 @@ bool CKeybindManager::onKeyEvent(wlr_keyboard_key_event* e, SKeyboard* pKeyboard
             m_pActiveKeybind            = nullptr;
         }
 
-        m_dPressedKeycodes.push_back(KEYCODE);
-        m_dPressedKeysyms.push_back(keysym);
+        m_dPressedKeys.push_back(KEY);
 
-        found = handleKeybinds(MODS, "", keysym, 0, true, e->time_msec) || found;
-
-        found = handleKeybinds(MODS, "", 0, KEYCODE, true, e->time_msec) || found;
+        found = handleKeybinds(MODS, KEY, true);
 
         if (found)
             shadowKeybinds(keysym, KEYCODE);
-    } else if (e->state == WL_KEYBOARD_KEY_STATE_RELEASED) {
+    } else { // key release
         // clean repeat
         if (m_pActiveKeybindEventSource) {
             wl_event_source_remove(m_pActiveKeybindEventSource);
@@ -319,12 +338,23 @@ bool CKeybindManager::onKeyEvent(wlr_keyboard_key_event* e, SKeyboard* pKeyboard
             m_pActiveKeybind            = nullptr;
         }
 
-        m_dPressedKeycodes.erase(std::remove(m_dPressedKeycodes.begin(), m_dPressedKeycodes.end(), KEYCODE), m_dPressedKeycodes.end());
-        m_dPressedKeysyms.erase(std::remove(m_dPressedKeysyms.begin(), m_dPressedKeysyms.end(), keysym), m_dPressedKeysyms.end());
-
-        found = handleKeybinds(MODS, "", keysym, 0, false, e->time_msec) || found;
-
-        found = handleKeybinds(MODS, "", 0, KEYCODE, false, e->time_msec) || found;
+        bool foundInPressedKeys = false;
+        for (auto it = m_dPressedKeys.begin(); it != m_dPressedKeys.end();) {
+            if (it->keycode == KEYCODE) {
+                if (!foundInPressedKeys) {
+                    found              = handleKeybinds(MODS, *it, false);
+                    foundInPressedKeys = true;
+                }
+                it = m_dPressedKeys.erase(it);
+            } else {
+                ++it;
+            }
+        }
+        if (!foundInPressedKeys) {
+            Debug::log(ERR, "BUG THIS: key not found in m_dPressedKeys");
+            // fallback with wrong `KEY.modmaskAtPressTime`, this can be buggy
+            found = handleKeybinds(MODS, KEY, false);
+        }
 
         shadowKeybinds();
     }
@@ -347,14 +377,14 @@ bool CKeybindManager::onAxisEvent(wlr_pointer_axis_event* e) {
     bool found = false;
     if (e->source == WLR_AXIS_SOURCE_WHEEL && e->orientation == WLR_AXIS_ORIENTATION_VERTICAL) {
         if (e->delta < 0)
-            found = handleKeybinds(MODS, "mouse_down", 0, 0, true, 0);
+            found = handleKeybinds(MODS, SPressedKeyWithMods{.keyName = "mouse_down"}, true);
         else
-            found = handleKeybinds(MODS, "mouse_up", 0, 0, true, 0);
+            found = handleKeybinds(MODS, SPressedKeyWithMods{.keyName = "mouse_up"}, true);
     } else if (e->source == WLR_AXIS_SOURCE_WHEEL && e->orientation == WLR_AXIS_ORIENTATION_HORIZONTAL) {
         if (e->delta < 0)
-            found = handleKeybinds(MODS, "mouse_left", 0, 0, true, 0);
+            found = handleKeybinds(MODS, SPressedKeyWithMods{.keyName = "mouse_left"}, true);
         else
-            found = handleKeybinds(MODS, "mouse_right", 0, 0, true, 0);
+            found = handleKeybinds(MODS, SPressedKeyWithMods{.keyName = "mouse_right"}, true);
     }
 
     if (found)
@@ -372,15 +402,40 @@ bool CKeybindManager::onMouseEvent(wlr_pointer_button_event* e) {
     m_uLastCode      = 0;
     m_uTimeLastMs    = e->time_msec;
 
-    bool mouseBindWasActive = ensureMouseBindState();
+    bool       mouseBindWasActive = ensureMouseBindState();
+
+    const auto KEY_NAME = "mouse:" + std::to_string(e->button);
+
+    const auto KEY = SPressedKeyWithMods{
+        .keyName            = KEY_NAME,
+        .modmaskAtPressTime = MODS,
+    };
 
     if (e->state == WLR_BUTTON_PRESSED) {
-        found = handleKeybinds(MODS, "mouse:" + std::to_string(e->button), 0, 0, true, 0);
+        m_dPressedKeys.push_back(KEY);
+
+        found = handleKeybinds(MODS, KEY, true);
 
         if (found)
             shadowKeybinds();
     } else {
-        found = handleKeybinds(MODS, "mouse:" + std::to_string(e->button), 0, 0, false, 0);
+        bool foundInPressedKeys = false;
+        for (auto it = m_dPressedKeys.begin(); it != m_dPressedKeys.end();) {
+            if (it->keyName == KEY_NAME) {
+                if (!foundInPressedKeys) {
+                    found              = handleKeybinds(MODS, *it, false);
+                    foundInPressedKeys = true;
+                }
+                it = m_dPressedKeys.erase(it);
+            } else {
+                ++it;
+            }
+        }
+        if (!foundInPressedKeys) {
+            Debug::log(ERR, "BUG THIS: key not found in m_dPressedKeys (2)");
+            // fallback with wrong `KEY.modmaskAtPressTime`, this can be buggy
+            found = handleKeybinds(MODS, KEY, false);
+        }
 
         shadowKeybinds();
     }
@@ -397,15 +452,15 @@ void CKeybindManager::resizeWithBorder(wlr_pointer_button_event* e) {
 }
 
 void CKeybindManager::onSwitchEvent(const std::string& switchName) {
-    handleKeybinds(0, "switch:" + switchName, 0, 0, true, 0);
+    handleKeybinds(0, SPressedKeyWithMods{.keyName = "switch:" + switchName}, true);
 }
 
 void CKeybindManager::onSwitchOnEvent(const std::string& switchName) {
-    handleKeybinds(0, "switch:on:" + switchName, 0, 0, true, 0);
+    handleKeybinds(0, SPressedKeyWithMods{.keyName = "switch:on:" + switchName}, true);
 }
 
 void CKeybindManager::onSwitchOffEvent(const std::string& switchName) {
-    handleKeybinds(0, "switch:off:" + switchName, 0, 0, true, 0);
+    handleKeybinds(0, SPressedKeyWithMods{.keyName = "switch:off:" + switchName}, true);
 }
 
 int repeatKeyHandler(void* data) {
@@ -424,7 +479,7 @@ int repeatKeyHandler(void* data) {
     return 0;
 }
 
-bool CKeybindManager::handleKeybinds(const uint32_t& modmask, const std::string& key, const xkb_keysym_t& keysym, const int& keycode, bool pressed, uint32_t time) {
+bool CKeybindManager::handleKeybinds(const uint32_t modmask, const SPressedKeyWithMods& key, bool pressed) {
     bool found = false;
 
     if (g_pCompositor->m_sSeat.exclusiveClient)
@@ -441,22 +496,19 @@ bool CKeybindManager::handleKeybinds(const uint32_t& modmask, const std::string&
             ((modmask != k.modmask && !k.ignoreMods) || (g_pCompositor->m_sSeat.exclusiveClient && !k.locked) || k.submap != m_szCurrentSelectedSubmap || k.shadowed))
             continue;
 
-        if (!key.empty()) {
-            if (key != k.key)
+        if (!key.keyName.empty()) {
+            if (key.keyName != k.key)
                 continue;
-        } else if (k.keycode != -1) {
-            if (keycode != k.keycode)
+        } else if (k.keycode != 0) {
+            if (key.keycode != k.keycode)
                 continue;
         } else {
-            if (keysym == 0)
-                continue; // this is a keycode check run
-
             // oMg such performance hit!!11!
             // this little maneouver is gonna cost us 4µs
             const auto KBKEY      = xkb_keysym_from_name(k.key.c_str(), XKB_KEYSYM_CASE_INSENSITIVE);
             const auto KBKEYUPPER = xkb_keysym_to_upper(KBKEY);
 
-            if (keysym != KBKEY && keysym != KBKEYUPPER)
+            if (key.keysym != KBKEY && key.keysym != KBKEYUPPER)
                 continue;
         }
 
@@ -468,12 +520,24 @@ bool CKeybindManager::handleKeybinds(const uint32_t& modmask, const std::string&
             continue;
         }
 
-        if (!pressed && !k.release && !SPECIALDISPATCHER) {
-            if (k.nonConsuming)
+        if (!pressed) {
+            // Require mods to be matching when the key was first pressed.
+            if (key.modmaskAtPressTime != modmask) {
+                // Handle properly `bindr` where a key is itself a bind mod for example:
+                // "bindr = SUPER, SUPER_L, exec, $launcher".
+                // This needs to be handled separately for the above case, because `key.modmaskAtPressTime` is set
+                // from currently pressed keys as programs see them, but it doesn't yet include the currently
+                // pressed mod key, which is still being handled internally.
+                if (keycodeToModifier(key.keycode) == key.modmaskAtPressTime)
+                    continue;
+
+            } else if (!k.release && !SPECIALDISPATCHER) {
+                if (k.nonConsuming)
+                    continue;
+
+                found = true; // suppress the event
                 continue;
-
-            found = true; // suppress the event
-            continue;
+            }
         }
 
         const auto DISPATCHER = m_mDispatchers.find(k.mouse ? "mouse" : k.handler);
@@ -488,7 +552,7 @@ bool CKeybindManager::handleKeybinds(const uint32_t& modmask, const std::string&
             Debug::log(ERR, "Invalid handler in a keybind! (handler {} does not exist)", k.handler);
         } else {
             // call the dispatcher
-            Debug::log(LOG, "Keybind triggered, calling dispatcher ({}, {}, {})", modmask, key, keysym);
+            Debug::log(LOG, "Keybind triggered, calling dispatcher ({}, {}, {})", modmask, key.keyName, key.keysym);
 
             m_iPassPressed = (int)pressed;
 
@@ -521,7 +585,7 @@ bool CKeybindManager::handleKeybinds(const uint32_t& modmask, const std::string&
     return found;
 }
 
-void CKeybindManager::shadowKeybinds(const xkb_keysym_t& doesntHave, const int& doesntHaveCode) {
+void CKeybindManager::shadowKeybinds(const xkb_keysym_t& doesntHave, const uint32_t doesntHaveCode) {
     // shadow disables keybinds after one has been triggered
 
     for (auto& k : m_lKeybinds) {
@@ -534,22 +598,20 @@ void CKeybindManager::shadowKeybinds(const xkb_keysym_t& doesntHave, const int&
         const auto KBKEY      = xkb_keysym_from_name(k.key.c_str(), XKB_KEYSYM_CASE_INSENSITIVE);
         const auto KBKEYUPPER = xkb_keysym_to_upper(KBKEY);
 
-        for (auto& pk : m_dPressedKeysyms) {
-            if ((pk == KBKEY || pk == KBKEYUPPER)) {
+        for (auto& pk : m_dPressedKeys) {
+            if ((pk.keysym != 0 && (pk.keysym == KBKEY || pk.keysym == KBKEYUPPER))) {
                 shadow = true;
 
-                if (pk == doesntHave && doesntHave != 0) {
+                if (pk.keysym == doesntHave && doesntHave != 0) {
                     shadow = false;
                     break;
                 }
             }
-        }
 
-        for (auto& pk : m_dPressedKeycodes) {
-            if (pk == k.keycode) {
+            if (pk.keycode != 0 && pk.keycode == k.keycode) {
                 shadow = true;
 
-                if (pk == doesntHaveCode && doesntHaveCode != 0 && doesntHaveCode != -1) {
+                if (pk.keycode == doesntHaveCode && doesntHaveCode != 0) {
                     shadow = false;
                     break;
                 }
diff --git a/src/managers/KeybindManager.hpp b/src/managers/KeybindManager.hpp
index 73997dd6..631b0147 100644
--- a/src/managers/KeybindManager.hpp
+++ b/src/managers/KeybindManager.hpp
@@ -12,7 +12,7 @@ class CPluginSystem;
 
 struct SKeybind {
     std::string key          = "";
-    int         keycode      = -1;
+    uint32_t    keycode      = 0;
     uint32_t    modmask      = 0;
     std::string handler      = "";
     std::string arg          = "";
@@ -36,6 +36,13 @@ enum eFocusWindowMode {
     MODE_PID
 };
 
+struct SPressedKeyWithMods {
+    std::string  keyName            = "";
+    xkb_keysym_t keysym             = 0;
+    uint32_t     keycode            = 0;
+    uint32_t     modmaskAtPressTime = 0;
+};
+
 class CKeybindManager {
   public:
     CKeybindManager();
@@ -51,8 +58,9 @@ class CKeybindManager {
     void                                                              addKeybind(SKeybind);
     void                                                              removeKeybind(uint32_t, const std::string&);
     uint32_t                                                          stringToModMask(std::string);
+    uint32_t                                                          keycodeToModifier(xkb_keycode_t);
     void                                                              clearKeybinds();
-    void                                                              shadowKeybinds(const xkb_keysym_t& doesntHave = 0, const int& doesntHaveCode = 0);
+    void                                                              shadowKeybinds(const xkb_keysym_t& doesntHave = 0, const uint32_t doesntHaveCode = 0);
 
     std::unordered_map<std::string, std::function<void(std::string)>> m_mDispatchers;
 
@@ -63,38 +71,37 @@ class CKeybindManager {
     std::list<SKeybind>                                               m_lKeybinds;
 
   private:
-    std::deque<xkb_keysym_t>  m_dPressedKeysyms;
-    std::deque<int>           m_dPressedKeycodes;
+    std::deque<SPressedKeyWithMods> m_dPressedKeys;
 
-    inline static std::string m_szCurrentSelectedSubmap = "";
+    inline static std::string       m_szCurrentSelectedSubmap = "";
 
-    SKeybind*                 m_pActiveKeybind = nullptr;
+    SKeybind*                       m_pActiveKeybind = nullptr;
 
-    uint32_t                  m_uTimeLastMs    = 0;
-    uint32_t                  m_uLastCode      = 0;
-    uint32_t                  m_uLastMouseCode = 0;
+    uint32_t                        m_uTimeLastMs    = 0;
+    uint32_t                        m_uLastCode      = 0;
+    uint32_t                        m_uLastMouseCode = 0;
 
-    bool                      m_bIsMouseBindActive = false;
-    std::vector<SKeybind*>    m_vPressedSpecialBinds;
+    bool                            m_bIsMouseBindActive = false;
+    std::vector<SKeybind*>          m_vPressedSpecialBinds;
 
-    int                       m_iPassPressed = -1; // used for pass
+    int                             m_iPassPressed = -1; // used for pass
 
-    CTimer                    m_tScrollTimer;
+    CTimer                          m_tScrollTimer;
 
-    bool                      handleKeybinds(const uint32_t&, const std::string&, const xkb_keysym_t&, const int&, bool, uint32_t);
+    bool                            handleKeybinds(const uint32_t, const SPressedKeyWithMods&, bool);
 
-    bool                      handleInternalKeybinds(xkb_keysym_t);
-    bool                      handleVT(xkb_keysym_t);
+    bool                            handleInternalKeybinds(xkb_keysym_t);
+    bool                            handleVT(xkb_keysym_t);
 
-    xkb_state*                m_pXKBTranslationState = nullptr;
+    xkb_state*                      m_pXKBTranslationState = nullptr;
 
-    void                      updateXKBTranslationState();
-    bool                      ensureMouseBindState();
+    void                            updateXKBTranslationState();
+    bool                            ensureMouseBindState();
 
-    static bool               tryMoveFocusToMonitor(CMonitor* monitor);
-    static void               moveWindowOutOfGroup(CWindow* pWindow, const std::string& dir = "");
-    static void               moveWindowIntoGroup(CWindow* pWindow, CWindow* pWindowInDirection);
-    static void               switchToWindow(CWindow* PWINDOWTOCHANGETO);
+    static bool                     tryMoveFocusToMonitor(CMonitor* monitor);
+    static void                     moveWindowOutOfGroup(CWindow* pWindow, const std::string& dir = "");
+    static void                     moveWindowIntoGroup(CWindow* pWindow, CWindow* pWindowInDirection);
+    static void                     switchToWindow(CWindow* PWINDOWTOCHANGETO);
 
     // -------------- Dispatchers -------------- //
     static void     killActive(std::string);