diff --git a/.github/workflows/msys2-build.yml b/.github/workflows/msys2-build.yml index cf8b6d57..f13bc70f 100644 --- a/.github/workflows/msys2-build.yml +++ b/.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 diff --git a/src/wx/config/CMakeLists.txt b/src/wx/config/CMakeLists.txt index b4e2f227..eab6a721 100644 --- a/src/wx/config/CMakeLists.txt +++ b/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 diff --git a/src/wx/config/bindings-test.cpp b/src/wx/config/bindings-test.cpp new file mode 100644 index 00000000..b57efd41 --- /dev/null +++ b/src/wx/config/bindings-test.cpp @@ -0,0 +1,152 @@ +#include "wx/config/bindings.h" + +#include + +#include +#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::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()); +} diff --git a/src/wx/config/bindings.cpp b/src/wx/config/bindings.cpp index 17bc375a..35adb2bc 100644 --- a/src/wx/config/bindings.cpp +++ b/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> Bindings::GetKeyboardConfiguration() const { - std::vector> config; +std::vector> Bindings::GetKeyboardConfiguration() const { + std::vector> config; config.reserve(control_to_inputs_.size() + 1); if (!disabled_defaults_.empty()) { @@ -52,7 +52,8 @@ std::vector> 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> Bindings::GetKeyboardConfiguration() const } if (!inputs.empty()) { - const int command_id = iter.first.shortcut().id(); - config.push_back(std::make_pair(command_id, UserInput::SpanToConfigString(inputs))); + config.push_back(std::make_pair(iter.first.shortcut().ToConfigString(), + UserInput::SpanToConfigString(inputs))); } } @@ -158,7 +159,7 @@ void Bindings::AssignInputToCommand(const UserInput& input, const Command& comma } void Bindings::AssignInputsToCommand(const std::unordered_set& inputs, - const Command& command) { + const Command& command) { // Remove the existing binding if it exists. const auto iter = control_to_inputs_.find(command); if (iter != control_to_inputs_.end()) { @@ -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) { - input_to_control_.erase(input); + // Keep a copy of the inputs to unassign. + std::unordered_set 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) { diff --git a/src/wx/config/bindings.h b/src/wx/config/bindings.h index 0fd80383..1dc85e51 100644 --- a/src/wx/config/bindings.h +++ b/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> GetKeyboardConfiguration() const; + std::vector> GetKeyboardConfiguration() const; // Returns the game control configuration for the INI file. These go in the // [Joypad] section of the INI file. diff --git a/src/wx/config/cmdtab.cpp b/src/wx/config/cmdtab.cpp index 5949d695..b50f732b 100644 --- a/src/wx/config/cmdtab.cpp +++ b/src/wx/config/cmdtab.cpp @@ -1,5 +1,7 @@ #include "wx/config/cmdtab.h" +#include + #include // 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) { - return wxStrcmp(cmd1.cmd, cmd2.cmd) < 0; -} +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 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 diff --git a/src/wx/config/cmdtab.h b/src/wx/config/cmdtab.h index a360a3cc..0e817426 100644 --- a/src/wx/config/cmdtab.h +++ b/src/wx/config/cmdtab.h @@ -3,6 +3,8 @@ #include +#include + #include // Forward declaration. @@ -29,8 +31,29 @@ cmditem new_cmditem(const wxString cmd = "", int mask_flags = 0, wxMenuItem* mi = nullptr); -// for binary search -bool cmditem_lt(const struct cmditem& cmd1, const struct cmditem& cmd2); +namespace config { + // 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 CommandFromConfigString(const wxString& config); +} // here are those conditions enum { CMDEN_GB = (1 << 0), // GB ROM loaded diff --git a/src/wx/config/command.cpp b/src/wx/config/command.cpp index f8c3db31..963d53fe 100644 --- a/src/wx/config/command.cpp +++ b/src/wx/config/command.cpp @@ -1,6 +1,5 @@ #include "wx/config/command.h" -#include #include #include @@ -109,15 +108,7 @@ wxString GameCommand::ToUXString() const { } wxString ShortcutCommand::ToConfigString() const { - for (const cmditem& cmd_item : cmdtab) { - 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; + return GetCommandINIEntry(id_); } // static @@ -158,14 +149,13 @@ nonstd::optional Command::FromString(const wxString& name) { return nonstd::nullopt; } - const auto iter = std::lower_bound(cmdtab.begin(), cmdtab.end(), - cmditem{parts[1], wxString(), 0, 0, NULL}, cmditem_lt); - if (iter == cmdtab.end() || iter->cmd != parts[1]) { + const auto xrc_id = CommandFromConfigString(parts[1]); + if (!xrc_id.has_value()) { wxLogDebug("Command ID %s not found", parts[1]); return nonstd::nullopt; } - return Command(ShortcutCommand(iter->cmd_id)); + return Command(ShortcutCommand(xrc_id.value())); } } diff --git a/src/wx/config/internal/bindings-internal.cpp b/src/wx/config/internal/bindings-internal.cpp index 5e86e02b..5916dcb5 100644 --- a/src/wx/config/internal/bindings-internal.cpp +++ b/src/wx/config/internal/bindings-internal.cpp @@ -26,7 +26,7 @@ const std::unordered_map>& 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>& 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>& 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>& 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>& 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. diff --git a/src/wx/dialogs/accel-config.cpp b/src/wx/dialogs/accel-config.cpp index 22ffa866..b3b2e9f5 100644 --- a/src/wx/dialogs/accel-config.cpp +++ b/src/wx/dialogs/accel-config.cpp @@ -59,23 +59,15 @@ void AppendItemToTree(std::unordered_map* int command, const wxString& prefix, int level) { - wxString name; - for (const cmditem& cmd_item : cmdtab) { - if (command == cmd_item.cmd_id) { - name = cmd_item.name; - break; - } - } - assert(!name.empty()); - + const wxString helper = config::GetCommandHelper(command); 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); } diff --git a/src/wx/opts.cpp b/src/wx/opts.cpp index cebd63d1..52affac5 100644 --- a/src/wx/opts.cpp +++ b/src/wx/opts.cpp @@ -1,6 +1,5 @@ #include "wx/opts.h" -#include #include #include #include @@ -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 (!std::binary_search(cmdtab.begin(), cmdtab.end(), dummy, cmditem_lt)) { + if (!config::CommandFromConfigString(e).has_value()) { 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; - 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->Write(iter.first, iter.second); } - cfg->SetPath("/"); // For joypads, we just compare the strings.