Improve api ergonomics and add missing locks

Author: NicksWorld

docs/dev/Lua API.rst

@@ -1439,11 +1439,11 @@ Hotkey module
   for details on format).
   Returns false on failure to create keybind.
 
-* ``dfhack.hotkey.clearKeybind(keyspec, any_focus, command)``
+* ``dfhack.hotkey.removeKeybind(keyspec, [match_focus=true, command])``
 
   Removes keybinds matching the provided keyspec.
-  If any_focus is true, the focus portion of the keyspec is ignored.
-  If command is not an empty string, the command is matched against as well.
+  If match_focus is set, the focus portion of the keyspec is matched against.
+  If command is provided and not an empty string, the command is matched against.
   Returns false if no keybinds were removed.
 
 * ``dfhack.hotkey.listActiveKeybinds()``
@@ -1460,15 +1460,16 @@ Hotkey module
   :spec: The keyspec for the hotkey
   :command: The command the hotkey runs when pressed
 
-* ``dfhack.hotkey.requestKeybindInput()``
+* ``dfhack.hotkey.requestKeybindingInput([cancel=false])``
 
-  Requests that the next hotkey-compatible input is saved and not processed,
-  retrievable with ``dfhack.hotkey.readKeybindInput()``.
+  Enqueues or cancels a request that the next hotkey-compatible input is saved
+  and not processed, retrievable with ``dfhack.hotkey.getKeybindingInput()``.
+  If cancel is true, any current request is cancelled.
 
-* ``dfhack.hotkey.readKeybindInput()``
+* ``dfhack.hotkey.getKeybindingInput()``
 
   Reads the latest saved keybind input that was requested.
-  Returns a keyspec string for the input, or nil if no input is saved.
+  Returns a keyspec string for the input, or nil if no input has been saved.
 
 Units module
 ------------

library/LuaApi.cpp

@@ -1874,20 +1874,19 @@ static bool hotkey_addKeybind(const std::string spec, const std::string cmd) {
     return hotkey_mgr->addKeybind(spec, cmd);
 }
 
-static bool hotkey_clearKeybind(const std::string spec, bool any_focus, std::string cmd) {
+static int hotkey_requestKeybindingInput(lua_State *L) {
     auto hotkey_mgr = Core::getInstance().getHotkeyManager();
-    if (!hotkey_mgr) return false;
-    return hotkey_mgr->clearKeybind(spec, any_focus, cmd);
-}
-
-static void hotkey_requestKeybindingInput() {
-    auto hotkey_mgr = Core::getInstance().getHotkeyManager();
-    if (hotkey_mgr) hotkey_mgr->requestKeybindInput();
+    if (!hotkey_mgr) return 0;
+    bool cancel = false;
+    if (lua_gettop(L) == 1)
+        cancel = lua_toboolean(L, -1);
+    hotkey_mgr->requestKeybindingInput(cancel);
+    return 0;
 }
 
-static int hotkey_readKeybindInput(lua_State *L) {
+static int hotkey_getKeybindingInput(lua_State *L) {
     auto hotkey_mgr = Core::getInstance().getHotkeyManager();
-    auto input = hotkey_mgr->readKeybindInput();
+    auto input = hotkey_mgr->getKeybindingInput();
 
     if (input.empty()) {
         lua_pushnil(L);
@@ -1897,6 +1896,38 @@ static int hotkey_readKeybindInput(lua_State *L) {
     return 1;
 }
 
+static int hotkey_removeKeybind(lua_State *L) {
+    auto hotkey_mgr = Core::getInstance().getHotkeyManager();
+    if (!hotkey_mgr) {
+        lua_pushboolean(L, false);
+        return 1;
+    }
+
+    bool res = false;
+    switch (lua_gettop(L)) {
+        case 1:
+            luaL_checkstring(L, -1);
+            res = hotkey_mgr->removeKeybind(lua_tostring(L, -1));
+            break;
+        case 2:
+            luaL_checkstring(L, -2);
+            res = hotkey_mgr->removeKeybind(lua_tostring(L, -2), lua_toboolean(L, -1));
+            break;
+        case 3:
+            luaL_checkstring(L, -3);
+            luaL_checkstring(L, -1);
+            res = hotkey_mgr->removeKeybind(
+                    lua_tostring(L, -3),
+                    lua_toboolean(L, -2),
+                    lua_tostring(L, -1)
+                );
+            break;
+    }
+
+    lua_pushboolean(L, res);
+    return 1;
+}
+
 void hotkey_pushBindArray(lua_State *L, const std::vector<Hotkey::KeyBinding>& binds) {
     lua_createtable(L, binds.size(), 0);
     int i = 1;
@@ -1932,16 +1963,16 @@ static int hotkey_listAllKeybinds(lua_State *L) {
 }
 
 static const luaL_Reg dfhack_hotkey_funcs[] = {
+    { "removeKeybind", hotkey_removeKeybind },
     { "listActiveKeybinds", hotkey_listActiveKeybinds },
     { "listAllKeybinds", hotkey_listAllKeybinds },
-    { "readKeybindInput", hotkey_readKeybindInput },
+    { "requestKeybindingInput", hotkey_requestKeybindingInput },
+    { "getKeybindingInput", hotkey_getKeybindingInput },
     { NULL, NULL }
 };
 
 static const LuaWrapper::FunctionReg dfhack_hotkey_module[] = {
     WRAPN(addKeybind, hotkey_addKeybind),
-    WRAPN(clearKeybind, hotkey_clearKeybind),
-    WRAPN(requestKeybindInput, hotkey_requestKeybindingInput),
     { NULL, NULL }
 };
 

library/include/modules/Hotkey.h

@@ -37,8 +37,8 @@ namespace DFHack {
             bool addKeybind(std::string keyspec, std::string cmd);
             bool addKeybind(Hotkey::KeySpec spec, std::string cmd);
             // Clear a keybind with the given keyspec, optionally for any focus, or with a specific command
-            bool clearKeybind(std::string keyspec, bool any_focus=false, std::string_view cmdline="");
-            bool clearKeybind(const Hotkey::KeySpec& spec, bool any_focus=false, std::string_view cmdline="");
+            bool removeKeybind(std::string keyspec, bool match_focus=true, std::string_view cmdline="");
+            bool removeKeybind(const Hotkey::KeySpec& spec, bool match_focus=true, std::string_view cmdline="");
 
             std::vector<std::string> listKeybinds(std::string keyspec);
             std::vector<std::string> listKeybinds(const Hotkey::KeySpec& spec);
@@ -49,11 +49,11 @@ namespace DFHack {
             bool handleKeybind(int sym, int modifiers);
             void setHotkeyCommand(std::string cmd);
 
-            // Used to request the next keybind input is saved.
+            // Used to request the next hotkey-compatible input is saved.
             // This is to allow for graphical keybinding menus.
-            void requestKeybindInput();
+            void requestKeybindingInput(bool cancel=false);
             // Returns the latest requested keybind input
-            std::string readKeybindInput();
+            std::string getKeybindingInput();
 
         private:
             std::thread hotkey_thread;
@@ -64,7 +64,7 @@ namespace DFHack {
             std::string requested_keybind;
 
             int hotkey_sig = 0;
-            std::string queued_command = "";
+            std::string queued_command;
 
             std::map<int, std::vector<Hotkey::KeyBinding>> bindings;
 

library/modules/Hotkey.cpp

@@ -181,16 +181,17 @@ bool HotkeyManager::addKeybind(std::string keyspec, std::string cmd) {
     return this->addKeybind(spec_opt.value(), cmd);
 }
 
-bool HotkeyManager::clearKeybind(const KeySpec& spec, bool any_focus, std::string_view cmdline) {
+bool HotkeyManager::removeKeybind(const KeySpec& spec, bool match_focus, std::string_view cmdline) {
     std::lock_guard<std::mutex> l(lock);
     if (!bindings.contains(spec.sym))
         return false;
     auto& binds = bindings[spec.sym];
 
-    auto new_end = std::remove_if(binds.begin(), binds.end(), [any_focus, spec, &cmdline](const auto& v) {
-            return (any_focus
-                ? v.spec.sym == spec.sym && v.spec.modifiers == spec.modifiers
-                : v.spec == spec) && (cmdline.empty() || v.cmdline == cmdline);
+    auto new_end = std::remove_if(binds.begin(), binds.end(), [match_focus, spec, &cmdline](const auto& v) {
+            return v.spec.sym == spec.sym
+                && v.spec.modifiers == spec.modifiers
+                && (!match_focus || v.spec.focus == spec.focus)
+                && (cmdline.empty() || v.cmdline == cmdline);
     });
     if (new_end == binds.end())
         return false; // No bindings removed
@@ -199,11 +200,11 @@ bool HotkeyManager::clearKeybind(const KeySpec& spec, bool any_focus, std::strin
     return true;
 }
 
-bool HotkeyManager::clearKeybind(std::string keyspec, bool any_focus, std::string_view cmdline) {
+bool HotkeyManager::removeKeybind(std::string keyspec, bool match_focus, std::string_view cmdline) {
     std::optional<KeySpec> spec_opt = Hotkey::parseKeySpec(keyspec);
     if (!spec_opt.has_value())
         return false;
-    return this->clearKeybind(spec_opt.value(), any_focus, cmdline);
+    return this->removeKeybind(spec_opt.value(), match_focus, cmdline);
 }
 
 std::vector<std::string> HotkeyManager::listKeybinds(const KeySpec& spec) {
@@ -237,13 +238,15 @@ std::vector<std::string> HotkeyManager::listKeybinds(const KeySpec& spec) {
 }
 
 std::vector<std::string> HotkeyManager::listKeybinds(std::string keyspec) {
+    std::lock_guard<std::mutex> l(lock);
     std::optional<KeySpec> spec_opt = Hotkey::parseKeySpec(keyspec);
     if (!spec_opt.has_value())
         return {};
     return this->listKeybinds(spec_opt.value());
 }
 
 std::vector<KeyBinding> HotkeyManager::listActiveKeybinds() {
+    std::lock_guard<std::mutex> l(lock);
     std::vector<KeyBinding> out;
 
     for(const auto& [_, bind_set] : bindings) {
@@ -267,6 +270,7 @@ std::vector<KeyBinding> HotkeyManager::listActiveKeybinds() {
 }
 
 std::vector<KeyBinding> HotkeyManager::listAllKeybinds() {
+    std::lock_guard<std::mutex> l(lock);
     std::vector<KeyBinding> out;
 
     for (const auto& [_, bind_set] : bindings) {
@@ -351,13 +355,13 @@ void HotkeyManager::setHotkeyCommand(std::string cmd) {
     cond.notify_all();
 }
 
-void HotkeyManager::requestKeybindInput() {
+void HotkeyManager::requestKeybindingInput(bool cancel) {
     std::lock_guard<std::mutex> l(lock);
-    keybind_save_requested = true;
-    requested_keybind = "";
+    keybind_save_requested = !cancel;
+    requested_keybind.clear();
 }
 
-std::string HotkeyManager::readKeybindInput() {
+std::string HotkeyManager::getKeybindingInput() {
     std::lock_guard<std::mutex> l(lock);
     return requested_keybind;
 }
@@ -367,7 +371,7 @@ void HotkeyManager::handleKeybindingCommand(color_ostream &con, const std::vecto
     if (parts.size() >= 3 && (parts[0] == "set" || parts[0] == "add")) {
         std::string keystr = parts[1];
         if (parts[0] == "set")
-            clearKeybind(keystr);
+            removeKeybind(keystr);
         for (const auto& part : parts | std::views::drop(2) | std::views::reverse) {
             auto spec = Hotkey::parseKeySpec(keystr, &parse_error);
             if (!spec.has_value()) {
@@ -386,7 +390,7 @@ void HotkeyManager::handleKeybindingCommand(color_ostream &con, const std::vecto
             if (!spec.has_value()) {
                 con.printerr("%s\n", parse_error.c_str());
             }
-            if (!clearKeybind(spec.value())) {
+            if (!removeKeybind(spec.value())) {
                 con.printerr("No matching keybinds to remove\n");
                 break;
             }

plugins/hotkeys.cpp

@@ -46,7 +46,7 @@ static int cleanupHotkeys(lua_State *) {
     std::for_each(sorted_keys.begin(), sorted_keys.end(), [](const string &sym) {
         string keyspec = sym + "@" + MENU_SCREEN_FOCUS_STRING;
         DEBUG(log).print("clearing keybinding: %s\n", keyspec.c_str());
-        Core::getInstance().getHotkeyManager()->clearKeybind(keyspec);
+        Core::getInstance().getHotkeyManager()->removeKeybind(keyspec);
     });
     valid = false;
     sorted_keys.clear();