Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add setTabColor and openTabColorPicker actions #6567

Merged
5 commits merged into from
Jun 25, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/actions/spell-check/expect/alphabet.txt
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ abcdefghijklmnop
ABCDEFGHIJKLMNOPQRST
abcdefghijklmnopqrstuvwxyz
ABE
BBGGRR
BBBBBBBBBBBBBBDDDD
QQQQQQQQQQABCDEFGHIJ
QQQQQQQQQQABCDEFGHIJKLMNOPQRSTQQQQQQQQQ
Expand Down
19 changes: 19 additions & 0 deletions doc/cascadia/profiles.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@
"switchToTab",
"toggleFullscreen",
"find",
"setTabColor",
"openTabColorPicker",
"unbound"
],
"type": "string"
Expand Down Expand Up @@ -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": {
Expand All @@ -272,6 +290,7 @@
{ "$ref": "#/definitions/ResizePaneAction" },
{ "$ref": "#/definitions/SplitPaneAction" },
{ "$ref": "#/definitions/OpenSettingsAction" },
{ "$ref": "#/definitions/SetTabColorAction" },
{ "type": "null" }
]
},
Expand Down
59 changes: 59 additions & 0 deletions src/cascadia/LocalTests_TerminalApp/KeyBindingsTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ namespace TerminalAppLocalTests

TEST_METHOD(TestStringOverload);

TEST_METHOD(TestSetTabColorArgs);

TEST_CLASS_SETUP(ClassSetup)
{
InitializeJsonReader();
Expand Down Expand Up @@ -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" } },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JsonUtilsNew will make this an Exception (!)

{ "keys": ["ctrl+f"], "command": "setTabColor" },
])" };

const auto bindings0Json = VerifyParseSucceeded(bindings0String);

auto appKeyBindings = winrt::make_self<implementation::AppKeyBindings>();
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<int32_t>('C') };
auto actionAndArgs = TestUtils::GetActionAndArgs(*appKeyBindings, kc);
VERIFY_ARE_EQUAL(ShortcutAction::SetTabColor, actionAndArgs.Action());
const auto& realArgs = actionAndArgs.Args().try_as<SetTabColorArgs>();
VERIFY_IS_NOT_NULL(realArgs);
// Verify the args have the expected value
VERIFY_IS_NULL(realArgs.TabColor());
}
{
KeyChord kc{ true, false, false, static_cast<int32_t>('D') };
auto actionAndArgs = TestUtils::GetActionAndArgs(*appKeyBindings, kc);
VERIFY_ARE_EQUAL(ShortcutAction::SetTabColor, actionAndArgs.Action());
const auto& realArgs = actionAndArgs.Args().try_as<SetTabColorArgs>();
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<uint32_t>(til::color(0x563412)), realArgs.TabColor().Value());
}
{
KeyChord kc{ true, false, false, static_cast<int32_t>('E') };
auto actionAndArgs = TestUtils::GetActionAndArgs(*appKeyBindings, kc);
VERIFY_ARE_EQUAL(ShortcutAction::SetTabColor, actionAndArgs.Action());
const auto& realArgs = actionAndArgs.Args().try_as<SetTabColorArgs>();
VERIFY_IS_NOT_NULL(realArgs);
// Verify the args have the expected value
VERIFY_IS_NULL(realArgs.TabColor());
}
{
KeyChord kc{ true, false, false, static_cast<int32_t>('F') };
auto actionAndArgs = TestUtils::GetActionAndArgs(*appKeyBindings, kc);
VERIFY_ARE_EQUAL(ShortcutAction::SetTabColor, actionAndArgs.Action());
const auto& realArgs = actionAndArgs.Args().try_as<SetTabColorArgs>();
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"([
Expand Down
6 changes: 6 additions & 0 deletions src/cascadia/TerminalApp/ActionAndArgs.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 }
Expand Down Expand Up @@ -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 },
Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalApp/ActionArgs.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,5 @@
#include "AdjustFontSizeArgs.g.cpp"
#include "SplitPaneArgs.g.cpp"
#include "OpenSettingsArgs.g.cpp"
#include "SetTabColorArgs.g.cpp"
#include "RenameTabArgs.g.cpp"
37 changes: 37 additions & 0 deletions src/cascadia/TerminalApp/ActionArgs.h
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -443,6 +445,41 @@ namespace winrt::TerminalApp::implementation
}
};

struct SetTabColorArgs : public SetTabColorArgsT<SetTabColorArgs>
{
SetTabColorArgs() = default;
GETSET_PROPERTY(Windows::Foundation::IReference<uint32_t>, TabColor, nullptr);

static constexpr std::string_view ColorKey{ "color" };

public:
bool Equals(const IActionArgs& other)
{
auto otherAsUs = other.try_as<SetTabColorArgs>();
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<SetTabColorArgs>();
std::optional<til::color> temp;
try
{
::TerminalApp::JsonUtils::GetOptionalColor(json, ColorKey, temp);
if (temp.has_value())
{
args->_TabColor = static_cast<uint32_t>(temp.value());
}
}
CATCH_LOG();
return { *args, {} };
}
};

struct RenameTabArgs : public RenameTabArgsT<RenameTabArgs>
{
RenameTabArgs() = default;
Expand Down
5 changes: 5 additions & 0 deletions src/cascadia/TerminalApp/ActionArgs.idl
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,11 @@ namespace TerminalApp
SettingsTarget Target { get; };
};

[default_interface] runtimeclass SetTabColorArgs : IActionArgs
{
Windows.Foundation.IReference<UInt32> TabColor { get; };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ugh uint32 this is gonna make JsonUtilsNew fun 😦

};

[default_interface] runtimeclass RenameTabArgs : IActionArgs
{
String Title { get; };
Expand Down
40 changes: 39 additions & 1 deletion src/cascadia/TerminalApp/AppActionHandlers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,45 @@ namespace winrt::TerminalApp::implementation
args.Handled(true);
}

void TerminalPage::_HandleSetTabColor(const IInspectable& /*sender*/,
const TerminalApp::ActionEventArgs& args)
{
std::optional<til::color> tabColor;

if (const auto& realArgs = args.ActionArgs().try_as<TerminalApp::SetTabColorArgs>())
{
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)
{
Expand All @@ -262,5 +301,4 @@ namespace winrt::TerminalApp::implementation
}
args.Handled(true);
}

}
10 changes: 10 additions & 0 deletions src/cascadia/TerminalApp/ShortcutActionDispatch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
2 changes: 2 additions & 0 deletions src/cascadia/TerminalApp/ShortcutActionDispatch.h
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
8 changes: 4 additions & 4 deletions src/cascadia/TerminalApp/ShortcutActionDispatch.idl
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -35,6 +32,8 @@ namespace TerminalApp
MoveFocus,
Find,
ToggleFullscreen,
SetTabColor,
OpenTabColorPicker,
OpenSettings,
RenameTab
};
Expand Down Expand Up @@ -74,7 +73,8 @@ namespace TerminalApp
event Windows.Foundation.TypedEventHandler<ShortcutActionDispatch, ActionEventArgs> Find;
event Windows.Foundation.TypedEventHandler<ShortcutActionDispatch, ActionEventArgs> MoveFocus;
event Windows.Foundation.TypedEventHandler<ShortcutActionDispatch, ActionEventArgs> ToggleFullscreen;
event Windows.Foundation.TypedEventHandler<ShortcutActionDispatch, ActionEventArgs> SetTabColor;
event Windows.Foundation.TypedEventHandler<ShortcutActionDispatch, ActionEventArgs> OpenTabColorPicker;
event Windows.Foundation.TypedEventHandler<ShortcutActionDispatch, ActionEventArgs> RenameTab;

}
}
21 changes: 16 additions & 5 deletions src/cascadia/TerminalApp/Tab.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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"));
Expand All @@ -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();
}
});

Expand Down Expand Up @@ -718,7 +718,7 @@ namespace winrt::TerminalApp::implementation
// - color: the shiny color the user picked for their tab
// Return Value:
// - <none>
void Tab::_SetTabColor(const winrt::Windows::UI::Color& color)
void Tab::SetTabColor(const winrt::Windows::UI::Color& color)
{
auto weakThis{ get_weak() };

Expand Down Expand Up @@ -779,7 +779,7 @@ namespace winrt::TerminalApp::implementation
// - <none>
// Return Value:
// - <none>
void Tab::_ResetTabColor()
void Tab::ResetTabColor()
{
auto weakThis{ get_weak() };

Expand Down Expand Up @@ -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:
// - <none>
// Return Value:
// - <none>
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
Expand Down
Loading