Add support for actions in fragments #16185
Conversation
microsoft-github-policy-service
bot
added
Issue-Feature
This comment has been minimized.
This comment has been minimized.
src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp
Outdated
Show resolved
Hide resolved
| // 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); |
There was a problem hiding this comment.
I don't quite get this change and how it replaces the previous code.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
ah, this is likely to have downstream effects on me as well. thanks!
src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp
Outdated
Show resolved
Hide resolved
meh? They won't be able to add a key for it, so I don't think it'll actually be able to do anything. I suppose an app can just add one that we'll store but ultimately won't be useful. |
| VERIFY_IS_TRUE(nested.HasNestedCommands()); | ||
| VERIFY_ARE_EQUAL(settings->GlobalSettings().ColorSchemes().Size(), nested.NestedCommands().Size()); | ||
| } | ||
| void DeserializationTests::FragmentActionRoundtrip() |
There was a problem hiding this comment.
i like the round-trip test verifying we didn't serialize someone else's action. clever.
microsoft-github-policy-service
bot
added
the
Needs-Author-Feedback
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:
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.keysfrom 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.