nits n fixes

src/cascadia/TerminalSettingsModel/ActionMap.cpp

@@ -76,9 +76,13 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
                 {
                     // 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
-                    continue;
+                    break;
                 }
                 foundParent = parent;
+            }
+
+            if (foundParent)
+            {
                 break;
             }
         }
@@ -91,13 +95,15 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
             }
         }
 
+        std::unordered_map<KeyChord, winrt::hstring, KeyChordHash, KeyChordEquality> keysToReassign;
+
         // now, look through our _ActionMap for commands that
         // - had an ID generated for them
         // - do not have a name/icon path
         // - have a hash that matches a command in the inbox actions
         std::erase_if(_ActionMap, [&](const auto& pair) {
             const auto userCmdImpl{ get_self<Command>(pair.second) };
-            if (userCmdImpl->IdWasGenerated() && !userCmdImpl->HasName() && userCmdImpl->IconPath().empty())
+            if (userCmdImpl->IDWasGenerated() && !userCmdImpl->HasName() && userCmdImpl->IconPath().empty())
             {
                 const auto userActionHash = Hash(userCmdImpl->ActionAndArgs());
                 if (const auto inboxCmd = inboxActions.find(userActionHash); inboxCmd != inboxActions.end())
@@ -107,7 +113,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
                         // for any of our keys that point to the user action, point them to the inbox action instead
                         if (cmdID == pair.first)
                         {
-                            _KeyMap.insert_or_assign(key, inboxCmd->second.ID());
+                            keysToReassign.insert_or_assign(key, inboxCmd->second.ID());
 
                             // register the keys with the inbox action
                             inboxCmd->second.RegisterKey(key);
@@ -120,11 +126,16 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
             }
             return false;
         });
+
+        for (const auto [key, cmdID] : keysToReassign)
+        {
+            _KeyMap.insert_or_assign(key, cmdID);
+        }
     }
 
-    bool ActionMap::FixUpsAppliedDuringLoad() const
+    bool ActionMap::FixupsAppliedDuringLoad() const
     {
-        return _fixUpsAppliedDuringLoad;
+        return _fixupsAppliedDuringLoad;
     }
 
     // Method Description:
@@ -134,7 +145,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
     // - actionID: the internal ID associated with a Command
     // Return Value:
     // - The command if it exists in this layer, otherwise nullptr
-    Model::Command ActionMap::_GetActionByID(const winrt::hstring actionID) const
+    Model::Command ActionMap::_GetActionByID(const winrt::hstring& actionID) const
     {
         // Check current layer
         const auto actionMapPair{ _ActionMap.find(actionID) };
@@ -204,9 +215,9 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
         {
             // Only populate AvailableActions with actions that haven't been visited already.
             const auto actionID = Hash(cmd.ActionAndArgs());
-            if (visitedActionIDs.find(actionID) == visitedActionIDs.end())
+            if (!visitedActionIDs.contains(actionID))
             {
-                const auto& name{ cmd.Name() };
+                const auto name{ cmd.Name() };
                 if (!name.empty())
                 {
                     // Update AvailableActions.
@@ -314,7 +325,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
 
     // Method Description:
     // - Recursively populate keyBindingsMap with ours and our parents' key -> id pairs
-    // - This is a bottom-up approach, ensuring that the keybindings of the parents are overridden by the children
+    // - This is a bottom-up approach
+    // - Keybindings of the parents are overridden by the children
     void ActionMap::_PopulateCumulativeKeyMap(std::unordered_map<Control::KeyChord, winrt::hstring, KeyChordHash, KeyChordEquality>& keyBindingsMap)
     {
         for (const auto& [keys, cmdID] : _KeyMap)
@@ -333,7 +345,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
 
     // Method Description:
     // - Recursively populate actionMap with ours and our parents' id -> command pairs
-    // - This is a bottom-up approach, ensuring that the actions of the parents are overridden by the children
+    // - This is a bottom-up approach
+    // - Actions of the parents are overridden by the children
     void ActionMap::_PopulateCumulativeActionMap(std::unordered_map<hstring, Model::Command>& actionMap)
     {
         for (const auto& [cmdID, cmd] : _ActionMap)
@@ -499,7 +512,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
             }
 
             // only add to the _ActionMap if there is an ID
-            if (auto cmdID = cmd.ID(); !cmdID.empty() && cmd.ActionAndArgs().Action() != ShortcutAction::Invalid)
+            if (auto cmdID = cmd.ID(); !cmdID.empty())
             {
                 // in the legacy scenario, a user might have several of the same action but only one of them has defined an icon or a name
                 // eg. { "command": "paste", "name": "myPaste", "keys":"ctrl+a" }
@@ -514,7 +527,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
                 // if so, we check if there already exists a command with that generated ID, and if there is we port over any name/icon there might be
                 // (this may cause us to overwrite in scenarios where the user has an existing command that has the same generated ID but
                 //  performs a different action or has different args, but that falls under "play stupid games")
-                if (cmdImpl->IdWasGenerated())
+                if (cmdImpl->IDWasGenerated())
                 {
                     if (const auto foundCmd{ _GetActionByID(cmdID) })
                     {
@@ -611,7 +624,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
     {
         if (const auto keyIDPair = _KeyMap.find(keys); keyIDPair != _KeyMap.end())
         {
-            // the keychord is defined in this layer, return the ID (ID be empty, in which case this key is explicitly unbound)
+            // the keychord is defined in this layer, return the ID
             return keyIDPair->second;
         }
 
@@ -624,7 +637,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
             }
         }
 
-        // we did not find the keychord anywhere, its not bound and not explicity unbound either
+        // we did not find the keychord anywhere, it's not bound and not explicitly unbound either
         return std::nullopt;
     }
 
@@ -641,18 +654,16 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
     {
         if (const auto actionIDOptional = _GetActionIdByKeyChordInternal(keys))
         {
-            if (!(*actionIDOptional).empty())
+            if (!actionIDOptional->empty())
             {
                 // there is an ID associated with these keys, find the command
                 if (const auto foundCmd = _GetActionByID(*actionIDOptional))
                 {
                     return foundCmd;
                 }
             }
-            {
-                // the ID is an empty string, these keys are explicitly unbound
-                return nullptr;
-            }
+            // the ID is an empty string, these keys are explicitly unbound
+            return nullptr;
         }
 
         return std::nullopt;
@@ -665,7 +676,7 @@ 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(winrt::hstring cmdID) const
+    Control::KeyChord ActionMap::GetKeyBindingForAction(const winrt::hstring& cmdID) const
     {
         // Check our internal state.
         if (const auto cmd{ _GetActionByID(cmdID) })

src/cascadia/TerminalSettingsModel/ActionMap.h

@@ -61,7 +61,7 @@ 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(winrt::hstring cmdID) const;
+        Control::KeyChord GetKeyBindingForAction(const winrt::hstring& cmdID) const;
 
         // population
         void AddAction(const Model::Command& cmd);
@@ -71,7 +71,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
         std::vector<SettingsLoadWarnings> LayerJson(const Json::Value& json, const OriginTag origin, const bool withKeybindings = true);
         Json::Value ToJson() const;
         Json::Value KeyBindingsToJson() const;
-        bool FixUpsAppliedDuringLoad() const;
+        bool FixupsAppliedDuringLoad() const;
 
         // modification
         bool RebindKeys(const Control::KeyChord& oldKeys, const Control::KeyChord& newKeys);
@@ -85,7 +85,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
         winrt::Windows::Foundation::Collections::IVector<Model::Command> FilterToSendInput(winrt::hstring currentCommandline);
 
     private:
-        Model::Command _GetActionByID(const winrt::hstring actionID) const;
+        Model::Command _GetActionByID(const winrt::hstring& actionID) const;
         std::optional<winrt::hstring> _GetActionIdByKeyChordInternal(const Control::KeyChord& keys) const;
         std::optional<Model::Command> _GetActionByKeyChordInternal(const Control::KeyChord& keys) const;
 
@@ -109,7 +109,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
         std::unordered_map<winrt::hstring, Model::Command> _NestedCommands;
         std::vector<Model::Command> _IterableCommands;
 
-        bool _fixUpsAppliedDuringLoad;
+        bool _fixupsAppliedDuringLoad{ false };
 
         void _AddKeyBindingHelper(const Json::Value& json, std::vector<SettingsLoadWarnings>& warnings);
 

src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp

@@ -69,15 +69,15 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
                 {
                     if (jsonBlock.isMember(JsonKey(KeysKey)))
                     {
-                        // there are keys in this command block - its the legacy style
-                        _fixUpsAppliedDuringLoad = true;
+                        // 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;
+                        _fixupsAppliedDuringLoad = true;
                     }
                 }
             }

src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp

@@ -461,7 +461,7 @@ bool SettingsLoader::FixupUserSettings()
     };
 
     auto fixedUp = userSettings.fixupsAppliedDuringLoad;
-    fixedUp = userSettings.globals->FixUpsAppliedDuringLoad() || fixedUp;
+    fixedUp = userSettings.globals->FixupsAppliedDuringLoad() || fixedUp;
 
     fixedUp = RemapColorSchemeForProfile(userSettings.baseLayerProfile) || fixedUp;
     for (const auto& profile : userSettings.profiles)

src/cascadia/TerminalSettingsModel/Command.cpp

@@ -121,14 +121,14 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
             if (const auto generatedID = actionAndArgsImpl->GenerateID(); !generatedID.empty())
             {
                 _ID = generatedID;
-                _IdWasGenerated = true;
+                _IDWasGenerated = true;
             }
         }
     }
 
-    bool Command::IdWasGenerated()
+    bool Command::IDWasGenerated()
     {
-        return _IdWasGenerated;
+        return _IDWasGenerated;
     }
 
     void Command::Name(const hstring& value)

src/cascadia/TerminalSettingsModel/Command.h

@@ -71,7 +71,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
 
         hstring ID() const noexcept;
         void GenerateID();
-        bool IdWasGenerated();
+        bool IDWasGenerated();
 
         Control::KeyChord Keys() const noexcept;
         hstring KeyChordText() const noexcept;
@@ -97,7 +97,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
         std::vector<Control::KeyChord> _keyMappings;
         std::optional<std::wstring> _name;
         std::wstring _ID;
-        bool _IdWasGenerated{ false };
+        bool _IDWasGenerated{ false };
         std::optional<std::wstring> _iconPath;
         bool _nestedCommand{ false };
 

src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp

@@ -17,7 +17,7 @@ using namespace winrt::Windows::UI::Xaml;
 using namespace ::Microsoft::Console;
 using namespace winrt::Microsoft::UI::Xaml::Controls;
 
-static constexpr std::string_view LegacyKeybindingsKey{ "keybindings" };
+static constexpr std::string_view KeybindingsKey{ "keybindings" };
 static constexpr std::string_view ActionsKey{ "actions" };
 static constexpr std::string_view ThemeKey{ "theme" };
 static constexpr std::string_view DefaultProfileKey{ "defaultProfile" };
@@ -158,7 +158,7 @@ void GlobalAppSettings::LayerActionsFrom(const Json::Value& json, const OriginTa
 {
     // we want to do the keybindings map after the actions map so that we overwrite any leftover keybindings
     // that might have existed in the first pass, in case the user did a partial update from legacy to modern
-    static constexpr std::array bindingsKeys{ ActionsKey, LegacyKeybindingsKey };
+    static constexpr std::array bindingsKeys{ ActionsKey, KeybindingsKey };
     for (const auto& jsonKey : bindingsKeys)
     {
         if (auto bindings{ json[JsonKey(jsonKey)] })
@@ -262,14 +262,14 @@ Json::Value GlobalAppSettings::ToJson()
 #undef GLOBAL_SETTINGS_TO_JSON
 
     json[JsonKey(ActionsKey)] = _actionMap->ToJson();
-    json[JsonKey(LegacyKeybindingsKey)] = _actionMap->KeyBindingsToJson();
+    json[JsonKey(KeybindingsKey)] = _actionMap->KeyBindingsToJson();
 
     return json;
 }
 
-bool GlobalAppSettings::FixUpsAppliedDuringLoad()
+bool GlobalAppSettings::FixupsAppliedDuringLoad()
 {
-    return _actionMap->FixUpsAppliedDuringLoad();
+    return _actionMap->FixupsAppliedDuringLoad();
 }
 
 winrt::Microsoft::Terminal::Settings::Model::Theme GlobalAppSettings::CurrentTheme() noexcept

src/cascadia/TerminalSettingsModel/GlobalAppSettings.h

@@ -53,7 +53,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
         void LayerActionsFrom(const Json::Value& json, const OriginTag origin, const bool withKeybindings = true);
 
         Json::Value ToJson();
-        bool FixUpsAppliedDuringLoad();
+        bool FixupsAppliedDuringLoad();
 
         const std::vector<SettingsLoadWarnings>& KeybindingsWarnings() const;