From 88def9ddcd1088f5c9431e8ea45820128572c74a Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Mon, 26 Feb 2024 12:27:57 -0800 Subject: [PATCH] Add support for actions in fragments (#16185) Surprisingly easier than I thought this would be. ActionMap already supports layering (from defaults.json), so this basically re-uses a lot of that for fun and profit. The trickiest bits: * In `SettingsLoader::_parseFragment`, I'm constructing a fake, empty JSON object, and taking _only_ the actions out from the fragment, and stuffing them into this temp json. Then, I parse that as a globals object, and set _that_ as the parent to the user settings file. That results in _only_ the actions from the fragment being parsed before the user's actions. * In that same method, I'm also explicitly preventing the ActionMap (et al.) from parsing `keys` from these actions. We don't want fragments to be able to say "ctrl+f is clear buffer" or something like that. This required a bit of annoying plumbing. Closes #16063 Tests added. Docs need to be updated. --- .../DeserializationTests.cpp | 190 ++++++++++++++++++ .../SerializationTests.cpp | 26 ++- .../TerminalSettingsModel/ActionMap.h | 2 +- .../ActionMapSerialization.cpp | 4 +- .../CascadiaSettingsSerialization.cpp | 15 +- .../TerminalSettingsModel/Command.cpp | 30 +-- src/cascadia/TerminalSettingsModel/Command.h | 3 +- .../GlobalAppSettings.cpp | 11 +- .../TerminalSettingsModel/GlobalAppSettings.h | 1 + 9 files changed, 246 insertions(+), 36 deletions(-) diff --git a/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp b/src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp index b5139a5263..50d649d7a5 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(std::move(loader)); + + const auto actionMap = winrt::get_self(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(std::move(loader)); + + const auto actionMap = winrt::get_self(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('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(std::move(loader)); + + const auto actionMap = winrt::get_self(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(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(std::move(loader)); + + const auto actionMap = winrt::get_self(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(std::move(loader)); + + const auto actionMap = winrt::get_self(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(std::move(newLoader)); + + const auto& newActionMap = winrt::get_self(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 2488f3f812..463d0dc047 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(actionsString4A); - RoundtripTest(actionsString4B); Log::Comment(L"command with name and icon and multiple key chords"); RoundtripTest(actionsString5); @@ -483,7 +486,8 @@ namespace SettingsModelLocalTests const auto settings{ winrt::make_self(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 4270a08111..d59dc0677d 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 FromJson(const Json::Value& json); - std::vector LayerJson(const Json::Value& json); + std::vector 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 a788724b43..248a0023d0 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 ActionMap::LayerJson(const Json::Value& json) + std::vector 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 7510bb0ebf..c22d421975 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 b211987b67..8ca3ebe95a 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::FromJson(const Json::Value& json, - std::vector& warnings) + std::vector& warnings, + const bool parseKeys) { auto result = winrt::make_self(); @@ -313,20 +314,23 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation result->_ActionAndArgs = make(); } - // 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 41037bf1a2..4418716c81 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 Copy() const; static winrt::com_ptr FromJson(const Json::Value& json, - std::vector& warnings); + std::vector& warnings, + const bool parseKeys = true); static void ExpandCommands(Windows::Foundation::Collections::IMap& commands, Windows::Foundation::Collections::IVectorView profiles, diff --git a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp index 1caef96b2c..5cc65d913e 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 9c14376d05..1af8479e0c 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 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;