Refactor ActionMap and Command to use ActionIDs

src/cascadia/TerminalSettingsModel/ActionMap.cpp

@@ -66,17 +66,17 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
     void ActionMap::_FinalizeInheritance()
     {
         // first, gather the inbox actions from the relevant parent
-        std::unordered_map<InternalActionID, Model::Command> InboxActions;
+        std::unordered_map<InternalActionID, Model::Command> inboxActions;
         winrt::com_ptr<implementation::ActionMap> foundParent{ nullptr };
-        for (const auto parent : _parents)
+        for (const auto& parent : _parents)
         {
-            for (const auto [_, cmd] : parent->_ActionMap)
+            for (const auto& [_, cmd] : parent->_ActionMap)
             {
                 if (cmd.Origin() != OriginTag::InBox)
                 {
                     // only one parent contains all the inbox actions and that parent contains only inbox actions,
                     // so if we found a non-inbox action we can just skip to the next parent
-                    break;
+                    continue;
                 }
                 foundParent = parent;
                 break;
@@ -85,9 +85,9 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
 
         if (foundParent)
         {
-            for (const auto [_, cmd] : foundParent->_ActionMap)
+            for (const auto& [_, cmd] : foundParent->_ActionMap)
             {
-                InboxActions.emplace(Hash(cmd.ActionAndArgs()), cmd);
+                inboxActions.emplace(Hash(cmd.ActionAndArgs()), cmd);
             }
         }
 
@@ -105,9 +105,9 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
             if (userCmdImpl->IdWasGenerated() && !userCmdImpl->HasName() && userCmd.IconPath().empty())
             {
                 const auto userActionHash = Hash(userCmd.ActionAndArgs());
-                if (const auto inboxCmd = InboxActions.find(userActionHash); inboxCmd != InboxActions.end())
+                if (const auto inboxCmd = inboxActions.find(userActionHash); inboxCmd != inboxActions.end())
                 {
-                    for (auto [key, cmdID] : _KeyMap)
+                    for (const auto& [key, cmdID] : _KeyMap)
                     {
                         // for any of our keys that point to the user action, point them to the inbox action instead
                         if (cmdID == userID)
@@ -126,10 +126,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
         }
 
         // now, remove the commands with the IDs we found
-        for (const auto id : IdsToRemove)
-        {
-            _ActionMap.erase(id);
-        }
+        std::erase_if(_ActionMap, [&IdsToRemove](const auto& pair) { return IdsToRemove.contains(pair.first); });
     }
 
     bool ActionMap::FixUpsAppliedDuringLoad() const
@@ -158,7 +155,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
             return cmd;
         }
 
-        for (const auto parent : _parents)
+        for (const auto& parent : _parents)
         {
             if (const auto inheritedCmd = parent->_GetActionByID(actionID))
             {
@@ -311,7 +308,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
                 // there might be a collision here, where there could be 2 different commands with the same name
                 // in this case, prioritize the user's action
                 // TODO GH #17166: we should no longer use Command.Name to identify commands anywhere
-                if (nameMap.find(name) == nameMap.end() || cmd.Origin() == OriginTag::User)
+                if (!nameMap.contains(name) || cmd.Origin() == OriginTag::User)
                 {
                     // either a command with this name does not exist, or this is a user-defined command with a name
                     // in either case, update the name map with the command (if this is a user-defined command with
@@ -329,13 +326,13 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
     {
         for (const auto& [keys, cmdID] : _KeyMap)
         {
-            if (keyBindingsMap.find(keys) == keyBindingsMap.end())
+            if (!keyBindingsMap.contains(keys))
             {
                 keyBindingsMap.emplace(keys, cmdID);
             }
         }
 
-        for (const auto parent : _parents)
+        for (const auto& parent : _parents)
         {
             parent->_PopulateCumulativeKeyMap(keyBindingsMap);
         }
@@ -348,13 +345,13 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
     {
         for (const auto& [cmdID, cmd] : _ActionMap)
         {
-            if (actionMap.find(cmdID) == actionMap.end())
+            if (!actionMap.contains(cmdID))
             {
                 actionMap.emplace(cmdID, cmd);
             }
         }
 
-        for (const auto parent : _parents)
+        for (const auto& parent : _parents)
         {
             parent->_PopulateCumulativeActionMap(actionMap);
         }
@@ -388,7 +385,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
         _PopulateCumulativeKeyMap(accumulatedKeybindingsMap);
         _PopulateCumulativeActionMap(accumulatedActionsMap);
 
-        for (const auto [keys, cmdID] : accumulatedKeybindingsMap)
+        for (const auto& [keys, cmdID] : accumulatedKeybindingsMap)
         {
             if (const auto idCmdPair = accumulatedActionsMap.find(cmdID); idCmdPair != accumulatedActionsMap.end())
             {
@@ -576,14 +573,9 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
         // i.e. they provided an ID for a null command (which they really shouldn't, there's no purpose)
         // in this case, we do _not_ want to use the id they provided, we want to use an empty id
         // (empty id in the _KeyMap indicates the keychord was explicitly unbound)
-        if (cmd.ActionAndArgs().Action() == ShortcutAction::Invalid)
-        {
-            _KeyMap.insert_or_assign(keys, L"");
-        }
-        else
-        {
-            _KeyMap.insert_or_assign(keys, cmd.ID());
-        }
+        const auto action = cmd.ActionAndArgs().Action();
+        const auto id = action == ShortcutAction::Invalid ? hstring{} : cmd.ID();
+        _KeyMap.insert_or_assign(keys, id);
     }
 
     // Method Description:
@@ -626,15 +618,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
     {
         if (const auto keyIDPair = _KeyMap.find(keys); keyIDPair != _KeyMap.end())
         {
-            if (const auto cmdID = keyIDPair->second; !cmdID.empty())
-            {
-                return cmdID;
-            }
-            else
-            {
-                // the keychord is defined in this layer, but points to an empty string - explicitly unbound
-                return L"";
-            }
+            // the keychord is defined in this layer, return the ID (ID be empty, in which case this key is explicitly unbound)
+            return keyIDPair->second;
         }
 
         // search through our parents

src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp

@@ -125,17 +125,13 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
     {
         Json::Value keybindingsList{ Json::ValueType::arrayValue };
 
-        auto toJson = [&keybindingsList](const KeyChord kc, const winrt::hstring cmdID) {
-            Json::Value keyIDPair{ Json::ValueType::objectValue };
-            JsonUtils::SetValueForKey(keyIDPair, KeysKey, kc);
-            JsonUtils::SetValueForKey(keyIDPair, IDKey, cmdID);
-            keybindingsList.append(keyIDPair);
-        };
-
         // Serialize all standard keybinding objects in the current layer
         for (const auto& [keys, cmdID] : _KeyMap)
         {
-            toJson(keys, cmdID);
+            Json::Value keyIDPair{ Json::ValueType::objectValue };
+            JsonUtils::SetValueForKey(keyIDPair, KeysKey, keys);
+            JsonUtils::SetValueForKey(keyIDPair, IDKey, cmdID);
+            keybindingsList.append(keyIDPair);
         }
 
         return keybindingsList;
@@ -151,40 +147,41 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
         if (keysJson.isArray() && keysJson.size() > 1)
         {
             warnings.push_back(SettingsLoadWarnings::TooManyKeysForChord);
+            return;
         }
-        else
+
+        Control::KeyChord keys{ nullptr };
+        winrt::hstring idJson;
+        if (!JsonUtils::GetValueForKey(json, KeysKey, keys))
         {
-            Control::KeyChord keys{ nullptr };
-            winrt::hstring idJson;
-            if (JsonUtils::GetValueForKey(json, KeysKey, keys))
-            {
-                // 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);
-                }
+            return;
+        }
 
-                // 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);
+        // 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);
+        }
 
-                // any existing keybinding with the same keychord in this layer will get overwritten
-                _KeyMap.insert_or_assign(keys, idJson);
+        // 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);
 
-                // 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);
-                    }
-                }
+        // 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;