Skip to content

Commit

Permalink
Add support for actions in fragments (#16185)
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@zadjii-msft
zadjii-msft authored Feb 26, 2024
1 parent a6a0e44 commit 88def9d
Show file tree
Hide file tree
Showing 9 changed files with 246 additions and 36 deletions.
190 changes: 190 additions & 0 deletions src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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"(
Expand Down
26 changes: 15 additions & 11 deletions src/cascadia/LocalTests_SettingsModel/SerializationTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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"([
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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()
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalSettingsModel/ActionMap.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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;
Expand Down
15 changes: 10 additions & 5 deletions src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand All @@ -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);
}

{
Expand Down Expand Up @@ -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)
Expand Down
30 changes: 17 additions & 13 deletions src/cascadia/TerminalSettingsModel/Command.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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>();

Expand Down Expand Up @@ -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);
}
}
}
}
Expand Down
3 changes: 2 additions & 1 deletion src/cascadia/TerminalSettingsModel/Command.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Loading

0 comments on commit 88def9d

Please sign in to comment.