Skip to content

Commit

Permalink
Add a 'splitPane' ShortcutAction (#3722)
Browse files Browse the repository at this point in the history
## Summary of the Pull Request

We already have "splitHorizontal" and "splitVertical", but those will both be deprecated in favor of "splitPane" with arguments. 

Currently, there's one argument: "style", which is one of "vertical" or "horizontal."

## References
This is being done in pursuit of supporting #607 and #998. I don't really want to lob #998 in with this one, since both that and this are hefty enough PRs even as they are. (I have a branch for #998, but it needs this first)

This will probably conflict with #3658
## PR Checklist
* [ ] Doesn't actually close anything, only enables #998
* [x] I work here
* [ ] Tests added/passed - yea okay no excuses here
* [x] Requires documentation to be updated

## Validation Steps Performed
Added new keybindings with the args - works
Tried the old keybindings without the args - still works
---------------------------------------
* Add a 'splitPane' keybinding that can be used for splitting a pane either vertically or horizontally

* Update documentation too

* Good lord this is important

* Add a test too, though I have no idea if it works

* "style" -> "split"

* pr comments from carlos
  • Loading branch information
zadjii-msft authored Nov 28, 2019
1 parent 111b88c commit eed351e
Show file tree
Hide file tree
Showing 17 changed files with 246 additions and 80 deletions.
3 changes: 1 addition & 2 deletions doc/cascadia/SettingsSchema.md
Original file line number Diff line number Diff line change
Expand Up @@ -127,8 +127,7 @@ Bindings listed below are per the implementation in `src/cascadia/TerminalApp/Ap
- switchToTab7
- switchToTab8
- openSettings
- splitHorizontal
- splitVertical
- splitPane
- resizePaneLeft
- resizePaneRight
- resizePaneUp
Expand Down
26 changes: 26 additions & 0 deletions doc/cascadia/profiles.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@
"scrollUpPage",
"splitHorizontal",
"splitVertical",
"splitPane",
"switchToTab",
"switchToTab0",
"switchToTab1",
Expand All @@ -79,6 +80,13 @@
],
"type": "string"
},
"SplitState": {
"enum": [
"vertical",
"horizontal"
],
"type": "string"
},
"ShortcutAction": {
"properties": {
"action": {
Expand Down Expand Up @@ -173,6 +181,23 @@
],
"required": [ "direction" ]
},
"SplitPaneAction": {
"description": "Arguments corresponding to a Split Pane Action",
"allOf": [
{ "$ref": "#/definitions/ShortcutAction" },
{
"properties": {
"action": { "type": "string", "pattern": "splitPane" },
"split": {
"$ref": "#/definitions/SplitState",
"default": "vertical",
"description": "The orientation to split the pane in, either vertical (think [|]) or horizontal (think [-])"
}
}
}
],
"required": [ "split" ]
},
"Keybinding": {
"additionalProperties": false,
"properties": {
Expand All @@ -185,6 +210,7 @@
{ "$ref": "#/definitions/SwitchToTabAction" },
{ "$ref": "#/definitions/MoveFocusAction" },
{ "$ref": "#/definitions/ResizePaneAction" }
{ "$ref": "#/definitions/SplitPaneAction" }
]
},
"keys": {
Expand Down
91 changes: 91 additions & 0 deletions src/cascadia/LocalTests_TerminalApp/KeyBindingsTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ namespace TerminalAppLocalTests
TEST_METHOD(UnbindKeybindings);

TEST_METHOD(TestArbitraryArgs);
TEST_METHOD(TestSplitPaneArgs);

TEST_CLASS_SETUP(ClassSetup)
{
Expand Down Expand Up @@ -365,4 +366,94 @@ namespace TerminalAppLocalTests
}
}

void KeyBindingsTests::TestSplitPaneArgs()
{
// TODO:GH#3536 - These tests _should_ work, but since the LocalTests
// fail to run at all right now, I can't be sure that they do. When
// #3536 is fixed, make sure that these tests were authored correctly.

const std::string bindings0String{ R"([
{ "command": "splitVertical", "keys": ["ctrl+a"] },
{ "command": "splitHorizontal", "keys": ["ctrl+b"] },
{ "command": { "action": "splitPane", "split": null }, "keys": ["ctrl+c"] },
{ "command": { "action": "splitPane", "split": "vertical" }, "keys": ["ctrl+d"] },
{ "command": { "action": "splitPane", "split": "horizontal" }, "keys": ["ctrl+e"] },
{ "command": { "action": "splitPane", "split": "none" }, "keys": ["ctrl+f"] },
{ "command": { "action": "splitPane" }, "keys": ["ctrl+g"] }
])" };

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(7u, appKeyBindings->_keyShortcuts.size());

{
KeyChord kc{ true, false, true, static_cast<int32_t>('A') };
auto actionAndArgs = GetActionAndArgs(*appKeyBindings, kc);
VERIFY_ARE_EQUAL(ShortcutAction::SplitPane, actionAndArgs.Action());
const auto& realArgs = actionAndArgs.Args().try_as<SplitPaneArgs>();
VERIFY_IS_NOT_NULL(realArgs);
// Verify the args have the expected value
VERIFY_ARE_EQUAL(winrt::TerminalApp::SplitState::Vertical, realArgs.SplitStyle());
}
{
KeyChord kc{ true, false, true, static_cast<int32_t>('B') };
auto actionAndArgs = GetActionAndArgs(*appKeyBindings, kc);
VERIFY_ARE_EQUAL(ShortcutAction::SplitPane, actionAndArgs.Action());
const auto& realArgs = actionAndArgs.Args().try_as<SplitPaneArgs>();
VERIFY_IS_NOT_NULL(realArgs);
// Verify the args have the expected value
VERIFY_ARE_EQUAL(winrt::TerminalApp::SplitState::Horizontal, realArgs.SplitStyle());
}
{
KeyChord kc{ true, false, true, static_cast<int32_t>('C') };
auto actionAndArgs = GetActionAndArgs(*appKeyBindings, kc);
VERIFY_ARE_EQUAL(ShortcutAction::SplitPane, actionAndArgs.Action());
const auto& realArgs = actionAndArgs.Args().try_as<SplitPaneArgs>();
VERIFY_IS_NOT_NULL(realArgs);
// Verify the args have the expected value
VERIFY_ARE_EQUAL(winrt::TerminalApp::SplitState::None, realArgs.SplitStyle());
}
{
KeyChord kc{ true, false, true, static_cast<int32_t>('D') };
auto actionAndArgs = GetActionAndArgs(*appKeyBindings, kc);
VERIFY_ARE_EQUAL(ShortcutAction::SplitPane, actionAndArgs.Action());
const auto& realArgs = actionAndArgs.Args().try_as<SplitPaneArgs>();
VERIFY_IS_NOT_NULL(realArgs);
// Verify the args have the expected value
VERIFY_ARE_EQUAL(winrt::TerminalApp::SplitState::Vertical, realArgs.SplitStyle());
}
{
KeyChord kc{ true, false, true, static_cast<int32_t>('E') };
auto actionAndArgs = GetActionAndArgs(*appKeyBindings, kc);
VERIFY_ARE_EQUAL(ShortcutAction::SplitPane, actionAndArgs.Action());
const auto& realArgs = actionAndArgs.Args().try_as<SplitPaneArgs>();
VERIFY_IS_NOT_NULL(realArgs);
// Verify the args have the expected value
VERIFY_ARE_EQUAL(winrt::TerminalApp::SplitState::Horizontal, realArgs.SplitStyle());
}
{
KeyChord kc{ true, false, true, static_cast<int32_t>('F') };
auto actionAndArgs = GetActionAndArgs(*appKeyBindings, kc);
VERIFY_ARE_EQUAL(ShortcutAction::SplitPane, actionAndArgs.Action());
const auto& realArgs = actionAndArgs.Args().try_as<SplitPaneArgs>();
VERIFY_IS_NOT_NULL(realArgs);
// Verify the args have the expected value
VERIFY_ARE_EQUAL(winrt::TerminalApp::SplitState::None, realArgs.SplitStyle());
}
{
KeyChord kc{ true, false, true, static_cast<int32_t>('G') };
auto actionAndArgs = GetActionAndArgs(*appKeyBindings, kc);
VERIFY_ARE_EQUAL(ShortcutAction::SplitPane, actionAndArgs.Action());
const auto& realArgs = actionAndArgs.Args().try_as<SplitPaneArgs>();
VERIFY_IS_NOT_NULL(realArgs);
// Verify the args have the expected value
VERIFY_ARE_EQUAL(winrt::TerminalApp::SplitState::None, realArgs.SplitStyle());
}
}

}
1 change: 1 addition & 0 deletions src/cascadia/TerminalApp/ActionArgs.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -12,3 +12,4 @@
#include "ResizePaneArgs.g.cpp"
#include "MoveFocusArgs.g.cpp"
#include "AdjustFontSizeArgs.g.cpp"
#include "SplitPaneArgs.g.cpp"
48 changes: 48 additions & 0 deletions src/cascadia/TerminalApp/ActionArgs.h
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include "ResizePaneArgs.g.h"
#include "MoveFocusArgs.g.h"
#include "AdjustFontSizeArgs.g.h"
#include "SplitPaneArgs.g.h"

#include "../../cascadia/inc/cppwinrt_utils.h"
#include "Utils.h"
Expand Down Expand Up @@ -241,6 +242,53 @@ namespace winrt::TerminalApp::implementation
return *args;
}
};

// Possible SplitState values
// TODO:GH#2550/#3475 - move these to a centralized deserializing place
static constexpr std::string_view VerticalKey{ "vertical" };
static constexpr std::string_view HorizontalKey{ "horizontal" };
static TerminalApp::SplitState ParseSplitState(const std::string& stateString)
{
if (stateString == VerticalKey)
{
return TerminalApp::SplitState::Vertical;
}
else if (stateString == HorizontalKey)
{
return TerminalApp::SplitState::Horizontal;
}
// default behavior for invalid data
return TerminalApp::SplitState::None;
};

struct SplitPaneArgs : public SplitPaneArgsT<SplitPaneArgs>
{
SplitPaneArgs() = default;
GETSET_PROPERTY(winrt::TerminalApp::SplitState, SplitStyle, winrt::TerminalApp::SplitState::None);

static constexpr std::string_view SplitKey{ "split" };

public:
bool Equals(const IActionArgs& other)
{
auto otherAsUs = other.try_as<SplitPaneArgs>();
if (otherAsUs)
{
return otherAsUs->_SplitStyle == _SplitStyle;
}
return false;
};
static winrt::TerminalApp::IActionArgs FromJson(const Json::Value& json)
{
// LOAD BEARING: Not using make_self here _will_ break you in the future!
auto args = winrt::make_self<SplitPaneArgs>();
if (auto jsonStyle{ json[JsonKey(SplitKey)] })
{
args->_SplitStyle = ParseSplitState(jsonStyle.asString());
}
return *args;
}
};
}

namespace winrt::TerminalApp::factory_implementation
Expand Down
11 changes: 11 additions & 0 deletions src/cascadia/TerminalApp/ActionArgs.idl
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,13 @@ namespace TerminalApp
Down
};

enum SplitState
{
None = 0,
Vertical = 1,
Horizontal = 2
};

[default_interface] runtimeclass ActionEventArgs : IActionEventArgs
{
ActionEventArgs(IActionArgs args);
Expand Down Expand Up @@ -60,4 +67,8 @@ namespace TerminalApp
Int32 Delta { get; };
};

[default_interface] runtimeclass SplitPaneArgs : IActionArgs
{
SplitState SplitStyle { get; };
};
}
22 changes: 11 additions & 11 deletions src/cascadia/TerminalApp/AppActionHandlers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -89,18 +89,18 @@ namespace winrt::TerminalApp::implementation
args.Handled(true);
}

void TerminalPage::_HandleSplitVertical(const IInspectable& /*sender*/,
const TerminalApp::ActionEventArgs& args)
{
_SplitVertical(std::nullopt);
args.Handled(true);
}

void TerminalPage::_HandleSplitHorizontal(const IInspectable& /*sender*/,
const TerminalApp::ActionEventArgs& args)
void TerminalPage::_HandleSplitPane(const IInspectable& /*sender*/,
const TerminalApp::ActionEventArgs& args)
{
_SplitHorizontal(std::nullopt);
args.Handled(true);
if (args == nullptr)
{
args.Handled(false);
}
else if (const auto& realArgs = args.ActionArgs().try_as<TerminalApp::SplitPaneArgs>())
{
_SplitPane(realArgs.SplitStyle(), std::nullopt);
args.Handled(true);
}
}

void TerminalPage::_HandleScrollUpPage(const IInspectable& /*sender*/,
Expand Down
31 changes: 29 additions & 2 deletions src/cascadia/TerminalApp/AppKeyBindingsSerialization.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,9 @@ static constexpr std::string_view SwitchToTab6Key{ "switchToTab6" }; // Legacy
static constexpr std::string_view SwitchToTab7Key{ "switchToTab7" }; // Legacy
static constexpr std::string_view SwitchToTab8Key{ "switchToTab8" }; // Legacy
static constexpr std::string_view OpenSettingsKey{ "openSettings" }; // Legacy
static constexpr std::string_view SplitHorizontalKey{ "splitHorizontal" };
static constexpr std::string_view SplitVerticalKey{ "splitVertical" };
static constexpr std::string_view SplitPaneKey{ "splitPane" };
static constexpr std::string_view SplitHorizontalKey{ "splitHorizontal" }; // Legacy
static constexpr std::string_view SplitVerticalKey{ "splitVertical" }; // Legacy
static constexpr std::string_view ResizePaneKey{ "resizePane" };
static constexpr std::string_view ResizePaneLeftKey{ "resizePaneLeft" }; // Legacy
static constexpr std::string_view ResizePaneRightKey{ "resizePaneRight" }; // Legacy
Expand Down Expand Up @@ -139,9 +140,31 @@ static const std::map<std::string_view, ShortcutAction, std::less<>> commandName
{ MoveFocusDownKey, ShortcutAction::MoveFocusDown },
{ OpenSettingsKey, ShortcutAction::OpenSettings },
{ ToggleFullscreenKey, ShortcutAction::ToggleFullscreen },
{ SplitPaneKey, ShortcutAction::SplitPane },
{ UnboundKey, ShortcutAction::Invalid },
};

// Function Description:
// - Creates a function that can be used to generate a SplitPaneArgs for the
// legacy Split[SplitState] actions. These actions don't accept args from
// json, instead, they just return a SplitPaneArgs with the style already
// pre-defined, based on the input param.
// - TODO: GH#1069 Remove this before 1.0, and force an upgrade to the new args.
// Arguments:
// - style: the split style to create the parse function for.
// Return Value:
// - A function that can be used to "parse" json into one of the legacy
// Split[SplitState] args.
std::function<IActionArgs(const Json::Value&)> LegacyParseSplitPaneArgs(SplitState style)
{
auto pfn = [style](const Json::Value & /*value*/) -> IActionArgs {
auto args = winrt::make_self<winrt::TerminalApp::implementation::SplitPaneArgs>();
args->SplitStyle(style);
return *args;
};
return pfn;
}

// Function Description:
// - Creates a function that can be used to generate a MoveFocusArgs for the
// legacy MoveFocus[Direction] actions. These actions don't accept args from
Expand Down Expand Up @@ -305,6 +328,10 @@ static const std::map<ShortcutAction, std::function<IActionArgs(const Json::Valu
{ ShortcutAction::DecreaseFontSize, LegacyParseAdjustFontSizeArgs(-1) },
{ ShortcutAction::IncreaseFontSize, LegacyParseAdjustFontSizeArgs(1) },

{ ShortcutAction::SplitPane, winrt::TerminalApp::implementation::SplitPaneArgs::FromJson },
{ ShortcutAction::SplitVertical, LegacyParseSplitPaneArgs(SplitState::Vertical) },
{ ShortcutAction::SplitHorizontal, LegacyParseSplitPaneArgs(SplitState::Horizontal) },

{ ShortcutAction::Invalid, nullptr },
};

Expand Down
Loading

0 comments on commit eed351e

Please sign in to comment.