From 90627b3ae5c4f0e3be694351adeb795c5e9eef8e Mon Sep 17 00:00:00 2001 From: Pankaj Bhojwani Date: Tue, 5 Mar 2024 11:35:26 -0800 Subject: [PATCH 01/28] add origin tag --- src/cascadia/TerminalSettingsModel/ActionMap.h | 3 +-- .../TerminalSettingsModel/ActionMapSerialization.cpp | 11 ++--------- .../CascadiaSettingsSerialization.cpp | 7 ++++--- src/cascadia/TerminalSettingsModel/Command.cpp | 11 +++++++---- src/cascadia/TerminalSettingsModel/Command.h | 5 ++++- src/cascadia/TerminalSettingsModel/Command.idl | 1 + .../TerminalSettingsModel/GlobalAppSettings.cpp | 12 ++++++------ .../TerminalSettingsModel/GlobalAppSettings.h | 6 +++--- 8 files changed, 28 insertions(+), 28 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/ActionMap.h b/src/cascadia/TerminalSettingsModel/ActionMap.h index 937d79f66cb..6fea0d4d7a6 100644 --- a/src/cascadia/TerminalSettingsModel/ActionMap.h +++ b/src/cascadia/TerminalSettingsModel/ActionMap.h @@ -66,8 +66,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation void AddAction(const Model::Command& cmd); // JSON - static com_ptr FromJson(const Json::Value& json); - std::vector LayerJson(const Json::Value& json, const bool withKeybindings = true); + std::vector LayerJson(const Json::Value& json, const OriginTag origin, const bool withKeybindings = true); Json::Value ToJson() const; // modification diff --git a/src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp b/src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp index 248a0023d00..094847dc740 100644 --- a/src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp +++ b/src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp @@ -19,13 +19,6 @@ using namespace winrt::Microsoft::Terminal::Settings::Model; namespace winrt::Microsoft::Terminal::Settings::Model::implementation { - com_ptr ActionMap::FromJson(const Json::Value& json) - { - auto result = make_self(); - result->LayerJson(json); - return result; - } - // Method Description: // - Deserialize an ActionMap from the array `json`. The json array should contain // an array of serialized `Command` objects. @@ -35,7 +28,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, const bool withKeybindings) + std::vector 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 @@ -50,7 +43,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation continue; } - AddAction(*Command::FromJson(cmdJson, warnings, withKeybindings)); + AddAction(*Command::FromJson(cmdJson, warnings, origin, withKeybindings)); } return warnings; diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp index e3796a9b7d6..1b07112a230 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp @@ -350,7 +350,8 @@ void SettingsLoader::FinalizeLayering() if (userSettings.globals->EnableColorSelection()) { const auto json = _parseJson(EnableColorSelectionSettingsJson); - const auto globals = GlobalAppSettings::FromJson(json.root); + // todo: figure out the correct origin tag here + const auto globals = GlobalAppSettings::FromJson(json.root, OriginTag::None); userSettings.globals->AddLeastImportantParent(globals); } @@ -616,7 +617,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) { @@ -704,7 +705,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); } { diff --git a/src/cascadia/TerminalSettingsModel/Command.cpp b/src/cascadia/TerminalSettingsModel/Command.cpp index 3b2f4075c5d..bc2e1b7b083 100644 --- a/src/cascadia/TerminalSettingsModel/Command.cpp +++ b/src/cascadia/TerminalSettingsModel/Command.cpp @@ -259,9 +259,11 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // - the newly constructed Command object. winrt::com_ptr Command::FromJson(const Json::Value& json, std::vector& warnings, + const OriginTag origin, const bool parseKeys) { auto result = winrt::make_self(); + result->_Origin = origin; auto nested = false; JsonUtils::GetValueForKey(json, IterateOnKey, result->_IterateOn); @@ -274,7 +276,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // Initialize our list of subcommands. result->_subcommands = winrt::single_threaded_map(); 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()); @@ -362,7 +364,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // Return Value: // - A vector containing any warnings detected while parsing std::vector Command::LayerJson(IMap& commands, - const Json::Value& json) + const Json::Value& json, + const OriginTag origin) { std::vector warnings; @@ -372,7 +375,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 @@ -583,7 +586,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // warnings, but ultimately, we don't care about warnings during // expansion. std::vector unused; - if (auto newCmd{ Command::FromJson(newJsonValue, unused) }) + if (auto newCmd{ Command::FromJson(newJsonValue, unused, expandable->_Origin) }) { newCommands.push_back(*newCmd); } diff --git a/src/cascadia/TerminalSettingsModel/Command.h b/src/cascadia/TerminalSettingsModel/Command.h index a432fe9a370..5e94ae721c8 100644 --- a/src/cascadia/TerminalSettingsModel/Command.h +++ b/src/cascadia/TerminalSettingsModel/Command.h @@ -40,6 +40,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation static winrt::com_ptr FromJson(const Json::Value& json, std::vector& warnings, + const OriginTag origin, const bool parseKeys = true); static void ExpandCommands(Windows::Foundation::Collections::IMap& commands, @@ -47,7 +48,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation Windows::Foundation::Collections::IVectorView schemes); static std::vector LayerJson(Windows::Foundation::Collections::IMap& commands, - const Json::Value& json); + const Json::Value& json, + const OriginTag origin); Json::Value ToJson() const; bool HasNestedCommands() const; @@ -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; diff --git a/src/cascadia/TerminalSettingsModel/Command.idl b/src/cascadia/TerminalSettingsModel/Command.idl index e92f459e69d..2ed4a7a37d5 100644 --- a/src/cascadia/TerminalSettingsModel/Command.idl +++ b/src/cascadia/TerminalSettingsModel/Command.idl @@ -36,6 +36,7 @@ namespace Microsoft.Terminal.Settings.Model Command(); String Name { get; }; + OriginTag Origin; ActionAndArgs ActionAndArgs { get; }; Microsoft.Terminal.Control.KeyChord Keys { get; }; void RegisterKey(Microsoft.Terminal.Control.KeyChord keys); diff --git a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp index a720c261b39..7238d38c0cd 100644 --- a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp +++ b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.cpp @@ -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::FromJson(const Json::Value& json) +winrt::com_ptr GlobalAppSettings::FromJson(const Json::Value& json, const OriginTag origin) { auto result = winrt::make_self(); - 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 @@ -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 diff --git a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.h b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.h index 79f40342254..d8100161f5c 100644 --- a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.h +++ b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.h @@ -48,9 +48,9 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation Model::ActionMap ActionMap() const noexcept; - static com_ptr FromJson(const Json::Value& json); - void LayerJson(const Json::Value& json); - void LayerActionsFrom(const Json::Value& json, const bool withKeybindings = true); + static com_ptr FromJson(const Json::Value& json, const OriginTag origin); + 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; From 9dff28f23d069230b5383fc8473d101c64dd17a9 Mon Sep 17 00:00:00 2001 From: Pankaj Bhojwani Date: Tue, 5 Mar 2024 13:51:18 -0800 Subject: [PATCH 02/28] update calls in tests --- .../TerminalSettingsModel/Command.idl | 3 +- .../UnitTests_SettingsModel/CommandTests.cpp | 26 +++--- .../KeyBindingsTests.cpp | 80 +++++++++---------- 3 files changed, 54 insertions(+), 55 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/Command.idl b/src/cascadia/TerminalSettingsModel/Command.idl index 2ed4a7a37d5..c9cec5012a4 100644 --- a/src/cascadia/TerminalSettingsModel/Command.idl +++ b/src/cascadia/TerminalSettingsModel/Command.idl @@ -31,12 +31,11 @@ namespace Microsoft.Terminal.Settings.Model ShortcutAction Action; }; - [default_interface] runtimeclass Command + [default_interface] runtimeclass Command : ISettingsModelObject { Command(); String Name { get; }; - OriginTag Origin; ActionAndArgs ActionAndArgs { get; }; Microsoft.Terminal.Control.KeyChord Keys { get; }; void RegisterKey(Microsoft.Terminal.Control.KeyChord keys); diff --git a/src/cascadia/UnitTests_SettingsModel/CommandTests.cpp b/src/cascadia/UnitTests_SettingsModel/CommandTests.cpp index f68b0224a47..8db1141736f 100644 --- a/src/cascadia/UnitTests_SettingsModel/CommandTests.cpp +++ b/src/cascadia/UnitTests_SettingsModel/CommandTests.cpp @@ -48,19 +48,19 @@ namespace SettingsModelUnitTests auto commands = winrt::single_threaded_map(); 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()); @@ -82,7 +82,7 @@ namespace SettingsModelUnitTests auto commands = winrt::single_threaded_map(); 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"); @@ -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"); @@ -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"); @@ -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()); } @@ -143,7 +143,7 @@ namespace SettingsModelUnitTests auto commands = winrt::single_threaded_map(); 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()); @@ -261,7 +261,7 @@ namespace SettingsModelUnitTests auto commands = winrt::single_threaded_map(); 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()); @@ -288,7 +288,7 @@ namespace SettingsModelUnitTests auto commands = winrt::single_threaded_map(); 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()); @@ -329,7 +329,7 @@ namespace SettingsModelUnitTests auto commands = winrt::single_threaded_map(); 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"`, @@ -399,7 +399,7 @@ namespace SettingsModelUnitTests auto commands = winrt::single_threaded_map(); 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()); @@ -462,7 +462,7 @@ namespace SettingsModelUnitTests auto commands = winrt::single_threaded_map(); 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()); diff --git a/src/cascadia/UnitTests_SettingsModel/KeyBindingsTests.cpp b/src/cascadia/UnitTests_SettingsModel/KeyBindingsTests.cpp index 8eed7276f1b..c5f4be49045 100644 --- a/src/cascadia/UnitTests_SettingsModel/KeyBindingsTests.cpp +++ b/src/cascadia/UnitTests_SettingsModel/KeyBindingsTests.cpp @@ -120,13 +120,13 @@ namespace SettingsModelUnitTests auto actionMap = winrt::make_self(); VERIFY_ARE_EQUAL(0u, actionMap->_KeyMap.size()); - actionMap->LayerJson(bindings0Json); + actionMap->LayerJson(bindings0Json, OriginTag::None); VERIFY_ARE_EQUAL(1u, actionMap->_KeyMap.size()); - actionMap->LayerJson(bindings1Json); + actionMap->LayerJson(bindings1Json, OriginTag::None); VERIFY_ARE_EQUAL(2u, actionMap->_KeyMap.size()); - actionMap->LayerJson(bindings2Json); + actionMap->LayerJson(bindings2Json, OriginTag::None); VERIFY_ARE_EQUAL(4u, actionMap->_KeyMap.size()); } @@ -143,21 +143,21 @@ namespace SettingsModelUnitTests auto actionMap = winrt::make_self(); VERIFY_ARE_EQUAL(0u, actionMap->_KeyMap.size()); - actionMap->LayerJson(bindings0Json); + actionMap->LayerJson(bindings0Json, OriginTag::None); VERIFY_ARE_EQUAL(1u, actionMap->_KeyMap.size()); - actionMap->LayerJson(bindings1Json); + actionMap->LayerJson(bindings1Json, OriginTag::None); VERIFY_ARE_EQUAL(1u, actionMap->_KeyMap.size()); - actionMap->LayerJson(bindings2Json); + actionMap->LayerJson(bindings2Json, OriginTag::None); VERIFY_ARE_EQUAL(2u, actionMap->_KeyMap.size()); } void KeyBindingsTests::HashDeduplication() { const auto actionMap = winrt::make_self(); - actionMap->LayerJson(VerifyParseSucceeded(R"([ { "command": "splitPane", "keys": ["ctrl+c"] } ])")); - actionMap->LayerJson(VerifyParseSucceeded(R"([ { "command": "splitPane", "keys": ["ctrl+c"] } ])")); + actionMap->LayerJson(VerifyParseSucceeded(R"([ { "command": "splitPane", "keys": ["ctrl+c"] } ])"), OriginTag::None); + actionMap->LayerJson(VerifyParseSucceeded(R"([ { "command": "splitPane", "keys": ["ctrl+c"] } ])"), OriginTag::None); VERIFY_ARE_EQUAL(1u, actionMap->_ActionMap.size()); } @@ -180,51 +180,51 @@ namespace SettingsModelUnitTests auto actionMap = winrt::make_self(); VERIFY_ARE_EQUAL(0u, actionMap->_KeyMap.size()); - actionMap->LayerJson(bindings0Json); + actionMap->LayerJson(bindings0Json, OriginTag::None); VERIFY_ARE_EQUAL(1u, actionMap->_KeyMap.size()); - actionMap->LayerJson(bindings1Json); + actionMap->LayerJson(bindings1Json, OriginTag::None); VERIFY_ARE_EQUAL(1u, actionMap->_KeyMap.size()); Log::Comment(NoThrowString().Format( L"Try unbinding a key using `\"unbound\"` to unbind the key")); - actionMap->LayerJson(bindings2Json); + actionMap->LayerJson(bindings2Json, OriginTag::None); VERIFY_ARE_EQUAL(1u, actionMap->_KeyMap.size()); VERIFY_IS_NULL(actionMap->GetActionByKeyChord({ VirtualKeyModifiers::Control, static_cast('C'), 0 })); Log::Comment(NoThrowString().Format( L"Try unbinding a key using `null` to unbind the key")); // First add back a good binding - actionMap->LayerJson(bindings0Json); + actionMap->LayerJson(bindings0Json, OriginTag::None); VERIFY_ARE_EQUAL(1u, actionMap->_KeyMap.size()); // Then try layering in the bad setting - actionMap->LayerJson(bindings3Json); + actionMap->LayerJson(bindings3Json, OriginTag::None); VERIFY_ARE_EQUAL(1u, actionMap->_KeyMap.size()); VERIFY_IS_NULL(actionMap->GetActionByKeyChord({ VirtualKeyModifiers::Control, static_cast('C'), 0 })); Log::Comment(NoThrowString().Format( L"Try unbinding a key using an unrecognized command to unbind the key")); // First add back a good binding - actionMap->LayerJson(bindings0Json); + actionMap->LayerJson(bindings0Json, OriginTag::None); VERIFY_ARE_EQUAL(1u, actionMap->_KeyMap.size()); // Then try layering in the bad setting - actionMap->LayerJson(bindings4Json); + actionMap->LayerJson(bindings4Json, OriginTag::None); VERIFY_ARE_EQUAL(1u, actionMap->_KeyMap.size()); VERIFY_IS_NULL(actionMap->GetActionByKeyChord({ VirtualKeyModifiers::Control, static_cast('C'), 0 })); Log::Comment(NoThrowString().Format( L"Try unbinding a key using a straight up invalid value to unbind the key")); // First add back a good binding - actionMap->LayerJson(bindings0Json); + actionMap->LayerJson(bindings0Json, OriginTag::None); VERIFY_ARE_EQUAL(1u, actionMap->_KeyMap.size()); // Then try layering in the bad setting - actionMap->LayerJson(bindings5Json); + actionMap->LayerJson(bindings5Json, OriginTag::None); VERIFY_ARE_EQUAL(1u, actionMap->_KeyMap.size()); VERIFY_IS_NULL(actionMap->GetActionByKeyChord({ VirtualKeyModifiers::Control, static_cast('C'), 0 })); Log::Comment(NoThrowString().Format( L"Try unbinding a key that wasn't bound at all")); - actionMap->LayerJson(bindings2Json); + actionMap->LayerJson(bindings2Json, OriginTag::None); VERIFY_ARE_EQUAL(1u, actionMap->_KeyMap.size()); VERIFY_IS_NULL(actionMap->GetActionByKeyChord({ VirtualKeyModifiers::Control, static_cast('C'), 0 })); } @@ -244,13 +244,13 @@ namespace SettingsModelUnitTests auto actionMap = winrt::make_self(); VERIFY_IS_FALSE(actionMap->IsKeyChordExplicitlyUnbound(keyChord)); - actionMap->LayerJson(bindings0Json); + actionMap->LayerJson(bindings0Json, OriginTag::None); VERIFY_IS_FALSE(actionMap->IsKeyChordExplicitlyUnbound(keyChord)); - actionMap->LayerJson(bindings1Json); + actionMap->LayerJson(bindings1Json, OriginTag::None); VERIFY_IS_TRUE(actionMap->IsKeyChordExplicitlyUnbound(keyChord)); - actionMap->LayerJson(bindings2Json); + actionMap->LayerJson(bindings2Json, OriginTag::None); VERIFY_IS_FALSE(actionMap->IsKeyChordExplicitlyUnbound(keyChord)); } @@ -277,7 +277,7 @@ namespace SettingsModelUnitTests auto actionMap = winrt::make_self(); VERIFY_ARE_EQUAL(0u, actionMap->_KeyMap.size()); - actionMap->LayerJson(bindings0Json); + actionMap->LayerJson(bindings0Json, OriginTag::None); VERIFY_ARE_EQUAL(10u, actionMap->_KeyMap.size()); { @@ -405,7 +405,7 @@ namespace SettingsModelUnitTests auto actionMap = winrt::make_self(); VERIFY_ARE_EQUAL(0u, actionMap->_KeyMap.size()); - actionMap->LayerJson(bindings0Json); + actionMap->LayerJson(bindings0Json, OriginTag::None); VERIFY_ARE_EQUAL(4u, actionMap->_KeyMap.size()); { @@ -454,7 +454,7 @@ namespace SettingsModelUnitTests auto actionMap = winrt::make_self(); VERIFY_ARE_EQUAL(0u, actionMap->_KeyMap.size()); - actionMap->LayerJson(bindings0Json); + actionMap->LayerJson(bindings0Json, OriginTag::None); VERIFY_ARE_EQUAL(3u, actionMap->_KeyMap.size()); { @@ -495,7 +495,7 @@ namespace SettingsModelUnitTests auto actionMap = winrt::make_self(); VERIFY_ARE_EQUAL(0u, actionMap->_KeyMap.size()); - actionMap->LayerJson(bindings0Json); + actionMap->LayerJson(bindings0Json, OriginTag::None); VERIFY_ARE_EQUAL(1u, actionMap->_KeyMap.size()); { @@ -522,7 +522,7 @@ namespace SettingsModelUnitTests auto actionMap = winrt::make_self(); VERIFY_ARE_EQUAL(0u, actionMap->_KeyMap.size()); - actionMap->LayerJson(bindings0Json); + actionMap->LayerJson(bindings0Json, OriginTag::None); VERIFY_ARE_EQUAL(6u, actionMap->_KeyMap.size()); { @@ -580,7 +580,7 @@ namespace SettingsModelUnitTests const auto bindingsInvalidJson = VerifyParseSucceeded(bindingsInvalidString); auto invalidActionMap = winrt::make_self(); VERIFY_ARE_EQUAL(0u, invalidActionMap->_KeyMap.size()); - VERIFY_THROWS(invalidActionMap->LayerJson(bindingsInvalidJson);, std::exception); + VERIFY_THROWS(invalidActionMap->LayerJson(bindingsInvalidJson, OriginTag::None);, std::exception); } } @@ -595,7 +595,7 @@ namespace SettingsModelUnitTests auto actionMap = winrt::make_self(); VERIFY_ARE_EQUAL(0u, actionMap->_KeyMap.size()); - actionMap->LayerJson(bindings0Json); + actionMap->LayerJson(bindings0Json, OriginTag::None); VERIFY_ARE_EQUAL(2u, actionMap->_KeyMap.size()); { @@ -617,7 +617,7 @@ namespace SettingsModelUnitTests { const std::string bindingsInvalidString{ R"([{ "keys": ["up"], "command": "moveTab" }])" }; auto actionMapNoArgs = winrt::make_self(); - actionMapNoArgs->LayerJson(bindingsInvalidString); + actionMapNoArgs->LayerJson(bindingsInvalidString, OriginTag::None); VERIFY_ARE_EQUAL(0u, actionMapNoArgs->_KeyMap.size()); } { @@ -625,7 +625,7 @@ namespace SettingsModelUnitTests const auto bindingsInvalidJson = VerifyParseSucceeded(bindingsInvalidString); auto invalidActionMap = winrt::make_self(); VERIFY_ARE_EQUAL(0u, invalidActionMap->_KeyMap.size()); - VERIFY_THROWS(invalidActionMap->LayerJson(bindingsInvalidJson);, std::exception); + VERIFY_THROWS(invalidActionMap->LayerJson(bindingsInvalidJson, OriginTag::None);, std::exception); } } @@ -641,7 +641,7 @@ namespace SettingsModelUnitTests auto actionMap = winrt::make_self(); VERIFY_ARE_EQUAL(0u, actionMap->_KeyMap.size()); - actionMap->LayerJson(bindings0Json); + actionMap->LayerJson(bindings0Json, OriginTag::None); VERIFY_ARE_EQUAL(3u, actionMap->_KeyMap.size()); { @@ -673,7 +673,7 @@ namespace SettingsModelUnitTests const auto bindingsInvalidJson = VerifyParseSucceeded(bindingsInvalidString); auto invalidActionMap = winrt::make_self(); VERIFY_ARE_EQUAL(0u, invalidActionMap->_KeyMap.size()); - VERIFY_THROWS(invalidActionMap->LayerJson(bindingsInvalidJson);, std::exception); + VERIFY_THROWS(invalidActionMap->LayerJson(bindingsInvalidJson, OriginTag::None);, std::exception); } } @@ -707,14 +707,14 @@ namespace SettingsModelUnitTests { Log::Comment(L"simple command: no args"); - actionMap->LayerJson(bindings0Json); + actionMap->LayerJson(bindings0Json, OriginTag::None); VERIFY_ARE_EQUAL(1u, actionMap->_KeyMap.size()); const auto& kbd{ actionMap->GetKeyBindingForAction(ShortcutAction::CloseWindow) }; VerifyKeyChordEquality({ VirtualKeyModifiers::Control, static_cast('A'), 0 }, kbd); } { Log::Comment(L"command with args"); - actionMap->LayerJson(bindings1Json); + actionMap->LayerJson(bindings1Json, OriginTag::None); VERIFY_ARE_EQUAL(2u, actionMap->_KeyMap.size()); auto args{ winrt::make_self() }; @@ -725,7 +725,7 @@ namespace SettingsModelUnitTests } { Log::Comment(L"command with new terminal args"); - actionMap->LayerJson(bindings2Json); + actionMap->LayerJson(bindings2Json, OriginTag::None); VERIFY_ARE_EQUAL(3u, actionMap->_KeyMap.size()); auto newTerminalArgs{ winrt::make_self() }; @@ -737,7 +737,7 @@ namespace SettingsModelUnitTests } { Log::Comment(L"command with hidden args"); - actionMap->LayerJson(bindings3Json); + actionMap->LayerJson(bindings3Json, OriginTag::None); VERIFY_ARE_EQUAL(4u, actionMap->_KeyMap.size()); const auto& kbd{ actionMap->GetKeyBindingForAction(ShortcutAction::ToggleCommandPalette) }; @@ -762,13 +762,13 @@ namespace SettingsModelUnitTests auto actionMap = winrt::make_self(); VERIFY_ARE_EQUAL(0u, actionMap->_KeyMap.size()); - actionMap->LayerJson(bindings0Json); + actionMap->LayerJson(bindings0Json, OriginTag::None); VERIFY_ARE_EQUAL(1u, actionMap->_KeyMap.size()); - actionMap->LayerJson(bindings1Json); + actionMap->LayerJson(bindings1Json, OriginTag::None); VERIFY_ARE_EQUAL(1u, actionMap->_KeyMap.size(), L"Layering the second action should replace the first one."); - actionMap->LayerJson(bindings2Json); + actionMap->LayerJson(bindings2Json, OriginTag::None); VERIFY_ARE_EQUAL(2u, actionMap->_KeyMap.size()); } @@ -777,7 +777,7 @@ namespace SettingsModelUnitTests const auto json = VerifyParseSucceeded(R"!([{"command": "quakeMode", "keys":"shift+sc(255)"}])!"); const auto actionMap = winrt::make_self(); - actionMap->LayerJson(json); + actionMap->LayerJson(json, OriginTag::None); const auto action = actionMap->GetActionByKeyChord({ VirtualKeyModifiers::Shift, 0, 255 }); VERIFY_IS_NOT_NULL(action); From 8bcbd0bd423e7c4c45d03a1b21ba34fa3452a3b5 Mon Sep 17 00:00:00 2001 From: Pankaj Bhojwani Date: Tue, 5 Mar 2024 14:20:14 -0800 Subject: [PATCH 03/28] fix tests --- src/cascadia/TerminalSettingsModel/GlobalAppSettings.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.h b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.h index d8100161f5c..aa7d8c0147f 100644 --- a/src/cascadia/TerminalSettingsModel/GlobalAppSettings.h +++ b/src/cascadia/TerminalSettingsModel/GlobalAppSettings.h @@ -48,7 +48,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation Model::ActionMap ActionMap() const noexcept; - static com_ptr FromJson(const Json::Value& json, const OriginTag origin); + static com_ptr 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); From 052d06368671ce1feaaf2e61dd1d7e3826dbe680 Mon Sep 17 00:00:00 2001 From: Pankaj Bhojwani Date: Tue, 5 Mar 2024 14:36:45 -0800 Subject: [PATCH 04/28] ah one of the tests uses this --- src/cascadia/TerminalSettingsModel/ActionMap.h | 1 + .../TerminalSettingsModel/ActionMapSerialization.cpp | 7 +++++++ 2 files changed, 8 insertions(+) diff --git a/src/cascadia/TerminalSettingsModel/ActionMap.h b/src/cascadia/TerminalSettingsModel/ActionMap.h index 6fea0d4d7a6..de9b9ca2361 100644 --- a/src/cascadia/TerminalSettingsModel/ActionMap.h +++ b/src/cascadia/TerminalSettingsModel/ActionMap.h @@ -66,6 +66,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation void AddAction(const Model::Command& cmd); // JSON + static com_ptr FromJson(const Json::Value& json, const OriginTag origin = OriginTag::None); std::vector LayerJson(const Json::Value& json, const OriginTag origin, const bool withKeybindings = true); Json::Value ToJson() const; diff --git a/src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp b/src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp index 094847dc740..c03fecd82f5 100644 --- a/src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp +++ b/src/cascadia/TerminalSettingsModel/ActionMapSerialization.cpp @@ -19,6 +19,13 @@ using namespace winrt::Microsoft::Terminal::Settings::Model; namespace winrt::Microsoft::Terminal::Settings::Model::implementation { + com_ptr ActionMap::FromJson(const Json::Value& json, const OriginTag origin) + { + auto result = make_self(); + result->LayerJson(json, origin); + return result; + } + // Method Description: // - Deserialize an ActionMap from the array `json`. The json array should contain // an array of serialized `Command` objects. From 8cc82de489f550748247632113576c438fdd94b8 Mon Sep 17 00:00:00 2001 From: Pankaj Bhojwani Date: Tue, 5 Mar 2024 15:42:38 -0800 Subject: [PATCH 05/28] generated --- .../TerminalSettingsModel/CascadiaSettingsSerialization.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp index 1b07112a230..494499ae336 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp @@ -350,8 +350,7 @@ void SettingsLoader::FinalizeLayering() if (userSettings.globals->EnableColorSelection()) { const auto json = _parseJson(EnableColorSelectionSettingsJson); - // todo: figure out the correct origin tag here - const auto globals = GlobalAppSettings::FromJson(json.root, OriginTag::None); + const auto globals = GlobalAppSettings::FromJson(json.root, OriginTag::Generated); userSettings.globals->AddLeastImportantParent(globals); } From 642d0ab2b7ae79cae049f6efce91785937f33a60 Mon Sep 17 00:00:00 2001 From: Pankaj Bhojwani Date: Tue, 5 Mar 2024 15:47:49 -0800 Subject: [PATCH 06/28] inbox makes more sense --- .../TerminalSettingsModel/CascadiaSettingsSerialization.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp index 494499ae336..0a9cc49cbfd 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp @@ -350,7 +350,7 @@ void SettingsLoader::FinalizeLayering() if (userSettings.globals->EnableColorSelection()) { const auto json = _parseJson(EnableColorSelectionSettingsJson); - const auto globals = GlobalAppSettings::FromJson(json.root, OriginTag::Generated); + const auto globals = GlobalAppSettings::FromJson(json.root, OriginTag::InBox); userSettings.globals->AddLeastImportantParent(globals); } From 66fe08f964e11b283344914e75a34c1d6df76d91 Mon Sep 17 00:00:00 2001 From: Pankaj Bhojwani Date: Wed, 6 Mar 2024 17:24:39 -0800 Subject: [PATCH 07/28] default ids --- src/cascadia/TerminalSettingsModel/Command.h | 1 + .../TerminalSettingsModel/Command.idl | 1 + .../TerminalSettingsModel/defaults.json | 246 +++++++++--------- 3 files changed, 125 insertions(+), 123 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/Command.h b/src/cascadia/TerminalSettingsModel/Command.h index 5e94ae721c8..c2c881b9b36 100644 --- a/src/cascadia/TerminalSettingsModel/Command.h +++ b/src/cascadia/TerminalSettingsModel/Command.h @@ -78,6 +78,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation WINRT_PROPERTY(ExpandCommandType, IterateOn, ExpandCommandType::None); WINRT_PROPERTY(Model::ActionAndArgs, ActionAndArgs); WINRT_PROPERTY(OriginTag, Origin); + WINRT_PROPERTY(hstring, ID); private: Json::Value _originalJson; diff --git a/src/cascadia/TerminalSettingsModel/Command.idl b/src/cascadia/TerminalSettingsModel/Command.idl index c9cec5012a4..0a808d3de62 100644 --- a/src/cascadia/TerminalSettingsModel/Command.idl +++ b/src/cascadia/TerminalSettingsModel/Command.idl @@ -36,6 +36,7 @@ namespace Microsoft.Terminal.Settings.Model Command(); String Name { get; }; + String ID; ActionAndArgs ActionAndArgs { get; }; Microsoft.Terminal.Control.KeyChord Keys { get; }; void RegisterKey(Microsoft.Terminal.Control.KeyChord keys); diff --git a/src/cascadia/TerminalSettingsModel/defaults.json b/src/cascadia/TerminalSettingsModel/defaults.json index 360f37afaad..7a9c4aad79b 100644 --- a/src/cascadia/TerminalSettingsModel/defaults.json +++ b/src/cascadia/TerminalSettingsModel/defaults.json @@ -354,143 +354,143 @@ "actions": [ // Application-level Keys - { "command": "closeWindow", "keys": "alt+f4" }, - { "command": "toggleFullscreen", "keys": "alt+enter" }, - { "command": "toggleFullscreen", "keys": "f11" }, - { "command": "toggleFocusMode" }, - { "command": "toggleAlwaysOnTop" }, - { "command": "openNewTabDropdown", "keys": "ctrl+shift+space" }, - { "command": { "action": "openSettings", "target": "settingsUI" }, "keys": "ctrl+," }, - { "command": { "action": "openSettings", "target": "settingsFile" }, "keys": "ctrl+shift+," }, - { "command": { "action": "openSettings", "target": "defaultsFile" }, "keys": "ctrl+alt+," }, - { "command": "find", "keys": "ctrl+shift+f" }, - { "command": { "action": "findMatch", "direction": "next" } }, - { "command": { "action": "findMatch", "direction": "prev" } }, - { "command": "toggleShaderEffects" }, - { "command": "openTabColorPicker" }, - { "command": "renameTab" }, - { "command": "openTabRenamer" }, - { "command": "commandPalette", "keys":"ctrl+shift+p" }, - { "command": "identifyWindow" }, - { "command": "openWindowRenamer" }, - { "command": "quakeMode", "keys":"win+sc(41)" }, - { "command": "openSystemMenu", "keys": "alt+space" }, - { "command": "quit" }, - { "command": "restoreLastClosed" }, - { "command": "openAbout" }, + { "command": "closeWindow", "keys": "alt+f4", "id": "Terminal.CloseWindow" }, + { "command": "toggleFullscreen", "keys": "alt+enter", "id": "Terminal.ToggleFullscreen" }, + { "command": "toggleFullscreen", "keys": "f11", "id": "Terminal.ToggleFullscreen" }, + { "command": "toggleFocusMode", "id": "Terminal.ToggleFocusMode" }, + { "command": "toggleAlwaysOnTop", "id": "Terminal.ToggleAlwaysOnTop" }, + { "command": "openNewTabDropdown", "keys": "ctrl+shift+space", "id": "Terminal.OpenNewTabDropdown" }, + { "command": { "action": "openSettings", "target": "settingsUI" }, "keys": "ctrl+,", "id": "Terminal.OpenSettingsUI" }, + { "command": { "action": "openSettings", "target": "settingsFile" }, "keys": "ctrl+shift+,", "id": "Terminal.OpenSettingsFile" }, + { "command": { "action": "openSettings", "target": "defaultsFile" }, "keys": "ctrl+alt+,", "id": "Terminal.OpenDefaultSettingsFile" }, + { "command": "find", "keys": "ctrl+shift+f", "id": "Terminal.FindText" }, + { "command": { "action": "findMatch", "direction": "next" }, "id": "Terminal.FindNextMatch" }, + { "command": { "action": "findMatch", "direction": "prev" }, "id": "Terminal.FindPrevMatch" }, + { "command": "toggleShaderEffects", "id": "Terminal.ToggleShaderEffects" }, + { "command": "openTabColorPicker", "id": "Terminal.OpenTabColorPicker" }, + { "command": "renameTab", "id": "Terminal.RenameTab" }, + { "command": "openTabRenamer", "id": "Terminal.OpenTabRenamer" }, + { "command": "commandPalette", "keys":"ctrl+shift+p", "id": "Terminal.ToggleCommandPalette" }, + { "command": "identifyWindow", "id": "Terminal.IdentifyWindow" }, + { "command": "openWindowRenamer", "id": "Terminal.OpenWindowRenamer" }, + { "command": "quakeMode", "keys":"win+sc(41)", "id": "Terminal.QuakeMode" }, + { "command": "openSystemMenu", "keys": "alt+space", "id": "Terminal.OpenSystemMenu" }, + { "command": "quit", "id": "Terminal.Quit" }, + { "command": "restoreLastClosed", "id": "Terminal.RestoreLastClosed" }, + { "command": "openAbout", "id": "Terminal.OpenAboutDialog" }, // Tab Management // "command": "closeTab" is unbound by default. // The closeTab command closes a tab without confirmation, even if it has multiple panes. - { "command": "closeOtherTabs" }, - { "command": "closeTabsAfter" }, - { "command": { "action" : "moveTab", "direction": "forward" }}, - { "command": { "action" : "moveTab", "direction": "backward" }}, - { "command": "newTab", "keys": "ctrl+shift+t" }, - { "command": "newWindow", "keys": "ctrl+shift+n" }, - { "command": { "action": "newTab", "index": 0 }, "keys": "ctrl+shift+1" }, - { "command": { "action": "newTab", "index": 1 }, "keys": "ctrl+shift+2" }, - { "command": { "action": "newTab", "index": 2 }, "keys": "ctrl+shift+3" }, - { "command": { "action": "newTab", "index": 3 }, "keys": "ctrl+shift+4" }, - { "command": { "action": "newTab", "index": 4 }, "keys": "ctrl+shift+5" }, - { "command": { "action": "newTab", "index": 5 }, "keys": "ctrl+shift+6" }, - { "command": { "action": "newTab", "index": 6 }, "keys": "ctrl+shift+7" }, - { "command": { "action": "newTab", "index": 7 }, "keys": "ctrl+shift+8" }, - { "command": { "action": "newTab", "index": 8 }, "keys": "ctrl+shift+9" }, - { "command": "duplicateTab", "keys": "ctrl+shift+d" }, - { "command": "nextTab", "keys": "ctrl+tab" }, - { "command": "prevTab", "keys": "ctrl+shift+tab" }, - { "command": { "action": "switchToTab", "index": 0 }, "keys": "ctrl+alt+1" }, - { "command": { "action": "switchToTab", "index": 1 }, "keys": "ctrl+alt+2" }, - { "command": { "action": "switchToTab", "index": 2 }, "keys": "ctrl+alt+3" }, - { "command": { "action": "switchToTab", "index": 3 }, "keys": "ctrl+alt+4" }, - { "command": { "action": "switchToTab", "index": 4 }, "keys": "ctrl+alt+5" }, - { "command": { "action": "switchToTab", "index": 5 }, "keys": "ctrl+alt+6" }, - { "command": { "action": "switchToTab", "index": 6 }, "keys": "ctrl+alt+7" }, - { "command": { "action": "switchToTab", "index": 7 }, "keys": "ctrl+alt+8" }, - { "command": { "action": "switchToTab", "index": 4294967295 }, "keys": "ctrl+alt+9" }, - { "command": { "action": "moveTab", "window": "new" }, }, + { "command": "closeOtherTabs", "id": "Terminal.CloseOtherTabs" }, + { "command": "closeTabsAfter", "id": "Terminal.CloseTabsAfter" }, + { "command": { "action" : "moveTab", "direction": "forward" }, "id": "Terminal.MoveTabForward" }, + { "command": { "action" : "moveTab", "direction": "backward" }, "id": "Terminal.MoveTabBackward" }, + { "command": "newTab", "keys": "ctrl+shift+t", "id": "Terminal.OpenNewTab" }, + { "command": "newWindow", "keys": "ctrl+shift+n", "id": "Terminal.OpenNewWindow" }, + { "command": { "action": "newTab", "index": 0 }, "keys": "ctrl+shift+1", "id": "Terminal.OpenNewTabProfile0" }, + { "command": { "action": "newTab", "index": 1 }, "keys": "ctrl+shift+2", "id": "Terminal.OpenNewTabProfile1" }, + { "command": { "action": "newTab", "index": 2 }, "keys": "ctrl+shift+3", "id": "Terminal.OpenNewTabProfile2" }, + { "command": { "action": "newTab", "index": 3 }, "keys": "ctrl+shift+4", "id": "Terminal.OpenNewTabProfile3" }, + { "command": { "action": "newTab", "index": 4 }, "keys": "ctrl+shift+5", "id": "Terminal.OpenNewTabProfile4" }, + { "command": { "action": "newTab", "index": 5 }, "keys": "ctrl+shift+6", "id": "Terminal.OpenNewTabProfile5" }, + { "command": { "action": "newTab", "index": 6 }, "keys": "ctrl+shift+7", "id": "Terminal.OpenNewTabProfile6" }, + { "command": { "action": "newTab", "index": 7 }, "keys": "ctrl+shift+8", "id": "Terminal.OpenNewTabProfile7" }, + { "command": { "action": "newTab", "index": 8 }, "keys": "ctrl+shift+9", "id": "Terminal.OpenNewTabProfile8" }, + { "command": "duplicateTab", "keys": "ctrl+shift+d", "id": "Terminal.DuplicateTab" }, + { "command": "nextTab", "keys": "ctrl+tab", "id": "Terminal.NextTab" }, + { "command": "prevTab", "keys": "ctrl+shift+tab", "id": "Terminal.PrevTab" }, + { "command": { "action": "switchToTab", "index": 0 }, "keys": "ctrl+alt+1", "id": "Terminal.SwitchToTab0" }, + { "command": { "action": "switchToTab", "index": 1 }, "keys": "ctrl+alt+2", "id": "Terminal.SwitchToTab1" }, + { "command": { "action": "switchToTab", "index": 2 }, "keys": "ctrl+alt+3", "id": "Terminal.SwitchToTab2" }, + { "command": { "action": "switchToTab", "index": 3 }, "keys": "ctrl+alt+4", "id": "Terminal.SwitchToTab3" }, + { "command": { "action": "switchToTab", "index": 4 }, "keys": "ctrl+alt+5", "id": "Terminal.SwitchToTab4" }, + { "command": { "action": "switchToTab", "index": 5 }, "keys": "ctrl+alt+6", "id": "Terminal.SwitchToTab5" }, + { "command": { "action": "switchToTab", "index": 6 }, "keys": "ctrl+alt+7", "id": "Terminal.SwitchToTab6" }, + { "command": { "action": "switchToTab", "index": 7 }, "keys": "ctrl+alt+8", "id": "Terminal.SwitchToTab7" }, + { "command": { "action": "switchToTab", "index": 4294967295 }, "keys": "ctrl+alt+9", "id": "Terminal.SwitchToLastTab" }, + { "command": { "action": "moveTab", "window": "new" }, "id": "Terminal.MoveTabToNewWindow" }, // Pane Management - { "command": "closeOtherPanes" }, - { "command": "closePane", "keys": "ctrl+shift+w" }, - { "command": { "action": "splitPane", "split": "up" } }, - { "command": { "action": "splitPane", "split": "down" }, "keys": "alt+shift+-" }, - { "command": { "action": "splitPane", "split": "left" } }, - { "command": { "action": "splitPane", "split": "right" }, "keys": "alt+shift+plus" }, - { "command": { "action": "resizePane", "direction": "down" }, "keys": "alt+shift+down" }, - { "command": { "action": "resizePane", "direction": "left" }, "keys": "alt+shift+left" }, - { "command": { "action": "resizePane", "direction": "right" }, "keys": "alt+shift+right" }, - { "command": { "action": "resizePane", "direction": "up" }, "keys": "alt+shift+up" }, - { "command": { "action": "moveFocus", "direction": "down" }, "keys": "alt+down" }, - { "command": { "action": "moveFocus", "direction": "left" }, "keys": "alt+left" }, - { "command": { "action": "moveFocus", "direction": "right" }, "keys": "alt+right" }, - { "command": { "action": "moveFocus", "direction": "up" }, "keys": "alt+up" }, - { "command": { "action": "moveFocus", "direction": "previous" }, "keys": "ctrl+alt+left"}, - { "command": { "action": "moveFocus", "direction": "previousInOrder" } }, - { "command": { "action": "moveFocus", "direction": "nextInOrder" } }, - { "command": { "action": "moveFocus", "direction": "first" } }, - { "command": { "action": "moveFocus", "direction": "parent" } }, - { "command": { "action": "moveFocus", "direction": "child" } }, - { "command": { "action": "swapPane", "direction": "down" } }, - { "command": { "action": "swapPane", "direction": "left" } }, - { "command": { "action": "swapPane", "direction": "right" } }, - { "command": { "action": "swapPane", "direction": "up" } }, - { "command": { "action": "swapPane", "direction": "previous"} }, - { "command": { "action": "swapPane", "direction": "previousInOrder"} }, - { "command": { "action": "swapPane", "direction": "nextInOrder"} }, - { "command": { "action": "swapPane", "direction": "first" } }, - { "command": "toggleBroadcastInput" }, - { "command": "togglePaneZoom" }, - { "command": "toggleSplitOrientation" }, - { "command": "toggleReadOnlyMode" }, - { "command": "enableReadOnlyMode" }, - { "command": "disableReadOnlyMode" }, - { "command": { "action": "movePane", "index": 0 } }, - { "command": { "action": "movePane", "index": 1 } }, - { "command": { "action": "movePane", "index": 2 } }, - { "command": { "action": "movePane", "index": 3 } }, - { "command": { "action": "movePane", "index": 4 } }, - { "command": { "action": "movePane", "index": 5 } }, - { "command": { "action": "movePane", "index": 6 } }, - { "command": { "action": "movePane", "index": 7 } }, - { "command": { "action": "movePane", "index": 8 } }, - { "command": { "action": "movePane", "window": "new" }, }, - { "command": "restartConnection" }, + { "command": "closeOtherPanes", "id": "Terminal.CloseOtherPanes" }, + { "command": "closePane", "keys": "ctrl+shift+w", "id": "Terminal.ClosePane" }, + { "command": { "action": "splitPane", "split": "up" }, "id": "Terminal.SplitPaneUp" }, + { "command": { "action": "splitPane", "split": "down" }, "keys": "alt+shift+-", "id": "Terminal.SplitPaneDown" }, + { "command": { "action": "splitPane", "split": "left" }, "id": "Terminal.SplitPaneLeft" }, + { "command": { "action": "splitPane", "split": "right" }, "keys": "alt+shift+plus", "id": "Terminal.SplitPaneRight" }, + { "command": { "action": "resizePane", "direction": "down" }, "keys": "alt+shift+down", "id": "Terminal.ResizePaneDown" }, + { "command": { "action": "resizePane", "direction": "left" }, "keys": "alt+shift+left", "id": "Terminal.ResizePaneLeft" }, + { "command": { "action": "resizePane", "direction": "right" }, "keys": "alt+shift+right", "id": "Terminal.ResizePaneRight" }, + { "command": { "action": "resizePane", "direction": "up" }, "keys": "alt+shift+up", "id": "Terminal.ResizePaneUp" }, + { "command": { "action": "moveFocus", "direction": "down" }, "keys": "alt+down", "id": "Terminal.MoveFocusDown" }, + { "command": { "action": "moveFocus", "direction": "left" }, "keys": "alt+left", "id": "Terminal.MoveFocusLeft" }, + { "command": { "action": "moveFocus", "direction": "right" }, "keys": "alt+right", "id": "Terminal.MoveFocusRight" }, + { "command": { "action": "moveFocus", "direction": "up" }, "keys": "alt+up", "id": "Terminal.MoveFocusUp" }, + { "command": { "action": "moveFocus", "direction": "previous" }, "keys": "ctrl+alt+left", "id": "Terminal.MoveFocusPrevious" }, + { "command": { "action": "moveFocus", "direction": "previousInOrder" }, "id": "Terminal.MoveFocusPreviousInOrder" }, + { "command": { "action": "moveFocus", "direction": "nextInOrder" }, "id": "Terminal.MoveFocusNextInOrder" }, + { "command": { "action": "moveFocus", "direction": "first" }, "id": "Terminal.MoveFocusFirst" }, + { "command": { "action": "moveFocus", "direction": "parent" }, "id": "Terminal.MoveFocusParent" }, + { "command": { "action": "moveFocus", "direction": "child" }, "id": "Terminal.MoveFocusChild" }, + { "command": { "action": "swapPane", "direction": "down" }, "id": "Terminal.SwapPaneDown" }, + { "command": { "action": "swapPane", "direction": "left" }, "id": "Terminal.SwapPaneLeft" }, + { "command": { "action": "swapPane", "direction": "right" }, "id": "Terminal.SwapPaneRight" }, + { "command": { "action": "swapPane", "direction": "up" }, "id": "Terminal.SwapPaneUp" }, + { "command": { "action": "swapPane", "direction": "previous"}, "id": "Terminal.SwapPanePrevious" }, + { "command": { "action": "swapPane", "direction": "previousInOrder"}, "id": "Terminal.SwapPanePreviousInOrder" }, + { "command": { "action": "swapPane", "direction": "nextInOrder"}, "id": "Terminal.SwapPaneNextInOrder" }, + { "command": { "action": "swapPane", "direction": "first" }, "id": "Terminal.SwapPaneFirst" }, + { "command": "toggleBroadcastInput", "id": "Terminal.ToggleBroadcastInput" }, + { "command": "togglePaneZoom", "id": "Terminal.TogglePaneZoom" }, + { "command": "toggleSplitOrientation", "id": "Terminal.ToggleSplitOrientation" }, + { "command": "toggleReadOnlyMode", "id": "Terminal.ToggleReadOnlyMode" }, + { "command": "enableReadOnlyMode", "id": "Terminal.EnableReadOnlyMode" }, + { "command": "disableReadOnlyMode", "id": "Terminal.DisableReadOnlyMode" }, + { "command": { "action": "movePane", "index": 0 }, "id": "Terminal.MovePaneToTab0" }, + { "command": { "action": "movePane", "index": 1 }, "id": "Terminal.MovePaneToTab1" }, + { "command": { "action": "movePane", "index": 2 }, "id": "Terminal.MovePaneToTab2" }, + { "command": { "action": "movePane", "index": 3 }, "id": "Terminal.MovePaneToTab3" }, + { "command": { "action": "movePane", "index": 4 }, "id": "Terminal.MovePaneToTab4" }, + { "command": { "action": "movePane", "index": 5 }, "id": "Terminal.MovePaneToTab5" }, + { "command": { "action": "movePane", "index": 6 }, "id": "Terminal.MovePaneToTab6" }, + { "command": { "action": "movePane", "index": 7 }, "id": "Terminal.MovePaneToTab7" }, + { "command": { "action": "movePane", "index": 8 }, "id": "Terminal.MovePaneToTab8" }, + { "command": { "action": "movePane", "window": "new" }, "id": "Terminal.MovePaneToNewWindow" }, + { "command": "restartConnection", "id": "Terminal.RestartConnection" }, // Clipboard Integration - { "command": { "action": "copy", "singleLine": false }, "keys": "ctrl+shift+c" }, - { "command": { "action": "copy", "singleLine": false }, "keys": "ctrl+insert" }, - { "command": { "action": "copy", "singleLine": false }, "keys": "enter" }, - { "command": "paste", "keys": "ctrl+shift+v" }, - { "command": "paste", "keys": "shift+insert" }, - { "command": "selectAll", "keys": "ctrl+shift+a" }, - { "command": "markMode", "keys": "ctrl+shift+m" }, - { "command": "toggleBlockSelection" }, - { "command": "switchSelectionEndpoint" }, - { "command": "expandSelectionToWord" }, - { "command": "showContextMenu", "keys": "menu" }, + { "command": { "action": "copy", "singleLine": false }, "keys": "ctrl+shift+c", "id": "Terminal.CopySelectedText" }, + { "command": { "action": "copy", "singleLine": false }, "keys": "ctrl+insert", "id": "Terminal.CopySelectedText" }, + { "command": { "action": "copy", "singleLine": false }, "keys": "enter", "id": "Terminal.CopySelectedText" }, + { "command": "paste", "keys": "ctrl+shift+v", "id": "Terminal.PasteFromClipboard" }, + { "command": "paste", "keys": "shift+insert", "id": "Terminal.PasteFromClipboard" }, + { "command": "selectAll", "keys": "ctrl+shift+a", "id": "Terminal.SelectAll" }, + { "command": "markMode", "keys": "ctrl+shift+m", "id": "Terminal.ToggleMarkMode" }, + { "command": "toggleBlockSelection", "id": "Terminal.ToggleBlockSelection" }, + { "command": "switchSelectionEndpoint", "id": "Terminal.SwitchSelectionEndpoint" }, + { "command": "expandSelectionToWord", "id": "Terminal.ExpandSelectionToWord" }, + { "command": "showContextMenu", "keys": "menu", "id": "Terminal.ShowContextMenu" }, // Web Search - { "command": { "action": "searchWeb" }, "name": { "key": "SearchWebCommandKey" } }, + { "command": { "action": "searchWeb" }, "name": { "key": "SearchWebCommandKey" }, "id": "Terminal.SearchWeb" }, // Scrollback - { "command": "scrollDown", "keys": "ctrl+shift+down" }, - { "command": "scrollDownPage", "keys": "ctrl+shift+pgdn" }, - { "command": "scrollUp", "keys": "ctrl+shift+up" }, - { "command": "scrollUpPage", "keys": "ctrl+shift+pgup" }, - { "command": "scrollToTop", "keys": "ctrl+shift+home" }, - { "command": "scrollToBottom", "keys": "ctrl+shift+end" }, - { "command": { "action": "clearBuffer", "clear": "all" } }, - { "command": "exportBuffer" }, + { "command": "scrollDown", "keys": "ctrl+shift+down", "id": "Terminal.ScrollDown" }, + { "command": "scrollDownPage", "keys": "ctrl+shift+pgdn", "id": "Terminal.ScrollDownPage" }, + { "command": "scrollUp", "keys": "ctrl+shift+up", "id": "Terminal.ScrollUp" }, + { "command": "scrollUpPage", "keys": "ctrl+shift+pgup", "id": "Terminal.ScrollUpPage" }, + { "command": "scrollToTop", "keys": "ctrl+shift+home", "id": "Terminal.ScrollToTop" }, + { "command": "scrollToBottom", "keys": "ctrl+shift+end", "id": "Terminal.ScrollToBottom" }, + { "command": { "action": "clearBuffer", "clear": "all" }, "id": "Terminal.ClearBuffer" }, + { "command": "exportBuffer", "id": "Terminal.ExportBuffer" }, // Visual Adjustments - { "command": { "action": "adjustFontSize", "delta": 1 }, "keys": "ctrl+plus" }, - { "command": { "action": "adjustFontSize", "delta": -1 }, "keys": "ctrl+minus" }, - { "command": { "action": "adjustFontSize", "delta": 1 }, "keys": "ctrl+numpad_plus" }, - { "command": { "action": "adjustFontSize", "delta": -1 }, "keys": "ctrl+numpad_minus" }, - { "command": "resetFontSize", "keys": "ctrl+0" }, - { "command": "resetFontSize", "keys": "ctrl+numpad_0" }, + { "command": { "action": "adjustFontSize", "delta": 1 }, "keys": "ctrl+plus", "id": "Terminal.IncreaseFontSize" }, + { "command": { "action": "adjustFontSize", "delta": -1 }, "keys": "ctrl+minus", "id": "Terminal.DecreaseFontSize" }, + { "command": { "action": "adjustFontSize", "delta": 1 }, "keys": "ctrl+numpad_plus", "id": "Terminal.IncreaseFontSize" }, + { "command": { "action": "adjustFontSize", "delta": -1 }, "keys": "ctrl+numpad_minus", "id": "Terminal.DecreaseFontSize" }, + { "command": "resetFontSize", "keys": "ctrl+0", "id": "Terminal.ResetFontSize" }, + { "command": "resetFontSize", "keys": "ctrl+numpad_0", "id": "Terminal.ResetFontSize" }, // Other commands { From db528c94fc1eee3668b5bb37966618cfc4a097b7 Mon Sep 17 00:00:00 2001 From: Pankaj Bhojwani Date: Tue, 26 Mar 2024 11:29:12 -0700 Subject: [PATCH 08/28] generate IDs for user commands --- .../TerminalSettingsModel/Command.cpp | 69 ++++++++++++++++++- src/cascadia/TerminalSettingsModel/Command.h | 6 +- .../TerminalSettingsModel/Command.idl | 2 +- .../Resources/en-US/Resources.resw | 3 + 4 files changed, 76 insertions(+), 4 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/Command.cpp b/src/cascadia/TerminalSettingsModel/Command.cpp index 516d767481b..ea34348cf85 100644 --- a/src/cascadia/TerminalSettingsModel/Command.cpp +++ b/src/cascadia/TerminalSettingsModel/Command.cpp @@ -9,6 +9,7 @@ #include "KeyChordSerialization.h" #include #include "TerminalSettingsSerializationHelpers.h" +#include "CascadiaSettings.h" using namespace winrt::Microsoft::Terminal::Settings::Model; using namespace winrt::Windows::Foundation::Collections; @@ -21,6 +22,7 @@ namespace winrt } static constexpr std::string_view NameKey{ "name" }; +static constexpr std::string_view IDKey{ "id" }; static constexpr std::string_view IconKey{ "icon" }; static constexpr std::string_view ActionKey{ "command" }; static constexpr std::string_view ArgsKey{ "args" }; @@ -40,7 +42,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation { auto command{ winrt::make_self() }; command->_name = _name; - command->_Origin = OriginTag::User; + command->_Origin = _Origin; + command->_ID = _ID; command->_ActionAndArgs = *get_self(_ActionAndArgs)->Copy(); command->_keyMappings = _keyMappings; command->_iconPath = _iconPath; @@ -57,6 +60,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation command->_subcommands.Insert(kv.Key(), *subCmd->Copy()); } } + return command; } @@ -115,6 +119,44 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation } } + hstring Command::ID() const noexcept + { + return hstring{ _ID }; + } + + // Function Description: + // - generate an ID for this command and populate the _ID field + // - this function _will_ overwrite an existing ID if there is one, it is + // on the caller to make sure that either there was no ID or the overwrite is okay + // - this function should only be called to generate IDs for user-created commands + void Command::_generateID() + { + if (_ActionAndArgs) + { + // lambda function to remove whitespace and capitalize each letter after a removed space + auto removeWhitespaceAndCapitalize = [](wchar_t& x, bool& capitalizeNext) { + if (std::iswspace(x)) + { + capitalizeNext = true; // Capitalize the next character + return true; // Remove the whitespace + } + else if (capitalizeNext) + { + x = std::towupper(x); // Capitalize the letter + capitalizeNext = false; // Reset flag + } + return false; // Keep the character + }; + + std::wstring noWhitespaceName{ get_self(_ActionAndArgs)->GenerateName() }; + bool capitalizeNext; + noWhitespaceName.erase(std::remove_if(noWhitespaceName.begin(), noWhitespaceName.end(), [&capitalizeNext, removeWhitespaceAndCapitalize](wchar_t& x) { + return removeWhitespaceAndCapitalize(x, capitalizeNext); + }), noWhitespaceName.end()); + _ID = RS_(L"OriginTagUser") + L"." + noWhitespaceName; + } + } + void Command::Name(const hstring& value) { if (!_name.has_value() || _name.value() != value) @@ -307,6 +349,22 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation if (const auto actionJson{ json[JsonKey(ActionKey)] }) { result->_ActionAndArgs = *ActionAndArgs::FromJson(actionJson, warnings); + + // we might need to generate an ID, check these: + // 1. the action is valid + // 2. there isn't already an ID + // 3. the origin is User + if (result->_ActionAndArgs.Action() != ShortcutAction::Invalid) + { + if (const auto id{ json[JsonKey("id")] }) + { + result->_ID = JsonUtils::GetValue(id); + } + else if (origin == OriginTag::User) + { + result->_generateID(); + } + } } else { @@ -424,6 +482,10 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation Json::Value cmdJson{ Json::ValueType::objectValue }; JsonUtils::SetValueForKey(cmdJson, IconKey, _iconPath); JsonUtils::SetValueForKey(cmdJson, NameKey, _name); + if (!_ID.empty()) + { + JsonUtils::SetValueForKey(cmdJson, IDKey, _ID); + } if (_ActionAndArgs) { @@ -444,6 +506,10 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // First iteration also writes icon and name JsonUtils::SetValueForKey(cmdJson, IconKey, _iconPath); JsonUtils::SetValueForKey(cmdJson, NameKey, _name); + if (!_ID.empty()) + { + JsonUtils::SetValueForKey(cmdJson, IDKey, _ID); + } } if (_ActionAndArgs) @@ -455,7 +521,6 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation cmdList.append(cmdJson); } } - return cmdList; } diff --git a/src/cascadia/TerminalSettingsModel/Command.h b/src/cascadia/TerminalSettingsModel/Command.h index c2c881b9b36..4a1969ab80e 100644 --- a/src/cascadia/TerminalSettingsModel/Command.h +++ b/src/cascadia/TerminalSettingsModel/Command.h @@ -61,6 +61,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation hstring Name() const noexcept; void Name(const hstring& name); + hstring ID() const noexcept; + Control::KeyChord Keys() const noexcept; hstring KeyChordText() const noexcept; std::vector KeyMappings() const noexcept; @@ -78,16 +80,18 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation WINRT_PROPERTY(ExpandCommandType, IterateOn, ExpandCommandType::None); WINRT_PROPERTY(Model::ActionAndArgs, ActionAndArgs); WINRT_PROPERTY(OriginTag, Origin); - WINRT_PROPERTY(hstring, ID); private: Json::Value _originalJson; Windows::Foundation::Collections::IMap _subcommands{ nullptr }; std::vector _keyMappings; std::optional _name; + std::wstring _ID; std::optional _iconPath; bool _nestedCommand{ false }; + void _generateID(); + static std::vector _expandCommand(Command* const expandable, Windows::Foundation::Collections::IVectorView profiles, Windows::Foundation::Collections::IVectorView schemes); diff --git a/src/cascadia/TerminalSettingsModel/Command.idl b/src/cascadia/TerminalSettingsModel/Command.idl index 0a808d3de62..aa23458f55d 100644 --- a/src/cascadia/TerminalSettingsModel/Command.idl +++ b/src/cascadia/TerminalSettingsModel/Command.idl @@ -36,7 +36,7 @@ namespace Microsoft.Terminal.Settings.Model Command(); String Name { get; }; - String ID; + String ID { get; }; ActionAndArgs ActionAndArgs { get; }; Microsoft.Terminal.Control.KeyChord Keys { get; }; void RegisterKey(Microsoft.Terminal.Control.KeyChord keys); diff --git a/src/cascadia/TerminalSettingsModel/Resources/en-US/Resources.resw b/src/cascadia/TerminalSettingsModel/Resources/en-US/Resources.resw index d6d6d9565f9..8528372249d 100644 --- a/src/cascadia/TerminalSettingsModel/Resources/en-US/Resources.resw +++ b/src/cascadia/TerminalSettingsModel/Resources/en-US/Resources.resw @@ -192,6 +192,9 @@ Open system menu + + User + Command Prompt This is the name of "Command Prompt", as localized in Windows. The localization here should match the one in the Windows product for "Command Prompt" From b43191d2c52773c63d9d24eeab09b6dc7bc57213 Mon Sep 17 00:00:00 2001 From: Pankaj Bhojwani Date: Tue, 26 Mar 2024 11:44:47 -0700 Subject: [PATCH 09/28] spacing --- src/cascadia/TerminalSettingsModel/Command.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/cascadia/TerminalSettingsModel/Command.cpp b/src/cascadia/TerminalSettingsModel/Command.cpp index 9758b6e8ede..efbb8fb7ae5 100644 --- a/src/cascadia/TerminalSettingsModel/Command.cpp +++ b/src/cascadia/TerminalSettingsModel/Command.cpp @@ -150,7 +150,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation bool capitalizeNext; noWhitespaceName.erase(std::remove_if(noWhitespaceName.begin(), noWhitespaceName.end(), [&capitalizeNext, removeWhitespaceAndCapitalize](wchar_t& x) { return removeWhitespaceAndCapitalize(x, capitalizeNext); - }), noWhitespaceName.end()); + }), + noWhitespaceName.end()); _ID = RS_(L"OriginTagUser") + L"." + noWhitespaceName; } } From 2093660ac126e0babb749dfa1809a65541900268 Mon Sep 17 00:00:00 2001 From: Pankaj Bhojwani Date: Tue, 26 Mar 2024 11:45:44 -0700 Subject: [PATCH 10/28] line --- src/cascadia/TerminalSettingsModel/Command.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/cascadia/TerminalSettingsModel/Command.cpp b/src/cascadia/TerminalSettingsModel/Command.cpp index efbb8fb7ae5..9e657833ac7 100644 --- a/src/cascadia/TerminalSettingsModel/Command.cpp +++ b/src/cascadia/TerminalSettingsModel/Command.cpp @@ -58,7 +58,6 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation command->_subcommands.Insert(kv.Key(), *subCmd->Copy()); } } - return command; } From 6c3253968f8ee9ac46f0256e4d2c14c06731bb51 Mon Sep 17 00:00:00 2001 From: Pankaj Bhojwani Date: Wed, 27 Mar 2024 15:37:19 -0700 Subject: [PATCH 11/28] string of numbers is unsightly but it works --- .../TerminalSettingsModel/ActionAndArgs.cpp | 23 ++++++++++++ .../TerminalSettingsModel/ActionAndArgs.h | 1 + .../TerminalSettingsModel/Command.cpp | 36 +------------------ src/cascadia/TerminalSettingsModel/Command.h | 2 -- 4 files changed, 25 insertions(+), 37 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/ActionAndArgs.cpp b/src/cascadia/TerminalSettingsModel/ActionAndArgs.cpp index e90a2b8dcfa..bf567689ac6 100644 --- a/src/cascadia/TerminalSettingsModel/ActionAndArgs.cpp +++ b/src/cascadia/TerminalSettingsModel/ActionAndArgs.cpp @@ -448,6 +448,29 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation return found != GeneratedActionNames.end() ? found->second : L""; } + // Function Description: + // - This will generate an ID for this ActionAndArgs, based on the ShortcutAction and the Args + // - It will always create the same ID if the ShortcutAction and the Args are the same + // - Note: this should only be called for User-created actions + // - Example: The "SendInput 'abc'" action will have the generated ID "User.sendInput." + // Return Value: + // - The ID, based on the ShortcutAction and the Args + winrt::hstring ActionAndArgs::GenerateID() const + { + if (_Action != ShortcutAction::Invalid) + { + auto actionKeyString = ActionToStringMap.find(_Action)->second; + auto result = RS_(L"OriginTagUser") + L"." + std::wstring{ actionKeyString.begin(), actionKeyString.end() }; + if (_Args) + { + // If there are args, append the hash of the args + result = result + L"." + std::to_wstring(_Args.Hash()); + } + return result; + } + return L""; + } + winrt::hstring ActionAndArgs::Serialize(const winrt::Windows::Foundation::Collections::IVector& args) { Json::Value json{ Json::objectValue }; diff --git a/src/cascadia/TerminalSettingsModel/ActionAndArgs.h b/src/cascadia/TerminalSettingsModel/ActionAndArgs.h index a2afefaa493..9c841ca727c 100644 --- a/src/cascadia/TerminalSettingsModel/ActionAndArgs.h +++ b/src/cascadia/TerminalSettingsModel/ActionAndArgs.h @@ -27,6 +27,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation com_ptr Copy() const; hstring GenerateName() const; + hstring GenerateID() const; WINRT_PROPERTY(ShortcutAction, Action, ShortcutAction::Invalid); WINRT_PROPERTY(IActionArgs, Args, nullptr); diff --git a/src/cascadia/TerminalSettingsModel/Command.cpp b/src/cascadia/TerminalSettingsModel/Command.cpp index 9e657833ac7..f76ab010ecd 100644 --- a/src/cascadia/TerminalSettingsModel/Command.cpp +++ b/src/cascadia/TerminalSettingsModel/Command.cpp @@ -121,40 +121,6 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation return hstring{ _ID }; } - // Function Description: - // - generate an ID for this command and populate the _ID field - // - this function _will_ overwrite an existing ID if there is one, it is - // on the caller to make sure that either there was no ID or the overwrite is okay - // - this function should only be called to generate IDs for user-created commands - void Command::_generateID() - { - if (_ActionAndArgs) - { - // lambda function to remove whitespace and capitalize each letter after a removed space - auto removeWhitespaceAndCapitalize = [](wchar_t& x, bool& capitalizeNext) { - if (std::iswspace(x)) - { - capitalizeNext = true; // Capitalize the next character - return true; // Remove the whitespace - } - else if (capitalizeNext) - { - x = std::towupper(x); // Capitalize the letter - capitalizeNext = false; // Reset flag - } - return false; // Keep the character - }; - - std::wstring noWhitespaceName{ get_self(_ActionAndArgs)->GenerateName() }; - bool capitalizeNext; - noWhitespaceName.erase(std::remove_if(noWhitespaceName.begin(), noWhitespaceName.end(), [&capitalizeNext, removeWhitespaceAndCapitalize](wchar_t& x) { - return removeWhitespaceAndCapitalize(x, capitalizeNext); - }), - noWhitespaceName.end()); - _ID = RS_(L"OriginTagUser") + L"." + noWhitespaceName; - } - } - void Command::Name(const hstring& value) { if (!_name.has_value() || _name.value() != value) @@ -360,7 +326,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation } else if (origin == OriginTag::User) { - result->_generateID(); + result->_ID = get_self(result->_ActionAndArgs)->GenerateID(); } } } diff --git a/src/cascadia/TerminalSettingsModel/Command.h b/src/cascadia/TerminalSettingsModel/Command.h index 4a1969ab80e..26668070a66 100644 --- a/src/cascadia/TerminalSettingsModel/Command.h +++ b/src/cascadia/TerminalSettingsModel/Command.h @@ -90,8 +90,6 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation std::optional _iconPath; bool _nestedCommand{ false }; - void _generateID(); - static std::vector _expandCommand(Command* const expandable, Windows::Foundation::Collections::IVectorView profiles, Windows::Foundation::Collections::IVectorView schemes); From eccd87f30354c702c14799b3fc3d828324450c99 Mon Sep 17 00:00:00 2001 From: Pankaj Bhojwani Date: Wed, 27 Mar 2024 15:38:50 -0700 Subject: [PATCH 12/28] update comment --- src/cascadia/TerminalSettingsModel/Command.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/cascadia/TerminalSettingsModel/Command.cpp b/src/cascadia/TerminalSettingsModel/Command.cpp index f76ab010ecd..66eec46cdbf 100644 --- a/src/cascadia/TerminalSettingsModel/Command.cpp +++ b/src/cascadia/TerminalSettingsModel/Command.cpp @@ -314,7 +314,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation { result->_ActionAndArgs = *ActionAndArgs::FromJson(actionJson, warnings); - // we might need to generate an ID, check these: + // we need to generate an ID if all these are true: // 1. the action is valid // 2. there isn't already an ID // 3. the origin is User @@ -485,6 +485,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation cmdList.append(cmdJson); } } + return cmdList; } From 44510dce1b6ae6ec21ce16d39ffa2c79a7b3c0a5 Mon Sep 17 00:00:00 2001 From: Pankaj Bhojwani Date: Wed, 27 Mar 2024 17:14:21 -0700 Subject: [PATCH 13/28] move id generation to fixupusersettings --- .../TerminalSettingsModel/ActionMap.cpp | 5 +++++ .../TerminalSettingsModel/ActionMap.h | 1 + .../CascadiaSettingsSerialization.cpp | 16 ++++++++++++++ .../TerminalSettingsModel/Command.cpp | 21 +++++-------------- src/cascadia/TerminalSettingsModel/Command.h | 1 + 5 files changed, 28 insertions(+), 16 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/ActionMap.cpp b/src/cascadia/TerminalSettingsModel/ActionMap.cpp index 76565dff444..d5d29fe576d 100644 --- a/src/cascadia/TerminalSettingsModel/ActionMap.cpp +++ b/src/cascadia/TerminalSettingsModel/ActionMap.cpp @@ -795,6 +795,11 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation return nullptr; } + std::unordered_map ActionMap::AllActions() + { + return _ActionMap; + } + // Method Description: // - Rebinds a key binding to a new key chord // Arguments: diff --git a/src/cascadia/TerminalSettingsModel/ActionMap.h b/src/cascadia/TerminalSettingsModel/ActionMap.h index de9b9ca2361..80fb121dc7d 100644 --- a/src/cascadia/TerminalSettingsModel/ActionMap.h +++ b/src/cascadia/TerminalSettingsModel/ActionMap.h @@ -71,6 +71,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation Json::Value ToJson() const; // modification + std::unordered_map AllActions(); bool RebindKeys(const Control::KeyChord& oldKeys, const Control::KeyChord& newKeys); void DeleteKeyBinding(const Control::KeyChord& keys); void RegisterKeyBinding(Control::KeyChord keys, Model::ActionAndArgs action); diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp index 244c200dad8..8bd4aacb26b 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp @@ -504,6 +504,22 @@ bool SettingsLoader::FixupUserSettings() fixedUp = true; } + // we need to generate an ID for a command in the user settings if it doesn't already have one + auto actionMap{ winrt::get_self(userSettings.globals->ActionMap()) }; + for (auto actionPair : actionMap->AllActions()) + { + auto cmdImpl{ winrt::get_self(actionPair.second) }; + if (cmdImpl->ID().empty()) + { + auto actionAndArgsImpl{ winrt::get_self(cmdImpl->ActionAndArgs()) }; + if (const auto generatedID = actionAndArgsImpl->GenerateID(); !generatedID.empty()) + { + cmdImpl->ID(generatedID); + fixedUp = true; + } + } + } + return fixedUp; } diff --git a/src/cascadia/TerminalSettingsModel/Command.cpp b/src/cascadia/TerminalSettingsModel/Command.cpp index 66eec46cdbf..b35d08218ef 100644 --- a/src/cascadia/TerminalSettingsModel/Command.cpp +++ b/src/cascadia/TerminalSettingsModel/Command.cpp @@ -121,6 +121,11 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation return hstring{ _ID }; } + void Command::ID(hstring id) + { + _ID = id; + } + void Command::Name(const hstring& value) { if (!_name.has_value() || _name.value() != value) @@ -313,22 +318,6 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation if (const auto actionJson{ json[JsonKey(ActionKey)] }) { result->_ActionAndArgs = *ActionAndArgs::FromJson(actionJson, warnings); - - // we need to generate an ID if all these are true: - // 1. the action is valid - // 2. there isn't already an ID - // 3. the origin is User - if (result->_ActionAndArgs.Action() != ShortcutAction::Invalid) - { - if (const auto id{ json[JsonKey("id")] }) - { - result->_ID = JsonUtils::GetValue(id); - } - else if (origin == OriginTag::User) - { - result->_ID = get_self(result->_ActionAndArgs)->GenerateID(); - } - } } else { diff --git a/src/cascadia/TerminalSettingsModel/Command.h b/src/cascadia/TerminalSettingsModel/Command.h index 26668070a66..f6c63e631f6 100644 --- a/src/cascadia/TerminalSettingsModel/Command.h +++ b/src/cascadia/TerminalSettingsModel/Command.h @@ -62,6 +62,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation void Name(const hstring& name); hstring ID() const noexcept; + void ID(hstring id); Control::KeyChord Keys() const noexcept; hstring KeyChordText() const noexcept; From 10d1fc8d60c7d97cad1e5178c068ada24bc82a3e Mon Sep 17 00:00:00 2001 From: Pankaj Bhojwani Date: Wed, 27 Mar 2024 17:30:55 -0700 Subject: [PATCH 14/28] this way is better --- .../TerminalSettingsModel/ActionMap.cpp | 19 +++++++++++++++++-- .../TerminalSettingsModel/ActionMap.h | 2 +- .../CascadiaSettingsSerialization.cpp | 14 +------------- 3 files changed, 19 insertions(+), 16 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/ActionMap.cpp b/src/cascadia/TerminalSettingsModel/ActionMap.cpp index d5d29fe576d..5e82a0d978e 100644 --- a/src/cascadia/TerminalSettingsModel/ActionMap.cpp +++ b/src/cascadia/TerminalSettingsModel/ActionMap.cpp @@ -795,9 +795,24 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation return nullptr; } - std::unordered_map ActionMap::AllActions() + // Note: this should only be called for the action map in the user's settings file + bool ActionMap::GenerateIDsForActions() { - return _ActionMap; + bool fixedUp{ false }; + for (auto actionPair : _ActionMap) + { + auto cmdImpl{ winrt::get_self(actionPair.second) }; + if (cmdImpl->ID().empty()) + { + auto actionAndArgsImpl{ winrt::get_self(cmdImpl->ActionAndArgs()) }; + if (const auto generatedID = actionAndArgsImpl->GenerateID(); !generatedID.empty()) + { + cmdImpl->ID(generatedID); + fixedUp = true; + } + } + } + return fixedUp; } // Method Description: diff --git a/src/cascadia/TerminalSettingsModel/ActionMap.h b/src/cascadia/TerminalSettingsModel/ActionMap.h index 80fb121dc7d..880b9989ff6 100644 --- a/src/cascadia/TerminalSettingsModel/ActionMap.h +++ b/src/cascadia/TerminalSettingsModel/ActionMap.h @@ -71,7 +71,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation Json::Value ToJson() const; // modification - std::unordered_map AllActions(); + bool GenerateIDsForActions(); bool RebindKeys(const Control::KeyChord& oldKeys, const Control::KeyChord& newKeys); void DeleteKeyBinding(const Control::KeyChord& keys); void RegisterKeyBinding(Control::KeyChord keys, Model::ActionAndArgs action); diff --git a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp index 8bd4aacb26b..c5a8eb2bbb5 100644 --- a/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp +++ b/src/cascadia/TerminalSettingsModel/CascadiaSettingsSerialization.cpp @@ -506,19 +506,7 @@ bool SettingsLoader::FixupUserSettings() // we need to generate an ID for a command in the user settings if it doesn't already have one auto actionMap{ winrt::get_self(userSettings.globals->ActionMap()) }; - for (auto actionPair : actionMap->AllActions()) - { - auto cmdImpl{ winrt::get_self(actionPair.second) }; - if (cmdImpl->ID().empty()) - { - auto actionAndArgsImpl{ winrt::get_self(cmdImpl->ActionAndArgs()) }; - if (const auto generatedID = actionAndArgsImpl->GenerateID(); !generatedID.empty()) - { - cmdImpl->ID(generatedID); - fixedUp = true; - } - } - } + fixedUp = actionMap->GenerateIDsForActions() || fixedUp; return fixedUp; } From 71bf90f2952062915544362a4a206e77f6401982 Mon Sep 17 00:00:00 2001 From: Pankaj Bhojwani Date: Wed, 27 Mar 2024 18:02:17 -0700 Subject: [PATCH 15/28] even better, also get the ID from json --- src/cascadia/TerminalSettingsModel/ActionMap.cpp | 7 +------ src/cascadia/TerminalSettingsModel/Command.cpp | 11 +++++++++-- src/cascadia/TerminalSettingsModel/Command.h | 2 +- 3 files changed, 11 insertions(+), 9 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/ActionMap.cpp b/src/cascadia/TerminalSettingsModel/ActionMap.cpp index 5e82a0d978e..6fac74e1204 100644 --- a/src/cascadia/TerminalSettingsModel/ActionMap.cpp +++ b/src/cascadia/TerminalSettingsModel/ActionMap.cpp @@ -804,12 +804,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation auto cmdImpl{ winrt::get_self(actionPair.second) }; if (cmdImpl->ID().empty()) { - auto actionAndArgsImpl{ winrt::get_self(cmdImpl->ActionAndArgs()) }; - if (const auto generatedID = actionAndArgsImpl->GenerateID(); !generatedID.empty()) - { - cmdImpl->ID(generatedID); - fixedUp = true; - } + fixedUp = cmdImpl->GenerateID() || fixedUp; } } return fixedUp; diff --git a/src/cascadia/TerminalSettingsModel/Command.cpp b/src/cascadia/TerminalSettingsModel/Command.cpp index b35d08218ef..63c6ab1776f 100644 --- a/src/cascadia/TerminalSettingsModel/Command.cpp +++ b/src/cascadia/TerminalSettingsModel/Command.cpp @@ -121,9 +121,15 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation return hstring{ _ID }; } - void Command::ID(hstring id) + bool Command::GenerateID() { - _ID = id; + auto actionAndArgsImpl{ winrt::get_self(_ActionAndArgs) }; + if (const auto generatedID = actionAndArgsImpl->GenerateID(); !generatedID.empty()) + { + _ID = generatedID; + return true; + } + return false; } void Command::Name(const hstring& value) @@ -276,6 +282,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation { auto result = winrt::make_self(); result->_Origin = origin; + JsonUtils::GetValueForKey(json, IDKey, result->_ID); auto nested = false; JsonUtils::GetValueForKey(json, IterateOnKey, result->_IterateOn); diff --git a/src/cascadia/TerminalSettingsModel/Command.h b/src/cascadia/TerminalSettingsModel/Command.h index f6c63e631f6..f22e35348c7 100644 --- a/src/cascadia/TerminalSettingsModel/Command.h +++ b/src/cascadia/TerminalSettingsModel/Command.h @@ -62,7 +62,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation void Name(const hstring& name); hstring ID() const noexcept; - void ID(hstring id); + bool GenerateID(); Control::KeyChord Keys() const noexcept; hstring KeyChordText() const noexcept; From d57c7a1f0352d829a55875e50a4c92976c616d0d Mon Sep 17 00:00:00 2001 From: Pankaj Bhojwani Date: Wed, 27 Mar 2024 18:04:12 -0700 Subject: [PATCH 16/28] move this --- src/cascadia/TerminalSettingsModel/ActionMap.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cascadia/TerminalSettingsModel/ActionMap.cpp b/src/cascadia/TerminalSettingsModel/ActionMap.cpp index 6fac74e1204..750a4ee5e43 100644 --- a/src/cascadia/TerminalSettingsModel/ActionMap.cpp +++ b/src/cascadia/TerminalSettingsModel/ActionMap.cpp @@ -795,9 +795,9 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation return nullptr; } - // Note: this should only be called for the action map in the user's settings file bool ActionMap::GenerateIDsForActions() { + // Note: this should ONLY be called for the action map in the user's settings file bool fixedUp{ false }; for (auto actionPair : _ActionMap) { From 5c2307c531e3ec5e6452a1dfc897ef55c2f63eda Mon Sep 17 00:00:00 2001 From: Pankaj Bhojwani Date: Thu, 28 Mar 2024 11:48:51 -0700 Subject: [PATCH 17/28] fix test --- src/cascadia/UnitTests_SettingsModel/SerializationTests.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cascadia/UnitTests_SettingsModel/SerializationTests.cpp b/src/cascadia/UnitTests_SettingsModel/SerializationTests.cpp index 455c0b78c27..80d1513de33 100644 --- a/src/cascadia/UnitTests_SettingsModel/SerializationTests.cpp +++ b/src/cascadia/UnitTests_SettingsModel/SerializationTests.cpp @@ -493,7 +493,7 @@ namespace SettingsModelUnitTests } ], "actions": [ - { "command": { "action": "sendInput", "input": "VT Griese Mode" }, "keys": "ctrl+k" } + { "command": { "action": "sendInput", "input": "VT Griese Mode" }, "id" : "User.sendInput.15093368865568800249", "keys": "ctrl+k" } ], "theme": "system", "themes": [] From 9fc69721c9c6f10cd9d8fc396341d424ff71589c Mon Sep 17 00:00:00 2001 From: Pankaj Bhojwani Date: Fri, 29 Mar 2024 13:40:53 -0700 Subject: [PATCH 18/28] add tests --- .../SerializationTests.cpp | 126 ++++++++++++++++++ 1 file changed, 126 insertions(+) diff --git a/src/cascadia/UnitTests_SettingsModel/SerializationTests.cpp b/src/cascadia/UnitTests_SettingsModel/SerializationTests.cpp index 80d1513de33..7c26228c096 100644 --- a/src/cascadia/UnitTests_SettingsModel/SerializationTests.cpp +++ b/src/cascadia/UnitTests_SettingsModel/SerializationTests.cpp @@ -36,6 +36,10 @@ namespace SettingsModelUnitTests TEST_METHOD(RoundtripUserModifiedColorSchemeCollisionUnusedByProfiles); TEST_METHOD(RoundtripUserDeletedColorSchemeCollision); + TEST_METHOD(RoundtripGenerateActionID); + TEST_METHOD(DoNotGenerateActionID); + TEST_METHOD(RoundtripActionIDsAreSame); + private: // Method Description: // - deserializes and reserializes a json string representing a settings object model of type T @@ -948,4 +952,126 @@ namespace SettingsModelUnitTests VERIFY_ARE_EQUAL(toString(newResult), toString(oldResult)); } + + void SerializationTests::RoundtripGenerateActionID() + { + static constexpr std::string_view oldSettingsJson{ R"( + { + "actions": [ + { + "name": "foo", + "command": "closePane", + "keys": "ctrl+shift+w" + } + ] + })" }; + + + // Key difference: the close pane action now has a generated ID + static constexpr std::string_view newSettingsJson{ R"( + { + "actions": [ + { + "name": "foo", + "command": "closePane", + "keys": "ctrl+shift+w", + "id" : "User.closePane" + } + ] + })" }; + + implementation::SettingsLoader oldLoader{ oldSettingsJson, implementation::LoadStringResource(IDR_DEFAULTS) }; + oldLoader.MergeInboxIntoUserSettings(); + oldLoader.FinalizeLayering(); + VERIFY_IS_TRUE(oldLoader.FixupUserSettings(), L"Validate that this will indicate we need to write them back to disk"); + const auto oldSettings = winrt::make_self(std::move(oldLoader)); + const auto oldResult{ oldSettings->ToJson() }; + + implementation::SettingsLoader newLoader{ newSettingsJson, implementation::LoadStringResource(IDR_DEFAULTS) }; + newLoader.MergeInboxIntoUserSettings(); + newLoader.FinalizeLayering(); + newLoader.FixupUserSettings(); + const auto newSettings = winrt::make_self(std::move(newLoader)); + const auto newResult{ newSettings->ToJson() }; + + VERIFY_ARE_EQUAL(toString(newResult), toString(oldResult)); + } + + void SerializationTests::DoNotGenerateActionID() + { + // for iterable commands, nested commands, and user-defined actions that already have + // an ID, we do not need to generate an ID + static constexpr std::string_view oldSettingsJson{ R"( + { + "actions": [ + { + "name": "foo", + "command": "closePane", + "keys": "ctrl+shift+w", + "id": "thisIsMyClosePane" + }, + { + "iterateOn": "profiles", + "icon": "${profile.icon}", + "name": "${profile.name}", + "command": { "action": "newTab", "profile": "${profile.name}" } + }, + { + "name": "Change font size...", + "commands": [ + { "command": { "action": "adjustFontSize", "delta": 1.0 } }, + { "command": { "action": "adjustFontSize", "delta": -1.0 } }, + { "command": "resetFontSize" }, + ] + } + ] + })" }; + + implementation::SettingsLoader oldLoader{ oldSettingsJson, implementation::LoadStringResource(IDR_DEFAULTS) }; + oldLoader.MergeInboxIntoUserSettings(); + oldLoader.FinalizeLayering(); + VERIFY_IS_FALSE(oldLoader.FixupUserSettings(), L"Validate that there is no need to write back to disk"); + } + + void SerializationTests::RoundtripActionIDsAreSame() + { + static constexpr std::string_view settingsJson1{ R"( + { + "actions": [ + { + "name": "foo", + "command": { "action": "sendInput", "input": "VT Griese Mode" }, + "keys": "ctrl+shift+w" + } + ] + })" }; + + // Both settings files define the same action, so the generated ID should be the same for both + static constexpr std::string_view settingsJson2{ R"( + { + "actions": [ + { + "name": "foo", + "command": { "action": "sendInput", "input": "VT Griese Mode" }, + "keys": "ctrl+shift+w" + } + ] + })" }; + + implementation::SettingsLoader loader1{ settingsJson1, implementation::LoadStringResource(IDR_DEFAULTS) }; + loader1.MergeInboxIntoUserSettings(); + loader1.FinalizeLayering(); + VERIFY_IS_TRUE(loader1.FixupUserSettings(), L"Validate that this will indicate we need to write them back to disk"); + const auto settings1 = winrt::make_self(std::move(loader1)); + const auto result1{ settings1->ToJson() }; + + implementation::SettingsLoader loader2{ settingsJson2, implementation::LoadStringResource(IDR_DEFAULTS) }; + loader2.MergeInboxIntoUserSettings(); + loader2.FinalizeLayering(); + VERIFY_IS_TRUE(loader2.FixupUserSettings(), L"Validate that this will indicate we need to write them back to disk"); + const auto settings2 = winrt::make_self(std::move(loader2)); + const auto result2{ settings2->ToJson() }; + + VERIFY_ARE_EQUAL(toString(result1), toString(result2)); + } } From dca7df50c88c37d08d020607d315db5ca93a7958 Mon Sep 17 00:00:00 2001 From: Pankaj Bhojwani Date: Fri, 29 Mar 2024 13:49:33 -0700 Subject: [PATCH 19/28] excess line --- src/cascadia/UnitTests_SettingsModel/SerializationTests.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/cascadia/UnitTests_SettingsModel/SerializationTests.cpp b/src/cascadia/UnitTests_SettingsModel/SerializationTests.cpp index 7c26228c096..2c1a48097a3 100644 --- a/src/cascadia/UnitTests_SettingsModel/SerializationTests.cpp +++ b/src/cascadia/UnitTests_SettingsModel/SerializationTests.cpp @@ -966,7 +966,6 @@ namespace SettingsModelUnitTests ] })" }; - // Key difference: the close pane action now has a generated ID static constexpr std::string_view newSettingsJson{ R"( { From dd25ed762f3663f930842b1b101ed3d3422f2f12 Mon Sep 17 00:00:00 2001 From: Pankaj Bhojwani Date: Mon, 1 Apr 2024 10:23:23 -0700 Subject: [PATCH 20/28] change tests --- .../UnitTests_SettingsModel/SerializationTests.cpp | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/cascadia/UnitTests_SettingsModel/SerializationTests.cpp b/src/cascadia/UnitTests_SettingsModel/SerializationTests.cpp index 2c1a48097a3..30cd15f574a 100644 --- a/src/cascadia/UnitTests_SettingsModel/SerializationTests.cpp +++ b/src/cascadia/UnitTests_SettingsModel/SerializationTests.cpp @@ -960,21 +960,23 @@ namespace SettingsModelUnitTests "actions": [ { "name": "foo", - "command": "closePane", + "command": { "action": "sendInput", "input": "just some input" }, "keys": "ctrl+shift+w" } ] })" }; - // Key difference: the close pane action now has a generated ID + // Key differences: - the sendInput action now has a generated ID + // - this generated ID was created at the time of writing this test, + // and should remain robust (i.e. everytime we hash the args we should get the same result) static constexpr std::string_view newSettingsJson{ R"( { "actions": [ { "name": "foo", - "command": "closePane", + "command": { "action": "sendInput", "input": "just some input" }, "keys": "ctrl+shift+w", - "id" : "User.closePane" + "id" : "User.sendInput.3448838294654165202" } ] })" }; @@ -1039,7 +1041,7 @@ namespace SettingsModelUnitTests "actions": [ { "name": "foo", - "command": { "action": "sendInput", "input": "VT Griese Mode" }, + "command": { "action": "sendInput", "input": "this is some other input string" }, "keys": "ctrl+shift+w" } ] @@ -1051,7 +1053,7 @@ namespace SettingsModelUnitTests "actions": [ { "name": "foo", - "command": { "action": "sendInput", "input": "VT Griese Mode" }, + "command": { "action": "sendInput", "input": "this is some other input string" }, "keys": "ctrl+shift+w" } ] From 6e293a5ee8504747286c9c54adc396a66afb84e9 Mon Sep 17 00:00:00 2001 From: Pankaj Bhojwani Date: Mon, 1 Apr 2024 10:26:54 -0700 Subject: [PATCH 21/28] Everytime --- src/cascadia/UnitTests_SettingsModel/SerializationTests.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cascadia/UnitTests_SettingsModel/SerializationTests.cpp b/src/cascadia/UnitTests_SettingsModel/SerializationTests.cpp index 30cd15f574a..3bae542bc5c 100644 --- a/src/cascadia/UnitTests_SettingsModel/SerializationTests.cpp +++ b/src/cascadia/UnitTests_SettingsModel/SerializationTests.cpp @@ -968,7 +968,7 @@ namespace SettingsModelUnitTests // Key differences: - the sendInput action now has a generated ID // - this generated ID was created at the time of writing this test, - // and should remain robust (i.e. everytime we hash the args we should get the same result) + // and should remain robust (i.e. every time we hash the args we should get the same result) static constexpr std::string_view newSettingsJson{ R"( { "actions": [ From bdf42c2d9c2ca4a925087603c3dd2fe1be127373 Mon Sep 17 00:00:00 2001 From: Pankaj Bhojwani Date: Thu, 11 Apr 2024 16:05:35 -0700 Subject: [PATCH 22/28] first round of nits --- src/cascadia/TerminalSettingsModel/ActionAndArgs.cpp | 6 +++--- .../TerminalSettingsModel/Resources/en-US/Resources.resw | 3 --- .../UnitTests_SettingsModel/SerializationTests.cpp | 8 ++++---- 3 files changed, 7 insertions(+), 10 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/ActionAndArgs.cpp b/src/cascadia/TerminalSettingsModel/ActionAndArgs.cpp index 90e6e92c777..e4365587c16 100644 --- a/src/cascadia/TerminalSettingsModel/ActionAndArgs.cpp +++ b/src/cascadia/TerminalSettingsModel/ActionAndArgs.cpp @@ -462,13 +462,13 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation if (_Action != ShortcutAction::Invalid) { auto actionKeyString = ActionToStringMap.find(_Action)->second; - auto result = RS_(L"OriginTagUser") + L"." + std::wstring{ actionKeyString.begin(), actionKeyString.end() }; + auto result = fmt::format(L"User.{}", std::wstring{ actionKeyString.begin(), actionKeyString.end() }); if (_Args) { // If there are args, append the hash of the args - result = result + L"." + std::to_wstring(_Args.Hash()); + fmt::format_to(std::back_inserter(result), L".{}", _Args.Hash()); } - return result; + return winrt::hstring{ result }; } return L""; } diff --git a/src/cascadia/TerminalSettingsModel/Resources/en-US/Resources.resw b/src/cascadia/TerminalSettingsModel/Resources/en-US/Resources.resw index 327945bc7f3..6179099bdf5 100644 --- a/src/cascadia/TerminalSettingsModel/Resources/en-US/Resources.resw +++ b/src/cascadia/TerminalSettingsModel/Resources/en-US/Resources.resw @@ -192,9 +192,6 @@ Open system menu - - User - Command Prompt This is the name of "Command Prompt", as localized in Windows. The localization here should match the one in the Windows product for "Command Prompt" diff --git a/src/cascadia/UnitTests_SettingsModel/SerializationTests.cpp b/src/cascadia/UnitTests_SettingsModel/SerializationTests.cpp index 68f9e406d39..ba4c6f2a1fb 100644 --- a/src/cascadia/UnitTests_SettingsModel/SerializationTests.cpp +++ b/src/cascadia/UnitTests_SettingsModel/SerializationTests.cpp @@ -37,8 +37,8 @@ namespace SettingsModelUnitTests TEST_METHOD(RoundtripUserDeletedColorSchemeCollision); TEST_METHOD(RoundtripGenerateActionID); - TEST_METHOD(DoNotGenerateActionID); - TEST_METHOD(RoundtripActionIDsAreSame); + TEST_METHOD(NoGeneratedIDsForIterableAndNestedCommands); + TEST_METHOD(GeneratedActionIDsEqualForIdenticalCommands); private: // Method Description: @@ -996,7 +996,7 @@ namespace SettingsModelUnitTests VERIFY_ARE_EQUAL(toString(newResult), toString(oldResult)); } - void SerializationTests::DoNotGenerateActionID() + void SerializationTests::NoGeneratedIDsForIterableAndNestedCommands() { // for iterable commands, nested commands, and user-defined actions that already have // an ID, we do not need to generate an ID @@ -1032,7 +1032,7 @@ namespace SettingsModelUnitTests VERIFY_IS_FALSE(oldLoader.FixupUserSettings(), L"Validate that there is no need to write back to disk"); } - void SerializationTests::RoundtripActionIDsAreSame() + void SerializationTests::GeneratedActionIDsEqualForIdenticalCommands() { static constexpr std::string_view settingsJson1{ R"( { From 12f3aa9d06eb1b98da2a4412f90da6e4a84f9dbb Mon Sep 17 00:00:00 2001 From: Pankaj Bhojwani Date: Fri, 12 Apr 2024 15:04:23 -0700 Subject: [PATCH 23/28] truncate and hex, debug assert --- .../TerminalSettingsModel/ActionAndArgs.cpp | 13 +++++++++++-- src/cascadia/TerminalSettingsModel/ActionMap.cpp | 6 +++++- .../UnitTests_SettingsModel/SerializationTests.cpp | 4 ++-- 3 files changed, 18 insertions(+), 5 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/ActionAndArgs.cpp b/src/cascadia/TerminalSettingsModel/ActionAndArgs.cpp index e4365587c16..19bc08458ea 100644 --- a/src/cascadia/TerminalSettingsModel/ActionAndArgs.cpp +++ b/src/cascadia/TerminalSettingsModel/ActionAndArgs.cpp @@ -465,8 +465,17 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation auto result = fmt::format(L"User.{}", std::wstring{ actionKeyString.begin(), actionKeyString.end() }); if (_Args) { - // If there are args, append the hash of the args - fmt::format_to(std::back_inserter(result), L".{}", _Args.Hash()); + // If there are args, we need to append the hash of the args + // However, to make it a little more presentable we + // 1. truncate the hash to 32 bits + // 2. convert it to a hex string + // there is a _tiny_ chance of collision because of the truncate but unlikely for + // the number of commands a user is expected to have + const auto argsHash32 = static_cast(_Args.Hash() & 0xFFFFFFFF); + std::wstringstream stream; + stream << std::hex << std::uppercase << argsHash32; + const auto argsHash32InHex = stream.str(); + fmt::format_to(std::back_inserter(result), L".{}", argsHash32InHex); } return winrt::hstring{ result }; } diff --git a/src/cascadia/TerminalSettingsModel/ActionMap.cpp b/src/cascadia/TerminalSettingsModel/ActionMap.cpp index 750a4ee5e43..220cd825565 100644 --- a/src/cascadia/TerminalSettingsModel/ActionMap.cpp +++ b/src/cascadia/TerminalSettingsModel/ActionMap.cpp @@ -797,11 +797,15 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation bool ActionMap::GenerateIDsForActions() { - // Note: this should ONLY be called for the action map in the user's settings file bool fixedUp{ false }; for (auto actionPair : _ActionMap) { auto cmdImpl{ winrt::get_self(actionPair.second) }; + + // Note: this function should ONLY be called for the action map in the user's settings file + // this debug assert should verify that for debug builds + assert(cmdImpl->Origin() == OriginTag::User); + if (cmdImpl->ID().empty()) { fixedUp = cmdImpl->GenerateID() || fixedUp; diff --git a/src/cascadia/UnitTests_SettingsModel/SerializationTests.cpp b/src/cascadia/UnitTests_SettingsModel/SerializationTests.cpp index ba4c6f2a1fb..39c16ff5264 100644 --- a/src/cascadia/UnitTests_SettingsModel/SerializationTests.cpp +++ b/src/cascadia/UnitTests_SettingsModel/SerializationTests.cpp @@ -495,7 +495,7 @@ namespace SettingsModelUnitTests } ], "actions": [ - { "command": { "action": "sendInput", "input": "VT Griese Mode" }, "id" : "User.sendInput.15093368865568800249", "keys": "ctrl+k" } + { "command": { "action": "sendInput", "input": "VT Griese Mode" }, "id": "User.sendInput.E02B3DF9", "keys": "ctrl+k" } ], "theme": "system", "themes": [] @@ -974,7 +974,7 @@ namespace SettingsModelUnitTests "name": "foo", "command": { "action": "sendInput", "input": "just some input" }, "keys": "ctrl+shift+w", - "id" : "User.sendInput.3448838294654165202" + "id" : "User.sendInput.A020D2" } ] })" }; From aa4921268e1531b49b24c141dd3620d99db6b8cb Mon Sep 17 00:00:00 2001 From: Pankaj Bhojwani Date: Fri, 12 Apr 2024 15:08:53 -0700 Subject: [PATCH 24/28] null check --- src/cascadia/TerminalSettingsModel/Command.cpp | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/Command.cpp b/src/cascadia/TerminalSettingsModel/Command.cpp index 63c6ab1776f..c20be960a67 100644 --- a/src/cascadia/TerminalSettingsModel/Command.cpp +++ b/src/cascadia/TerminalSettingsModel/Command.cpp @@ -123,11 +123,14 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation bool Command::GenerateID() { - auto actionAndArgsImpl{ winrt::get_self(_ActionAndArgs) }; - if (const auto generatedID = actionAndArgsImpl->GenerateID(); !generatedID.empty()) + if (_ActionAndArgs) { - _ID = generatedID; - return true; + auto actionAndArgsImpl{ winrt::get_self(_ActionAndArgs) }; + if (const auto generatedID = actionAndArgsImpl->GenerateID(); !generatedID.empty()) + { + _ID = generatedID; + return true; + } } return false; } From 5ee630ec82b795328d696b042e8d2f5f1127544f Mon Sep 17 00:00:00 2001 From: Pankaj Bhojwani Date: Fri, 12 Apr 2024 15:16:36 -0700 Subject: [PATCH 25/28] fmt is smart --- src/cascadia/TerminalSettingsModel/ActionAndArgs.cpp | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/ActionAndArgs.cpp b/src/cascadia/TerminalSettingsModel/ActionAndArgs.cpp index 19bc08458ea..798ac12752c 100644 --- a/src/cascadia/TerminalSettingsModel/ActionAndArgs.cpp +++ b/src/cascadia/TerminalSettingsModel/ActionAndArgs.cpp @@ -472,10 +472,8 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // there is a _tiny_ chance of collision because of the truncate but unlikely for // the number of commands a user is expected to have const auto argsHash32 = static_cast(_Args.Hash() & 0xFFFFFFFF); - std::wstringstream stream; - stream << std::hex << std::uppercase << argsHash32; - const auto argsHash32InHex = stream.str(); - fmt::format_to(std::back_inserter(result), L".{}", argsHash32InHex); + // {0:X} formats the truncated hash to an uppercase hex string + fmt::format_to(std::back_inserter(result), L".{0:X}", argsHash32); } return winrt::hstring{ result }; } From 360b92e56764033d1f4e943abde803bc056066f1 Mon Sep 17 00:00:00 2001 From: Pankaj Bhojwani Date: Wed, 17 Apr 2024 16:38:52 -0700 Subject: [PATCH 26/28] fmt_compile, fix test --- src/cascadia/TerminalSettingsModel/ActionAndArgs.cpp | 2 +- .../UnitTests_SettingsModel/SerializationTests.cpp | 8 +++++++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/cascadia/TerminalSettingsModel/ActionAndArgs.cpp b/src/cascadia/TerminalSettingsModel/ActionAndArgs.cpp index 798ac12752c..3ad4278d968 100644 --- a/src/cascadia/TerminalSettingsModel/ActionAndArgs.cpp +++ b/src/cascadia/TerminalSettingsModel/ActionAndArgs.cpp @@ -462,7 +462,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation if (_Action != ShortcutAction::Invalid) { auto actionKeyString = ActionToStringMap.find(_Action)->second; - auto result = fmt::format(L"User.{}", std::wstring{ actionKeyString.begin(), actionKeyString.end() }); + auto result = fmt::format(FMT_COMPILE(L"User.{}"), actionKeyString); if (_Args) { // If there are args, we need to append the hash of the args diff --git a/src/cascadia/UnitTests_SettingsModel/SerializationTests.cpp b/src/cascadia/UnitTests_SettingsModel/SerializationTests.cpp index 39c16ff5264..c7bbf5f6b38 100644 --- a/src/cascadia/UnitTests_SettingsModel/SerializationTests.cpp +++ b/src/cascadia/UnitTests_SettingsModel/SerializationTests.cpp @@ -16,6 +16,12 @@ using namespace WEX::Common; using namespace winrt::Microsoft::Terminal::Settings::Model; using namespace winrt::Microsoft::Terminal::Control; +#if defined(_M_IX86) +#define sendInputID "56911147" +#else +#define sendInputID "A020D2" +#endif + namespace SettingsModelUnitTests { class SerializationTests : public JsonTestClass @@ -974,7 +980,7 @@ namespace SettingsModelUnitTests "name": "foo", "command": { "action": "sendInput", "input": "just some input" }, "keys": "ctrl+shift+w", - "id" : "User.sendInput.A020D2" + "id" : "User.sendInput.)" sendInputID R"(" } ] })" }; From 5e70911a68b2c5482cbe0682c615d61a680c9274 Mon Sep 17 00:00:00 2001 From: Pankaj Bhojwani Date: Wed, 17 Apr 2024 16:49:58 -0700 Subject: [PATCH 27/28] remove 0 --- src/cascadia/TerminalSettingsModel/ActionAndArgs.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cascadia/TerminalSettingsModel/ActionAndArgs.cpp b/src/cascadia/TerminalSettingsModel/ActionAndArgs.cpp index 3ad4278d968..7bba13bc4cc 100644 --- a/src/cascadia/TerminalSettingsModel/ActionAndArgs.cpp +++ b/src/cascadia/TerminalSettingsModel/ActionAndArgs.cpp @@ -473,7 +473,7 @@ namespace winrt::Microsoft::Terminal::Settings::Model::implementation // the number of commands a user is expected to have const auto argsHash32 = static_cast(_Args.Hash() & 0xFFFFFFFF); // {0:X} formats the truncated hash to an uppercase hex string - fmt::format_to(std::back_inserter(result), L".{0:X}", argsHash32); + fmt::format_to(std::back_inserter(result), FMT_COMPILE(L".{:X}"), argsHash32); } return winrt::hstring{ result }; } From ca3eb87301044355f0afb995f4d0822a847b19cd Mon Sep 17 00:00:00 2001 From: Pankaj Bhojwani Date: Wed, 17 Apr 2024 16:58:04 -0700 Subject: [PATCH 28/28] rename and comment --- .../UnitTests_SettingsModel/SerializationTests.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/cascadia/UnitTests_SettingsModel/SerializationTests.cpp b/src/cascadia/UnitTests_SettingsModel/SerializationTests.cpp index c7bbf5f6b38..8d9c55b6f89 100644 --- a/src/cascadia/UnitTests_SettingsModel/SerializationTests.cpp +++ b/src/cascadia/UnitTests_SettingsModel/SerializationTests.cpp @@ -16,10 +16,12 @@ using namespace WEX::Common; using namespace winrt::Microsoft::Terminal::Settings::Model; using namespace winrt::Microsoft::Terminal::Control; +// Different architectures will hash the same SendInput command to a different ID +// Check for the correct ID based on the architecture #if defined(_M_IX86) -#define sendInputID "56911147" +#define SEND_INPUT_ARCH_SPECIFIC_ACTION_HASH "56911147" #else -#define sendInputID "A020D2" +#define SEND_INPUT_ARCH_SPECIFIC_ACTION_HASH "A020D2" #endif namespace SettingsModelUnitTests @@ -980,7 +982,7 @@ namespace SettingsModelUnitTests "name": "foo", "command": { "action": "sendInput", "input": "just some input" }, "keys": "ctrl+shift+w", - "id" : "User.sendInput.)" sendInputID R"(" + "id" : "User.sendInput.)" SEND_INPUT_ARCH_SPECIFIC_ACTION_HASH R"(" } ] })" };