Repository: microsoft/terminal (language: cpp)

Issue:
Fragments should be able to add actions
See 
* #6899 
* #7584
* #4000

Useful for:
* #15664 
* #14002
* #13445
* #14864

To a lesser extent:
* #2205
* #14002
* #12927
* #12862


Discussion / hints:
I think we'd need to: 

* ~Add an internal / private `source` to actions, to indicate where we got the action from (ala profiles)~
* ~in `SettingsLoader::_parseFragment` move them all over into the user settings~
* ~don't serialize ones back that weren't from the user settings~

NOPE not that

* `GlobalAppSettings::_FinalizeInheritance` sets the LeastImportantParent for the user's action map on _each_ parent global settings. Well, easy enough. 
* Create a "blank" GAS for each fragment
* don't parse keys
* add as a parent of the user's GAS

Fix patch:
diff --git a/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp b/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp
index b5139a5263f..50d649d7a5d 100644
--- a/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp
+++ b/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp
@@ -74,6 +74,13 @@ namespace SettingsModelLocalTests
         TEST_METHOD(TestInheritedCommand);
         TEST_METHOD(LoadFragmentsWithMultipleUpdates);
 
+        TEST_METHOD(FragmentActionSimple);
+        TEST_METHOD(FragmentActionNoKeys);
+        TEST_METHOD(FragmentActionNested);
+        TEST_METHOD(FragmentActionNestedNoName);
+        TEST_METHOD(FragmentActionIterable);
+        TEST_METHOD(FragmentActionRoundtrip);
+
         TEST_METHOD(MigrateReloadEnvVars);
 
     private:
@@ -2023,6 +2030,189 @@ namespace SettingsModelLocalTests
         VERIFY_ARE_EQUAL(L"NewName", loader.userSettings.profiles[0]->Name());
     }
 
+    void DeserializationTests::FragmentActionSimple()
+    {
+        static constexpr std::wstring_view fragmentSource{ L"fragment" };
+        static constexpr std::string_view fragmentJson{ R"({
+            "actions": [
+                {
+                    "command": { "action": "addMark" },
+                    "name": "Test Action"
+                },
+            ]
+        })" };
+
+        implementation::SettingsLoader loader{ std::string_view{}, DefaultJson };
+        loader.MergeInboxIntoUserSettings();
+        loader.MergeFragmentIntoUserSettings(winrt::hstring{ fragmentSource }, fragmentJson);
+        loader.FinalizeLayering();
+
+        const auto settings = winrt::make_self<implementation::CascadiaSettings>(std::move(loader));
+
+        const auto actionMap = winrt::get_self<implementation::ActionMap>(settings->GlobalSettings().ActionMap());
+        const auto actionsByName = actionMap->NameMap();
+        VERIFY_IS_NOT_NULL(actionsByName.TryLookup(L"Test Action"));
+    }
+
+    void DeserializationTests::FragmentActionNoKeys()
+    {
+        static constexpr std::wstring_view fragmentSource{ L"fragment" };
+        static constexpr std::string_view fragmentJson{ R"({
+            "actions": [
+                {
+                    "command": { "action": "addMark" },
+                    "keys": "ctrl+f",
+                    "name": "Test Action"
+                },
+            ]
+        })" };
+
+        implementation::SettingsLoader loader{ std::string_view{}, DefaultJson };
+        loader.MergeInboxIntoUserSettings();
+        loader.MergeFragmentIntoUserSettings(winrt::hstring{ fragmentSource }, fragmentJson);
+        loader.FinalizeLayering();
+
+        const auto settings = winrt::make_self<implementation::CascadiaSettings>(std::move(loader));
+
+        const auto actionMap = winrt::get_self<implementation::ActionMap>(settings->GlobalSettings().ActionMap());
+        const auto actionsByName = actionMap->NameMap();
+        VERIFY_IS_NOT_NULL(actionsByName.TryLookup(L"Test Action"));
+        VERIFY_IS_NULL(actionMap->GetActionByKeyChord({ VirtualKeyModifiers::Control, static_cast<int32_t>('F'), 0 }));
+    }
+
+    void DeserializationTests::FragmentActionNested()
+    {
+        static constexpr std::wstring_view fragmentSource{ L"fragment" };
+        static constexpr std::string_view fragmentJson{ R"({
+            "actions": [
+                {
+                    "name": "nested command",
+                    "commands": [
+                        {
+                            "name": "child1",
+                            "command": { "action": "newTab", "commandline": "ssh me@first.com" }
+                        },
+                        {
+                            "name": "child2",
+                            "command": { "action": "newTab", "commandline": "ssh me@second.com" }
+                        }
+                    ]
+                },
+            ]
+        })" };
+
+        implementation::SettingsLoader loader{ std::string_view{}, DefaultJson };
+        loader.MergeInboxIntoUserSettings();
+        loader.MergeFragmentIntoUserSettings(winrt::hstring{ fragmentSource }, fragmentJson);
+        loader.FinalizeLayering();
+
+        const auto settings = winrt::make_self<implementation::CascadiaSettings>(std::move(loader));
+
+        const auto actionMap = winrt::get_self<implementation::ActionMap>(settings->GlobalSettings().ActionMap());
+        const auto actionsByName = actionMap->NameMap();
+        const auto& nested{ actionsByName.TryLookup(L"nested command") };
+        VERIFY_IS_NOT_NULL(nested);
+        VERIFY_IS_TRUE(nested.HasNestedCommands());
+    }
+
+    void DeserializationTests::FragmentActionNestedNoName()
+    {
+        // Basically the same as TestNestedCommandWithoutName
+        static constexpr std::wstring_view fragmentSource{ L"fragment" };
+        static constexpr std::string_view fragmentJson{ R"({
+            "actions": [
+                {
+                    "commands": [
+                        {
+                            "name": "child1",
+                            "command": { "action": "newTab", "commandline": "ssh me@first.com" }
+                        },
+                        {
+                            "name": "child2",
+                            "command": { "action": "newTab", "commandline": "ssh me@second.com" }
+                        }
+                    ]
+                },
+            ]
+        })" };
+
+        implementation::SettingsLoader loader{ std::string_view{}, DefaultJson };
+        loader.MergeInboxIntoUserSettings();
+        loader.MergeFragmentIntoUserSettings(winrt::hstring{ fragmentSource }, fragmentJson);
+        loader.FinalizeLayering();
+
+        const auto settings = winrt::make_self<implementation::CascadiaSettings>(std::move(loader));
+        VERIFY_ARE_EQUAL(0u, settings->Warnings().Size());
+    }
+    void DeserializationTests::FragmentActionIterable()
+    {
+        static constexpr std::wstring_view fragmentSource{ L"fragment" };
+        static constexpr std::string_view fragmentJson{ R"({
+            "actions": [
+                {
+                    "name": "nested",
+                    "commands": [
+                        {
+                            "iterateOn": "schemes",
+                            "name": "${scheme.name}",
+                            "command": { "action": "setColorScheme", "colorScheme": "${scheme.name}" }
+                        }
+                    ]
+                },
+            ]
+        })" };
+
+        implementation::SettingsLoader loader{ std::string_view{}, DefaultJson };
+        loader.MergeInboxIntoUserSettings();
+        loader.MergeFragmentIntoUserSettings(winrt::hstring{ fragmentSource }, fragmentJson);
+        loader.FinalizeLayering();
+
+        const auto settings = winrt::make_self<implementation::CascadiaSettings>(std::move(loader));
+
+        const auto actionMap = winrt::get_self<implementation::ActionMap>(settings->GlobalSettings().ActionMap());
+        const auto actionsByName = actionMap->NameMap();
+        const auto& nested{ actionsByName.TryLookup(L"nested") };
+        VERIFY_IS_NOT_NULL(nested);
+        VERIFY_IS_TRUE(nested.HasNestedCommands());
+        VERIFY_ARE_EQUAL(settings->GlobalSettings().ColorSchemes().Size(), nested.NestedCommands().Size());
+    }
+    void DeserializationTests::FragmentActionRoundtrip()
+    {
+        static constexpr std::wstring_view fragmentSource{ L"fragment" };
+        static constexpr std::string_view fragmentJson{ R"({
+            "actions": [
+                {
+                    "command": { "action": "addMark" },
+                    "name": "Test Action"
+                },
+            ]
+        })" };
+
+        implementation::SettingsLoader loader{ std::string_view{}, DefaultJson };
+        loader.MergeInboxIntoUserSettings();
+        loader.MergeFragmentIntoUserSettings(winrt::hstring{ fragmentSource }, fragmentJson);
+        loader.FinalizeLayering();
+
+        const auto oldSettings = winrt::make_self<implementation::CascadiaSettings>(std::move(loader));
+
+        const auto actionMap = winrt::get_self<implementation::ActionMap>(oldSettings->GlobalSettings().ActionMap());
+        const auto actionsByName = actionMap->NameMap();
+        VERIFY_IS_NOT_NULL(actionsByName.TryLookup(L"Test Action"));
+
+        const auto oldResult{ oldSettings->ToJson() };
+
+        Log::Comment(L"Now, create a _new_ settings object from the re-serialization of the first");
+        implementation::SettingsLoader newLoader{ toString(oldResult), DefaultJson };
+        // NOTABLY! Don't load the fragment here.
+        newLoader.MergeInboxIntoUserSettings();
+        newLoader.FinalizeLayering();
+        const auto newSettings = winrt::make_self<implementation::CascadiaSettings>(std::move(newLoader));
+
+        const auto& newActionMap = winrt::get_self<implementation::ActionMap>(newSettings->GlobalSettings().ActionMap());
+        const auto newActionsByName = newActionMap->NameMap();
+        VERIFY_IS_NULL(newActionsByName.TryLookup(L"Test Action"));
+    }
+
     void DeserializationTests::MigrateReloadEnvVars()
     {
         static constexpr std::string_view settings1Json{ R"(
diff --git a/src/cascadia/LocalTests_SettingsModel/SerializationTests.cpp b/src/cascadia/LocalTests_SettingsModel/SerializationTests.cpp
index 2488f3f812d..463d0dc0478 100644
--- a/src/cascadia/LocalTests_SettingsModel/SerializationTests.cpp
+++ b/src/cascadia/LocalTests_SettingsModel/SerializationTests.cpp
@@ -68,6 +68,19 @@ namespace SettingsModelLocalTests
             // written alphabetically.
             VERIFY_ARE_EQUAL(toString(json), toString(result));
         }
+
+        // Helper to remove the `$schema` property from a json object. We
+        // populate that based off the local path to the settings file. Of
+        // course, that's entirely unpredictable in tests. So cut it out before
+        // we do any sort of roundtrip testing.
+        static Json::Value removeSchema(Json::Value json)
+        {
+            if (json.isMember("$schema"))
+            {
+                json.removeMember("$schema");
+            }
+            return json;
+        }
     };
 
     void SerializationTests::GlobalSettings()
@@ -262,15 +275,6 @@ namespace SettingsModelLocalTests
                                                 { "command": { "action": "adjustFontSize", "delta": 1.0 }, "keys": "ctrl+c" },
                                                 { "command": { "action": "adjustFontSize", "delta": 1.0 }, "keys": "ctrl+d" }
                                             ])" };
-        // GH#13323 - these can be fragile. In the past, the order these get
-        // re-serialized as has been not entirely stable. We don't really care
-        // about the order they get re-serialized in, but the tests aren't
-        // clever enough to compare the structure, only the literal string
-        // itself. Feel free to change as needed.
-        static constexpr std::string_view actionsString4B{ R"([
-                                                { "command": { "action": "findMatch", "direction": "prev" }, "keys": "ctrl+shift+r" },
-                                                { "command": { "action": "adjustFontSize", "delta": 1.0 }, "keys": "ctrl+d" }
-                                            ])" };
 
         // command with name and icon and multiple key chords
         static constexpr std::string_view actionsString5{ R"([
@@ -384,7 +388,6 @@ namespace SettingsModelLocalTests
 
         Log::Comment(L"complex commands with key chords");
         RoundtripTest<implementation::ActionMap>(actionsString4A);
-        RoundtripTest<implementation::ActionMap>(actionsString4B);
 
         Log::Comment(L"command with name and icon and multiple key chords");
         RoundtripTest<implementation::ActionMap>(actionsString5);
@@ -483,7 +486,8 @@ namespace SettingsModelLocalTests
         const auto settings{ winrt::make_self<implementation::CascadiaSettings>(settingsString) };
 
         const auto result{ settings->ToJson() };
-        VERIFY_ARE_EQUAL(toString(VerifyParseSucceeded(settingsString)), toString(result));
+        VERIFY_ARE_EQUAL(toString(removeSchema(VerifyParseSucceeded(settingsString))),
+                         toString(removeSchema(result)));
     }
 
     void SerializationTests::LegacyFontSettings()
diff --git a/src/cascadia/TerminalSettingsModel/ActionMap.h b/src/cascadia/TerminalSettingsModel/ActionMap.h
index 4270a081118..d59dc0677d8 100644
--- a/src/cascadia/TerminalSettingsModel/ActionMap.h
+++ b/src/cascadia/TerminalSettingsModel/ActionMap.h
@@ -67,7 +67,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
 
         // JSON
         static com_ptr<ActionMap> FromJson(const Json::Value& json);
-        std::vector<SettingsLoadWarnings> LayerJson(const Json::Value& json);
+        std::vector<SettingsLoadWarnings> LayerJson(const Json::Value& json, const bool withKeybindings = true);
         Json::Value ToJson() const;
 
         // modification
diff --git a/src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp b/src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp
index a788724b43f..248a0023d00 100644
--- a/src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp
+++ b/src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp
@@ -35,7 +35,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
     // - json: an array of Json::Value's to deserialize into our ActionMap.
     // Return value:
     // - a list of warnings encountered while deserializing the json
-    std::vector<SettingsLoadWarnings> ActionMap::LayerJson(const Json::Value& json)
+    std::vector<SettingsLoadWarnings> ActionMap::LayerJson(const Json::Value& json, const bool withKeybindings)
     {
         // It's possible that the user provided keybindings have some warnings in
         // them - problems that we should alert the user to, but we can recover
@@ -50,7 +50,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
                 continue;
             }
 
-            AddAction(*Command::FromJson(cmdJson, warnings));
+            AddAction(*Command::FromJson(cmdJson, warnings, withKeybindings));
         }
 
         return warnings;
diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp
index 8964169f207..9e53487bb02 100644
--- a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp
+++ b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp
@@ -629,7 +629,7 @@ void SettingsLoader::_parse(const OriginTag origin, const winrt::hstring& source
 // schemes and profiles. Additionally this function supports profiles which specify an "updates" key.
 void SettingsLoader::_parseFragment(const winrt::hstring& source, const std::string_view& content, ParsedSettings& settings)
 {
-    const auto json = _parseJson(content);
+    auto json = _parseJson(content);
 
     settings.clear();
 
@@ -647,6 +647,11 @@ void SettingsLoader::_parseFragment(const winrt::hstring& source, const std::str
             }
             CATCH_LOG()
         }
+
+        // Parse out actions from the fragment. Manually opt-out of keybinding
+        // parsing - fragments shouldn't be allowed to bind actions to keys
+        // directly. We may want to revisit circa GH#2205
+        settings.globals->LayerActionsFrom(json.root, false);
     }
 
     {
@@ -688,10 +693,10 @@ void SettingsLoader::_parseFragment(const winrt::hstring& source, const std::str
         }
     }
 
-    for (const auto& kv : settings.globals->ColorSchemes())
-    {
-        userSettings.globals->AddColorScheme(kv.Value());
-    }
+    // Add the parsed fragment globals as a parent of the user's settings.
+    // Later, in FinalizeInheritance, this will result in the action map from
+    // the fragments being applied before the user's own settings.
+    userSettings.globals->AddLeastImportantParent(settings.globals);
 }
 
 SettingsLoader::JsonSettings SettingsLoader::_parseJson(const std::string_view& content)
diff --git a/src/cascadia/TerminalSettingsModel/Command.cpp b/src/cascadia/TerminalSettingsModel/Command.cpp
index b211987b677..8ca3ebe95a7 100644
--- a/src/cascadia/TerminalSettingsModel/Command.cpp
+++ b/src/cascadia/TerminalSettingsModel/Command.cpp
@@ -258,7 +258,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
     // Return Value:
     // - the newly constructed Command object.
     winrt::com_ptr<Command> Command::FromJson(const Json::Value& json,
-                                              std::vector<SettingsLoadWarnings>& warnings)
+                                              std::vector<SettingsLoadWarnings>& warnings,
+                                              const bool parseKeys)
     {
         auto result = winrt::make_self<Command>();
 
@@ -313,20 +314,23 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
                 result->_ActionAndArgs = make<implementation::ActionAndArgs>();
             }
 
-            // 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)
+            if (parseKeys)
             {
-                warnings.push_back(SettingsLoadWarnings::TooManyKeysForChord);
-            }
-            else
-            {
-                Control::KeyChord keys{ nullptr };
-                if (JsonUtils::GetValueForKey(json, KeysKey, keys))
+                // 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)
                 {
-                    result->RegisterKey(keys);
+                    warnings.push_back(SettingsLoadWarnings::TooManyKeysForChord);
+                }
+                else
+                {
+                    Control::KeyChord keys{ nullptr };
+                    if (JsonUtils::GetValueForKey(json, KeysKey, keys))
+                    {
+                        result->RegisterKey(keys);
+                    }
                 }
             }
         }
diff --git a/src/cascadia/TerminalSettingsModel/Command.h b/src/cascadia/TerminalSettingsModel/Command.h
index 41037bf1a29..4418716c815 100644
--- a/src/cascadia/TerminalSettingsModel/Command.h
+++ b/src/cascadia/TerminalSettingsModel/Command.h
@@ -39,7 +39,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
         com_ptr<Command> Copy() const;
 
         static winrt::com_ptr<Command> FromJson(const Json::Value& json,
-                                                std::vector<SettingsLoadWarnings>& warnings);
+                                                std::vector<SettingsLoadWarnings>& warnings,
+                                                const bool parseKeys = true);
 
         static void ExpandCommands(Windows::Foundation::Collections::IMap<winrt::hstring, Model::Command>& commands,
                                    Windows::Foundation::Collections::IVectorView<Model::Profile> profiles,
diff --git a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp
index 1caef96b2cf..5cc65d913e4 100644
--- a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp
+++ b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp
@@ -142,12 +142,19 @@ void GlobalAppSettings::LayerJson(const Json::Value& json)
     MTSM_GLOBAL_SETTINGS(GLOBAL_SETTINGS_LAYER_JSON)
 #undef GLOBAL_SETTINGS_LAYER_JSON
 
+    LayerActionsFrom(json, true);
+
+    JsonUtils::GetValueForKey(json, LegacyReloadEnvironmentVariablesKey, _legacyReloadEnvironmentVariables);
+}
+
+void GlobalAppSettings::LayerActionsFrom(const Json::Value& json, const bool withKeybindings)
+{
     static constexpr std::array bindingsKeys{ LegacyKeybindingsKey, ActionsKey };
     for (const auto& jsonKey : bindingsKeys)
     {
         if (auto bindings{ json[JsonKey(jsonKey)] })
         {
-            auto warnings = _actionMap->LayerJson(bindings);
+            auto warnings = _actionMap->LayerJson(bindings, withKeybindings);
 
             // It's possible that the user provided keybindings have some warnings
             // in them - problems that we should alert the user to, but we can
@@ -158,8 +165,6 @@ void GlobalAppSettings::LayerJson(const Json::Value& json)
             _keybindingsWarnings.insert(_keybindingsWarnings.end(), warnings.begin(), warnings.end());
         }
     }
-
-    JsonUtils::GetValueForKey(json, LegacyReloadEnvironmentVariablesKey, _legacyReloadEnvironmentVariables);
 }
 
 // Method Description:
diff --git a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.h b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.h
index 3ccc0972bea..251239d07e4 100644
--- a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.h
+++ b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.h
@@ -49,6 +49,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
 
         static com_ptr<GlobalAppSettings> FromJson(const Json::Value& json);
         void LayerJson(const Json::Value& json);
+        void LayerActionsFrom(const Json::Value& json, const bool withKeybindings = true);
 
         Json::Value ToJson() const;
 


Test patch: