From a3a9df82b5194b31fff3c7bd173c0cd2ff1f2915 Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Thu, 25 Jun 2020 08:06:21 -0500 Subject: [PATCH] Add `setTabColor` and `openTabColorPicker` actions (#6567) ## Summary of the Pull Request Adds a pair of `ShortcutAction`s for setting the tab color. * `setTabColor`: This changes the color of the current tab to the provided color, or can be used to clear the color. * `openTabColorPicker`: This keybinding immediately activates the tab color picker for the currently focused tab. ## References ## PR Checklist * [x] scratches my own itch * [x] I work here * [x] Tests added/passed * [x] https://github.com/MicrosoftDocs/terminal/pull/69 ## Detailed Description of the Pull Request / Additional comments ## Validation Steps Performed * hey look there are tests * Tested with the following: ```json // { "command": "setTabColor", "keys": [ "alt+c" ] }, { "keys": "ctrl+alt+c", "command": { "action": "setTabColor", "color": "#123456" } }, { "keys": "alt+shift+c", "command": { "action": "setTabColor", "color": null} }, { "keys": "alt+c", "command": "openTabColorPicker" }, ``` --- .../actions/spell-check/expect/alphabet.txt | 1 + doc/cascadia/profiles.schema.json | 19 ++++++ .../KeyBindingsTests.cpp | 59 +++++++++++++++++++ src/cascadia/TerminalApp/ActionAndArgs.cpp | 6 ++ src/cascadia/TerminalApp/ActionArgs.cpp | 1 + src/cascadia/TerminalApp/ActionArgs.h | 37 ++++++++++++ src/cascadia/TerminalApp/ActionArgs.idl | 5 ++ .../TerminalApp/AppActionHandlers.cpp | 40 ++++++++++++- .../TerminalApp/ShortcutActionDispatch.cpp | 10 ++++ .../TerminalApp/ShortcutActionDispatch.h | 2 + .../TerminalApp/ShortcutActionDispatch.idl | 8 +-- src/cascadia/TerminalApp/Tab.cpp | 21 +++++-- src/cascadia/TerminalApp/Tab.h | 6 +- src/cascadia/TerminalApp/TerminalPage.cpp | 2 + src/cascadia/TerminalApp/TerminalPage.h | 3 +- 15 files changed, 207 insertions(+), 13 deletions(-) diff --git a/.github/actions/spell-check/expect/alphabet.txt b/.github/actions/spell-check/expect/alphabet.txt index 3644d8e592..6d395ae7a5 100644 --- a/.github/actions/spell-check/expect/alphabet.txt +++ b/.github/actions/spell-check/expect/alphabet.txt @@ -8,6 +8,7 @@ abcdefghijklmnop ABCDEFGHIJKLMNOPQRST abcdefghijklmnopqrstuvwxyz ABE +BBGGRR BBBBBBBBBBBBBBDDDD QQQQQQQQQQABCDEFGHIJ QQQQQQQQQQABCDEFGHIJKLMNOPQRSTQQQQQQQQQ diff --git a/doc/cascadia/profiles.schema.json b/doc/cascadia/profiles.schema.json index 66a912a19b..2d84bb86f5 100644 --- a/doc/cascadia/profiles.schema.json +++ b/doc/cascadia/profiles.schema.json @@ -56,6 +56,8 @@ "switchToTab", "toggleFullscreen", "find", + "setTabColor", + "openTabColorPicker", "unbound" ], "type": "string" @@ -257,6 +259,22 @@ } ] }, + "SetTabColorAction": { + "description": "Arguments corresponding to a Set Tab Color Action", + "allOf": [ + { "$ref": "#/definitions/ShortcutAction" }, + { + "properties": { + "action": { "type": "string", "pattern": "setTabColor" }, + "color": { + "$ref": "#/definitions/Color", + "default": null, + "description": "If provided, will set the tab's color to the given value. If omitted, will reset the tab's color." + } + } + } + ] + }, "Keybinding": { "additionalProperties": false, "properties": { @@ -272,6 +290,7 @@ { "$ref": "#/definitions/ResizePaneAction" }, { "$ref": "#/definitions/SplitPaneAction" }, { "$ref": "#/definitions/OpenSettingsAction" }, + { "$ref": "#/definitions/SetTabColorAction" }, { "type": "null" } ] }, diff --git a/src/cascadia/LocalTests_TerminalApp/KeyBindingsTests.cpp b/src/cascadia/LocalTests_TerminalApp/KeyBindingsTests.cpp index 7c16e90eee..760bfd94c8 100644 --- a/src/cascadia/LocalTests_TerminalApp/KeyBindingsTests.cpp +++ b/src/cascadia/LocalTests_TerminalApp/KeyBindingsTests.cpp @@ -44,6 +44,8 @@ namespace TerminalAppLocalTests TEST_METHOD(TestStringOverload); + TEST_METHOD(TestSetTabColorArgs); + TEST_CLASS_SETUP(ClassSetup) { InitializeJsonReader(); @@ -400,6 +402,63 @@ namespace TerminalAppLocalTests } } + void KeyBindingsTests::TestSetTabColorArgs() + { + const std::string bindings0String{ R"([ + { "keys": ["ctrl+c"], "command": { "action": "setTabColor", "color": null } }, + { "keys": ["ctrl+d"], "command": { "action": "setTabColor", "color": "#123456" } }, + { "keys": ["ctrl+e"], "command": { "action": "setTabColor", "color": "thisStringObviouslyWontWork" } }, + { "keys": ["ctrl+f"], "command": "setTabColor" }, + ])" }; + + const auto bindings0Json = VerifyParseSucceeded(bindings0String); + + auto appKeyBindings = winrt::make_self(); + VERIFY_IS_NOT_NULL(appKeyBindings); + VERIFY_ARE_EQUAL(0u, appKeyBindings->_keyShortcuts.size()); + appKeyBindings->LayerJson(bindings0Json); + VERIFY_ARE_EQUAL(4u, appKeyBindings->_keyShortcuts.size()); + + { + KeyChord kc{ true, false, false, static_cast('C') }; + auto actionAndArgs = TestUtils::GetActionAndArgs(*appKeyBindings, kc); + VERIFY_ARE_EQUAL(ShortcutAction::SetTabColor, actionAndArgs.Action()); + const auto& realArgs = actionAndArgs.Args().try_as(); + VERIFY_IS_NOT_NULL(realArgs); + // Verify the args have the expected value + VERIFY_IS_NULL(realArgs.TabColor()); + } + { + KeyChord kc{ true, false, false, static_cast('D') }; + auto actionAndArgs = TestUtils::GetActionAndArgs(*appKeyBindings, kc); + VERIFY_ARE_EQUAL(ShortcutAction::SetTabColor, actionAndArgs.Action()); + const auto& realArgs = actionAndArgs.Args().try_as(); + VERIFY_IS_NOT_NULL(realArgs); + // Verify the args have the expected value + VERIFY_IS_NOT_NULL(realArgs.TabColor()); + // Remember that COLORREFs are actually BBGGRR order, while the string is in #RRGGBB order + VERIFY_ARE_EQUAL(static_cast(til::color(0x563412)), realArgs.TabColor().Value()); + } + { + KeyChord kc{ true, false, false, static_cast('E') }; + auto actionAndArgs = TestUtils::GetActionAndArgs(*appKeyBindings, kc); + VERIFY_ARE_EQUAL(ShortcutAction::SetTabColor, actionAndArgs.Action()); + const auto& realArgs = actionAndArgs.Args().try_as(); + VERIFY_IS_NOT_NULL(realArgs); + // Verify the args have the expected value + VERIFY_IS_NULL(realArgs.TabColor()); + } + { + KeyChord kc{ true, false, false, static_cast('F') }; + auto actionAndArgs = TestUtils::GetActionAndArgs(*appKeyBindings, kc); + VERIFY_ARE_EQUAL(ShortcutAction::SetTabColor, actionAndArgs.Action()); + const auto& realArgs = actionAndArgs.Args().try_as(); + VERIFY_IS_NOT_NULL(realArgs); + // Verify the args have the expected value + VERIFY_IS_NULL(realArgs.TabColor()); + } + } + void KeyBindingsTests::TestStringOverload() { const std::string bindings0String{ R"([ diff --git a/src/cascadia/TerminalApp/ActionAndArgs.cpp b/src/cascadia/TerminalApp/ActionAndArgs.cpp index a7cf78bb8a..b69dd97869 100644 --- a/src/cascadia/TerminalApp/ActionAndArgs.cpp +++ b/src/cascadia/TerminalApp/ActionAndArgs.cpp @@ -33,6 +33,8 @@ static constexpr std::string_view ResizePaneKey{ "resizePane" }; static constexpr std::string_view MoveFocusKey{ "moveFocus" }; static constexpr std::string_view FindKey{ "find" }; static constexpr std::string_view ToggleFullscreenKey{ "toggleFullscreen" }; +static constexpr std::string_view SetTabColorKey{ "setTabColor" }; +static constexpr std::string_view OpenTabColorPickerKey{ "openTabColorPicker" }; static constexpr std::string_view RenameTabKey{ "renameTab" }; namespace winrt::TerminalApp::implementation @@ -69,6 +71,8 @@ namespace winrt::TerminalApp::implementation { OpenSettingsKey, ShortcutAction::OpenSettings }, { ToggleFullscreenKey, ShortcutAction::ToggleFullscreen }, { SplitPaneKey, ShortcutAction::SplitPane }, + { SetTabColorKey, ShortcutAction::SetTabColor }, + { OpenTabColorPickerKey, ShortcutAction::OpenTabColorPicker }, { UnboundKey, ShortcutAction::Invalid }, { FindKey, ShortcutAction::Find }, { RenameTabKey, ShortcutAction::RenameTab } @@ -99,6 +103,8 @@ namespace winrt::TerminalApp::implementation { ShortcutAction::OpenSettings, winrt::TerminalApp::implementation::OpenSettingsArgs::FromJson }, + { ShortcutAction::SetTabColor, winrt::TerminalApp::implementation::SetTabColorArgs::FromJson }, + { ShortcutAction::RenameTab, winrt::TerminalApp::implementation::RenameTabArgs::FromJson }, { ShortcutAction::Invalid, nullptr }, diff --git a/src/cascadia/TerminalApp/ActionArgs.cpp b/src/cascadia/TerminalApp/ActionArgs.cpp index d535509989..1c7eae2aba 100644 --- a/src/cascadia/TerminalApp/ActionArgs.cpp +++ b/src/cascadia/TerminalApp/ActionArgs.cpp @@ -15,4 +15,5 @@ #include "AdjustFontSizeArgs.g.cpp" #include "SplitPaneArgs.g.cpp" #include "OpenSettingsArgs.g.cpp" +#include "SetTabColorArgs.g.cpp" #include "RenameTabArgs.g.cpp" diff --git a/src/cascadia/TerminalApp/ActionArgs.h b/src/cascadia/TerminalApp/ActionArgs.h index 57c3e4a66d..76426637b0 100644 --- a/src/cascadia/TerminalApp/ActionArgs.h +++ b/src/cascadia/TerminalApp/ActionArgs.h @@ -15,10 +15,12 @@ #include "AdjustFontSizeArgs.g.h" #include "SplitPaneArgs.g.h" #include "OpenSettingsArgs.g.h" +#include "SetTabColorArgs.g.h" #include "RenameTabArgs.g.h" #include "../../cascadia/inc/cppwinrt_utils.h" #include "Utils.h" +#include "JsonUtils.h" #include "TerminalWarnings.h" // Notes on defining ActionArgs and ActionEventArgs: @@ -443,6 +445,41 @@ namespace winrt::TerminalApp::implementation } }; + struct SetTabColorArgs : public SetTabColorArgsT + { + SetTabColorArgs() = default; + GETSET_PROPERTY(Windows::Foundation::IReference, TabColor, nullptr); + + static constexpr std::string_view ColorKey{ "color" }; + + public: + bool Equals(const IActionArgs& other) + { + auto otherAsUs = other.try_as(); + if (otherAsUs) + { + return otherAsUs->_TabColor == _TabColor; + } + return false; + }; + static FromJsonResult FromJson(const Json::Value& json) + { + // LOAD BEARING: Not using make_self here _will_ break you in the future! + auto args = winrt::make_self(); + std::optional temp; + try + { + ::TerminalApp::JsonUtils::GetOptionalColor(json, ColorKey, temp); + if (temp.has_value()) + { + args->_TabColor = static_cast(temp.value()); + } + } + CATCH_LOG(); + return { *args, {} }; + } + }; + struct RenameTabArgs : public RenameTabArgsT { RenameTabArgs() = default; diff --git a/src/cascadia/TerminalApp/ActionArgs.idl b/src/cascadia/TerminalApp/ActionArgs.idl index 1e353f7c96..771d938764 100644 --- a/src/cascadia/TerminalApp/ActionArgs.idl +++ b/src/cascadia/TerminalApp/ActionArgs.idl @@ -103,6 +103,11 @@ namespace TerminalApp SettingsTarget Target { get; }; }; + [default_interface] runtimeclass SetTabColorArgs : IActionArgs + { + Windows.Foundation.IReference TabColor { get; }; + }; + [default_interface] runtimeclass RenameTabArgs : IActionArgs { String Title { get; }; diff --git a/src/cascadia/TerminalApp/AppActionHandlers.cpp b/src/cascadia/TerminalApp/AppActionHandlers.cpp index c679d4290d..1f1ab07241 100644 --- a/src/cascadia/TerminalApp/AppActionHandlers.cpp +++ b/src/cascadia/TerminalApp/AppActionHandlers.cpp @@ -238,6 +238,45 @@ namespace winrt::TerminalApp::implementation args.Handled(true); } + void TerminalPage::_HandleSetTabColor(const IInspectable& /*sender*/, + const TerminalApp::ActionEventArgs& args) + { + std::optional tabColor; + + if (const auto& realArgs = args.ActionArgs().try_as()) + { + if (realArgs.TabColor() != nullptr) + { + tabColor = realArgs.TabColor().Value(); + } + } + + auto activeTab = _GetFocusedTab(); + if (activeTab) + { + if (tabColor.has_value()) + { + activeTab->SetTabColor(tabColor.value()); + } + else + { + activeTab->ResetTabColor(); + } + } + args.Handled(true); + } + + void TerminalPage::_HandleOpenTabColorPicker(const IInspectable& /*sender*/, + const TerminalApp::ActionEventArgs& args) + { + auto activeTab = _GetFocusedTab(); + if (activeTab) + { + activeTab->ActivateColorPicker(); + } + args.Handled(true); + } + void TerminalPage::_HandleRenameTab(const IInspectable& /*sender*/, const TerminalApp::ActionEventArgs& args) { @@ -262,5 +301,4 @@ namespace winrt::TerminalApp::implementation } args.Handled(true); } - } diff --git a/src/cascadia/TerminalApp/ShortcutActionDispatch.cpp b/src/cascadia/TerminalApp/ShortcutActionDispatch.cpp index b9a6bae618..682b2a4594 100644 --- a/src/cascadia/TerminalApp/ShortcutActionDispatch.cpp +++ b/src/cascadia/TerminalApp/ShortcutActionDispatch.cpp @@ -159,6 +159,16 @@ namespace winrt::TerminalApp::implementation _ToggleFullscreenHandlers(*this, *eventArgs); break; } + case ShortcutAction::SetTabColor: + { + _SetTabColorHandlers(*this, *eventArgs); + break; + } + case ShortcutAction::OpenTabColorPicker: + { + _OpenTabColorPickerHandlers(*this, *eventArgs); + break; + } case ShortcutAction::RenameTab: { _RenameTabHandlers(*this, *eventArgs); diff --git a/src/cascadia/TerminalApp/ShortcutActionDispatch.h b/src/cascadia/TerminalApp/ShortcutActionDispatch.h index f1c485d65e..422858a0e1 100644 --- a/src/cascadia/TerminalApp/ShortcutActionDispatch.h +++ b/src/cascadia/TerminalApp/ShortcutActionDispatch.h @@ -47,6 +47,8 @@ namespace winrt::TerminalApp::implementation TYPED_EVENT(Find, TerminalApp::ShortcutActionDispatch, TerminalApp::ActionEventArgs); TYPED_EVENT(MoveFocus, TerminalApp::ShortcutActionDispatch, TerminalApp::ActionEventArgs); TYPED_EVENT(ToggleFullscreen, TerminalApp::ShortcutActionDispatch, TerminalApp::ActionEventArgs); + TYPED_EVENT(SetTabColor, TerminalApp::ShortcutActionDispatch, TerminalApp::ActionEventArgs); + TYPED_EVENT(OpenTabColorPicker,TerminalApp::ShortcutActionDispatch, TerminalApp::ActionEventArgs); TYPED_EVENT(RenameTab, TerminalApp::ShortcutActionDispatch, TerminalApp::ActionEventArgs); // clang-format on diff --git a/src/cascadia/TerminalApp/ShortcutActionDispatch.idl b/src/cascadia/TerminalApp/ShortcutActionDispatch.idl index ea048a08d5..a09390ebae 100644 --- a/src/cascadia/TerminalApp/ShortcutActionDispatch.idl +++ b/src/cascadia/TerminalApp/ShortcutActionDispatch.idl @@ -4,9 +4,6 @@ import "../ActionArgs.idl"; namespace TerminalApp { - // TODO: GH#1069 - Many of these shortcut actions are "legacy" now that we - // have support for arbitrary args (#1142). We should remove them, and our - // legacy deserializers. enum ShortcutAction { Invalid = 0, @@ -35,6 +32,8 @@ namespace TerminalApp MoveFocus, Find, ToggleFullscreen, + SetTabColor, + OpenTabColorPicker, OpenSettings, RenameTab }; @@ -74,7 +73,8 @@ namespace TerminalApp event Windows.Foundation.TypedEventHandler Find; event Windows.Foundation.TypedEventHandler MoveFocus; event Windows.Foundation.TypedEventHandler ToggleFullscreen; + event Windows.Foundation.TypedEventHandler SetTabColor; + event Windows.Foundation.TypedEventHandler OpenTabColorPicker; event Windows.Foundation.TypedEventHandler RenameTab; - } } diff --git a/src/cascadia/TerminalApp/Tab.cpp b/src/cascadia/TerminalApp/Tab.cpp index 2a866527ef..0e9838ec69 100644 --- a/src/cascadia/TerminalApp/Tab.cpp +++ b/src/cascadia/TerminalApp/Tab.cpp @@ -520,7 +520,7 @@ namespace winrt::TerminalApp::implementation chooseColorMenuItem.Click([weakThis](auto&&, auto&&) { if (auto tab{ weakThis.get() }) { - tab->_tabColorPickup.ShowAt(tab->_tabViewItem); + tab->ActivateColorPicker(); } }); chooseColorMenuItem.Text(RS_(L"TabColorChoose")); @@ -530,14 +530,14 @@ namespace winrt::TerminalApp::implementation _tabColorPickup.ColorSelected([weakThis](auto newTabColor) { if (auto tab{ weakThis.get() }) { - tab->_SetTabColor(newTabColor); + tab->SetTabColor(newTabColor); } }); _tabColorPickup.ColorCleared([weakThis]() { if (auto tab{ weakThis.get() }) { - tab->_ResetTabColor(); + tab->ResetTabColor(); } }); @@ -718,7 +718,7 @@ namespace winrt::TerminalApp::implementation // - color: the shiny color the user picked for their tab // Return Value: // - - void Tab::_SetTabColor(const winrt::Windows::UI::Color& color) + void Tab::SetTabColor(const winrt::Windows::UI::Color& color) { auto weakThis{ get_weak() }; @@ -779,7 +779,7 @@ namespace winrt::TerminalApp::implementation // - // Return Value: // - - void Tab::_ResetTabColor() + void Tab::ResetTabColor() { auto weakThis{ get_weak() }; @@ -817,6 +817,17 @@ namespace winrt::TerminalApp::implementation }); } + // Method Description: + // - Display the tab color picker at the location of the TabViewItem for this tab. + // Arguments: + // - + // Return Value: + // - + void Tab::ActivateColorPicker() + { + _tabColorPickup.ShowAt(_tabViewItem); + } + // Method Description: // Toggles the visual state of the tab view item, // so that changes to the tab color are reflected immediately diff --git a/src/cascadia/TerminalApp/Tab.h b/src/cascadia/TerminalApp/Tab.h index 5095311fc8..be76c1b791 100644 --- a/src/cascadia/TerminalApp/Tab.h +++ b/src/cascadia/TerminalApp/Tab.h @@ -56,6 +56,10 @@ namespace winrt::TerminalApp::implementation std::optional GetTabColor(); + void SetTabColor(const winrt::Windows::UI::Color& color); + void ResetTabColor(); + void ActivateColorPicker(); + WINRT_CALLBACK(Closed, winrt::Windows::Foundation::EventHandler); WINRT_CALLBACK(PropertyChanged, Windows::UI::Xaml::Data::PropertyChangedEventHandler); DECLARE_EVENT(ActivePaneChanged, _ActivePaneChangedHandlers, winrt::delegate<>); @@ -83,8 +87,6 @@ namespace winrt::TerminalApp::implementation void _Focus(); void _CreateContextMenu(); - void _SetTabColor(const winrt::Windows::UI::Color& color); - void _ResetTabColor(); void _RefreshVisualState(); void _BindEventHandlers(const winrt::Microsoft::Terminal::TerminalControl::TermControl& control) noexcept; diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index 4d12cb4037..fcdae329c6 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -755,6 +755,8 @@ namespace winrt::TerminalApp::implementation _actionDispatch->Find({ this, &TerminalPage::_HandleFind }); _actionDispatch->ResetFontSize({ this, &TerminalPage::_HandleResetFontSize }); _actionDispatch->ToggleFullscreen({ this, &TerminalPage::_HandleToggleFullscreen }); + _actionDispatch->SetTabColor({ this, &TerminalPage::_HandleSetTabColor }); + _actionDispatch->OpenTabColorPicker({ this, &TerminalPage::_HandleOpenTabColorPicker }); _actionDispatch->RenameTab({ this, &TerminalPage::_HandleRenameTab }); } diff --git a/src/cascadia/TerminalApp/TerminalPage.h b/src/cascadia/TerminalApp/TerminalPage.h index dfb1656d8c..e6eb55d63d 100644 --- a/src/cascadia/TerminalApp/TerminalPage.h +++ b/src/cascadia/TerminalApp/TerminalPage.h @@ -198,8 +198,9 @@ namespace winrt::TerminalApp::implementation void _HandleFind(const IInspectable& sender, const TerminalApp::ActionEventArgs& args); void _HandleResetFontSize(const IInspectable& sender, const TerminalApp::ActionEventArgs& args); void _HandleToggleFullscreen(const IInspectable& sender, const TerminalApp::ActionEventArgs& args); + void _HandleSetTabColor(const IInspectable& sender, const TerminalApp::ActionEventArgs& args); + void _HandleOpenTabColorPicker(const IInspectable& sender, const TerminalApp::ActionEventArgs& args); void _HandleRenameTab(const IInspectable& sender, const TerminalApp::ActionEventArgs& args); - #pragma endregion friend class TerminalAppLocalTests::TabTests;