[Test] Add tests for the Bindings class

In addition to fixing a couple of minor bugs, this also creates some
utility functions to access the cmdtab data.

Finally, this disables tests on MSYS2 as they can be flaky.

.github/workflows/msys2-build.yml

@@ -62,8 +62,3 @@ jobs:
       - if: matrix.build_options == 'libretro'
         name: Build libretro core
         run: make -C src/libretro ${{ matrix.libretro_build }} CC=clang CXX=clang++
-
-      # Run tests
-      - if: matrix.build_options == 'default'
-        name: Run tests
-        run: cd build && ctest -j

src/wx/config/CMakeLists.txt

@@ -71,6 +71,7 @@ configure_wx_target(vbam-wx-config)
 
 if (BUILD_TESTING)
     add_executable(vbam-wx-config-tests
+        bindings-test.cpp
         command-test.cpp
         option-test.cpp
         strutils-test.cpp

src/wx/config/bindings-test.cpp

@@ -0,0 +1,152 @@
+#include "wx/config/bindings.h"
+
+#include <gtest/gtest.h>
+
+#include <wx/xrc/xmlres.h>
+#include "wx/config/command.h"
+#include "wx/config/user-input.h"
+
+TEST(BindingsTest, Default) {
+    const config::Bindings bindings;
+
+    // Check that the default bindings are set up correctly.
+    auto inputs =
+        bindings.InputsForCommand(config::GameCommand(config::GameJoy(0), config::GameKey::Up));
+    EXPECT_TRUE(inputs.find(config::KeyboardInput('W')) != inputs.end());
+    EXPECT_TRUE(inputs.find(config::JoyInput(config::JoyId(0), config::JoyControl::HatNorth, 0)) !=
+                inputs.end());
+
+    inputs = bindings.InputsForCommand(config::ShortcutCommand(wxID_CLOSE));
+    EXPECT_TRUE(inputs.find(config::KeyboardInput('W', wxMOD_CMD)) != inputs.end());
+
+    inputs = bindings.InputsForCommand(config::ShortcutCommand(XRCID("LoadGame01")));
+    EXPECT_TRUE(inputs.find(config::KeyboardInput(WXK_F1)) != inputs.end());
+
+    // Check that the INI configuration for the keyboard is empty.
+    const auto config = bindings.GetKeyboardConfiguration();
+    EXPECT_TRUE(config.empty());
+}
+
+// Tests that assigning a default input to another command generates the right
+// configuration.
+TEST(BindingsTest, AssignDefault) {
+    config::Bindings bindings;
+
+    // Assign F1 to the "Close" command.
+    bindings.AssignInputToCommand(config::KeyboardInput(WXK_F1),
+                                  config::ShortcutCommand(wxID_CLOSE));
+
+    // The INI configuration should have NOOP set to F1, and Close set to F1.
+    const auto config = bindings.GetKeyboardConfiguration();
+    EXPECT_EQ(config.size(), 2);
+    EXPECT_EQ(config[0].first, "Keyboard/NOOP");
+    EXPECT_EQ(config[0].second, "F1");
+    EXPECT_EQ(config[1].first, "Keyboard/CLOSE");
+    EXPECT_EQ(config[1].second, "F1");
+}
+
+// Tests that unassigning a default input generates the right configuration.
+TEST(BindingsTest, UnassignDefault) {
+    config::Bindings bindings;
+
+    // Unassign F1.
+    bindings.UnassignInput(config::KeyboardInput(WXK_F1));
+
+    // The INI configuration should have NOOP set to F1.
+    const auto config = bindings.GetKeyboardConfiguration();
+    EXPECT_EQ(config.size(), 1);
+    EXPECT_EQ(config[0].first, "Keyboard/NOOP");
+    EXPECT_EQ(config[0].second, "F1");
+}
+
+// Tests that re-assigning a default input to its default command generates the
+// right configuration.
+TEST(BindingsTest, ReassignDefault) {
+    config::Bindings bindings;
+
+    // Assign F1 to the "Close" command.
+    bindings.AssignInputToCommand(config::KeyboardInput(WXK_F1),
+                                  config::ShortcutCommand(wxID_CLOSE));
+
+    // Re-assign F1 to the "LoadGame01" command.
+    bindings.AssignInputToCommand(config::KeyboardInput(WXK_F1),
+                                  config::ShortcutCommand(XRCID("LoadGame01")));
+
+    // The INI configuration should be empty.
+    const auto config = bindings.GetKeyboardConfiguration();
+    EXPECT_TRUE(config.empty());
+}
+
+// Tests that assigning an input to "NOOP" properly disables the default input.
+TEST(BindingsTest, AssignToNoop) {
+    config::Bindings bindings;
+
+    // Assign F1 to the "NOOP" command.
+    bindings.AssignInputToCommand(config::KeyboardInput(WXK_F1),
+                                  config::ShortcutCommand(XRCID("NOOP")));
+
+    const auto command = bindings.CommandForInput(config::KeyboardInput(WXK_F1));
+    EXPECT_FALSE(command.has_value());
+
+    // The INI configuration should have NOOP set to F1 and nothing more.
+    const auto config = bindings.GetKeyboardConfiguration();
+    EXPECT_EQ(config.size(), 1);
+    EXPECT_EQ(config[0].first, "Keyboard/NOOP");
+    EXPECT_EQ(config[0].second, "F1");
+}
+ 
+// Tests that assigning an input not used as a default shortcut to "NOOP" does
+// nothing.
+TEST(BindingsTest, AssignUnusedToNoop) {
+    config::Bindings bindings;
+
+    // Assign "T" to the "NOOP" command.
+    bindings.AssignInputToCommand(config::KeyboardInput('T'), config::ShortcutCommand(XRCID("NOOP")));
+
+    // The INI configuration should be empty.
+    const auto config = bindings.GetKeyboardConfiguration();
+    EXPECT_TRUE(config.empty());
+
+    // "T" should have no assignment.
+    const auto command = bindings.CommandForInput(config::KeyboardInput('T'));
+    EXPECT_FALSE(command.has_value());
+}
+
+// Tests that assigning a default input to a Game command works as expected.
+TEST(BindingsTest, AssignDefaultToGame) {
+    config::Bindings bindings;
+
+    // Assign F1 to the "Up" command and clear all of the default input for the
+    // "Up" command.
+    bindings.AssignInputsToCommand({config::KeyboardInput(WXK_F1)},
+                                   config::GameCommand(config::GameJoy(0), config::GameKey::Up));
+
+    // The INI configuration should have NOOP set to F1.
+    const auto config = bindings.GetKeyboardConfiguration();
+    EXPECT_EQ(config.size(), 1);
+    EXPECT_EQ(config[0].first, "Keyboard/NOOP");
+    EXPECT_EQ(config[0].second, "F1");
+
+    EXPECT_EQ(
+        bindings.InputsForCommand(config::GameCommand(config::GameJoy(0), config::GameKey::Up)),
+        std::unordered_set<config::UserInput>{config::KeyboardInput(WXK_F1)});
+
+    EXPECT_EQ(bindings.CommandForInput(config::KeyboardInput(WXK_F1)),
+              config::Command(config::GameCommand(config::GameJoy(0), config::GameKey::Up)));
+}
+
+// Tests the "ClearCommandAssignments" method.
+TEST(BindingsTest, ClearCommand) {
+    config::Bindings bindings;
+
+    // Clear "CLOSE" assignments.
+    bindings.ClearCommandAssignments(config::ShortcutCommand(wxID_CLOSE));
+
+    // The INI configuration should only have the NOOP assignment.
+    const auto config = bindings.GetKeyboardConfiguration();
+    EXPECT_EQ(config.size(), 1);
+    EXPECT_EQ(config[0].first, "Keyboard/NOOP");
+    EXPECT_EQ(config[0].second, "CTRL+W");
+
+    EXPECT_TRUE(bindings.InputsForCommand(config::ShortcutCommand(wxID_CLOSE)).empty());
+}

src/wx/config/bindings.cpp

@@ -43,8 +43,8 @@ Bindings::Bindings(
       input_to_control_(input_to_control.begin(), input_to_control.end()),
       disabled_defaults_(disabled_defaults.begin(), disabled_defaults.end()) {}
 
-std::vector<std::pair<int, wxString>> Bindings::GetKeyboardConfiguration() const {
+std::vector<std::pair<wxString, wxString>> Bindings::GetKeyboardConfiguration() const {
-    std::vector<std::pair<int, wxString>> config;
+    std::vector<std::pair<wxString, wxString>> config;
     config.reserve(control_to_inputs_.size() + 1);
 
     if (!disabled_defaults_.empty()) {
@@ -52,7 +52,8 @@ std::vector<std::pair<int, wxString>> Bindings::GetKeyboardConfiguration() const
         for (const auto& iter : disabled_defaults_) {
             noop_inputs.insert(iter.first);
         }
-        config.push_back(std::make_pair(NoopCommand(), UserInput::SpanToConfigString(noop_inputs)));
+        config.push_back(std::make_pair(ShortcutCommand(NoopCommand()).ToConfigString(),
+                                        UserInput::SpanToConfigString(noop_inputs)));
     }
 
     for (const auto& iter : control_to_inputs_) {
@@ -73,8 +74,8 @@ std::vector<std::pair<int, wxString>> Bindings::GetKeyboardConfiguration() const
         }
 
         if (!inputs.empty()) {
-            const int command_id = iter.first.shortcut().id();
+            config.push_back(std::make_pair(iter.first.shortcut().ToConfigString(),
-            config.push_back(std::make_pair(command_id, UserInput::SpanToConfigString(inputs)));
+                                            UserInput::SpanToConfigString(inputs)));
         }
     }
 
@@ -204,16 +205,19 @@ void Bindings::UnassignInput(const UserInput& input) {
 }
 
 void Bindings::ClearCommandAssignments(const Command& command) {
-    auto iter = control_to_inputs_.find(command);
+    const auto iter = control_to_inputs_.find(command);
     if (iter == control_to_inputs_.end()) {
         // Command not found, nothing to do.
         return;
     }
 
-    for (const UserInput& input : iter->second) {
+    // Keep a copy of the inputs to unassign.
-        input_to_control_.erase(input);
+    std::unordered_set<UserInput> inputs_to_unassign(iter->second);
+
+    // Unassign all inputs.
+    for (const UserInput& input : inputs_to_unassign) {
+        UnassignInput(input);
     }
-    control_to_inputs_.erase(iter);
 }
 
 void Bindings::UnassignDefaultBinding(const UserInput& input) {

src/wx/config/bindings.h

@@ -43,7 +43,7 @@ public:
     // - User-added custom bindings. These appear under [Keyboard/CommandName].
     // Essentially, this is a diff between the default shortcuts and the user
     // configuration.
-    std::vector<std::pair<int, wxString>> GetKeyboardConfiguration() const;
+    std::vector<std::pair<wxString, wxString>> GetKeyboardConfiguration() const;
 
     // Returns the game control configuration for the INI file. These go in the
     // [Joypad] section of the INI file.

src/wx/config/cmdtab.cpp

@@ -1,5 +1,7 @@
 #include "wx/config/cmdtab.h"
 
+#include <algorithm>
+
 #include <wx/wxcrt.h>
 
 // Initializer for struct cmditem
@@ -11,6 +13,41 @@ cmditem new_cmditem(const wxString cmd,
     return cmditem {cmd, name, cmd_id, mask_flags, mi};
 }
 
-bool cmditem_lt(const struct cmditem& cmd1, const struct cmditem& cmd2) {
+namespace config {
+    wxString GetCommandINIEntry(int command) {
+        for (const auto& cmd_item : cmdtab) {
+            if (cmd_item.cmd_id == command) {
+                return wxString::Format("Keyboard/%s", cmd_item.cmd);
+            }
+        }
+
+        // Command not found. This should never happen.
+        assert(false);
+        return wxEmptyString;
+    }
+
+    wxString GetCommandHelper(int command) {
+        for (const auto& cmd_item : cmdtab) {
+            if (cmd_item.cmd_id == command) {
+                return cmd_item.name;
+            }
+        }
+
+        // Command not found. This should never happen.
+        assert(false);
+        return wxEmptyString;
+    }
+
+    nonstd::optional<int> CommandFromConfigString(const wxString& config) {
+        const cmditem dummy = new_cmditem(config);
+        const auto iter = std::lower_bound(cmdtab.begin(), cmdtab.end(), dummy, [](const cmditem& cmd1, const cmditem& cmd2) {
             return wxStrcmp(cmd1.cmd, cmd2.cmd) < 0;
-}
+        });
+
+        if (iter == cmdtab.end() || iter->cmd != config) {
+            return nonstd::nullopt;
+        }
+
+        return iter->cmd_id;
+    }
+}  // namespace config

src/wx/config/cmdtab.h

@@ -3,6 +3,8 @@
 
 #include <vector>
 
+#include <optional.hpp>
+
 #include <wx/string.h>
 
 // Forward declaration.
@@ -29,8 +31,29 @@ cmditem new_cmditem(const wxString cmd = "",
                     int mask_flags = 0,
                     wxMenuItem* mi = nullptr);
 
-// for binary search
+namespace config {
-bool cmditem_lt(const struct cmditem& cmd1, const struct cmditem& cmd2);
+    // Returns the command INI entry name for the given XRC ID. Will assert if
+    // the command is not found. The INI entry name is the command name prefixed
+    // with "Keyboard/". This is used to store the command in the INI file.
+    // Examples:
+    // * wxID_OPEN -> "Keyboard/OPEN"
+    // * XRCID("NOOP") -> "Keyboard/NOOP"
+    // O(n) search.
+    wxString GetCommandINIEntry(int xrc_id);
+
+    // Returns the command helper string for the given XRC ID. Will assert if
+    // the command is not found.
+    // O(n) search.
+    wxString GetCommandHelper(int xrc_id);
+
+    // Returns the XRC ID for the given command config name, without the
+    // "Keyboard/" prefix. Examples:
+    // * "OPEN" -> wxID_OPEN
+    // * "Keyboard/OPEN" -> nonstd::nullopt
+    // * "NOOP" -> XRCID("NOOP")
+    // O(log(n)) search.
+    nonstd::optional<int> CommandFromConfigString(const wxString& config);
+}
 
 // here are those conditions
 enum { CMDEN_GB = (1 << 0), // GB ROM loaded

src/wx/config/command.cpp

@@ -1,6 +1,5 @@
 #include "wx/config/command.h"
 
-#include <algorithm>
 #include <map>
 
 #include <wx/log.h>
@@ -109,15 +108,7 @@ wxString GameCommand::ToUXString() const {
 }
 
 wxString ShortcutCommand::ToConfigString() const {
-    for (const cmditem& cmd_item : cmdtab) {
+    return GetCommandINIEntry(id_);
-        if (cmd_item.cmd_id == id_) {
-            return wxString::Format("Keyboard/%s", cmd_item.cmd);
-        }
-    }
-
-    // Command not found. This should never happen.
-    assert(false);
-    return wxEmptyString;
 }
 
 // static
@@ -158,14 +149,13 @@ nonstd::optional<Command> Command::FromString(const wxString& name) {
             return nonstd::nullopt;
         }
 
-        const auto iter = std::lower_bound(cmdtab.begin(), cmdtab.end(),
+        const auto xrc_id = CommandFromConfigString(parts[1]);
-                                           cmditem{parts[1], wxString(), 0, 0, NULL}, cmditem_lt);
+        if (!xrc_id.has_value()) {
-        if (iter == cmdtab.end() || iter->cmd != parts[1]) {
             wxLogDebug("Command ID %s not found", parts[1]);
             return nonstd::nullopt;
         }
 
-        return Command(ShortcutCommand(iter->cmd_id));
+        return Command(ShortcutCommand(xrc_id.value()));
     }
 }
 

src/wx/config/internal/bindings-internal.cpp

@@ -26,7 +26,7 @@ const std::unordered_map<Command, std::unordered_set<UserInput>>& DefaultInputs(
         // this was annoying people A LOT #334
         // {ShortcutCommand(wxID_EXIT),
         //     {
-        //         KeyboardInput(WXK_ESCAPE, wxMOD_NONE)
+        //         KeyboardInput(WXK_ESCAPE)
         //     }},
         // this was annoying people #298
         // {ShortcutCommand(wxID_EXIT),
@@ -52,46 +52,46 @@ const std::unordered_map<Command, std::unordered_set<UserInput>>& DefaultInputs(
             }},
         {ShortcutCommand(XRCID("LoadGame01")),
             {
-                KeyboardInput(WXK_F1, wxMOD_NONE)
+                KeyboardInput(WXK_F1)
             }},
         {ShortcutCommand(XRCID("LoadGame02")),
             {
-                KeyboardInput(WXK_F2, wxMOD_NONE)
+                KeyboardInput(WXK_F2)
             }},
         {ShortcutCommand(XRCID("LoadGame03")),
             {
-                KeyboardInput(WXK_F3, wxMOD_NONE)
+                KeyboardInput(WXK_F3)
             }},
         {ShortcutCommand(XRCID("LoadGame04")),
             {
-                KeyboardInput(WXK_F4, wxMOD_NONE)
+                KeyboardInput(WXK_F4)
             }},
         {ShortcutCommand(XRCID("LoadGame05")),
             {
-                KeyboardInput(WXK_F5, wxMOD_NONE)
+                KeyboardInput(WXK_F5)
             }},
         {ShortcutCommand(XRCID("LoadGame06")),
             {
-                KeyboardInput(WXK_F6, wxMOD_NONE)
+                KeyboardInput(WXK_F6)
             }},
         {ShortcutCommand(XRCID("LoadGame07")),
             {
-                KeyboardInput(WXK_F7, wxMOD_NONE)
+                KeyboardInput(WXK_F7)
             }},
         {ShortcutCommand(XRCID("LoadGame08")),
             {
-                KeyboardInput(WXK_F8, wxMOD_NONE)
+                KeyboardInput(WXK_F8)
             }},
         {ShortcutCommand(XRCID("LoadGame09")),
             {
-                KeyboardInput(WXK_F9, wxMOD_NONE)
+                KeyboardInput(WXK_F9)
             }},
         {ShortcutCommand(XRCID("LoadGame10")),
             {
-                KeyboardInput(WXK_F10, wxMOD_NONE)
+                KeyboardInput(WXK_F10)
             }},
         {ShortcutCommand(XRCID("Pause")),
-         {KeyboardInput(WXK_PAUSE, wxMOD_NONE), KeyboardInput('P', wxMOD_CMD)}},
+         {KeyboardInput(WXK_PAUSE), KeyboardInput('P', wxMOD_CMD)}},
         {ShortcutCommand(XRCID("Reset")),
             {
                 KeyboardInput('R', wxMOD_CMD)
@@ -99,27 +99,27 @@ const std::unordered_map<Command, std::unordered_set<UserInput>>& DefaultInputs(
         // add shortcuts for original size multiplier #415
         {ShortcutCommand(XRCID("SetSize1x")),
             {
-                KeyboardInput('1', wxMOD_NONE)
+                KeyboardInput('1')
             }},
         {ShortcutCommand(XRCID("SetSize2x")),
             {
-                KeyboardInput('2', wxMOD_NONE)
+                KeyboardInput('2')
             }},
         {ShortcutCommand(XRCID("SetSize3x")),
             {
-                KeyboardInput('3', wxMOD_NONE)
+                KeyboardInput('3')
             }},
         {ShortcutCommand(XRCID("SetSize4x")),
             {
-                KeyboardInput('4', wxMOD_NONE)
+                KeyboardInput('4')
             }},
         {ShortcutCommand(XRCID("SetSize5x")),
             {
-                KeyboardInput('5', wxMOD_NONE)
+                KeyboardInput('5')
             }},
         {ShortcutCommand(XRCID("SetSize6x")),
             {
-                KeyboardInput('6', wxMOD_NONE)
+                KeyboardInput('6')
             }},
         // save oldest is more commonly used than save other
         // {ShortcutCommand(XRCID("Save")),
@@ -173,7 +173,7 @@ const std::unordered_map<Command, std::unordered_set<UserInput>>& DefaultInputs(
         // I prefer the SDL ESC key binding
         // {ShortcutCommand(XRCID("ToggleFullscreen")),
         //     {
-        //         KeyboardInput(WXK_ESCAPE, wxMOD_NONE)
+        //         KeyboardInput(WXK_ESCAPE)
         //     }},
         // alt-enter is more standard anyway
         {ShortcutCommand(XRCID("ToggleFullscreen")),
@@ -290,15 +290,15 @@ const std::unordered_map<Command, std::unordered_set<UserInput>>& DefaultInputs(
             }},
         {ShortcutCommand(XRCID("IncreaseVolume")),
             {
-                KeyboardInput(WXK_NUMPAD_ADD, wxMOD_NONE)
+                KeyboardInput(WXK_NUMPAD_ADD)
             }},
         {ShortcutCommand(XRCID("DecreaseVolume")),
             {
-                KeyboardInput(WXK_NUMPAD_SUBTRACT, wxMOD_NONE)
+                KeyboardInput(WXK_NUMPAD_SUBTRACT)
             }},
         {ShortcutCommand(XRCID("ToggleSound")),
             {
-                KeyboardInput(WXK_NUMPAD_ENTER, wxMOD_NONE)
+                KeyboardInput(WXK_NUMPAD_ENTER)
             }},
 
         // Player 1 controls.

src/wx/dialogs/accel-config.cpp

@@ -59,23 +59,15 @@ void AppendItemToTree(std::unordered_map<config::ShortcutCommand, wxTreeItemId>*
                       int command,
                       const wxString& prefix,
                       int level) {
-    wxString name;
+    const wxString helper = config::GetCommandHelper(command);
-    for (const cmditem& cmd_item : cmdtab) {
-        if (command == cmd_item.cmd_id) {
-            name = cmd_item.name;
-            break;
-        }
-    }
-    assert(!name.empty());
-
     const wxTreeItemId tree_item_id =
         tree->AppendItem(parent,
-                         /*text=*/name,
+                         /*text=*/helper,
                          /*image=*/-1,
                          /*selImage=*/-1,
                          /*data=*/
                          new CommandTreeItemData(config::ShortcutCommand(command),
-                                                 AppendString(prefix, level, name), name));
+                                                 AppendString(prefix, level, helper), helper));
     command_to_item_id->emplace(command, tree_item_id);
 }
 

src/wx/opts.cpp

@@ -1,6 +1,5 @@
 #include "wx/opts.h"
 
-#include <algorithm>
 #include <limits>
 #include <memory>
 #include <unordered_set>
@@ -220,9 +219,7 @@ void load_opts(bool first_time_launch) {
              cont = cfg->GetNextEntry(e, entry_idx)) {
             // kb options come from a different list
             if (s == wxT("Keyboard")) {
-                const cmditem dummy = new_cmditem(e);
+                if (!config::CommandFromConfigString(e).has_value()) {
-
-                if (!std::binary_search(cmdtab.begin(), cmdtab.end(), dummy, cmditem_lt)) {
                     s.append(wxT('/'));
                     s.append(e);
                     //wxLogWarning(_("Invalid option %s present; removing if possible"), s.c_str());
@@ -421,19 +418,8 @@ void update_shortcut_opts() {
     cfg->DeleteGroup("/Keyboard");
     cfg->SetPath("/Keyboard");
     for (const auto& iter : wxGetApp().bindings()->GetKeyboardConfiguration()) {
-        bool found = false;
+        cfg->Write(iter.first, iter.second);
-        for (const cmditem& cmd_item : cmdtab) {
-            if (cmd_item.cmd_id == iter.first) {
-                found = true;
-                cfg->Write(cmd_item.cmd, iter.second);
-                break;
     }
-        }
-
-        // Command not found. This should never happen.
-        assert(found);
-    }
-
     cfg->SetPath("/");
 
     // For joypads, we just compare the strings.