Add support for actions in fragments

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 [email protected]" }
+                        },
+                        {
+                            "name": "child2",
+                            "command": { "action": "newTab", "commandline": "ssh [email protected]" }
+                        }
+                    ]
+                },
+            ]
+        })" };
+
+        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 [email protected]" }
+                        },
+                        {
+                            "name": "child2",
+                            "command": { "action": "newTab", "commandline": "ssh [email protected]" }
+                        }
+                    ]
+                },
+            ]
+        })" };
+
+        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"(

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()

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

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;

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)

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);
+                    }
                 }
             }
         }

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,