Remove command's knowledge of its keys (#17215)

With the move to Action IDs, it doesn't quite make sense anymore for a
`Command` to know which keys map to it. This PR removes all `Keys` from
`Command`, and any callers to that now instead query the `ActionMap` for
that Command's keys.


Closes #17160 
Closes #13943

.github/actions/spelling/allow/allow.txt

@@ -56,6 +56,7 @@ hyperlink
 hyperlinking
 hyperlinks
 iconify
+ID
 img
 inlined
 issuetitle

.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

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());
     }
 }

src/cascadia/TerminalApp/ActionPaletteItem.h

@@ -11,7 +11,7 @@ namespace winrt::TerminalApp::implementation
     struct ActionPaletteItem : ActionPaletteItemT<ActionPaletteItem, PaletteItem>
     {
         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);
 

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; };
     }

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<Command>& actions)
+    void CommandPalette::_populateCommands()
     {
         _allCommands.Clear();
-        for (const auto& action : actions)
+        if (_actionMap)
         {
-            auto actionPaletteItem{ winrt::make<winrt::TerminalApp::implementation::ActionPaletteItem>(action) };
+            const auto expandedCommands{ _actionMap.ExpandedCommands() };
-            auto filteredCommand{ winrt::make<FilteredCommand>(actionPaletteItem) };
+            for (const auto& action : expandedCommands)
-            _allCommands.Append(filteredCommand);
+            {
-        }
+                const auto keyChordText{ KeyChordSerialization::ToString(_actionMap.GetKeyBindingForAction(action.ID())) };
+                auto actionPaletteItem{ winrt::make<winrt::TerminalApp::implementation::ActionPaletteItem>(action, keyChordText) };
+                auto filteredCommand{ winrt::make<FilteredCommand>(actionPaletteItem) };
+                _allCommands.Append(filteredCommand);
+            }
 
-        if (Visibility() == Visibility::Visible && _currentMode == CommandPaletteMode::ActionMode)
+            if (Visibility() == Visibility::Visible && _currentMode == CommandPaletteMode::ActionMode)
-        {
+            {
-            _updateFilteredActions();
+                _updateFilteredActions();
+            }
         }
     }
 
@@ -1178,7 +1184,8 @@ namespace winrt::TerminalApp::implementation
         for (const auto& nameAndCommand : parentCommand.NestedCommands())
         {
             const auto action = nameAndCommand.Value();
-            auto nestedActionPaletteItem{ winrt::make<winrt::TerminalApp::implementation::ActionPaletteItem>(action) };
+            // nested commands cannot have keys bound to them, so just pass in the command and no keys
+            auto nestedActionPaletteItem{ winrt::make<winrt::TerminalApp::implementation::ActionPaletteItem>(action, winrt::hstring{}) };
             auto nestedFilteredCommand{ winrt::make<FilteredCommand>(nestedActionPaletteItem) };
             _currentNestedCommands.Append(nestedFilteredCommand);
         }

src/cascadia/TerminalApp/CommandPalette.h

@@ -31,7 +31,6 @@ namespace winrt::TerminalApp::implementation
 
         Windows::Foundation::Collections::IObservableVector<winrt::TerminalApp::FilteredCommand> FilteredActions();
 
-        void SetCommands(const Windows::Foundation::Collections::IVector<Microsoft::Terminal::Settings::Model::Command>& actions);
         void SetTabs(const Windows::Foundation::Collections::IObservableVector<winrt::TerminalApp::TabBase>& tabs, const Windows::Foundation::Collections::IObservableVector<winrt::TerminalApp::TabBase>& 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,

src/cascadia/TerminalApp/CommandPalette.idl

@@ -20,8 +20,6 @@ namespace TerminalApp
 
         Windows.Foundation.Collections.IObservableVector<FilteredCommand> FilteredActions { get; };
 
-        void SetCommands(Windows.Foundation.Collections.IVector<Microsoft.Terminal.Settings.Model.Command> actions);
-
         void SetTabs(Windows.Foundation.Collections.IObservableVector<TabBase> tabs, Windows.Foundation.Collections.IObservableVector<TabBase> mruTabs);
 
         void SetActionMap(Microsoft.Terminal.Settings.Model.IActionMapView actionMap);

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<Command>& actions)
     {
         _allCommands.Clear();
         for (const auto& action : actions)
         {
-            auto actionPaletteItem{ winrt::make<winrt::TerminalApp::implementation::ActionPaletteItem>(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<winrt::TerminalApp::implementation::ActionPaletteItem>(action, winrt::hstring{}) };
             auto filteredCommand{ winrt::make<FilteredCommand>(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<winrt::TerminalApp::implementation::ActionPaletteItem>(action) };
+            auto nestedActionPaletteItem{ winrt::make<winrt::TerminalApp::implementation::ActionPaletteItem>(action, winrt::hstring{}) };
             auto nestedFilteredCommand{ winrt::make<FilteredCommand>(nestedActionPaletteItem) };
             _currentNestedCommands.Append(nestedFilteredCommand);
         }

src/cascadia/TerminalApp/SuggestionsControl.h

@@ -24,7 +24,6 @@ namespace winrt::TerminalApp::implementation
         Windows::Foundation::Collections::IObservableVector<winrt::TerminalApp::FilteredCommand> FilteredActions();
 
         void SetCommands(const Windows::Foundation::Collections::IVector<Microsoft::Terminal::Settings::Model::Command>& actions);
-        void SetActionMap(const Microsoft::Terminal::Settings::Model::IActionMapView& actionMap);
 
         bool OnDirectKeyEvent(const uint32_t vkey, const uint8_t scanCode, const bool down);
 

src/cascadia/TerminalApp/SuggestionsControl.idl

@@ -36,7 +36,6 @@ namespace TerminalApp
         SuggestionsMode Mode { get; set; };
 
         void SetCommands(Windows.Foundation.Collections.IVector<Microsoft.Terminal.Settings.Model.Command> actions);
-        void SetActionMap(Microsoft.Terminal.Settings.Model.IActionMapView actionMap);
         void SelectNextItem(Boolean moveDown);
 
         void Open(SuggestionsMode mode, IVector<Microsoft.Terminal.Settings.Model.Command> commands, String filterText, Windows.Foundation.Point anchor, Windows.Foundation.Size space, Single characterHeight);

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" />
             </DataTemplate>

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<CommandPalette>();
 
-        p.SetCommands(_settings.GlobalSettings().ActionMap().ExpandedCommands());
         p.SetActionMap(_settings.ActionMap());
 
         // When the visibility of the command palette changes to "collapsed",

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<hstring, Model::Command>& 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
+    // - Child's pairs override parents' pairs
-    void ActionMap::_PopulateCumulativeKeyMap(std::unordered_map<Control::KeyChord, winrt::hstring, KeyChordHash, KeyChordEquality>& keyBindingsMap)
+    void ActionMap::_PopulateCumulativeKeyMaps(std::unordered_map<Control::KeyChord, winrt::hstring, KeyChordHash, KeyChordEquality>& keyToActionMap, std::unordered_map<winrt::hstring, Control::KeyChord>& 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<Control::KeyChord, Model::Command> 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<KeyChord, Model::Command, KeyChordHash, KeyChordEquality> globalHotkeys;
-        std::unordered_map<KeyChord, winrt::hstring, KeyChordHash, KeyChordEquality> accumulatedKeybindingsMap;
+        std::unordered_map<KeyChord, Model::Command, KeyChordHash, KeyChordEquality> resolvedKeyToActionMap;
-        std::unordered_map<winrt::hstring, Model::Command> accumulatedActionsMap;
-        std::unordered_map<KeyChord, Model::Command, KeyChordHash, KeyChordEquality> resolvedKeyActionMap;
 
-        _PopulateCumulativeKeyMap(accumulatedKeybindingsMap);
+        _PopulateCumulativeKeyMaps(_CumulativeKeyToActionMapCache, _CumulativeActionToKeyMapCache);
-        _PopulateCumulativeActionMap(accumulatedActionsMap);
+        _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));
+        _ResolvedKeyToActionMapCache = single_threaded_map(std::move(resolvedKeyToActionMap));
-        _CumulativeActionMapCache = single_threaded_map(std::move(accumulatedActionsMap));
-        _ResolvedKeyActionMapCache = single_threaded_map(std::move(resolvedKeyActionMap));
         _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;
+        _ResolvedKeyToActionMapCache = nullptr;
-        _CumulativeActionMapCache = nullptr;
-        _ResolvedKeyActionMapCache = nullptr;
 
         // Handle nested commands
         const auto cmdImpl{ get_self<Command>(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<implementation::Command>(*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 (!_ResolvedKeyToActionMapCache)
-        if (const auto cmd{ _GetActionByID(cmdID) })
         {
-            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<implementation::Command>(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<Command>() };
-        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.

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<ActionMap> 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<hstring, Model::Command>& nameMap) const;
         void _PopulateNameMapWithStandardCommands(std::unordered_map<hstring, Model::Command>& nameMap) const;
 
-        void _PopulateCumulativeKeyMap(std::unordered_map<Control::KeyChord, winrt::hstring, KeyChordHash, KeyChordEquality>& keyBindingsMap);
+        void _PopulateCumulativeKeyMaps(std::unordered_map<Control::KeyChord, winrt::hstring, KeyChordHash, KeyChordEquality>& keyToActionMap, std::unordered_map<winrt::hstring, Control::KeyChord>& actionToKeyMap);
         void _PopulateCumulativeActionMap(std::unordered_map<hstring, Model::Command>& 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<hstring, Model::ActionAndArgs> _AvailableActionsCache{ nullptr };
         Windows::Foundation::Collections::IMap<hstring, Model::Command> _NameMapCache{ nullptr };
@@ -111,8 +111,6 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
 
         bool _fixupsAppliedDuringLoad{ false };
 
-        void _AddKeyBindingHelper(const Json::Value& json, std::vector<SettingsLoadWarnings>& 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<winrt::hstring, Model::Command> _ActionMap;
 
         // _CumulativeKeyMapCache is the map of key chords -> action IDs defined in all layers, with child layers overriding parent layers
-        Windows::Foundation::Collections::IMap<Control::KeyChord, winrt::hstring> _CumulativeKeyMapCache{ nullptr };
+        std::unordered_map<Control::KeyChord, winrt::hstring, KeyChordHash, KeyChordEquality> _CumulativeKeyToActionMapCache;
         // _CumulativeActionMapCache is the map of action IDs -> commands defined in all layers, with child layers overriding parent layers
-        Windows::Foundation::Collections::IMap<winrt::hstring, Model::Command> _CumulativeActionMapCache{ nullptr };
+        std::unordered_map<winrt::hstring, Model::Command> _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<winrt::hstring, Control::KeyChord> _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<Control::KeyChord, Model::Command> _ResolvedKeyActionMapCache{ nullptr };
+        Windows::Foundation::Collections::IMap<Control::KeyChord, Model::Command> _ResolvedKeyToActionMapCache{ nullptr };
 
         friend class SettingsModelUnitTests::KeyBindingsTests;
         friend class SettingsModelUnitTests::DeserializationTests;

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));
+                const auto keysJson{ jsonBlock[JsonKey(KeysKey)] };
-
+                if (keysJson.isArray() && keysJson.size() > 1)
-                // 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)))
                 {
-                    if (jsonBlock.isMember(JsonKey(KeysKey)))
+                    warnings.push_back(SettingsLoadWarnings::TooManyKeysForChord);
-                    {
+                }
-                        // there are keys in this command block - it's the legacy style
+                else
-                        _fixupsAppliedDuringLoad = true;
+                {
-                    }
+                    JsonUtils::GetValueForKey(jsonBlock, KeysKey, keys);
-
-                    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;
-                    }
                 }
             }
-            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<SettingsLoadWarnings>& 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<implementation::Command>(*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;
-    }
 }

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<implementation::ActionAndArgs>(_ActionAndArgs)->Copy();
-        command->_keyMappings = _keyMappings;
         command->_iconPath = _iconPath;
         command->_IterateOn = _IterateOn;
 
@@ -139,70 +138,6 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
         }
     }
 
-    std::vector<Control::KeyChord> 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:
-    // - <none>
-    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:
-    // - <none>
-    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:
-    // - <none>
-    // 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> Command::FromJson(const Json::Value& json,
                                               std::vector<SettingsLoadWarnings>& warnings,
-                                              const OriginTag origin,
+                                              const OriginTag origin)
-                                              const bool parseKeys)
     {
         auto result = winrt::make_self<Command>();
         result->_Origin = origin;
@@ -333,26 +267,6 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
                 // create an "invalid" ActionAndArgs
                 result->_ActionAndArgs = make<implementation::ActionAndArgs>();
             }
-
-            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

src/cascadia/TerminalSettingsModel/Command.h

@@ -48,8 +48,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
 
         static winrt::com_ptr<Command> FromJson(const Json::Value& json,
                                                 std::vector<SettingsLoadWarnings>& warnings,
-                                                const OriginTag origin,
+                                                const OriginTag origin);
-                                                const bool parseKeys = true);
 
         static void ExpandCommands(Windows::Foundation::Collections::IMap<winrt::hstring, Model::Command>& commands,
                                    Windows::Foundation::Collections::IVectorView<Model::Profile> 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<Control::KeyChord> 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<winrt::hstring, Model::Command> _subcommands{ nullptr };
-        std::vector<Control::KeyChord> _keyMappings;
         std::optional<std::wstring> _name;
         std::wstring _ID;
         bool _IDWasGenerated{ false };

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;
 

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());
         }
         {