diff --git a/.github/actions/spelling/allow/allow.txt b/.github/actions/spelling/allow/allow.txt index d5f5f9c2a7..35e28c1aa6 100644 --- a/.github/actions/spelling/allow/allow.txt +++ b/.github/actions/spelling/allow/allow.txt @@ -56,6 +56,7 @@ hyperlink hyperlinking hyperlinks iconify +ID img inlined issuetitle diff --git a/.github/actions/spelling/expect/expect.txt b/.github/actions/spelling/expect/expect.txt index 377c2f0364..a2f9f7be00 100644 --- a/.github/actions/spelling/expect/expect.txt +++ b/.github/actions/spelling/expect/expect.txt @@ -822,6 +822,7 @@ idllib IDOK IDR idth +IDTo IDXGI IEnd IEnum @@ -1870,7 +1871,7 @@ unk unknwn UNORM unparseable -unregistering +Unregistering untextured UPDATEDISPLAY UPDOWN diff --git a/src/cascadia/TerminalApp/ActionPaletteItem.cpp b/src/cascadia/TerminalApp/ActionPaletteItem.cpp index ecfd849c5d..0c4a439b81 100644 --- a/src/cascadia/TerminalApp/ActionPaletteItem.cpp +++ b/src/cascadia/TerminalApp/ActionPaletteItem.cpp @@ -18,11 +18,11 @@ using namespace winrt::Microsoft::Terminal::Settings::Model; namespace winrt::TerminalApp::implementation { - ActionPaletteItem::ActionPaletteItem(const Microsoft::Terminal::Settings::Model::Command& command) : + ActionPaletteItem::ActionPaletteItem(const Microsoft::Terminal::Settings::Model::Command& command, const winrt::hstring keyChordText) : _Command(command) { Name(command.Name()); - KeyChordText(command.KeyChordText()); + KeyChordText(keyChordText); Icon(command.IconPath()); } } diff --git a/src/cascadia/TerminalApp/ActionPaletteItem.h b/src/cascadia/TerminalApp/ActionPaletteItem.h index 30177396a2..6679c38086 100644 --- a/src/cascadia/TerminalApp/ActionPaletteItem.h +++ b/src/cascadia/TerminalApp/ActionPaletteItem.h @@ -11,7 +11,7 @@ namespace winrt::TerminalApp::implementation struct ActionPaletteItem : ActionPaletteItemT { ActionPaletteItem() = default; - ActionPaletteItem(const Microsoft::Terminal::Settings::Model::Command& command); + ActionPaletteItem(const Microsoft::Terminal::Settings::Model::Command& command, const winrt::hstring keyChordText); WINRT_PROPERTY(Microsoft::Terminal::Settings::Model::Command, Command, nullptr); diff --git a/src/cascadia/TerminalApp/ActionPaletteItem.idl b/src/cascadia/TerminalApp/ActionPaletteItem.idl index 3858ebdfda..f272c7eb00 100644 --- a/src/cascadia/TerminalApp/ActionPaletteItem.idl +++ b/src/cascadia/TerminalApp/ActionPaletteItem.idl @@ -7,7 +7,7 @@ namespace TerminalApp { [default_interface] runtimeclass ActionPaletteItem : PaletteItem { - ActionPaletteItem(Microsoft.Terminal.Settings.Model.Command command); + ActionPaletteItem(Microsoft.Terminal.Settings.Model.Command command, String keyChordText); Microsoft.Terminal.Settings.Model.Command Command { get; }; } diff --git a/src/cascadia/TerminalApp/CommandPalette.cpp b/src/cascadia/TerminalApp/CommandPalette.cpp index 95dd92380d..2e6338234e 100644 --- a/src/cascadia/TerminalApp/CommandPalette.cpp +++ b/src/cascadia/TerminalApp/CommandPalette.cpp @@ -950,21 +950,27 @@ namespace winrt::TerminalApp::implementation void CommandPalette::SetActionMap(const Microsoft::Terminal::Settings::Model::IActionMapView& actionMap) { _actionMap = actionMap; + _populateCommands(); } - void CommandPalette::SetCommands(const Collections::IVector& actions) + void CommandPalette::_populateCommands() { _allCommands.Clear(); - for (const auto& action : actions) + if (_actionMap) { - auto actionPaletteItem{ winrt::make(action) }; - auto filteredCommand{ winrt::make(actionPaletteItem) }; - _allCommands.Append(filteredCommand); - } + const auto expandedCommands{ _actionMap.ExpandedCommands() }; + for (const auto& action : expandedCommands) + { + const auto keyChordText{ KeyChordSerialization::ToString(_actionMap.GetKeyBindingForAction(action.ID())) }; + auto actionPaletteItem{ winrt::make(action, keyChordText) }; + auto filteredCommand{ winrt::make(actionPaletteItem) }; + _allCommands.Append(filteredCommand); + } - if (Visibility() == Visibility::Visible && _currentMode == CommandPaletteMode::ActionMode) - { - _updateFilteredActions(); + if (Visibility() == Visibility::Visible && _currentMode == CommandPaletteMode::ActionMode) + { + _updateFilteredActions(); + } } } @@ -1178,7 +1184,8 @@ namespace winrt::TerminalApp::implementation for (const auto& nameAndCommand : parentCommand.NestedCommands()) { const auto action = nameAndCommand.Value(); - auto nestedActionPaletteItem{ winrt::make(action) }; + // nested commands cannot have keys bound to them, so just pass in the command and no keys + auto nestedActionPaletteItem{ winrt::make(action, winrt::hstring{}) }; auto nestedFilteredCommand{ winrt::make(nestedActionPaletteItem) }; _currentNestedCommands.Append(nestedFilteredCommand); } diff --git a/src/cascadia/TerminalApp/CommandPalette.h b/src/cascadia/TerminalApp/CommandPalette.h index e9dd73cbdf..58e34bb1f9 100644 --- a/src/cascadia/TerminalApp/CommandPalette.h +++ b/src/cascadia/TerminalApp/CommandPalette.h @@ -31,7 +31,6 @@ namespace winrt::TerminalApp::implementation Windows::Foundation::Collections::IObservableVector FilteredActions(); - void SetCommands(const Windows::Foundation::Collections::IVector& actions); void SetTabs(const Windows::Foundation::Collections::IObservableVector& tabs, const Windows::Foundation::Collections::IObservableVector& mruTabs); void SetActionMap(const Microsoft::Terminal::Settings::Model::IActionMapView& actionMap); @@ -81,6 +80,8 @@ namespace winrt::TerminalApp::implementation bool _lastFilterTextWasEmpty{ true }; + void _populateCommands(); + void _filterTextChanged(const Windows::Foundation::IInspectable& sender, const Windows::UI::Xaml::RoutedEventArgs& args); void _previewKeyDownHandler(const Windows::Foundation::IInspectable& sender, diff --git a/src/cascadia/TerminalApp/CommandPalette.idl b/src/cascadia/TerminalApp/CommandPalette.idl index e73d935a88..48787135a9 100644 --- a/src/cascadia/TerminalApp/CommandPalette.idl +++ b/src/cascadia/TerminalApp/CommandPalette.idl @@ -20,8 +20,6 @@ namespace TerminalApp Windows.Foundation.Collections.IObservableVector FilteredActions { get; }; - void SetCommands(Windows.Foundation.Collections.IVector actions); - void SetTabs(Windows.Foundation.Collections.IObservableVector tabs, Windows.Foundation.Collections.IObservableVector mruTabs); void SetActionMap(Microsoft.Terminal.Settings.Model.IActionMapView actionMap); diff --git a/src/cascadia/TerminalApp/SuggestionsControl.cpp b/src/cascadia/TerminalApp/SuggestionsControl.cpp index 902cd06ebb..d8d11a71ef 100644 --- a/src/cascadia/TerminalApp/SuggestionsControl.cpp +++ b/src/cascadia/TerminalApp/SuggestionsControl.cpp @@ -508,7 +508,7 @@ namespace winrt::TerminalApp::implementation automationPeer.RaiseNotificationEvent( Automation::Peers::AutomationNotificationKind::ItemAdded, Automation::Peers::AutomationNotificationProcessing::MostRecent, - paletteItem.Name() + L" " + paletteItem.KeyChordText(), + paletteItem.Name(), L"SuggestionsControlSelectedItemChanged" /* unique name for this notification category */); } } @@ -751,17 +751,13 @@ namespace winrt::TerminalApp::implementation return _filteredActions; } - void SuggestionsControl::SetActionMap(const Microsoft::Terminal::Settings::Model::IActionMapView& actionMap) - { - _actionMap = actionMap; - } - void SuggestionsControl::SetCommands(const Collections::IVector& actions) { _allCommands.Clear(); for (const auto& action : actions) { - auto actionPaletteItem{ winrt::make(action) }; + // key chords aren't relevant in the suggestions control, so make the palette item with just the command and no keys + auto actionPaletteItem{ winrt::make(action, winrt::hstring{}) }; auto filteredCommand{ winrt::make(actionPaletteItem) }; _allCommands.Append(filteredCommand); } @@ -915,7 +911,7 @@ namespace winrt::TerminalApp::implementation for (const auto& nameAndCommand : parentCommand.NestedCommands()) { const auto action = nameAndCommand.Value(); - auto nestedActionPaletteItem{ winrt::make(action) }; + auto nestedActionPaletteItem{ winrt::make(action, winrt::hstring{}) }; auto nestedFilteredCommand{ winrt::make(nestedActionPaletteItem) }; _currentNestedCommands.Append(nestedFilteredCommand); } diff --git a/src/cascadia/TerminalApp/SuggestionsControl.h b/src/cascadia/TerminalApp/SuggestionsControl.h index 77596ce307..356251ebc6 100644 --- a/src/cascadia/TerminalApp/SuggestionsControl.h +++ b/src/cascadia/TerminalApp/SuggestionsControl.h @@ -24,7 +24,6 @@ namespace winrt::TerminalApp::implementation Windows::Foundation::Collections::IObservableVector FilteredActions(); void SetCommands(const Windows::Foundation::Collections::IVector& actions); - void SetActionMap(const Microsoft::Terminal::Settings::Model::IActionMapView& actionMap); bool OnDirectKeyEvent(const uint32_t vkey, const uint8_t scanCode, const bool down); diff --git a/src/cascadia/TerminalApp/SuggestionsControl.idl b/src/cascadia/TerminalApp/SuggestionsControl.idl index 103d3e17d2..0faea43928 100644 --- a/src/cascadia/TerminalApp/SuggestionsControl.idl +++ b/src/cascadia/TerminalApp/SuggestionsControl.idl @@ -36,7 +36,6 @@ namespace TerminalApp SuggestionsMode Mode { get; set; }; void SetCommands(Windows.Foundation.Collections.IVector actions); - void SetActionMap(Microsoft.Terminal.Settings.Model.IActionMapView actionMap); void SelectNextItem(Boolean moveDown); void Open(SuggestionsMode mode, IVector commands, String filterText, Windows.Foundation.Point anchor, Windows.Foundation.Size space, Single characterHeight); diff --git a/src/cascadia/TerminalApp/SuggestionsControl.xaml b/src/cascadia/TerminalApp/SuggestionsControl.xaml index 49525aa4ad..c1cbdbab97 100644 --- a/src/cascadia/TerminalApp/SuggestionsControl.xaml +++ b/src/cascadia/TerminalApp/SuggestionsControl.xaml @@ -31,7 +31,6 @@ MinHeight="0" Padding="16,0,12,0" HorizontalContentAlignment="Stretch" - AutomationProperties.AcceleratorKey="{x:Bind Item.KeyChordText, Mode=OneWay}" AutomationProperties.Name="{x:Bind Item.Name, Mode=OneWay}" FontSize="12" /> diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index 1d2953167f..039b99390d 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -123,7 +123,6 @@ namespace winrt::TerminalApp::implementation // to happen before the Settings UI is reloaded and tries to re-read those values. if (const auto p = CommandPaletteElement()) { - p.SetCommands(_settings.GlobalSettings().ActionMap().ExpandedCommands()); p.SetActionMap(_settings.ActionMap()); } @@ -1828,7 +1827,6 @@ namespace winrt::TerminalApp::implementation { const auto p = FindName(L"CommandPaletteElement").as(); - p.SetCommands(_settings.GlobalSettings().ActionMap().ExpandedCommands()); p.SetActionMap(_settings.ActionMap()); // When the visibility of the command palette changes to "collapsed", diff --git a/src/cascadia/TerminalSettingsModel/ActionMap.cpp b/src/cascadia/TerminalSettingsModel/ActionMap.cpp index 04e40ef328..0110839fe1 100644 --- a/src/cascadia/TerminalSettingsModel/ActionMap.cpp +++ b/src/cascadia/TerminalSettingsModel/ActionMap.cpp @@ -108,9 +108,6 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation if (cmdID == pair.first) { keysToReassign.insert_or_assign(key, inboxCmd->second.ID()); - - // register the keys with the inbox action - inboxCmd->second.RegisterKey(key); } } @@ -239,7 +236,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation { if (!_NameMapCache) { - if (!_CumulativeActionMapCache) + if (_CumulativeIDToActionMapCache.empty()) { _RefreshKeyBindingCaches(); } @@ -298,7 +295,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // - nameMap: the nameMap we're populating, this maps the name (hstring) of a command to the command itself void ActionMap::_PopulateNameMapWithStandardCommands(std::unordered_map& nameMap) const { - for (const auto& [_, cmd] : _CumulativeActionMapCache) + for (const auto& [_, cmd] : _CumulativeIDToActionMapCache) { const auto& name{ cmd.Name() }; if (!name.empty()) @@ -318,22 +315,27 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation } // Method Description: - // - Recursively populate keyBindingsMap with ours and our parents' key -> id pairs + // - Recursively populate keyToActionMap with ours and our parents' key -> id pairs + // - Recursively populate actionToKeyMap with ours and our parents' id -> key pairs // - This is a bottom-up approach - // - Keybindings of the parents are overridden by the children - void ActionMap::_PopulateCumulativeKeyMap(std::unordered_map& keyBindingsMap) + // - Child's pairs override parents' pairs + void ActionMap::_PopulateCumulativeKeyMaps(std::unordered_map& keyToActionMap, std::unordered_map& actionToKeyMap) { for (const auto& [keys, cmdID] : _KeyMap) { - if (!keyBindingsMap.contains(keys)) + if (!keyToActionMap.contains(keys)) { - keyBindingsMap.emplace(keys, cmdID); + keyToActionMap.emplace(keys, cmdID); + } + if (!actionToKeyMap.contains(cmdID)) + { + actionToKeyMap.emplace(cmdID, keys); } } for (const auto& parent : _parents) { - parent->_PopulateCumulativeKeyMap(keyBindingsMap); + parent->_PopulateCumulativeKeyMaps(keyToActionMap, actionToKeyMap); } } @@ -368,28 +370,29 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation IMapView ActionMap::KeyBindings() { - if (!_ResolvedKeyActionMapCache) + if (!_ResolvedKeyToActionMapCache) { _RefreshKeyBindingCaches(); } - return _ResolvedKeyActionMapCache.GetView(); + return _ResolvedKeyToActionMapCache.GetView(); } void ActionMap::_RefreshKeyBindingCaches() { + _CumulativeKeyToActionMapCache.clear(); + _CumulativeIDToActionMapCache.clear(); + _CumulativeActionToKeyMapCache.clear(); std::unordered_map globalHotkeys; - std::unordered_map accumulatedKeybindingsMap; - std::unordered_map accumulatedActionsMap; - std::unordered_map resolvedKeyActionMap; + std::unordered_map resolvedKeyToActionMap; - _PopulateCumulativeKeyMap(accumulatedKeybindingsMap); - _PopulateCumulativeActionMap(accumulatedActionsMap); + _PopulateCumulativeKeyMaps(_CumulativeKeyToActionMapCache, _CumulativeActionToKeyMapCache); + _PopulateCumulativeActionMap(_CumulativeIDToActionMapCache); - for (const auto& [keys, cmdID] : accumulatedKeybindingsMap) + for (const auto& [keys, cmdID] : _CumulativeKeyToActionMapCache) { - if (const auto idCmdPair = accumulatedActionsMap.find(cmdID); idCmdPair != accumulatedActionsMap.end()) + if (const auto idCmdPair = _CumulativeIDToActionMapCache.find(cmdID); idCmdPair != _CumulativeIDToActionMapCache.end()) { - resolvedKeyActionMap.emplace(keys, idCmdPair->second); + resolvedKeyToActionMap.emplace(keys, idCmdPair->second); // Only populate GlobalHotkeys with actions whose // ShortcutAction is GlobalSummon or QuakeMode @@ -400,9 +403,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation } } - _CumulativeKeyMapCache = single_threaded_map(std::move(accumulatedKeybindingsMap)); - _CumulativeActionMapCache = single_threaded_map(std::move(accumulatedActionsMap)); - _ResolvedKeyActionMapCache = single_threaded_map(std::move(resolvedKeyActionMap)); + _ResolvedKeyToActionMapCache = single_threaded_map(std::move(resolvedKeyToActionMap)); _GlobalHotkeysCache = single_threaded_map(std::move(globalHotkeys)); } @@ -446,7 +447,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // - Adds a command to the ActionMap // Arguments: // - cmd: the command we're adding - void ActionMap::AddAction(const Model::Command& cmd) + void ActionMap::AddAction(const Model::Command& cmd, const Control::KeyChord& keys) { // _Never_ add null to the ActionMap if (!cmd) @@ -455,11 +456,12 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation } // invalidate caches + _CumulativeKeyToActionMapCache.clear(); + _CumulativeIDToActionMapCache.clear(); + _CumulativeActionToKeyMapCache.clear(); _NameMapCache = nullptr; _GlobalHotkeysCache = nullptr; - _CumulativeKeyMapCache = nullptr; - _CumulativeActionMapCache = nullptr; - _ResolvedKeyActionMapCache = nullptr; + _ResolvedKeyToActionMapCache = nullptr; // Handle nested commands const auto cmdImpl{ get_self(cmd) }; @@ -486,7 +488,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // Add the new keybinding to the _KeyMap _TryUpdateActionMap(cmd); - _TryUpdateKeyChord(cmd); + _TryUpdateKeyChord(cmd, keys); } // Method Description: @@ -545,28 +547,17 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // - Update our internal state with the key chord of the newly registered action // Arguments: // - cmd: the action we're trying to register - void ActionMap::_TryUpdateKeyChord(const Model::Command& cmd) + void ActionMap::_TryUpdateKeyChord(const Model::Command& cmd, const Control::KeyChord& keys) { // Example (this is a legacy case, where the keys are provided in the same block as the command): // { "command": "copy", "keys": "ctrl+c" } --> we are registering a new key chord // { "name": "foo", "command": "copy" } --> no change to keys, exit early - const auto keys{ cmd.Keys() }; if (!keys) { // the user is not trying to update the keys. return; } - // Handle collisions - if (const auto foundCommand = _GetActionByKeyChordInternal(keys); foundCommand && *foundCommand) - { - // collision: the key chord is bound to some command, make sure that command erases - // this key chord as we are about to overwrite it - - const auto foundCommandImpl{ get_self(*foundCommand) }; - foundCommandImpl->EraseKey(keys); - } - // Assign the new action in the _KeyMap // However, there's a strange edge case here - since we're parsing a legacy or modern block, // the user might have { "command": null, "id": "someID", "keys": "ctrl+c" } @@ -670,12 +661,16 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // Return Value: // - the key chord that executes the given action // - nullptr if the action is not bound to a key chord - Control::KeyChord ActionMap::GetKeyBindingForAction(const winrt::hstring& cmdID) const + Control::KeyChord ActionMap::GetKeyBindingForAction(const winrt::hstring& cmdID) { - // Check our internal state. - if (const auto cmd{ _GetActionByID(cmdID) }) + if (!_ResolvedKeyToActionMapCache) { - return cmd.Keys(); + _RefreshKeyBindingCaches(); + } + + if (_CumulativeActionToKeyMapCache.contains(cmdID)) + { + return _CumulativeActionToKeyMapCache.at(cmdID); } // This key binding does not exist @@ -711,11 +706,6 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation _KeyMap.insert_or_assign(oldKeys, L""); } - // make sure to update the Command with these changes - const auto cmdImpl{ get_self(cmd) }; - cmdImpl->EraseKey(oldKeys); - cmdImpl->RegisterKey(newKeys); - return true; } @@ -753,10 +743,9 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation void ActionMap::RegisterKeyBinding(Control::KeyChord keys, Model::ActionAndArgs action) { auto cmd{ make_self() }; - cmd->RegisterKey(keys); cmd->ActionAndArgs(action); cmd->GenerateID(); - AddAction(*cmd); + AddAction(*cmd, keys); } // This is a helper to aid in sorting commands by their `Name`s, alphabetically. diff --git a/src/cascadia/TerminalSettingsModel/ActionMap.h b/src/cascadia/TerminalSettingsModel/ActionMap.h index 87bd5efb58..3810e55f1f 100644 --- a/src/cascadia/TerminalSettingsModel/ActionMap.h +++ b/src/cascadia/TerminalSettingsModel/ActionMap.h @@ -61,10 +61,10 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // queries Model::Command GetActionByKeyChord(const Control::KeyChord& keys) const; bool IsKeyChordExplicitlyUnbound(const Control::KeyChord& keys) const; - Control::KeyChord GetKeyBindingForAction(const winrt::hstring& cmdID) const; + Control::KeyChord GetKeyBindingForAction(const winrt::hstring& cmdID); // population - void AddAction(const Model::Command& cmd); + void AddAction(const Model::Command& cmd, const Control::KeyChord& keys); // JSON static com_ptr FromJson(const Json::Value& json, const OriginTag origin = OriginTag::None); @@ -94,11 +94,11 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation void _PopulateNameMapWithSpecialCommands(std::unordered_map& nameMap) const; void _PopulateNameMapWithStandardCommands(std::unordered_map& nameMap) const; - void _PopulateCumulativeKeyMap(std::unordered_map& keyBindingsMap); + void _PopulateCumulativeKeyMaps(std::unordered_map& keyToActionMap, std::unordered_map& actionToKeyMap); void _PopulateCumulativeActionMap(std::unordered_map& actionMap); void _TryUpdateActionMap(const Model::Command& cmd); - void _TryUpdateKeyChord(const Model::Command& cmd); + void _TryUpdateKeyChord(const Model::Command& cmd, const Control::KeyChord& keys); Windows::Foundation::Collections::IMap _AvailableActionsCache{ nullptr }; Windows::Foundation::Collections::IMap _NameMapCache{ nullptr }; @@ -111,8 +111,6 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation bool _fixupsAppliedDuringLoad{ false }; - void _AddKeyBindingHelper(const Json::Value& json, std::vector& warnings); - // _KeyMap is the map of key chords -> action IDs defined in this layer // _ActionMap is the map of action IDs -> commands defined in this layer // These maps are the ones that we deserialize into when parsing the user json and vice-versa @@ -120,14 +118,19 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation std::unordered_map _ActionMap; // _CumulativeKeyMapCache is the map of key chords -> action IDs defined in all layers, with child layers overriding parent layers - Windows::Foundation::Collections::IMap _CumulativeKeyMapCache{ nullptr }; + std::unordered_map _CumulativeKeyToActionMapCache; // _CumulativeActionMapCache is the map of action IDs -> commands defined in all layers, with child layers overriding parent layers - Windows::Foundation::Collections::IMap _CumulativeActionMapCache{ nullptr }; + std::unordered_map _CumulativeIDToActionMapCache; + // _CumulativeActionKeyMapCache stores the same data as _CumulativeKeyMapCache, but in the other direction (actionID -> keyChord) + // This is so we have O(1) lookup time when we want to get the keybinding for a specific action + // Note that an action could have multiple keybindings, the one we store in this map is one of the user's ones if present, + // otherwise the default one + std::unordered_map _CumulativeActionToKeyMapCache; // _ResolvedKeyActionMapCache is the map of key chords -> commands defined in all layers, with child layers overriding parent layers // This is effectively a combination of _CumulativeKeyMapCache and _CumulativeActionMapCache and its purpose is so that // we can give the SUI a view of the key chords and the commands they map to - Windows::Foundation::Collections::IMap _ResolvedKeyActionMapCache{ nullptr }; + Windows::Foundation::Collections::IMap _ResolvedKeyToActionMapCache{ nullptr }; friend class SettingsModelUnitTests::KeyBindingsTests; friend class SettingsModelUnitTests::DeserializationTests; diff --git a/src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp b/src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp index e81c9dbd05..b6216971ce 100644 --- a/src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp +++ b/src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp @@ -59,32 +59,52 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // and we can call Command::FromJson on it (Command::FromJson can handle parsing both legacy or modern) // if there is no "command" field, then it is a modern style keys block - if (jsonBlock.isMember(JsonKey(CommandsKey)) || jsonBlock.isMember(JsonKey(ActionKey))) + + // if there are keys, extract them first + Control::KeyChord keys{ nullptr }; + if (withKeybindings && jsonBlock.isMember(JsonKey(KeysKey))) { - AddAction(*Command::FromJson(jsonBlock, warnings, origin, withKeybindings)); - - // for non-nested non-iterable commands, - // check if this is a legacy-style command block so we can inform the loader that fixups are needed - if (jsonBlock.isMember(JsonKey(ActionKey)) && !jsonBlock.isMember(JsonKey(IterateOnKey))) + const auto keysJson{ jsonBlock[JsonKey(KeysKey)] }; + if (keysJson.isArray() && keysJson.size() > 1) { - if (jsonBlock.isMember(JsonKey(KeysKey))) - { - // there are keys in this command block - it's the legacy style - _fixupsAppliedDuringLoad = true; - } - - if (origin == OriginTag::User && !jsonBlock.isMember(JsonKey(IDKey))) - { - // there's no ID in this command block - we will generate one for the user - // inform the loader that the ID needs to be written into the json - _fixupsAppliedDuringLoad = true; - } + warnings.push_back(SettingsLoadWarnings::TooManyKeysForChord); + } + else + { + JsonUtils::GetValueForKey(jsonBlock, KeysKey, keys); } } - else + + // Now check if this is a command block + if (jsonBlock.isMember(JsonKey(CommandsKey)) || jsonBlock.isMember(JsonKey(ActionKey))) + { + AddAction(*Command::FromJson(jsonBlock, warnings, origin), keys); + + if (jsonBlock.isMember(JsonKey(KeysKey))) + { + // there are keys in this command block meaning this is the legacy style - + // inform the loader that fixups are needed + _fixupsAppliedDuringLoad = true; + } + + if (jsonBlock.isMember(JsonKey(ActionKey)) && !jsonBlock.isMember(JsonKey(IterateOnKey)) && origin == OriginTag::User && !jsonBlock.isMember(JsonKey(IDKey))) + { + // for non-nested non-iterable commands, + // if there's no ID in the command block we will generate one for the user - + // inform the loader that the ID needs to be written into the json + _fixupsAppliedDuringLoad = true; + } + } + else if (keys) { // this is not a command block, so it is a keybinding block - _AddKeyBindingHelper(jsonBlock, warnings); + + // if the "id" field doesn't exist in the json, then idJson will be an empty string which is fine + winrt::hstring idJson; + JsonUtils::GetValueForKey(jsonBlock, IDKey, idJson); + + // any existing keybinding with the same keychord in this layer will get overwritten + _KeyMap.insert_or_assign(keys, idJson); } } @@ -136,54 +156,4 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation return keybindingsList; } - - void ActionMap::_AddKeyBindingHelper(const Json::Value& json, std::vector& warnings) - { - // There should always be a "keys" field - // - If there is also an "id" field - we add the pair to our _KeyMap - // - If there is no "id" field - this is an explicit unbinding, still add it to the _KeyMap, - // when this key chord is queried for we will know it is an explicit unbinding - const auto keysJson{ json[JsonKey(KeysKey)] }; - if (keysJson.isArray() && keysJson.size() > 1) - { - warnings.push_back(SettingsLoadWarnings::TooManyKeysForChord); - return; - } - - Control::KeyChord keys{ nullptr }; - winrt::hstring idJson; - if (!JsonUtils::GetValueForKey(json, KeysKey, keys)) - { - return; - } - - // if these keys are already bound to some command, - // we need to update that command to erase these keys as we are about to overwrite them - if (const auto foundCommand = _GetActionByKeyChordInternal(keys); foundCommand && *foundCommand) - { - const auto foundCommandImpl{ get_self(*foundCommand) }; - foundCommandImpl->EraseKey(keys); - } - - // if the "id" field doesn't exist in the json, then idJson will be an empty string which is fine - JsonUtils::GetValueForKey(json, IDKey, idJson); - - // any existing keybinding with the same keychord in this layer will get overwritten - _KeyMap.insert_or_assign(keys, idJson); - - // make sure the command registers these keys - if (!idJson.empty()) - { - // TODO GH#17160 - // if the command with this id is only going to appear later during settings load - // then this will return null, meaning that the command created later on will not register this keybinding - // the keybinding will still work fine within the app, its just that the Command object itself won't know about this key mapping - // we are going to move away from Command needing to know its key mappings in a followup, so this shouldn't matter for very long - if (const auto cmd = _GetActionByID(idJson)) - { - cmd.RegisterKey(keys); - } - } - return; - } } diff --git a/src/cascadia/TerminalSettingsModel/Command.cpp b/src/cascadia/TerminalSettingsModel/Command.cpp index cc80010825..519ffc5e98 100644 --- a/src/cascadia/TerminalSettingsModel/Command.cpp +++ b/src/cascadia/TerminalSettingsModel/Command.cpp @@ -35,7 +35,6 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation command->_Origin = _Origin; command->_ID = _ID; command->_ActionAndArgs = *get_self(_ActionAndArgs)->Copy(); - command->_keyMappings = _keyMappings; command->_iconPath = _iconPath; command->_IterateOn = _IterateOn; @@ -139,70 +138,6 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation } } - std::vector Command::KeyMappings() const noexcept - { - return _keyMappings; - } - - // Function Description: - // - Add the key chord to the command's list of key mappings. - // - If the key chord was already registered, move it to the back - // of the line, and dispatch a notification that Command::Keys changed. - // Arguments: - // - keys: the new key chord that we are registering this command to - // Return Value: - // - - void Command::RegisterKey(const Control::KeyChord& keys) - { - if (!keys) - { - return; - } - - // Remove the KeyChord and add it to the back of the line. - // This makes it so that the main key chord associated with this - // command is updated. - EraseKey(keys); - _keyMappings.push_back(keys); - } - - // Function Description: - // - Remove the key chord from the command's list of key mappings. - // Arguments: - // - keys: the key chord that we are unregistering - // Return Value: - // - - void Command::EraseKey(const Control::KeyChord& keys) - { - _keyMappings.erase(std::remove_if(_keyMappings.begin(), _keyMappings.end(), [&keys](const Control::KeyChord& iterKey) { - return keys.Modifiers() == iterKey.Modifiers() && keys.Vkey() == iterKey.Vkey(); - }), - _keyMappings.end()); - } - - // Function Description: - // - Keys is the Command's identifying KeyChord. The command may have multiple keys associated - // with it, but we'll only ever display the most recently added one externally. To do this, - // _keyMappings stores all of the associated key chords, but ensures that the last entry - // is the most recently added one. - // Arguments: - // - - // Return Value: - // - the primary key chord associated with this Command - Control::KeyChord Command::Keys() const noexcept - { - if (_keyMappings.empty()) - { - return nullptr; - } - return _keyMappings.back(); - } - - hstring Command::KeyChordText() const noexcept - { - return KeyChordSerialization::ToString(Keys()); - } - hstring Command::IconPath() const noexcept { if (_iconPath.has_value()) @@ -276,8 +211,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // - the newly constructed Command object. winrt::com_ptr Command::FromJson(const Json::Value& json, std::vector& warnings, - const OriginTag origin, - const bool parseKeys) + const OriginTag origin) { auto result = winrt::make_self(); result->_Origin = origin; @@ -333,26 +267,6 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // create an "invalid" ActionAndArgs result->_ActionAndArgs = make(); } - - if (parseKeys) - { - // GH#4239 - If the user provided more than one key - // chord to a "keys" array, warn the user here. - // TODO: GH#1334 - remove this check. - const auto keysJson{ json[JsonKey(KeysKey)] }; - if (keysJson.isArray() && keysJson.size() > 1) - { - warnings.push_back(SettingsLoadWarnings::TooManyKeysForChord); - } - else - { - Control::KeyChord keys{ nullptr }; - if (JsonUtils::GetValueForKey(json, KeysKey, keys)) - { - result->RegisterKey(keys); - } - } - } } // If an iterable command doesn't have a name set, we'll still just diff --git a/src/cascadia/TerminalSettingsModel/Command.h b/src/cascadia/TerminalSettingsModel/Command.h index f2d29daa3f..7f19eb5694 100644 --- a/src/cascadia/TerminalSettingsModel/Command.h +++ b/src/cascadia/TerminalSettingsModel/Command.h @@ -48,8 +48,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation static winrt::com_ptr FromJson(const Json::Value& json, std::vector& warnings, - const OriginTag origin, - const bool parseKeys = true); + const OriginTag origin); static void ExpandCommands(Windows::Foundation::Collections::IMap& commands, Windows::Foundation::Collections::IVectorView profiles, @@ -73,12 +72,6 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation void GenerateID(); bool IDWasGenerated(); - Control::KeyChord Keys() const noexcept; - hstring KeyChordText() const noexcept; - std::vector KeyMappings() const noexcept; - void RegisterKey(const Control::KeyChord& keys); - void EraseKey(const Control::KeyChord& keys); - hstring IconPath() const noexcept; void IconPath(const hstring& val); @@ -94,7 +87,6 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation private: Json::Value _originalJson; Windows::Foundation::Collections::IMap _subcommands{ nullptr }; - std::vector _keyMappings; std::optional _name; std::wstring _ID; bool _IDWasGenerated{ false }; diff --git a/src/cascadia/TerminalSettingsModel/Command.idl b/src/cascadia/TerminalSettingsModel/Command.idl index aa23458f55..78d09d7809 100644 --- a/src/cascadia/TerminalSettingsModel/Command.idl +++ b/src/cascadia/TerminalSettingsModel/Command.idl @@ -38,9 +38,6 @@ namespace Microsoft.Terminal.Settings.Model String Name { get; }; String ID { get; }; ActionAndArgs ActionAndArgs { get; }; - Microsoft.Terminal.Control.KeyChord Keys { get; }; - void RegisterKey(Microsoft.Terminal.Control.KeyChord keys); - String KeyChordText { get; }; String IconPath; diff --git a/src/cascadia/UnitTests_SettingsModel/DeserializationTests.cpp b/src/cascadia/UnitTests_SettingsModel/DeserializationTests.cpp index aabd6fb06f..ce674c5fe6 100644 --- a/src/cascadia/UnitTests_SettingsModel/DeserializationTests.cpp +++ b/src/cascadia/UnitTests_SettingsModel/DeserializationTests.cpp @@ -1997,7 +1997,6 @@ namespace SettingsModelUnitTests // Verify NameMap returns correct value const auto& cmd{ nameMap.TryLookup(L"bar") }; VERIFY_IS_NOT_NULL(cmd); - VERIFY_IS_NULL(cmd.Keys()); VERIFY_ARE_EQUAL(L"bar", cmd.Name()); } {