From 9bd31759d6cfa29cccc9daa3cb7df84a55a119dd Mon Sep 17 00:00:00 2001 From: Michael Kazakov Date: Thu, 26 Dec 2024 18:17:10 +0000 Subject: [PATCH] Refactoring ActionShortcutsManager's API and internals to support multiple shortcuts per action --- .../Bootstrap/AppDelegate+ViewerCreation.mm | 6 +- .../Core/ActionsShortcutsManager.h | 34 +++- .../Core/ActionsShortcutsManager.mm | 130 ++++++++------- .../PreferencesWindowHotkeysTab.mm | 71 +++++---- ...ainWindowFilePanelsStateToolbarDelegate.mm | 6 +- .../Tests/ActionsShortcutsManager_UT.cpp | 148 +++++++++++------- 6 files changed, 247 insertions(+), 148 deletions(-) diff --git a/Source/NimbleCommander/NimbleCommander/Bootstrap/AppDelegate+ViewerCreation.mm b/Source/NimbleCommander/NimbleCommander/Bootstrap/AppDelegate+ViewerCreation.mm index 91cade132..73a0b781b 100644 --- a/Source/NimbleCommander/NimbleCommander/Bootstrap/AppDelegate+ViewerCreation.mm +++ b/Source/NimbleCommander/NimbleCommander/Bootstrap/AppDelegate+ViewerCreation.mm @@ -20,8 +20,10 @@ - (NCViewerView *)makeViewerWithFrame:(NSRect)frame - (NCViewerViewController *)makeViewerController { - auto shortcuts = [](std::string_view _name) { - return nc::core::ActionsShortcutsManager::Instance().ShortCutFromAction(_name).value(); + using nc::core::ActionsShortcutsManager; + auto shortcuts = [](std::string_view _name) -> ActionsShortcutsManager::ShortCut { + auto sc = ActionsShortcutsManager::Instance().ShortCutFromAction(_name).value(); + return sc.empty() ? ActionsShortcutsManager::ShortCut{} : sc.front(); }; return [[NCViewerViewController alloc] initWithHistory:self.internalViewerHistory config:self.globalConfig diff --git a/Source/NimbleCommander/NimbleCommander/Core/ActionsShortcutsManager.h b/Source/NimbleCommander/NimbleCommander/Core/ActionsShortcutsManager.h index e78b85ada..05f4bde9a 100644 --- a/Source/NimbleCommander/NimbleCommander/Core/ActionsShortcutsManager.h +++ b/Source/NimbleCommander/NimbleCommander/Core/ActionsShortcutsManager.h @@ -25,8 +25,15 @@ namespace nc::core { class ActionsShortcutsManager : nc::base::ObservableBase { public: + // Shortcut represents a key and its modifiers that have to be pressed to trigger an action. using ShortCut = nc::utility::ActionShortcut; + // An ordered list of shortcuts. + // The relative order of the shortcuts must be preserved as it has semantic meaning for e.g. menus. + // Empty shortcuts should not be stored in such vectors. + // An inlined vector is used to avoid memory allocating for such tiny memory blocks. + using ShortCuts = absl::InlinedVector; + // ActionTags represents a list of numberic action tags. // Normally they are tiny, thus an inline vector is used to avoid memory allocation. using ActionTags = absl::InlinedVector; @@ -46,16 +53,16 @@ class ActionsShortcutsManager : nc::base::ObservableBase // Returns a shortcut assigned to the specified action. // Returns std::nullopt such action cannot be found. // Overrides have priority over the default shortcuts. - std::optional ShortCutFromAction(std::string_view _action) const noexcept; + std::optional ShortCutFromAction(std::string_view _action) const noexcept; // Returns a shortcut assigned to the specified numeric action tag. // Returns std::nullopt such action cannot be found. // Overrides have priority over the default shortcuts. - std::optional ShortCutFromTag(int _tag) const noexcept; + std::optional ShortCutFromTag(int _tag) const noexcept; // Returns a default shortcut for an action specified by its numeric tag. // Returns std::nullopt such action cannot be found. - std::optional DefaultShortCutFromTag(int _tag) const noexcept; + std::optional DefaultShortCutFromTag(int _tag) const noexcept; // Returns an unordered list of numeric tags of actions that have the specified shortcut. // An optional domain parameter can be specified to filter the output by only leaving actions that have the @@ -72,10 +79,17 @@ class ActionsShortcutsManager : nc::base::ObservableBase // Removes any hotkeys overrides. void RevertToDefaults(); + // Sets the custom shortkey for the specified action. // Returns true if any change was done to the actions maps. // If the _action doesn't exist or already has the same value, returns false. + // This function is effectively a syntax sugar for SetShortCutsOverride(_action, {&_sc, 1}). bool SetShortCutOverride(std::string_view _action, ShortCut _sc); + // Sets the custom shortkeys for the specified action. + // Returns true if any change was done to the actions maps. + // If the _action doesn't exist or already has the same value, returns false. + bool SetShortCutsOverride(std::string_view _action, std::span _shortcuts); + #ifdef __OBJC__ void SetMenuShortCuts(NSMenu *_menu) const; #endif @@ -105,9 +119,19 @@ class ActionsShortcutsManager : nc::base::ObservableBase // Removes the specified actions tag from the list of action tags that use the specified shortcut. void UnregisterShortcutUsage(ShortCut _shortcut, int _tag) noexcept; - ankerl::unordered_dense::map m_ShortCutsDefaults; - ankerl::unordered_dense::map m_ShortCutsOverrides; + // Returns a container without empty shortcuts, while preserving the original relative order of the remaining items. + static ShortCuts WithoutEmptyShortCuts(const ShortCuts &_shortcuts) noexcept; + + // Maps an action tag to the default ordered list of its shortcuts. + ankerl::unordered_dense::map m_ShortCutsDefaults; + + // Maps an action tag to the overriden ordered list of its shortcuts. + ankerl::unordered_dense::map m_ShortCutsOverrides; + + // Maps a shortcut to an unordered list of action tags that use it. ankerl::unordered_dense::map m_ShortCutsUsage; + + // Config instance used to read from and write to the shortcut overrides. nc::config::Config &m_Config; }; diff --git a/Source/NimbleCommander/NimbleCommander/Core/ActionsShortcutsManager.mm b/Source/NimbleCommander/NimbleCommander/Core/ActionsShortcutsManager.mm index 67e703e5b..dee8d359e 100644 --- a/Source/NimbleCommander/NimbleCommander/Core/ActionsShortcutsManager.mm +++ b/Source/NimbleCommander/NimbleCommander/Core/ActionsShortcutsManager.mm @@ -460,9 +460,9 @@ static constexpr auto make_array_n(T &&value) .size() == std::size(g_ActionsTags)); // Set up the shortcut defaults from the hardcoded map - for( auto [action, shortcut] : g_DefaultShortcuts ) { - if( auto i = g_ActionToTag.find(std::string_view{action}); i != g_ActionToTag.end() ) { - m_ShortCutsDefaults[i->second] = ShortCut{shortcut}; + 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}}); } } @@ -499,22 +499,21 @@ static constexpr auto make_array_n(T &&value) for( NSMenuItem *i : array ) { if( i.submenu != nil ) { SetMenuShortCuts(i.submenu); + continue; } - else { - const int tag = static_cast(i.tag); - auto scover = m_ShortCutsOverrides.find(tag); - if( scover != m_ShortCutsOverrides.end() ) { - [i nc_setKeyEquivalentWithShortcut:scover->second]; - } - else { - auto sc = m_ShortCutsDefaults.find(tag); - if( sc != m_ShortCutsDefaults.end() ) { - [i nc_setKeyEquivalentWithShortcut:sc->second]; - } - else if( g_TagToAction.find(tag) != g_TagToAction.end() ) { - [i nc_setKeyEquivalentWithShortcut:nc::utility::ActionShortcut{}]; - } - } + + const int tag = static_cast(i.tag); + if( auto shortcut_override = m_ShortCutsOverrides.find(tag); shortcut_override != m_ShortCutsOverrides.end() ) { + const auto &shortcuts = shortcut_override->second; + [i nc_setKeyEquivalentWithShortcut:shortcuts.empty() ? ShortCut{} : shortcuts.front()]; + } + else if( auto shortcut_defaults = m_ShortCutsDefaults.find(tag); + shortcut_defaults != m_ShortCutsDefaults.end() ) { + const auto &shortcuts = shortcut_defaults->second; + [i nc_setKeyEquivalentWithShortcut:shortcuts.empty() ? ShortCut{} : shortcuts.front()]; + } + else if( g_TagToAction.contains(tag) ) { + [i nc_setKeyEquivalentWithShortcut:ShortCut{}]; } } } @@ -528,15 +527,18 @@ static constexpr auto make_array_n(T &&value) return; m_ShortCutsOverrides.clear(); - for( auto i = v.MemberBegin(), e = v.MemberEnd(); i != e; ++i ) + for( auto i = v.MemberBegin(), e = v.MemberEnd(); i != e; ++i ) { if( i->name.GetType() == kStringType && i->value.GetType() == kStringType ) { auto att = g_ActionToTag.find(std::string_view{i->name.GetString()}); - if( att != g_ActionToTag.end() ) - m_ShortCutsOverrides[att->second] = nc::utility::ActionShortcut{i->value.GetString()}; + if( att != g_ActionToTag.end() ) { + m_ShortCutsOverrides[att->second] = WithoutEmptyShortCuts(ShortCuts{ShortCut{i->value.GetString()}}); + } } + // TODO: support for array values when multiple shortcuts are stored + } } -std::optional +std::optional ActionsShortcutsManager::ShortCutFromAction(std::string_view _action) const noexcept { const std::optional tag = TagFromAction(_action); @@ -545,26 +547,25 @@ static constexpr auto make_array_n(T &&value) return ShortCutFromTag(*tag); } -std::optional ActionsShortcutsManager::ShortCutFromTag(int _tag) const noexcept +std::optional ActionsShortcutsManager::ShortCutFromTag(int _tag) const noexcept { - auto sc_override = m_ShortCutsOverrides.find(_tag); - if( sc_override != m_ShortCutsOverrides.end() ) + if( auto sc_override = m_ShortCutsOverrides.find(_tag); sc_override != m_ShortCutsOverrides.end() ) { return sc_override->second; + } - auto sc_default = m_ShortCutsDefaults.find(_tag); - if( sc_default != m_ShortCutsDefaults.end() ) + if( auto sc_default = m_ShortCutsDefaults.find(_tag); sc_default != m_ShortCutsDefaults.end() ) { return sc_default->second; + } return {}; } -std::optional +std::optional ActionsShortcutsManager::DefaultShortCutFromTag(int _tag) const noexcept { - auto sc_default = m_ShortCutsDefaults.find(_tag); - if( sc_default != m_ShortCutsDefaults.end() ) + if( auto sc_default = m_ShortCutsDefaults.find(_tag); sc_default != m_ShortCutsDefaults.end() ) { return sc_default->second; - + } return {}; } @@ -605,6 +606,11 @@ static constexpr auto make_array_n(T &&value) } bool ActionsShortcutsManager::SetShortCutOverride(const std::string_view _action, const ShortCut _sc) +{ + return SetShortCutsOverride(_action, std::span{&_sc, 1}); +} + +bool ActionsShortcutsManager::SetShortCutsOverride(std::string_view _action, std::span _shortcuts) { const std::optional tag = TagFromAction(_action); if( !tag ) @@ -614,9 +620,10 @@ static constexpr auto make_array_n(T &&value) if( default_it == m_ShortCutsDefaults.end() ) return false; // this should never happen - const auto override_it = m_ShortCutsOverrides.find(*tag); + const ShortCuts new_shortcuts = WithoutEmptyShortCuts(ShortCuts(_shortcuts.begin(), _shortcuts.end())); - if( default_it->second == _sc ) { + const auto override_it = m_ShortCutsOverrides.find(*tag); + if( std::ranges::equal(default_it->second, new_shortcuts) ) { // The shortcut is same as the default one for this action if( override_it == m_ShortCutsOverrides.end() ) { @@ -624,13 +631,13 @@ static constexpr auto make_array_n(T &&value) return false; } - // Unregister the usage of the override shortcut - UnregisterShortcutUsage(override_it->second, *tag); + // Unregister the usage of the override shortcuts + for( const ShortCut &shortcut : override_it->second ) + UnregisterShortcutUsage(shortcut, *tag); - // Register the usage of the default shortcut if it's defined - if( default_it->second ) { - RegisterShortcutUsage(default_it->second, *tag); - } + // Register the usage of the default shortcuts + for( const ShortCut &shortcut : default_it->second ) + RegisterShortcutUsage(shortcut, *tag); // Remove the override m_ShortCutsOverrides.erase(*tag); @@ -638,22 +645,22 @@ static constexpr auto make_array_n(T &&value) else { // The shortcut is not the same as the default for this action - if( override_it != m_ShortCutsOverrides.end() && override_it->second == _sc ) { + if( override_it != m_ShortCutsOverrides.end() && override_it->second == new_shortcuts ) { return false; // Nothing new, it's the same as currently defined in the overrides } if( override_it != m_ShortCutsOverrides.end() ) { - // Unregister the usage of the override shortcut - UnregisterShortcutUsage(override_it->second, *tag); + // Unregister the usage of the override shortcuts + for( const ShortCut &shortcut : override_it->second ) + UnregisterShortcutUsage(shortcut, *tag); } - if( _sc ) { - // Register the usage of the new override if it's defined - RegisterShortcutUsage(_sc, *tag); - } + // Register the usage of the new override shortcuts + for( const ShortCut &shortcut : new_shortcuts ) + RegisterShortcutUsage(shortcut, *tag); // Set the override - m_ShortCutsOverrides[*tag] = _sc; + m_ShortCutsOverrides[*tag] = new_shortcuts; } // immediately write to config file @@ -674,12 +681,15 @@ static constexpr auto make_array_n(T &&value) using namespace rapidjson; nc::config::Value overrides{kObjectType}; + // TODO: add support for storing multiple shortcuts for( auto &i : g_ActionsTags ) { auto scover = m_ShortCutsOverrides.find(i.second); - if( scover != end(m_ShortCutsOverrides) ) + if( scover != m_ShortCutsOverrides.end() ) { + const std::string shortcut = scover->second.empty() ? std::string{} : scover->second.front().ToPersString(); overrides.AddMember(nc::config::MakeStandaloneString(i.first), - nc::config::MakeStandaloneString(scover->second.ToPersString()), + nc::config::MakeStandaloneString(shortcut), nc::config::g_CrtAllocator); + } } m_Config.Set(g_OverridesConfigPath, overrides); @@ -732,16 +742,28 @@ static constexpr auto make_array_n(T &&value) { m_ShortCutsUsage.clear(); // build the map means starting from scratch - for( const auto [tag, default_shortcut] : m_ShortCutsDefaults ) { + for( const auto [tag, default_shortcuts] : m_ShortCutsDefaults ) { if( const auto it = m_ShortCutsOverrides.find(tag); it != m_ShortCutsOverrides.end() ) { - if( it->second ) - RegisterShortcutUsage(it->second, tag); + for( const ShortCut &shortcut : it->second ) { + if( shortcut ) + RegisterShortcutUsage(shortcut, tag); + } } else { - if( default_shortcut ) - RegisterShortcutUsage(default_shortcut, tag); + for( const ShortCut &shortcut : default_shortcuts ) { + if( shortcut ) + RegisterShortcutUsage(shortcut, tag); + } } } } +ActionsShortcutsManager::ShortCuts ActionsShortcutsManager::WithoutEmptyShortCuts(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()); + return shortcuts; +} + } // namespace nc::core diff --git a/Source/NimbleCommander/NimbleCommander/Preferences/PreferencesWindowHotkeysTab.mm b/Source/NimbleCommander/NimbleCommander/Preferences/PreferencesWindowHotkeysTab.mm index e3bc30340..69db3b6be 100644 --- a/Source/NimbleCommander/NimbleCommander/Preferences/PreferencesWindowHotkeysTab.mm +++ b/Source/NimbleCommander/NimbleCommander/Preferences/PreferencesWindowHotkeysTab.mm @@ -10,9 +10,13 @@ #include #include #include +#include +#include #include +using nc::core::ActionsShortcutsManager; using nc::panel::ExternalTool; +using nc::utility::ActionShortcut; static NSString *ComposeVerboseMenuItemTitle(NSMenuItem *_item); static NSString *ComposeVerboseNonMenuActionTitle(const std::string &_action); @@ -23,8 +27,8 @@ struct ActionShortcutNode { std::pair tag; - nc::utility::ActionShortcut current_shortcut; - nc::utility::ActionShortcut default_shortcut; + nc::core::ActionsShortcutsManager::ShortCuts current_shortcuts; + nc::core::ActionsShortcutsManager::ShortCuts default_shortcuts; NSString *label = @""; bool is_menu_action = false; bool has_submenu = false; @@ -124,7 +128,7 @@ - (void)buildData { const auto &sm = nc::core::ActionsShortcutsManager::Instance(); m_AllNodes.clear(); - std::unordered_map counts; + ankerl::unordered_dense::map counts; for( auto &v : m_Shortcuts ) { if( v.first == "menu.file.open_with_submenu" || v.first == "menu.file.always_open_with_submenu" ) { // Skip the menu items that are actually placeholders for submenus as shortcuts don't work for them. @@ -137,14 +141,16 @@ - (void)buildData ActionShortcutNode shortcut; shortcut.tag = v; shortcut.label = LabelTitleForAction(v.first, menu_item); - shortcut.current_shortcut = sm.ShortCutFromTag(v.second).value(); - shortcut.default_shortcut = sm.DefaultShortCutFromTag(v.second).value(); + shortcut.current_shortcuts = sm.ShortCutFromTag(v.second).value(); + shortcut.default_shortcuts = sm.DefaultShortCutFromTag(v.second).value(); shortcut.is_menu_action = v.first.find_first_of("menu.") == 0; - shortcut.is_customized = shortcut.current_shortcut != shortcut.default_shortcut; + shortcut.is_customized = shortcut.current_shortcuts != shortcut.default_shortcuts; shortcut.has_submenu = menu_item != nil && menu_item.hasSubmenu; shortcut.participates_in_conflicts = ParticipatesInConflicts(v.first); - if( shortcut.participates_in_conflicts ) - counts[shortcut.current_shortcut]++; + if( shortcut.participates_in_conflicts ) { + for( auto &sc : shortcut.current_shortcuts ) + counts[sc]++; + } m_AllNodes.emplace_back(std::move(shortcut)); } @@ -164,8 +170,8 @@ - (void)buildData if( auto node = std::any_cast(&v) ) { if( !node->participates_in_conflicts ) continue; - - node->is_conflicted = node->current_shortcut && counts[node->current_shortcut] > 1; + node->is_conflicted = + std::ranges::any_of(node->current_shortcuts, [&](auto &_sc) { return counts[_sc] > 1; }); if( node->is_conflicted ) conflicts_amount++; } @@ -297,27 +303,34 @@ - (GTMHotKeyTextField *)makeDefaultGTMHotKeyTextField return iv; } -- (NSView *)tableView:(NSTableView *) [[maybe_unused]] tableView - viewForTableColumn:(NSTableColumn *)tableColumn - row:(NSInteger)row +- (NSView *)tableView:(NSTableView *) [[maybe_unused]] _table_view + viewForTableColumn:(NSTableColumn *)_table_column + row:(NSInteger)_row { - if( row >= 0 && row < static_cast(m_FilteredNodes.size()) ) { - if( auto node = std::any_cast(&m_FilteredNodes[row]) ) { - if( [tableColumn.identifier isEqualToString:@"action"] ) { + auto first_or_empty = [](const ActionsShortcutsManager::ShortCuts &_shortcuts) -> ActionShortcut { + return _shortcuts.empty() ? ActionShortcut{} : _shortcuts.front(); + }; + + if( _row >= 0 && _row < static_cast(m_FilteredNodes.size()) ) { + if( auto node = std::any_cast(&m_FilteredNodes[_row]) ) { + if( [_table_column.identifier isEqualToString:@"action"] ) { return SpawnLabelForAction(*node); } - if( [tableColumn.identifier isEqualToString:@"hotkey"] ) { + if( [_table_column.identifier isEqualToString:@"hotkey"] ) { const auto key_text_field = [self makeDefaultGTMHotKeyTextField]; assert(key_text_field); key_text_field.action = @selector(onHKChanged:); key_text_field.target = self; key_text_field.tag = node->tag.second; + const ActionShortcut default_shortcut = first_or_empty(node->default_shortcuts); + const ActionShortcut current_shortcut = first_or_empty(node->current_shortcuts); + const auto field_cell = nc::objc_cast(key_text_field.cell); - field_cell.hotKey = [GTMHotKey hotKeyWithKey:node->current_shortcut.Key() - modifiers:node->current_shortcut.modifiers]; - field_cell.defaultHotKey = [GTMHotKey hotKeyWithKey:node->default_shortcut.Key() - modifiers:node->default_shortcut.modifiers]; + field_cell.hotKey = [GTMHotKey hotKeyWithKey:current_shortcut.Key() + modifiers:current_shortcut.modifiers]; + field_cell.defaultHotKey = [GTMHotKey hotKeyWithKey:default_shortcut.Key() + modifiers:default_shortcut.modifiers]; field_cell.menuHotKey = node->is_menu_action; if( node->is_customized ) @@ -327,15 +340,15 @@ - (NSView *)tableView:(NSTableView *) [[maybe_unused]] tableView return key_text_field; } - if( [tableColumn.identifier isEqualToString:@"flag"] ) { + if( [_table_column.identifier isEqualToString:@"flag"] ) { return node->is_conflicted ? SpawnCautionSign() : nil; } } - if( auto node = std::any_cast(&m_FilteredNodes[row]) ) { - if( [tableColumn.identifier isEqualToString:@"action"] ) { + if( auto node = std::any_cast(&m_FilteredNodes[_row]) ) { + if( [_table_column.identifier isEqualToString:@"action"] ) { return SpawnLabelForTool(*node); } - if( [tableColumn.identifier isEqualToString:@"hotkey"] ) { + if( [_table_column.identifier isEqualToString:@"hotkey"] ) { const auto &tool = *node->tool; const auto key_text_field = [self makeDefaultGTMHotKeyTextField]; assert(key_text_field); @@ -352,7 +365,7 @@ - (NSView *)tableView:(NSTableView *) [[maybe_unused]] tableView return key_text_field; } - if( [tableColumn.identifier isEqualToString:@"flag"] ) { + if( [_table_column.identifier isEqualToString:@"flag"] ) { return node->is_conflicted ? SpawnCautionSign() : nil; } } @@ -448,8 +461,10 @@ static bool ValidateNodeForFilter(const std::any &_node, NSString *_filter) if( [scid rangeOfString:_filter options:NSCaseInsensitiveSearch].length != 0 ) return true; - const auto prettry_hotkey = node->current_shortcut.PrettyString(); - return [prettry_hotkey rangeOfString:_filter options:NSCaseInsensitiveSearch].length != 0; + return std::ranges::any_of(node->current_shortcuts, [&](const ActionShortcut &_sc) { + const auto prettry_hotkey = _sc.PrettyString(); + return [prettry_hotkey rangeOfString:_filter options:NSCaseInsensitiveSearch].length != 0; + }); } if( auto node = std::any_cast(&_node) ) { const auto label = node->label; diff --git a/Source/NimbleCommander/NimbleCommander/States/FilePanels/MainWindowFilePanelsStateToolbarDelegate.mm b/Source/NimbleCommander/NimbleCommander/States/FilePanels/MainWindowFilePanelsStateToolbarDelegate.mm index 9ff45079c..865f09c41 100644 --- a/Source/NimbleCommander/NimbleCommander/States/FilePanels/MainWindowFilePanelsStateToolbarDelegate.mm +++ b/Source/NimbleCommander/NimbleCommander/States/FilePanels/MainWindowFilePanelsStateToolbarDelegate.mm @@ -172,14 +172,16 @@ - (NSToolbarItem *)toolbar:(NSToolbar *) [[maybe_unused]] _toolbar NSToolbarItem *item = [[NSToolbarItem alloc] initWithItemIdentifier:itemIdentifier]; item.view = m_LeftPanelGoToButton; item.paletteLabel = item.label = NSLocalizedString(@"Left GoTo", "Toolbar palette"); - item.toolTip = actman.ShortCutFromAction("menu.go.left_panel").value().PrettyString(); + auto shortcuts = actman.ShortCutFromAction("menu.go.left_panel").value(); + item.toolTip = shortcuts.empty() ? @"" : shortcuts.front().PrettyString(); return item; } if( [itemIdentifier isEqualToString:@"filepanels_right_goto_button"] ) { NSToolbarItem *item = [[NSToolbarItem alloc] initWithItemIdentifier:itemIdentifier]; item.view = m_RightPanelGoToButton; item.paletteLabel = item.label = NSLocalizedString(@"Right GoTo", "Toolbar palette"); - item.toolTip = actman.ShortCutFromAction("menu.go.right_panel").value().PrettyString(); + auto shortcuts = actman.ShortCutFromAction("menu.go.right_panel").value(); + item.toolTip = shortcuts.empty() ? @"" : shortcuts.front().PrettyString(); return item; } if( [itemIdentifier isEqualToString:@"operations_pool"] ) { diff --git a/Source/NimbleCommander/NimbleCommander/Tests/ActionsShortcutsManager_UT.cpp b/Source/NimbleCommander/NimbleCommander/Tests/ActionsShortcutsManager_UT.cpp index 35735d450..23911b277 100644 --- a/Source/NimbleCommander/NimbleCommander/Tests/ActionsShortcutsManager_UT.cpp +++ b/Source/NimbleCommander/NimbleCommander/Tests/ActionsShortcutsManager_UT.cpp @@ -5,9 +5,9 @@ #include using Catch::Matchers::UnorderedEquals; -using nc::core::ActionsShortcutsManager; -using nc::utility::ActionShortcut; -using ASM = ActionsShortcutsManager; +using ASM = nc::core::ActionsShortcutsManager; +using AS = ASM::ShortCut; +using ASs = ASM::ShortCuts; #define PREFIX "nc::core::ActionsShortcutsManager " @@ -17,113 +17,148 @@ static const auto g_EmptyConfigJSON = R"({ TEST_CASE(PREFIX "TagFromAction") { - CHECK(ActionsShortcutsManager::TagFromAction("menu.edit.copy") == 12'000); // Valid query - CHECK(ActionsShortcutsManager::TagFromAction("menu.i.dont.exist") == std::nullopt); // Invalid query + CHECK(ASM::TagFromAction("menu.edit.copy") == 12'000); // Valid query + CHECK(ASM::TagFromAction("menu.i.dont.exist") == std::nullopt); // Invalid query } TEST_CASE(PREFIX "ActionFromTag") { - CHECK(ActionsShortcutsManager::ActionFromTag(12'000) == "menu.edit.copy"); // Valid query - CHECK(ActionsShortcutsManager::ActionFromTag(346'242) == std::nullopt); // Invalid query + CHECK(ASM::ActionFromTag(12'000) == "menu.edit.copy"); // Valid query + CHECK(ASM::ActionFromTag(346'242) == std::nullopt); // Invalid query } TEST_CASE(PREFIX "ShortCutFromAction") { nc::config::ConfigImpl config{g_EmptyConfigJSON, std::make_shared("")}; - ActionsShortcutsManager manager{config}; + ASM manager{config}; - REQUIRE(manager.ShortCutFromAction("menu.i.dont.exist") == std::nullopt); - REQUIRE(manager.ShortCutFromAction("menu.edit.copy") == ActionShortcut("⌘c")); - REQUIRE(manager.SetShortCutOverride("menu.edit.copy", ActionShortcut("⌘j"))); - REQUIRE(manager.ShortCutFromAction("menu.edit.copy") == ActionShortcut("⌘j")); + SECTION("Non-existent") + { + REQUIRE(manager.ShortCutFromAction("menu.i.dont.exist") == std::nullopt); + } + SECTION("Default value") + { + REQUIRE(manager.ShortCutFromAction("menu.edit.copy") == ASs{AS("⌘c")}); + } + SECTION("Override with a single shortcut") + { + REQUIRE(manager.SetShortCutOverride("menu.edit.copy", AS("⌘j"))); + REQUIRE(manager.ShortCutFromAction("menu.edit.copy") == ASs{AS("⌘j")}); + } + SECTION("Override with an empty shortcut") + { + REQUIRE(manager.SetShortCutOverride("menu.edit.copy", AS())); + REQUIRE(manager.ShortCutFromAction("menu.edit.copy") == ASs{}); + } + SECTION("Override with two shortcuts") + { + REQUIRE(manager.SetShortCutsOverride("menu.edit.copy", std::array{AS("⌘j"), AS("⌘k")})); + REQUIRE(manager.ShortCutFromAction("menu.edit.copy") == ASs{AS("⌘j"), AS("⌘k")}); + } + SECTION("Override with two shortcuts and some empty bogus ones") + { + REQUIRE(manager.SetShortCutsOverride("menu.edit.copy", std::array{AS(), AS("⌘j"), AS(), AS("⌘k"), AS()})); + REQUIRE(manager.ShortCutFromAction("menu.edit.copy") == ASs{AS("⌘j"), AS("⌘k")}); + } } TEST_CASE(PREFIX "ShortCutFromTag") { nc::config::ConfigImpl config{g_EmptyConfigJSON, std::make_shared("")}; - ActionsShortcutsManager manager{config}; + ASM manager{config}; REQUIRE(manager.ShortCutFromTag(346'242) == std::nullopt); - REQUIRE(manager.ShortCutFromTag(12'000) == ActionShortcut("⌘c")); - REQUIRE(manager.SetShortCutOverride("menu.edit.copy", ActionShortcut("⌘j"))); - REQUIRE(manager.ShortCutFromTag(12'000) == ActionShortcut("⌘j")); + REQUIRE(manager.ShortCutFromTag(12'000) == ASs{AS("⌘c")}); + REQUIRE(manager.SetShortCutOverride("menu.edit.copy", AS("⌘j"))); + REQUIRE(manager.ShortCutFromTag(12'000) == ASs{AS("⌘j")}); } TEST_CASE(PREFIX "DefaultShortCutFromTag") { nc::config::ConfigImpl config{g_EmptyConfigJSON, std::make_shared("")}; - ActionsShortcutsManager manager{config}; + ASM manager{config}; REQUIRE(manager.DefaultShortCutFromTag(346'242) == std::nullopt); - REQUIRE(manager.DefaultShortCutFromTag(12'000) == ActionShortcut("⌘c")); - REQUIRE(manager.SetShortCutOverride("menu.edit.copy", ActionShortcut("⌘j"))); - REQUIRE(manager.DefaultShortCutFromTag(12'000) == ActionShortcut("⌘c")); + REQUIRE(manager.DefaultShortCutFromTag(12'000) == ASs{AS("⌘c")}); + REQUIRE(manager.SetShortCutOverride("menu.edit.copy", AS("⌘j"))); + REQUIRE(manager.DefaultShortCutFromTag(12'000) == ASs{AS("⌘c")}); } TEST_CASE(PREFIX "RevertToDefaults") { nc::config::ConfigImpl config{g_EmptyConfigJSON, std::make_shared("")}; - ActionsShortcutsManager manager{config}; + ASM manager{config}; - REQUIRE(manager.SetShortCutOverride("menu.edit.copy", ActionShortcut("⌘j"))); + REQUIRE(manager.SetShortCutOverride("menu.edit.copy", AS("⌘j"))); manager.RevertToDefaults(); - REQUIRE(manager.ShortCutFromAction("menu.edit.copy") == ActionShortcut("⌘c")); + REQUIRE(manager.ShortCutFromAction("menu.edit.copy") == ASs{AS("⌘c")}); } TEST_CASE(PREFIX "ActionTagsFromShortCut") { nc::config::ConfigImpl config{g_EmptyConfigJSON, std::make_shared("")}; - ActionsShortcutsManager manager{config}; + ASM manager{config}; SECTION("Non-existent shortcut") { - REQUIRE(manager.ActionTagsFromShortCut(ActionShortcut("⇧^⌘⌥j")) == std::nullopt); + REQUIRE(manager.ActionTagsFromShortCut(AS("⇧^⌘⌥j")) == std::nullopt); } SECTION("Non-existent shortcut when a domain is specified") { - REQUIRE(manager.ActionTagsFromShortCut(ActionShortcut("⇧^⌘⌥j"), "this.domain.doesnt.exist.") == std::nullopt); + REQUIRE(manager.ActionTagsFromShortCut(AS("⇧^⌘⌥j"), "this.domain.doesnt.exist.") == std::nullopt); } SECTION("Existent shortcut, but a domain doesn't match") { - REQUIRE(manager.ActionTagsFromShortCut(ActionShortcut("⌘1"), "this.domain.doesnt.exist.") == std::nullopt); + REQUIRE(manager.ActionTagsFromShortCut(AS("⌘1"), "this.domain.doesnt.exist.") == std::nullopt); } SECTION("Shortcut used by two actions in different domains") { - auto tags = manager.ActionTagsFromShortCut(ActionShortcut("⌘1")); + auto tags = manager.ActionTagsFromShortCut(AS("⌘1")); REQUIRE(tags); REQUIRE(std::set(tags->begin(), tags->end()) == - std::set{ActionsShortcutsManager::TagFromAction("menu.go.quick_lists.parent_folders").value(), - ActionsShortcutsManager::TagFromAction("viewer.toggle_text").value()}); + std::set{ASM::TagFromAction("menu.go.quick_lists.parent_folders").value(), + ASM::TagFromAction("viewer.toggle_text").value()}); } SECTION("Shortcut used by two actions in different domains, specify first") { - REQUIRE(manager.ActionTagsFromShortCut(ActionShortcut("⌘1"), "menu.") == - ActionsShortcutsManager::ActionTags{ - ActionsShortcutsManager::TagFromAction("menu.go.quick_lists.parent_folders").value()}); + REQUIRE(manager.ActionTagsFromShortCut(AS("⌘1"), "menu.") == + ASM::ActionTags{ASM::TagFromAction("menu.go.quick_lists.parent_folders").value()}); } SECTION("Shortcut used by two actions in different domains, specify second") { - REQUIRE( - manager.ActionTagsFromShortCut(ActionShortcut("⌘1"), "viewer.") == - ActionsShortcutsManager::ActionTags{ActionsShortcutsManager::TagFromAction("viewer.toggle_text").value()}); + REQUIRE(manager.ActionTagsFromShortCut(AS("⌘1"), "viewer.") == + ASM::ActionTags{ASM::TagFromAction("viewer.toggle_text").value()}); } SECTION("Shortcut is used by by two actions by default and one via override") { - REQUIRE(manager.SetShortCutOverride("menu.window.zoom", ActionShortcut("⌘1"))); - auto tags = manager.ActionTagsFromShortCut(ActionShortcut("⌘1")); + REQUIRE(manager.SetShortCutOverride("menu.window.zoom", AS("⌘1"))); + auto tags = manager.ActionTagsFromShortCut(AS("⌘1")); REQUIRE(tags); REQUIRE(std::set(tags->begin(), tags->end()) == std::set{ - ActionsShortcutsManager::TagFromAction("menu.go.quick_lists.parent_folders").value(), - ActionsShortcutsManager::TagFromAction("viewer.toggle_text").value(), - ActionsShortcutsManager::TagFromAction("menu.window.zoom").value(), + ASM::TagFromAction("menu.go.quick_lists.parent_folders").value(), + ASM::TagFromAction("viewer.toggle_text").value(), + ASM::TagFromAction("menu.window.zoom").value(), + }); + } + SECTION("Shortcut is used by by two actions by default and one via override (multiple shortcuts)") + { + REQUIRE(manager.SetShortCutsOverride("menu.window.zoom", std::array{AS("⇧^⌘⌥j"), AS("⌘1")})); + auto tags = manager.ActionTagsFromShortCut(AS("⌘1")); + REQUIRE(tags); + REQUIRE(std::set(tags->begin(), tags->end()) == + std::set{ + ASM::TagFromAction("menu.go.quick_lists.parent_folders").value(), + ASM::TagFromAction("viewer.toggle_text").value(), + ASM::TagFromAction("menu.window.zoom").value(), }); } SECTION("After setting and removing the override its not reported as being used") { - REQUIRE(manager.SetShortCutOverride("menu.window.zoom", ActionShortcut("⇧^⌘⌥j"))); - REQUIRE(manager.SetShortCutOverride("menu.window.zoom", {})); - REQUIRE(manager.ActionTagsFromShortCut(ActionShortcut("⇧^⌘⌥j")) == std::nullopt); + REQUIRE(manager.SetShortCutsOverride("menu.window.zoom", std::array{AS("⇧^⌘⌥k"), AS("⇧^⌘⌥j")})); + REQUIRE(manager.SetShortCutsOverride("menu.window.zoom", {})); + REQUIRE(manager.ActionTagsFromShortCut(AS("⇧^⌘⌥k")) == std::nullopt); + REQUIRE(manager.ActionTagsFromShortCut(AS("⇧^⌘⌥j")) == std::nullopt); } } @@ -131,32 +166,31 @@ TEST_CASE(PREFIX "FirstOfActionTagsFromShortCut") { nc::config::ConfigImpl config{g_EmptyConfigJSON, std::make_shared("")}; ASM manager{config}; - REQUIRE(manager.FirstOfActionTagsFromShortCut({}, ActionShortcut("⌘1")) == std::nullopt); - REQUIRE(manager.FirstOfActionTagsFromShortCut(std::initializer_list{346'242}, ActionShortcut("⌘1")) == - std::nullopt); + REQUIRE(manager.FirstOfActionTagsFromShortCut({}, AS("⌘1")) == std::nullopt); + REQUIRE(manager.FirstOfActionTagsFromShortCut(std::initializer_list{346'242}, AS("⌘1")) == std::nullopt); REQUIRE(manager.FirstOfActionTagsFromShortCut( std::initializer_list{ASM::TagFromAction("menu.go.quick_lists.parent_folders").value()}, - ActionShortcut("⌘1")) == ASM::TagFromAction("menu.go.quick_lists.parent_folders").value()); + AS("⌘1")) == ASM::TagFromAction("menu.go.quick_lists.parent_folders").value()); REQUIRE(manager.FirstOfActionTagsFromShortCut( std::initializer_list{ASM::TagFromAction("menu.go.quick_lists.parent_folders").value()}, - ActionShortcut("⌘1"), + AS("⌘1"), "menu.") == ASM::TagFromAction("menu.go.quick_lists.parent_folders").value()); REQUIRE(manager.FirstOfActionTagsFromShortCut( std::initializer_list{ASM::TagFromAction("menu.go.quick_lists.parent_folders").value()}, - ActionShortcut("⌘1"), + AS("⌘1"), "viewer.") == std::nullopt); REQUIRE(manager.FirstOfActionTagsFromShortCut( - std::initializer_list{ASM::TagFromAction("viewer.toggle_text").value()}, ActionShortcut("⌘1")) == + std::initializer_list{ASM::TagFromAction("viewer.toggle_text").value()}, AS("⌘1")) == ASM::TagFromAction("viewer.toggle_text").value()); REQUIRE(manager.FirstOfActionTagsFromShortCut( - std::initializer_list{ASM::TagFromAction("viewer.toggle_text").value()}, - ActionShortcut("⌘1"), - "menu.") == std::nullopt); + std::initializer_list{ASM::TagFromAction("viewer.toggle_text").value()}, AS("⌘1"), "menu.") == + std::nullopt); REQUIRE(manager.FirstOfActionTagsFromShortCut( - std::initializer_list{ASM::TagFromAction("viewer.toggle_text").value()}, - ActionShortcut("⌘1"), - "viewer.") == ASM::TagFromAction("viewer.toggle_text").value()); + std::initializer_list{ASM::TagFromAction("viewer.toggle_text").value()}, AS("⌘1"), "viewer.") == + ASM::TagFromAction("viewer.toggle_text").value()); } + +// TODO: unit tests for overrides persistence. include both variants of overrides.