Skip to content

Commit

Permalink
Add OriginTag to Command (#16823)
Browse files Browse the repository at this point in the history 
## Summary of the Pull Request
As outlined in #16816 , adding `OriginTag` to `Command` is one of the
prerequisites to implementing Action IDs. This PR does that.

## Validation Steps Performed
Actions/Commands still get parsed and work

## PR Checklist
- [ ] Closes #xxx
- [ ] Tests added/passed
- [ ] Documentation updated
- If checked, please file a pull request on [our docs
repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx
- [ ] Schema updated (if necessary)
  • Loading branch information
@PankajBhojwani
PankajBhojwani authored Mar 19, 2024
1 parent 5383cb3 commit 5f27280
Show file tree
Hide file tree
Showing 10 changed files with 84 additions and 77 deletions.
4 changes: 2 additions & 2 deletions src/cascadia/TerminalSettingsModel/ActionMap.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
void AddAction(const Model::Command& cmd);

// JSON
static com_ptr<ActionMap> FromJson(const Json::Value& json);
std::vector<SettingsLoadWarnings> LayerJson(const Json::Value& json, const bool withKeybindings = true);
static com_ptr<ActionMap> FromJson(const Json::Value& json, const OriginTag origin = OriginTag::None);
std::vector<SettingsLoadWarnings> LayerJson(const Json::Value& json, const OriginTag origin, const bool withKeybindings = true);
Json::Value ToJson() const;

// modification
Expand Down
8 changes: 4 additions & 4 deletions src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,10 @@ using namespace winrt::Microsoft::Terminal::Settings::Model;

namespace winrt::Microsoft::Terminal::Settings::Model::implementation
{
com_ptr<ActionMap> ActionMap::FromJson(const Json::Value& json)
com_ptr<ActionMap> ActionMap::FromJson(const Json::Value& json, const OriginTag origin)
{
auto result = make_self<ActionMap>();
result->LayerJson(json);
result->LayerJson(json, origin);
return result;
}

Expand All @@ -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, const bool withKeybindings)
std::vector<SettingsLoadWarnings> ActionMap::LayerJson(const Json::Value& json, const OriginTag origin, 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, withKeybindings));
AddAction(*Command::FromJson(cmdJson, warnings, origin, withKeybindings));
}

return warnings;
Expand Down
6 changes: 3 additions & 3 deletions src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,7 @@ void SettingsLoader::FinalizeLayering()
if (userSettings.globals->EnableColorSelection())
{
const auto json = _parseJson(LoadStringResource(IDR_ENABLE_COLOR_SELECTION));
const auto globals = GlobalAppSettings::FromJson(json.root);
const auto globals = GlobalAppSettings::FromJson(json.root, OriginTag::InBox);
userSettings.globals->AddLeastImportantParent(globals);
}

Expand Down Expand Up @@ -611,7 +611,7 @@ void SettingsLoader::_parse(const OriginTag origin, const winrt::hstring& source
settings.clear();

{
settings.globals = GlobalAppSettings::FromJson(json.root);
settings.globals = GlobalAppSettings::FromJson(json.root, origin);

for (const auto& schemeJson : json.colorSchemes)
{
Expand Down Expand Up @@ -699,7 +699,7 @@ void SettingsLoader::_parseFragment(const winrt::hstring& source, const std::str
// 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);
settings.globals->LayerActionsFrom(json.root, OriginTag::Fragment, false);
}

{
Expand Down
12 changes: 8 additions & 4 deletions src/cascadia/TerminalSettingsModel/Command.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
{
auto command{ winrt::make_self<Command>() };
command->_name = _name;
command->_Origin = OriginTag::User;
command->_ActionAndArgs = *get_self<implementation::ActionAndArgs>(_ActionAndArgs)->Copy();
command->_keyMappings = _keyMappings;
command->_iconPath = _iconPath;
Expand Down Expand Up @@ -259,9 +260,11 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
// - the newly constructed Command object.
winrt::com_ptr<Command> Command::FromJson(const Json::Value& json,
std::vector<SettingsLoadWarnings>& warnings,
const OriginTag origin,
const bool parseKeys)
{
auto result = winrt::make_self<Command>();
result->_Origin = origin;

auto nested = false;
JsonUtils::GetValueForKey(json, IterateOnKey, result->_IterateOn);
Expand All @@ -274,7 +277,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
// Initialize our list of subcommands.
result->_subcommands = winrt::single_threaded_map<winrt::hstring, Model::Command>();
result->_nestedCommand = true;
auto nestedWarnings = Command::LayerJson(result->_subcommands, nestedCommandsJson);
auto nestedWarnings = Command::LayerJson(result->_subcommands, nestedCommandsJson, origin);
// It's possible that the nested commands have some warnings
warnings.insert(warnings.end(), nestedWarnings.begin(), nestedWarnings.end());

Expand Down Expand Up @@ -362,7 +365,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
// Return Value:
// - A vector containing any warnings detected while parsing
std::vector<SettingsLoadWarnings> Command::LayerJson(IMap<winrt::hstring, Model::Command>& commands,
const Json::Value& json)
const Json::Value& json,
const OriginTag origin)
{
std::vector<SettingsLoadWarnings> warnings;

Expand All @@ -372,7 +376,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
{
try
{
const auto result = Command::FromJson(value, warnings);
const auto result = Command::FromJson(value, warnings, origin);
if (result->ActionAndArgs().Action() == ShortcutAction::Invalid && !result->HasNestedCommands())
{
// If there wasn't a parsed command, then try to get the
Expand Down Expand Up @@ -583,7 +587,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation
// warnings, but ultimately, we don't care about warnings during
// expansion.
std::vector<SettingsLoadWarnings> unused;
if (auto newCmd{ Command::FromJson(newJsonValue, unused) })
if (auto newCmd{ Command::FromJson(newJsonValue, unused, expandable->_Origin) })
{
newCommands.push_back(*newCmd);
}
Expand Down
5 changes: 4 additions & 1 deletion src/cascadia/TerminalSettingsModel/Command.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,14 +40,16 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation

static winrt::com_ptr<Command> FromJson(const Json::Value& json,
std::vector<SettingsLoadWarnings>& warnings,
const OriginTag origin,
const bool parseKeys = true);

static void ExpandCommands(Windows::Foundation::Collections::IMap<winrt::hstring, Model::Command>& commands,
Windows::Foundation::Collections::IVectorView<Model::Profile> profiles,
Windows::Foundation::Collections::IVectorView<Model::ColorScheme> schemes);

static std::vector<SettingsLoadWarnings> LayerJson(Windows::Foundation::Collections::IMap<winrt::hstring, Model::Command>& commands,
const Json::Value& json);
const Json::Value& json,
const OriginTag origin);
Json::Value ToJson() const;

bool HasNestedCommands() const;
Expand Down Expand Up @@ -75,6 +77,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation

WINRT_PROPERTY(ExpandCommandType, IterateOn, ExpandCommandType::None);
WINRT_PROPERTY(Model::ActionAndArgs, ActionAndArgs);
WINRT_PROPERTY(OriginTag, Origin);

private:
Json::Value _originalJson;
Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalSettingsModel/Command.idl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ namespace Microsoft.Terminal.Settings.Model
ShortcutAction Action;
};

[default_interface] runtimeclass Command
[default_interface] runtimeclass Command : ISettingsModelObject
{
Command();

Expand Down
12 changes: 6 additions & 6 deletions src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -117,14 +117,14 @@ winrt::Microsoft::Terminal::Settings::Model::ActionMap GlobalAppSettings::Action
// - json: an object which should be a serialization of a GlobalAppSettings object.
// Return Value:
// - a new GlobalAppSettings instance created from the values in `json`
winrt::com_ptr<GlobalAppSettings> GlobalAppSettings::FromJson(const Json::Value& json)
winrt::com_ptr<GlobalAppSettings> GlobalAppSettings::FromJson(const Json::Value& json, const OriginTag origin)
{
auto result = winrt::make_self<GlobalAppSettings>();
result->LayerJson(json);
result->LayerJson(json, origin);
return result;
}

void GlobalAppSettings::LayerJson(const Json::Value& json)
void GlobalAppSettings::LayerJson(const Json::Value& json, const OriginTag origin)
{
JsonUtils::GetValueForKey(json, DefaultProfileKey, _UnparsedDefaultProfile);
// GH#8076 - when adding enum values to this key, we also changed it from
Expand All @@ -137,19 +137,19 @@ void GlobalAppSettings::LayerJson(const Json::Value& json)
MTSM_GLOBAL_SETTINGS(GLOBAL_SETTINGS_LAYER_JSON)
#undef GLOBAL_SETTINGS_LAYER_JSON

LayerActionsFrom(json, true);
LayerActionsFrom(json, origin, true);

JsonUtils::GetValueForKey(json, LegacyReloadEnvironmentVariablesKey, _legacyReloadEnvironmentVariables);
}

void GlobalAppSettings::LayerActionsFrom(const Json::Value& json, const bool withKeybindings)
void GlobalAppSettings::LayerActionsFrom(const Json::Value& json, const OriginTag origin, 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, withKeybindings);
auto warnings = _actionMap->LayerJson(bindings, origin, 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
Expand Down
6 changes: 3 additions & 3 deletions src/cascadia/TerminalSettingsModel/GlobalAppSettings.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,9 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation

Model::ActionMap ActionMap() const noexcept;

static com_ptr<GlobalAppSettings> FromJson(const Json::Value& json);
void LayerJson(const Json::Value& json);
void LayerActionsFrom(const Json::Value& json, const bool withKeybindings = true);
static com_ptr<GlobalAppSettings> FromJson(const Json::Value& json, const OriginTag origin = OriginTag::None);
void LayerJson(const Json::Value& json, const OriginTag origin);
void LayerActionsFrom(const Json::Value& json, const OriginTag origin, const bool withKeybindings = true);

Json::Value ToJson() const;

Expand Down
26 changes: 13 additions & 13 deletions src/cascadia/UnitTests_SettingsModel/CommandTests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,19 +48,19 @@ namespace SettingsModelUnitTests
auto commands = winrt::single_threaded_map<winrt::hstring, Command>();
VERIFY_ARE_EQUAL(0u, commands.Size());
{
auto warnings = implementation::Command::LayerJson(commands, commands0Json);
auto warnings = implementation::Command::LayerJson(commands, commands0Json, OriginTag::None);
VERIFY_ARE_EQUAL(0u, warnings.size());
}
VERIFY_ARE_EQUAL(1u, commands.Size());

{
auto warnings = implementation::Command::LayerJson(commands, commands1Json);
auto warnings = implementation::Command::LayerJson(commands, commands1Json, OriginTag::None);
VERIFY_ARE_EQUAL(0u, warnings.size());
}
VERIFY_ARE_EQUAL(2u, commands.Size());

{
auto warnings = implementation::Command::LayerJson(commands, commands2Json);
auto warnings = implementation::Command::LayerJson(commands, commands2Json, OriginTag::None);
VERIFY_ARE_EQUAL(0u, warnings.size());
}
VERIFY_ARE_EQUAL(4u, commands.Size());
Expand All @@ -82,7 +82,7 @@ namespace SettingsModelUnitTests
auto commands = winrt::single_threaded_map<winrt::hstring, Command>();
VERIFY_ARE_EQUAL(0u, commands.Size());
{
auto warnings = implementation::Command::LayerJson(commands, commands0Json);
auto warnings = implementation::Command::LayerJson(commands, commands0Json, OriginTag::None);
VERIFY_ARE_EQUAL(0u, warnings.size());
VERIFY_ARE_EQUAL(1u, commands.Size());
auto command = commands.Lookup(L"action0");
Expand All @@ -93,7 +93,7 @@ namespace SettingsModelUnitTests
VERIFY_IS_NOT_NULL(realArgs);
}
{
auto warnings = implementation::Command::LayerJson(commands, commands1Json);
auto warnings = implementation::Command::LayerJson(commands, commands1Json, OriginTag::None);
VERIFY_ARE_EQUAL(0u, warnings.size());
VERIFY_ARE_EQUAL(1u, commands.Size());
auto command = commands.Lookup(L"action0");
Expand All @@ -103,7 +103,7 @@ namespace SettingsModelUnitTests
VERIFY_IS_NULL(command.ActionAndArgs().Args());
}
{
auto warnings = implementation::Command::LayerJson(commands, commands2Json);
auto warnings = implementation::Command::LayerJson(commands, commands2Json, OriginTag::None);
VERIFY_ARE_EQUAL(0u, warnings.size());
VERIFY_ARE_EQUAL(1u, commands.Size());
auto command = commands.Lookup(L"action0");
Expand All @@ -115,7 +115,7 @@ namespace SettingsModelUnitTests
}
{
// This last command should "unbind" the action.
auto warnings = implementation::Command::LayerJson(commands, commands3Json);
auto warnings = implementation::Command::LayerJson(commands, commands3Json, OriginTag::None);
VERIFY_ARE_EQUAL(0u, warnings.size());
VERIFY_ARE_EQUAL(0u, commands.Size());
}
Expand Down Expand Up @@ -143,7 +143,7 @@ namespace SettingsModelUnitTests

auto commands = winrt::single_threaded_map<winrt::hstring, Command>();
VERIFY_ARE_EQUAL(0u, commands.Size());
auto warnings = implementation::Command::LayerJson(commands, commands0Json);
auto warnings = implementation::Command::LayerJson(commands, commands0Json, OriginTag::None);
VERIFY_ARE_EQUAL(0u, warnings.size());
VERIFY_ARE_EQUAL(9u, commands.Size());

Expand Down Expand Up @@ -261,7 +261,7 @@ namespace SettingsModelUnitTests

auto commands = winrt::single_threaded_map<winrt::hstring, Command>();
VERIFY_ARE_EQUAL(0u, commands.Size());
auto warnings = implementation::Command::LayerJson(commands, commands0Json);
auto warnings = implementation::Command::LayerJson(commands, commands0Json, OriginTag::None);
VERIFY_ARE_EQUAL(3u, warnings.size());
VERIFY_ARE_EQUAL(1u, commands.Size());

Expand All @@ -288,7 +288,7 @@ namespace SettingsModelUnitTests
auto commands = winrt::single_threaded_map<winrt::hstring, Command>();
VERIFY_ARE_EQUAL(0u, commands.Size());
{
auto warnings = implementation::Command::LayerJson(commands, commands0Json);
auto warnings = implementation::Command::LayerJson(commands, commands0Json, OriginTag::None);
VERIFY_ARE_EQUAL(0u, warnings.size());
VERIFY_ARE_EQUAL(1u, commands.Size());

Expand Down Expand Up @@ -329,7 +329,7 @@ namespace SettingsModelUnitTests

auto commands = winrt::single_threaded_map<winrt::hstring, Command>();
VERIFY_ARE_EQUAL(0u, commands.Size());
auto warnings = implementation::Command::LayerJson(commands, commands0Json);
auto warnings = implementation::Command::LayerJson(commands, commands0Json, OriginTag::None);
VERIFY_ARE_EQUAL(0u, warnings.size());

// There are only 5 commands here: all of the `"none"`, `"auto"`,
Expand Down Expand Up @@ -399,7 +399,7 @@ namespace SettingsModelUnitTests

auto commands = winrt::single_threaded_map<winrt::hstring, Command>();
VERIFY_ARE_EQUAL(0u, commands.Size());
auto warnings = implementation::Command::LayerJson(commands, commands0Json);
auto warnings = implementation::Command::LayerJson(commands, commands0Json, OriginTag::None);
VERIFY_ARE_EQUAL(0u, warnings.size());
VERIFY_ARE_EQUAL(1u, commands.Size());

Expand Down Expand Up @@ -462,7 +462,7 @@ namespace SettingsModelUnitTests

auto commands = winrt::single_threaded_map<winrt::hstring, Command>();
VERIFY_ARE_EQUAL(0u, commands.Size());
auto warnings = implementation::Command::LayerJson(commands, commands0Json);
auto warnings = implementation::Command::LayerJson(commands, commands0Json, OriginTag::None);
VERIFY_ARE_EQUAL(0u, warnings.size());
VERIFY_ARE_EQUAL(9u, commands.Size());

Expand Down
Loading

0 comments on commit 5f27280

Please sign in to comment.