From 586b977f2552c510f1e74234de0f16d784ef0e45 Mon Sep 17 00:00:00 2001
From: Carlos Zamora <cazamor@microsoft.com>
Date: Thu, 22 Apr 2021 16:58:17 -0700
Subject: [PATCH] handle and test nested and unbound commands

---
 .../SerializationTests.cpp                    | 41 ++++++++++++++++++-
 .../TerminalSettingsModel/ActionAndArgs.cpp   |  8 ++--
 .../ActionMapSerialization.cpp                |  9 ++++
 .../TerminalSettingsModel/Command.cpp         | 10 ++++-
 4 files changed, 60 insertions(+), 8 deletions(-)

diff --git a/src/cascadia/LocalTests_SettingsModel/SerializationTests.cpp b/src/cascadia/LocalTests_SettingsModel/SerializationTests.cpp
index 8deac700042..227b437b859 100644
--- a/src/cascadia/LocalTests_SettingsModel/SerializationTests.cpp
+++ b/src/cascadia/LocalTests_SettingsModel/SerializationTests.cpp
@@ -234,11 +234,9 @@ namespace SettingsModelLocalTests
         const std::string actionsString2A{ R"([
                                                 { "command": { "action": "setTabColor" } }
                                             ])" };
-
         const std::string actionsString2B{ R"([
                                                 { "command": { "action": "setTabColor", "color": "#112233" } }
                                             ])" };
-
         const std::string actionsString2C{ R"([
                                                 { "command": { "action": "copy" } },
                                                 { "command": { "action": "copy", "singleLine": true, "copyFormatting": "html" } }
@@ -262,10 +260,40 @@ namespace SettingsModelLocalTests
         const std::string actionsString6{ R"([
                                                 { "command": { "action": "newTab", "index": 0 }, "keys": "ctrl+g" },
                                             ])" };
+
         const std::string actionsString7{ R"([
                                                 { "command": { "action": "renameWindow", "name": null }, "keys": "ctrl+h" }
                                             ])" };
 
+        const std::string actionsString8{ R"([
+                                                {
+                                                    "name": "Change font size...",
+                                                    "commands": [
+                                                        { "command": { "action": "adjustFontSize", "delta": 1 } },
+                                                        { "command": { "action": "adjustFontSize", "delta": -1 } },
+                                                        { "command": "resetFontSize" },
+                                                    ]
+                                                }
+                                            ])" };
+
+        const std::string actionsString9{ R"([
+                                                {
+                                                    "name": "New tab",
+                                                    "commands": [
+                                                        {
+                                                            "iterateOn": "profiles",
+                                                            "icon": "${profile.icon}",
+                                                            "name": "${profile.name}",
+                                                            "command": { "action": "newTab", "profile": "${profile.name}" }
+                                                        }
+                                                    ]
+                                                }
+                                            ])" };
+
+        const std::string actionsString10{ R"([
+                                                { "command": "unbound", "keys": "ctrl+c" }
+                                            ])" };
+
         Log::Comment(L"simple command");
         RoundtripTest<implementation::ActionMap>(actionsString1);
 
@@ -288,6 +316,15 @@ namespace SettingsModelLocalTests
 
         Log::Comment(L"complex command with meaningful null arg");
         RoundtripTest<implementation::ActionMap>(actionsString7);
+
+        Log::Comment(L"nested command");
+        RoundtripTest<implementation::ActionMap>(actionsString8);
+
+        Log::Comment(L"iterable command");
+        RoundtripTest<implementation::ActionMap>(actionsString9);
+
+        Log::Comment(L"unbound command");
+        RoundtripTest<implementation::ActionMap>(actionsString10);
     }
 
     void SerializationTests::CascadiaSettings()
diff --git a/src/cascadia/TerminalSettingsModel/ActionAndArgs.cpp b/src/cascadia/TerminalSettingsModel/ActionAndArgs.cpp
index 7babf60bcb2..31016e50ca4 100644
--- a/src/cascadia/TerminalSettingsModel/ActionAndArgs.cpp
+++ b/src/cascadia/TerminalSettingsModel/ActionAndArgs.cpp
@@ -158,9 +158,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
         { ShortcutAction::NewWindow, NewWindowArgs::ToJson },
         { ShortcutAction::PrevTab, PrevTabArgs::ToJson },
         { ShortcutAction::NextTab, NextTabArgs::ToJson },
-        { ShortcutAction::RenameWindow, RenameWindowArgs::ToJson },
-
-        { ShortcutAction::Invalid, nullptr },
+        { ShortcutAction::RenameWindow, RenameWindowArgs::ToJson }
     };
 
     // Function Description:
@@ -301,8 +299,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
             }
         }
 
-        // "command": null
-        return { Json::ValueType::nullValue };
+        // "command": "unbound"
+        return JsonKey(UnboundKey);
     }
 
     com_ptr<ActionAndArgs> ActionAndArgs::Copy() const
diff --git a/src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp b/src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp
index a1c975a6744..ffb4ec56828 100644
--- a/src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp
+++ b/src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp
@@ -68,6 +68,15 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
                 actionList.append(cmdJson);
             }
         }
+        for (const auto& [_, cmd] : _NestedCommands)
+        {
+            const auto cmdImpl{ winrt::get_self<implementation::Command>(cmd) };
+            const auto& cmdJsonArray{ cmdImpl->ToJson() };
+            for (const auto& cmdJson : cmdJsonArray)
+            {
+                actionList.append(cmdJson);
+            }
+        }
         return actionList;
     }
 
diff --git a/src/cascadia/TerminalSettingsModel/Command.cpp b/src/cascadia/TerminalSettingsModel/Command.cpp
index fc5ca0d17ba..a172c7b731a 100644
--- a/src/cascadia/TerminalSettingsModel/Command.cpp
+++ b/src/cascadia/TerminalSettingsModel/Command.cpp
@@ -393,7 +393,15 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
     {
         Json::Value cmdList{ Json::ValueType::arrayValue };
 
-        if (_keyMappings.empty())
+        if (_nestedCommand)
+        {
+            // handle nested command
+            // For nested commands, we can trust _originalJson to be correct.
+            // In fact, we _need_ to use it here because we don't actually deserialize `iterateOn`
+            //   until we expand the command.
+            cmdList.append(_originalJson);
+        }
+        else if (_keyMappings.empty())
         {
             // only write out one command
             Json::Value cmdJson{ Json::ValueType::objectValue };