From 0390dad84906e2749a177e4b02b9ee0f5b364e93 Mon Sep 17 00:00:00 2001 From: Michael Kazakov Date: Fri, 27 Dec 2024 15:03:43 +0000 Subject: [PATCH] Filtering out duplicate shortcuts per action --- .../Core/ActionsShortcutsManager.h | 3 +- .../Core/ActionsShortcutsManager.mm | 27 ++++++++---- .../PreferencesWindowHotkeysTab.mm | 12 ++++-- .../Tests/ActionsShortcutsManager_UT.cpp | 41 +++++++++++++++++++ 4 files changed, 70 insertions(+), 13 deletions(-) diff --git a/Source/NimbleCommander/NimbleCommander/Core/ActionsShortcutsManager.h b/Source/NimbleCommander/NimbleCommander/Core/ActionsShortcutsManager.h index 07aba163c..2ac6b63b5 100644 --- a/Source/NimbleCommander/NimbleCommander/Core/ActionsShortcutsManager.h +++ b/Source/NimbleCommander/NimbleCommander/Core/ActionsShortcutsManager.h @@ -120,7 +120,8 @@ class ActionsShortcutsManager : nc::base::ObservableBase void UnregisterShortcutUsage(Shortcut _shortcut, int _tag) noexcept; // Returns a container without empty shortcuts, while preserving the original relative order of the remaining items. - static Shortcuts WithoutEmptyShortcuts(const Shortcuts &_shortcuts) noexcept; + // Duplicates are removed as well. + static Shortcuts SanitizedShortcuts(const Shortcuts &_shortcuts) noexcept; // Maps an action tag to the default ordered list of its shortcuts. ankerl::unordered_dense::map m_ShortcutsDefaults; diff --git a/Source/NimbleCommander/NimbleCommander/Core/ActionsShortcutsManager.mm b/Source/NimbleCommander/NimbleCommander/Core/ActionsShortcutsManager.mm index ce0f621bc..f51417f84 100644 --- a/Source/NimbleCommander/NimbleCommander/Core/ActionsShortcutsManager.mm +++ b/Source/NimbleCommander/NimbleCommander/Core/ActionsShortcutsManager.mm @@ -462,7 +462,7 @@ static constexpr auto make_array_n(T &&value) // Set up the shortcut defaults from the hardcoded map for( auto [action, shortcut_string] : g_DefaultShortcuts ) { if( auto it = g_ActionToTag.find(std::string_view{action}); it != g_ActionToTag.end() ) { - m_ShortcutsDefaults[it->second] = WithoutEmptyShortcuts(Shortcuts{Shortcut{shortcut_string}}); + m_ShortcutsDefaults[it->second] = SanitizedShortcuts(Shortcuts{Shortcut{shortcut_string}}); } } @@ -536,7 +536,7 @@ static constexpr auto make_array_n(T &&value) continue; if( it->value.GetType() == kStringType ) { - m_ShortcutsOverrides[att->second] = WithoutEmptyShortcuts(Shortcuts{Shortcut{it->value.GetString()}}); + m_ShortcutsOverrides[att->second] = SanitizedShortcuts(Shortcuts{Shortcut{it->value.GetString()}}); } if( it->value.GetType() == kArrayType ) { Shortcuts shortcuts; @@ -546,7 +546,7 @@ static constexpr auto make_array_n(T &&value) if( shortcut.IsString() ) shortcuts.push_back(Shortcut{shortcut.GetString()}); } - m_ShortcutsOverrides[att->second] = WithoutEmptyShortcuts(shortcuts); + m_ShortcutsOverrides[att->second] = SanitizedShortcuts(shortcuts); } } } @@ -633,7 +633,7 @@ static constexpr auto make_array_n(T &&value) if( default_it == m_ShortcutsDefaults.end() ) return false; // this should never happen - const Shortcuts new_shortcuts = WithoutEmptyShortcuts(Shortcuts(_shortcuts.begin(), _shortcuts.end())); + const Shortcuts new_shortcuts = SanitizedShortcuts(Shortcuts(_shortcuts.begin(), _shortcuts.end())); const auto override_it = m_ShortcutsOverrides.find(*tag); if( std::ranges::equal(default_it->second, new_shortcuts) ) { @@ -780,12 +780,23 @@ static constexpr auto make_array_n(T &&value) } } -// TODO: also should sanitize possible duplicates in the shortcuts -ActionsShortcutsManager::Shortcuts ActionsShortcutsManager::WithoutEmptyShortcuts(const Shortcuts &_shortcuts) noexcept +ActionsShortcutsManager::Shortcuts ActionsShortcutsManager::SanitizedShortcuts(const Shortcuts &_shortcuts) noexcept { Shortcuts shortcuts = _shortcuts; - auto to_erase = std::ranges::remove_if(shortcuts, [](const Shortcut &_sc) { return _sc == Shortcut{}; }); - shortcuts.erase(to_erase.begin(), to_erase.end()); + + // Remove any empty shortcuts. + { + auto to_erase = std::ranges::remove_if(shortcuts, [](const Shortcut &_sc) { return _sc == Shortcut{}; }); + shortcuts.erase(to_erase.begin(), to_erase.end()); + } + + // Remove any duplicates. + // Technically speaking this is O(N^2), but N is normally ~= 1, so it doesn't matter. + for( auto it = shortcuts.begin(); it != shortcuts.end(); ++it ) { + shortcuts.erase(std::remove_if(std::next(it), shortcuts.end(), [&](const Shortcut &_sc) { return _sc == *it; }), + shortcuts.end()); + } + return shortcuts; } diff --git a/Source/NimbleCommander/NimbleCommander/Preferences/PreferencesWindowHotkeysTab.mm b/Source/NimbleCommander/NimbleCommander/Preferences/PreferencesWindowHotkeysTab.mm index 896df74ff..b80aba602 100644 --- a/Source/NimbleCommander/NimbleCommander/Preferences/PreferencesWindowHotkeysTab.mm +++ b/Source/NimbleCommander/NimbleCommander/Preferences/PreferencesWindowHotkeysTab.mm @@ -321,9 +321,11 @@ - (NSView *)tableView:(NSTableView *) [[maybe_unused]] _table_view if( [_table_column.identifier isEqualToString:@"action"] ) { return SpawnLabelForAction(*node); } - - const std::array columns{ - self.firstShortcutColumn, self.secondShortcutColumn, self.thirdShortcutColumn, self.fourthShortcutColumn}; + + const std::array columns{self.firstShortcutColumn, + self.secondShortcutColumn, + self.thirdShortcutColumn, + self.fourthShortcutColumn}; if( auto it = std::ranges::find(columns, _table_column); it != columns.end() ) { const size_t shortcut_idx = std::distance(columns.begin(), it); @@ -440,8 +442,10 @@ - (IBAction)onHKChanged:(id)sender if( am.SetShortcutsOverride(*action, updated_shortcuts) ) { am.SetMenuShortcuts(NSApp.mainMenu); - [self rebuildAll]; } + + // Rebuild everything just in case the shortcut was a duplicate that ASM filtered out + [self rebuildAll]; } - (IBAction)OnDefaults:(id) [[maybe_unused]] sender diff --git a/Source/NimbleCommander/NimbleCommander/Tests/ActionsShortcutsManager_UT.cpp b/Source/NimbleCommander/NimbleCommander/Tests/ActionsShortcutsManager_UT.cpp index d4fd6ce47..3b74f4538 100644 --- a/Source/NimbleCommander/NimbleCommander/Tests/ActionsShortcutsManager_UT.cpp +++ b/Source/NimbleCommander/NimbleCommander/Tests/ActionsShortcutsManager_UT.cpp @@ -317,3 +317,44 @@ TEST_CASE(PREFIX "Configuration persistence") REQUIRE(config.Get("hotkeyOverrides_v1") == expected_config.Get("hotkeyOverrides_v1")); } } + +TEST_CASE(PREFIX "SetShortcutsOverride") +{ + ConfigImpl config{g_EmptyConfigJSON, std::make_shared("")}; + ASM manager{config}; + SECTION("Empty") + { + REQUIRE(manager.SetShortcutsOverride("menu.edit.copy", {})); + REQUIRE(manager.ShortcutsFromAction("menu.edit.copy") == ASs{}); + } + SECTION("Single") + { + REQUIRE(manager.SetShortcutsOverride("menu.edit.copy", std::array{AS("⌘j")})); + REQUIRE(manager.ShortcutsFromAction("menu.edit.copy") == ASs{AS("⌘j")}); + } + SECTION("Single and empty") + { + REQUIRE(manager.SetShortcutsOverride("menu.edit.copy", std::array{AS(), AS("⌘j"), AS()})); + REQUIRE(manager.ShortcutsFromAction("menu.edit.copy") == ASs{AS("⌘j")}); + } + SECTION("Single and duplicates") + { + REQUIRE(manager.SetShortcutsOverride("menu.edit.copy", std::array{AS("⌘j"), AS("⌘j")})); + REQUIRE(manager.ShortcutsFromAction("menu.edit.copy") == ASs{AS("⌘j")}); + } + SECTION("Two") + { + REQUIRE(manager.SetShortcutsOverride("menu.edit.copy", std::array{AS("⌘j"), AS("⌘k")})); + REQUIRE(manager.ShortcutsFromAction("menu.edit.copy") == ASs{AS("⌘j"), AS("⌘k")}); + } + SECTION("Two and empty") + { + REQUIRE(manager.SetShortcutsOverride("menu.edit.copy", std::array{AS(), AS("⌘j"), AS(), AS("⌘k"), AS()})); + REQUIRE(manager.ShortcutsFromAction("menu.edit.copy") == ASs{AS("⌘j"), AS("⌘k")}); + } + SECTION("Two and duplicates") + { + REQUIRE(manager.SetShortcutsOverride("menu.edit.copy", std::array{AS("⌘j"), AS("⌘k"), AS("⌘j"), AS("⌘k")})); + REQUIRE(manager.ShortcutsFromAction("menu.edit.copy") == ASs{AS("⌘j"), AS("⌘k")}); + } +}