From 0a3e17eebb3453268f7723c3bd57c6e0404d23ff Mon Sep 17 00:00:00 2001
From: Pankaj Bhojwani <pankaj.d.bhoj@gmail.com>
Date: Wed, 24 Apr 2024 15:33:35 -0700
Subject: [PATCH] edge cases

---
 .../TerminalSettingsModel/ActionMap.cpp       | 47 +++++++++++++++----
 .../ActionMapSerialization.cpp                |  2 +-
 2 files changed, 39 insertions(+), 10 deletions(-)

diff --git a/src/cascadia/TerminalSettingsModel/ActionMap.cpp b/src/cascadia/TerminalSettingsModel/ActionMap.cpp
index 648323b93e7..53eb69bdf45 100644
--- a/src/cascadia/TerminalSettingsModel/ActionMap.cpp
+++ b/src/cascadia/TerminalSettingsModel/ActionMap.cpp
@@ -584,11 +584,11 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
             // If the actionID is empty, this keybinding is explicitly unbound
             if (!actionID.empty())
             {
-                const auto cmd{ _GetActionByID2(actionID).value() };
-                if (cmd)
+                const auto cmd{ _GetActionByID2(actionID) };
+                if (cmd.has_value())
                 {
                     // iterate over all of the action's bound keys
-                    const auto cmdImpl{ get_self<Command>(cmd) };
+                    const auto cmdImpl{ get_self<Command>(cmd.value()) };
                     for (const auto& keys : cmdImpl->KeyMappings())
                     {
                         // Only populate KeyBindingsMap with actions that...
@@ -596,7 +596,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
                         // (2) aren't explicitly unbound
                         if (keyBindingsMap.find(keys) == keyBindingsMap.end() && unboundKeys.find(keys) == unboundKeys.end())
                         {
-                            keyBindingsMap.emplace(keys, cmd);
+                            keyBindingsMap.emplace(keys, cmd.value());
                         }
                     }
                 }
@@ -1081,23 +1081,52 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
     // - nullopt if it was not bound in this layer
     std::optional<Model::Command> ActionMap::_GetActionByKeyChordInternal2(const Control::KeyChord& keys) const
     {
+        // todo: stage 3 - why does this function search through parents but _GetActionByID2 doesn't? even the original implementation was similar with some recursive and some non-recursive functions
         if (const auto keyIDPair = _KeyMap2.find(keys); keyIDPair != _KeyMap2.end())
         {
             if (const auto cmdID = keyIDPair->second; !cmdID.empty())
             {
-                return _GetActionByID2(cmdID);
+                if (const auto cmd = _GetActionByID2(cmdID); cmd.has_value())
+                {
+                    // standard case: both the keys and the ID are defined in this layer
+                    return cmd;
+                }
+                else
+                {
+                    for (const auto parent : _parents)
+                    {
+                        if (const auto inheritedCmd = parent->_GetActionByID2(cmdID); inheritedCmd.has_value())
+                        {
+                            // edge case 1: the keys are bound to an ID in this layer, but the ID is defined in one of our parents
+                            return inheritedCmd;
+                        }
+                    }
+                }
             }
             else
             {
-                // the keychord is in our map, but points to an empty string - explicitly unbound
+                // the keychord is defined in this layer, but points to an empty string - explicitly unbound
                 return nullptr;
             }
         }
 
-        // the command was not bound in this layer,
-        // ask my parents
+        // search through our parents
         for (const auto& parent : _parents)
         {
+            if (const auto parentKeyIDPair = parent->_KeyMap2.find(keys); parentKeyIDPair != parent->_KeyMap2.end())
+            {
+                if (const auto cmdID = parentKeyIDPair->second; !cmdID.empty())
+                {
+                    if (const auto cmd = _GetActionByID2(cmdID); cmd.has_value())
+                    {
+                        // edge case 2: the keychord maps to an ID in one of our parents, but a command with that ID exists in this layer
+                        //              use the command from this layer
+                        return cmd;
+                    }
+                }
+            }
+
+            // we've checked for the standard case and the 2 edge cases, now we can recurse
             const auto& inheritedCmd{ parent->_GetActionByKeyChordInternal2(keys) };
             if (inheritedCmd)
             {
@@ -1105,7 +1134,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
             }
         }
 
-        // we did not find the keychord in our map, its not bound and not explicity unbound either
+        // we did not find the keychord anywhere, its not bound and not explicity unbound either
         return std::nullopt;
     }
 
diff --git a/src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp b/src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp
index 21803c845be..6cc158892c7 100644
--- a/src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp
+++ b/src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp
@@ -175,7 +175,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
             winrt::hstring idJson;
             if (JsonUtils::GetValueForKey(json, "keys", keys))
             {
-                // even if the "id" field doesn't exist in the json, idJson will be an empty string which is fine
+                // if the "id" field doesn't exist in the json, idJson will be an empty string which is fine
                 JsonUtils::GetValueForKey(json, "id", idJson);
 
                 // any existing keybinding with the same keychord in this layer will get overwritten