Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support for actions in fragments #16185

Merged
merged 7 commits into from
Feb 26, 2024
Merged

Add support for actions in fragments #16185

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
37d7def
I think this might just work, but we need MOAR tests
zadjii-msft Oct 17, 2023
d0fcdd9
It cannot have been this easy
zadjii-msft Oct 17, 2023
2f39e1e
some test fixes, originally from f516c5f7d625ce69d57f04527e027ed9dd25…
zadjii-msft Oct 17, 2023
79cead1
cleanup
zadjii-msft Oct 17, 2023
35cde03
99% sure this should work, but the manifest is failing to generate ag…
zadjii-msft Oct 17, 2023
f80a897
Merge remote-tracking branch 'origin/main' into dev/migrie/f/fragment…
zadjii-msft Oct 24, 2023
d9aa152
less leaky abstraction here
zadjii-msft Oct 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
190 changes: 190 additions & 0 deletions src/cascadia/LocalTests_SettingsModel/DeserializationTests.cpp
View file
Open in desktop
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 [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()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i like the round-trip test verifying we didn't serialize someone else's action. clever.

{
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
23 changes: 18 additions & 5 deletions src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
static constexpr std::string_view ProfilesListKey{ "list" };
static constexpr std::string_view SchemesKey{ "schemes" };
static constexpr std::string_view ThemesKey{ "themes" };
static constexpr std::string_view ActionsKey{ "actions" };

constexpr std::wstring_view systemThemeName{ L"system" };
constexpr std::wstring_view darkThemeName{ L"dark" };
Expand Down Expand Up @@ -629,7 +630,7 @@
// 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 +648,18 @@
}
CATCH_LOG()
}

// Construct a temp Json::Value that contains ONLY the actions from the
// fragment. This will allow fragments to add actions, but not
// necessarily set other global properties.
Json::Value tmp = {};
tmp[ActionsKey.data()] = json.root[ActionsKey.data()];

// Now parse that tmep json object, as if it were a global settings

Check failure on line 658 in src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp

View workflow job for this annotation

GitHub Actions / Spell checking 

`tmep` is not a recognized word. (unrecognized-spelling)

Check failure on line 658 in src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp

View workflow job for this annotation

GitHub Actions / Spell checking 

`tmep` is not a recognized word. (unrecognized-spelling)
zadjii-msft marked this conversation as resolved.
Show resolved Hide resolved
// blob. 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->LayerJson(tmp, false);
zadjii-msft marked this conversation as resolved.
Show resolved Hide resolved
}

{
Expand Down Expand Up @@ -688,10 +701,10 @@
}
}

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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite get this change and how it replaces the previous code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this one was tricky - it would manually insert schemes from fragments into the userSettings, which was weird but okay. Now, instead of doing that, I just set the whole globals object to be a parent of the user's globals. We'll still resolve the schemes in a similar way in GlobalAppSettings::_FinalizeInheritance, but we'll also do the right thing for actions now too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah, this is likely to have downstream effects on me as well. thanks!

}

SettingsLoader::JsonSettings SettingsLoader::_parseJson(const std::string_view& content)
Expand Down
30 changes: 17 additions & 13 deletions src/cascadia/TerminalSettingsModel/Command.cpp
View file
Open in desktop
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)
zadjii-msft marked this conversation as resolved.
Show resolved Hide resolved
{
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
Open in desktop
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
Loading