Filtering out duplicate shortcuts per action

mikekazakov · Dec 27, 2024 · 0390dad · 0390dad
1 parent e9d15b8
commit 0390dad
Show file tree

Hide file tree

Showing 4 changed files with 70 additions and 13 deletions.
diff --git 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<int, Shortcuts> m_ShortcutsDefaults;

diff --git 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
@@ -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
@@ -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<NonPersistentOverwritesStorage>("")};
+    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")});
+    }
+}