diff --git a/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp b/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp index f518a80251..9a90948dfa 100644 --- a/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp +++ b/src/cascadia/LocalTests_TerminalApp/SettingsTests.cpp @@ -91,6 +91,8 @@ namespace TerminalAppLocalTests TEST_METHOD(TestIterableInNestedCommand); TEST_METHOD(TestMixedNestedAndIterableCommand); TEST_METHOD(TestNestedCommandWithoutName); + TEST_METHOD(TestUnbindNestedCommand); + TEST_METHOD(TestRebindNestedCommand); TEST_CLASS_SETUP(ClassSetup) { @@ -101,7 +103,10 @@ namespace TerminalAppLocalTests private: void _logCommandNames(std::unordered_map& commands, const int indentation = 1) { - Log::Comment(L"Commands:"); + if (indentation == 1) + { + Log::Comment(commands.empty() ? L"Commands:\n " : L"Commands:"); + } for (auto& nameAndCommand : commands) { Log::Comment(fmt::format(L"{0:>{1}}* {2}->{3}", @@ -2957,9 +2962,7 @@ namespace TerminalAppLocalTests void SettingsTests::TestNestedCommands() { - // This test TODODODOD - - // This test checks a iterable command that includes a nested command. + // This test checks a nested command. // The commands should look like: // // @@ -3060,15 +3063,14 @@ namespace TerminalAppLocalTests void SettingsTests::TestNestedInNestedCommand() { - // This test TODODODOD - - // This test checks a iterable command that includes a nested command. + // This test checks a nested command that includes nested commands. // The commands should look like: // // - // └─ Connect to ssh... - // ├─ first.com - // └─ second.com + // └─ grandparent + // └─ parent + // ├─ child1 + // └─ child2 const std::string settingsJson{ R"( { @@ -3629,12 +3631,233 @@ namespace TerminalAppLocalTests } } } + void SettingsTests::TestNestedCommandWithoutName() { // This test tests a nested command without a name specified. This type // of command should just be ignored, since we can't auto-generate names // for nested commands, they _must_ have names specified. - Log::Result(WEX::Logging::TestResults::Skipped); + + const std::string settingsJson{ R"( + { + "defaultProfile": "{6239a42c-0000-49a3-80bd-e8fdd045185c}", + "profiles": [ + { + "name": "profile0", + "guid": "{6239a42c-0000-49a3-80bd-e8fdd045185c}", + "historySize": 1, + "commandline": "cmd.exe" + }, + { + "name": "profile1", + "guid": "{6239a42c-1111-49a3-80bd-e8fdd045185c}", + "historySize": 2, + "commandline": "pwsh.exe" + }, + { + "name": "profile2", + "historySize": 3, + "commandline": "wsl.exe" + } + ], + "bindings": [ + { + "commands": [ + { + "name": "child1", + "command": { "action": "newTab", "commandline": "ssh me@first.com" } + }, + { + "name": "child2", + "command": { "action": "newTab", "commandline": "ssh me@second.com" } + } + ] + }, + ], + "schemes": [ { "name": "Campbell" } ] // This is included here to prevent settings validation errors. + })" }; + + VerifyParseSucceeded(settingsJson); + CascadiaSettings settings{}; + settings._ParseJsonString(settingsJson, false); + settings.LayerJson(settings._userSettings); + + VERIFY_ARE_EQUAL(0u, settings._warnings.size()); + VERIFY_ARE_EQUAL(3u, settings.GetProfiles().size()); + + auto& commands = settings._globals.GetCommands(); + settings._ValidateSettings(); + _logCommandNames(commands); + + VERIFY_ARE_EQUAL(0u, settings._warnings.size()); + + // Because the "parent" command didn't have a name, it couldn't be + // placed into the list of commands. It and it's children are just + // ignored. + VERIFY_ARE_EQUAL(0u, commands.size()); + } + + void SettingsTests::TestUnbindNestedCommand() + { + // TODO: description + + const std::string settingsJson{ R"( + { + "defaultProfile": "{6239a42c-0000-49a3-80bd-e8fdd045185c}", + "profiles": [ + { + "name": "profile0", + "guid": "{6239a42c-0000-49a3-80bd-e8fdd045185c}", + "historySize": 1, + "commandline": "cmd.exe" + }, + { + "name": "profile1", + "guid": "{6239a42c-1111-49a3-80bd-e8fdd045185c}", + "historySize": 2, + "commandline": "pwsh.exe" + }, + { + "name": "profile2", + "historySize": 3, + "commandline": "wsl.exe" + } + ], + "bindings": [ + { + "name": "parent", + "commands": [ + { + "name": "child1", + "command": { "action": "newTab", "commandline": "ssh me@first.com" } + }, + { + "name": "child2", + "command": { "action": "newTab", "commandline": "ssh me@second.com" } + } + ] + }, + ], + "schemes": [ { "name": "Campbell" } ] // This is included here to prevent settings validation errors. + })" }; + + const std::string settings1Json{ R"( + { + "defaultProfile": "{6239a42c-0000-49a3-80bd-e8fdd045185c}", + "bindings": [ + { + "name": "parent", + "commands": null + }, + ], + })" }; + + VerifyParseSucceeded(settingsJson); + VerifyParseSucceeded(settings1Json); + + CascadiaSettings settings{}; + settings._ParseJsonString(settingsJson, false); + settings.LayerJson(settings._userSettings); + + VERIFY_ARE_EQUAL(0u, settings._warnings.size()); + VERIFY_ARE_EQUAL(3u, settings.GetProfiles().size()); + + auto& commands = settings._globals.GetCommands(); + settings._ValidateSettings(); + _logCommandNames(commands); + + VERIFY_ARE_EQUAL(0u, settings._warnings.size()); + VERIFY_ARE_EQUAL(1u, commands.size()); + + Log::Comment(L"Layer second bit of json, to unbind the original command."); + + settings._ParseJsonString(settings1Json, false); + settings.LayerJson(settings._userSettings); + settings._ValidateSettings(); + _logCommandNames(commands); + VERIFY_ARE_EQUAL(0u, settings._warnings.size()); + VERIFY_ARE_EQUAL(0u, commands.size()); + } + + void SettingsTests::TestRebindNestedCommand() + { + // TODO: description + + const std::string settingsJson{ R"( + { + "defaultProfile": "{6239a42c-0000-49a3-80bd-e8fdd045185c}", + "profiles": [ + { + "name": "profile0", + "guid": "{6239a42c-0000-49a3-80bd-e8fdd045185c}", + "historySize": 1, + "commandline": "cmd.exe" + }, + { + "name": "profile1", + "guid": "{6239a42c-1111-49a3-80bd-e8fdd045185c}", + "historySize": 2, + "commandline": "pwsh.exe" + }, + { + "name": "profile2", + "historySize": 3, + "commandline": "wsl.exe" + } + ], + "bindings": [ + { + "name": "parent", + "commands": [ + { + "name": "child1", + "command": { "action": "newTab", "commandline": "ssh me@first.com" } + }, + { + "name": "child2", + "command": { "action": "newTab", "commandline": "ssh me@second.com" } + } + ] + }, + ], + "schemes": [ { "name": "Campbell" } ] // This is included here to prevent settings validation errors. + })" }; + + const std::string settings1Json{ R"( + { + "defaultProfile": "{6239a42c-0000-49a3-80bd-e8fdd045185c}", + "bindings": [ + { + "name": "parent", + "command": "newTab" + }, + ], + })" }; + + VerifyParseSucceeded(settingsJson); + VerifyParseSucceeded(settings1Json); + + CascadiaSettings settings{}; + settings._ParseJsonString(settingsJson, false); + settings.LayerJson(settings._userSettings); + + VERIFY_ARE_EQUAL(0u, settings._warnings.size()); + VERIFY_ARE_EQUAL(3u, settings.GetProfiles().size()); + + auto& commands = settings._globals.GetCommands(); + settings._ValidateSettings(); + _logCommandNames(commands); + + VERIFY_ARE_EQUAL(0u, settings._warnings.size()); + VERIFY_ARE_EQUAL(1u, commands.size()); + + Log::Comment(L"Layer second bit of json, to unbind the original command."); + settings._ParseJsonString(settings1Json, false); + settings.LayerJson(settings._userSettings); + settings._ValidateSettings(); + _logCommandNames(commands); + VERIFY_ARE_EQUAL(0u, settings._warnings.size()); + VERIFY_ARE_EQUAL(1u, commands.size()); } } diff --git a/src/cascadia/TerminalApp/Command.cpp b/src/cascadia/TerminalApp/Command.cpp index fc9c3e80af..a2962ca5d6 100644 --- a/src/cascadia/TerminalApp/Command.cpp +++ b/src/cascadia/TerminalApp/Command.cpp @@ -145,13 +145,12 @@ namespace winrt::TerminalApp::implementation nested = true; } - - // TODO: else if (hasKey(CommandKey) ) - // { - // // { name: "foo", commands: null } will land in this case, which - // // should also be used for unbinding. - // return nullptr; - // } + else if (json.isMember(JsonKey(CommandsKey))) + { + // { "name": "foo", "commands": null } will land in this case, which + // should also be used for unbinding. + return nullptr; + } // TODO GH#6644: iconPath not implemented quite yet. Can't seem to get // the binding quite right. Additionally, do we want it to be an image, @@ -443,10 +442,6 @@ namespace winrt::TerminalApp::implementation { warnings.push_back(::TerminalApp::SettingsLoadWarnings::FailedToParseCommandJson); // If we encounter a re-parsing error, just stop processing the rest of the commands. - - // TODO: if we fail to expand the json, we should return NO - // commands, and also remove the current command from the - // list of commands. break; } diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index 11a6de41d3..51e8856153 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -80,6 +80,37 @@ namespace winrt::TerminalApp::implementation InitializeComponent(); } + // Function Description: + // - Recursively check our commands to see if there's a keybinding for + // exactly their action. If there is, label that command with the text + // corresponding to that key chord. + // - Will recurse into nested commands as well. + // Arguments: + // - settings: The settings who's keybindings we should use to look up the key chords from + // - commands: The list fo commands to label. + // Return Value: + // - + static void _recursiveUpdateCommandKeybindingLabels(std::shared_ptr<::TerminalApp::CascadiaSettings> settings, + Windows::Foundation::Collections::IVector const& commands) + { + for (auto command : commands) + { + // If there's a keybinding that's bound to exactly this command, + // then get the string for that keychord and display it as a + // part of the command in the UI. Each Command's KeyChordText is + // unset by default, so we don't need to worry about clearing it + // if there isn't a key associated with it. + auto keyChord{ settings->GetKeybindings().GetKeyBindingForActionWithArgs(command.Action()) }; + + if (keyChord) + { + command.KeyChordText(KeyChordSerialization::ToString(keyChord)); + } + + _recursiveUpdateCommandKeybindingLabels(settings, command.NestedCommands()); + } + } + winrt::fire_and_forget TerminalPage::SetSettings(std::shared_ptr<::TerminalApp::CascadiaSettings> settings, bool needRefreshUI) { @@ -97,23 +128,11 @@ namespace winrt::TerminalApp::implementation auto commandsCollection = winrt::single_threaded_vector(); for (auto& nameAndCommand : _settings->GlobalSettings().GetCommands()) { - auto command = nameAndCommand.second; - - // If there's a keybinding that's bound to exactly this command, - // then get the string for that keychord and display it as a - // part of the command in the UI. Each Command's KeyChordText is - // unset by default, so we don't need to worry about clearing it - // if there isn't a key associated with it. - auto keyChord{ _settings->GetKeybindings().GetKeyBindingForActionWithArgs(command.Action()) }; - - // TODO: Make this recursive, because commands might have - // subcommands that also have keybindings - if (keyChord) - { - command.KeyChordText(KeyChordSerialization::ToString(keyChord)); - } - commandsCollection.Append(command); + commandsCollection.Append(nameAndCommand.second); } + + _recursiveUpdateCommandKeybindingLabels(_settings, commandsCollection); + CommandPalette().SetActions(commandsCollection); } }