Skip to content

Commit

Permalink
Move Pane to Tab (GH7075) (#10780)
Browse files Browse the repository at this point in the history
<!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? -->
## Summary of the Pull Request
Add functionality to move a pane to another tab. If the tab index is greater than the number of current tabs a new tab will be created with the pane as its root. Similarly, if the last pane on a tab is moved to another tab, the original tab will be closed.

This is largely complete, but I know that I'm messing around with things that I am unfamiliar with, and would like to avoid footguns where possible. 

<!-- Other than the issue solved, is this relevant to any other issues/existing PRs? --> 
## References
#4587 

<!-- Please review the items on the PR checklist before submitting-->
## PR Checklist
* [x] Closes #7075
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [ ] Tests added/passed
* [x] Documentation updated. If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/terminal) and link it here: #xxx
* [x] Schema updated.
* [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #xxx

<!-- Provide a more detailed description of the PR, other things fixed or any additional comments/features here -->
## Detailed Description of the Pull Request / Additional comments
Things done:
- Moving a pane to a new tab appears to work. Moving a pane to an existing tab mostly works. Moving a pane back to its original tab appears to work.
- Set up {Attach,Detach}Pane methods to add or remove a pane from a pane. Detach is slightly different than Close in that we want to persist the tree structure and terminal controls.
- Add `Detached` event on a pane that can be subscribed to to remove other event handlers if desired. 
- Added simple WalkTree abstraction for one-off recursion use cases that calls a provided function on each pane in order (and optionally terminates early).
- Fixed an in-prod bug with closing panes. Specifically, if you have a tree (1; 2 3) and close the 1 pane, then 3 will lose its borders because of these lines clearing the border on both children https://github.com/microsoft/terminal/blob/main/src/cascadia/TerminalApp/Pane.cpp#L1197-L1201 .

To do:
- Right now I have `TerminalTab` as a friend class of `Pane` so I can access some extra properties in my `WalkTree` callbacks, but there is probably a better choice for the abstraction boundary.

Next Steps:
- In a future PR Drag & Drop handlers could be added that utilize the Attach/Detach infrastructure to provide a better UI.
- Similarly once this is working, it should be possible to convert an entire tab into a pane on an existing tab (Tab::DetachRoot on original tab followed by Tab::AttachPane on the target tab).
- Its been 10 years, I just really want to use concepts already.

<!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well -->
## Validation Steps Performed
Manual testing by creating pane(s), and moving them between tabs and creating new tabs and destroying tabs by moving the last remaining pane.
  • Loading branch information
Rosefield authored Aug 12, 2021
1 parent d465a47 commit 9eb9bc9
Show file tree
Hide file tree
Showing 22 changed files with 805 additions and 218 deletions.
1 change: 1 addition & 0 deletions .github/actions/spelling/allow/apis.txt
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ localtime
lround
LSHIFT
memicmp
mptt
mov
msappx
MULTIPLEUSE
Expand Down
25 changes: 22 additions & 3 deletions doc/cascadia/profiles.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,7 @@
"identifyWindows",
"moveFocus",
"movePane",
"swapPane",
"moveTab",
"newTab",
"newWindow",
Expand Down Expand Up @@ -493,6 +494,23 @@
"type": "integer",
"default": 0,
"description": "Which tab to switch to, with the first being 0"
}
}
}
],
"required": [ "index" ]
},
"MovePaneAction": {
"description": "Arguments corresponding to a Move Pane Action",
"allOf": [
{ "$ref": "#/definitions/ShortcutAction" },
{
"properties": {
"action": { "type": "string", "pattern": "movePane" },
"index": {
"type": "integer",
"default": 0,
"description": "Which tab to move the pane to, with the first being 0"
}
}
}
Expand All @@ -516,13 +534,13 @@
],
"required": [ "direction" ]
},
"MovePaneAction": {
"description": "Arguments corresponding to a Move Pane Action",
"SwapPaneAction": {
"description": "Arguments corresponding to a Swap Pane Action",
"allOf": [
{ "$ref": "#/definitions/ShortcutAction" },
{
"properties": {
"action": { "type": "string", "pattern": "movePane" },
"action": { "type": "string", "pattern": "swapPane" },
"direction": {
"$ref": "#/definitions/FocusDirection",
"default": "left",
Expand Down Expand Up @@ -972,6 +990,7 @@
{ "$ref": "#/definitions/SwitchToTabAction" },
{ "$ref": "#/definitions/MoveFocusAction" },
{ "$ref": "#/definitions/MovePaneAction" },
{ "$ref": "#/definitions/SwapPaneAction" },
{ "$ref": "#/definitions/ResizePaneAction" },
{ "$ref": "#/definitions/SendInputAction" },
{ "$ref": "#/definitions/SplitPaneAction" },
Expand Down
35 changes: 15 additions & 20 deletions src/cascadia/LocalTests_TerminalApp/CommandlineTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ namespace TerminalAppLocalTests
TEST_METHOD(ParseComboCommandlineIntoArgs);
TEST_METHOD(ParseFocusTabArgs);
TEST_METHOD(ParseMoveFocusArgs);
TEST_METHOD(ParseMovePaneArgs);
TEST_METHOD(ParseSwapPaneArgs);
TEST_METHOD(ParseArgumentsWithParsingTerminators);
TEST_METHOD(ParseFocusPaneArgs);

Expand Down Expand Up @@ -1208,14 +1208,9 @@ namespace TerminalAppLocalTests
}
}

void CommandlineTest::ParseMovePaneArgs()
void CommandlineTest::ParseSwapPaneArgs()
{
BEGIN_TEST_METHOD_PROPERTIES()
TEST_METHOD_PROPERTY(L"Data:useShortForm", L"{false, true}")
END_TEST_METHOD_PROPERTIES()

INIT_TEST_PROPERTY(bool, useShortForm, L"If true, use `mp` instead of `move-pane`");
const wchar_t* subcommand = useShortForm ? L"mp" : L"move-pane";
const wchar_t* subcommand = L"swap-pane";

{
AppCommandlineArgs appArgs{};
Expand All @@ -1236,9 +1231,9 @@ namespace TerminalAppLocalTests
VERIFY_ARE_EQUAL(ShortcutAction::NewTab, appArgs._startupActions.at(0).Action());

auto actionAndArgs = appArgs._startupActions.at(1);
VERIFY_ARE_EQUAL(ShortcutAction::MovePane, actionAndArgs.Action());
VERIFY_ARE_EQUAL(ShortcutAction::SwapPane, actionAndArgs.Action());
VERIFY_IS_NOT_NULL(actionAndArgs.Args());
auto myArgs = actionAndArgs.Args().try_as<MovePaneArgs>();
auto myArgs = actionAndArgs.Args().try_as<SwapPaneArgs>();
VERIFY_IS_NOT_NULL(myArgs);
VERIFY_ARE_EQUAL(FocusDirection::Left, myArgs.Direction());
}
Expand All @@ -1253,9 +1248,9 @@ namespace TerminalAppLocalTests
VERIFY_ARE_EQUAL(ShortcutAction::NewTab, appArgs._startupActions.at(0).Action());

auto actionAndArgs = appArgs._startupActions.at(1);
VERIFY_ARE_EQUAL(ShortcutAction::MovePane, actionAndArgs.Action());
VERIFY_ARE_EQUAL(ShortcutAction::SwapPane, actionAndArgs.Action());
VERIFY_IS_NOT_NULL(actionAndArgs.Args());
auto myArgs = actionAndArgs.Args().try_as<MovePaneArgs>();
auto myArgs = actionAndArgs.Args().try_as<SwapPaneArgs>();
VERIFY_IS_NOT_NULL(myArgs);
VERIFY_ARE_EQUAL(FocusDirection::Right, myArgs.Direction());
}
Expand All @@ -1270,9 +1265,9 @@ namespace TerminalAppLocalTests
VERIFY_ARE_EQUAL(ShortcutAction::NewTab, appArgs._startupActions.at(0).Action());

auto actionAndArgs = appArgs._startupActions.at(1);
VERIFY_ARE_EQUAL(ShortcutAction::MovePane, actionAndArgs.Action());
VERIFY_ARE_EQUAL(ShortcutAction::SwapPane, actionAndArgs.Action());
VERIFY_IS_NOT_NULL(actionAndArgs.Args());
auto myArgs = actionAndArgs.Args().try_as<MovePaneArgs>();
auto myArgs = actionAndArgs.Args().try_as<SwapPaneArgs>();
VERIFY_IS_NOT_NULL(myArgs);
VERIFY_ARE_EQUAL(FocusDirection::Up, myArgs.Direction());
}
Expand All @@ -1287,9 +1282,9 @@ namespace TerminalAppLocalTests
VERIFY_ARE_EQUAL(ShortcutAction::NewTab, appArgs._startupActions.at(0).Action());

auto actionAndArgs = appArgs._startupActions.at(1);
VERIFY_ARE_EQUAL(ShortcutAction::MovePane, actionAndArgs.Action());
VERIFY_ARE_EQUAL(ShortcutAction::SwapPane, actionAndArgs.Action());
VERIFY_IS_NOT_NULL(actionAndArgs.Args());
auto myArgs = actionAndArgs.Args().try_as<MovePaneArgs>();
auto myArgs = actionAndArgs.Args().try_as<SwapPaneArgs>();
VERIFY_IS_NOT_NULL(myArgs);
VERIFY_ARE_EQUAL(FocusDirection::Down, myArgs.Direction());
}
Expand All @@ -1311,16 +1306,16 @@ namespace TerminalAppLocalTests
VERIFY_ARE_EQUAL(ShortcutAction::NewTab, appArgs._startupActions.at(0).Action());

auto actionAndArgs = appArgs._startupActions.at(1);
VERIFY_ARE_EQUAL(ShortcutAction::MovePane, actionAndArgs.Action());
VERIFY_ARE_EQUAL(ShortcutAction::SwapPane, actionAndArgs.Action());
VERIFY_IS_NOT_NULL(actionAndArgs.Args());
auto myArgs = actionAndArgs.Args().try_as<MovePaneArgs>();
auto myArgs = actionAndArgs.Args().try_as<SwapPaneArgs>();
VERIFY_IS_NOT_NULL(myArgs);
VERIFY_ARE_EQUAL(FocusDirection::Left, myArgs.Direction());

actionAndArgs = appArgs._startupActions.at(2);
VERIFY_ARE_EQUAL(ShortcutAction::MovePane, actionAndArgs.Action());
VERIFY_ARE_EQUAL(ShortcutAction::SwapPane, actionAndArgs.Action());
VERIFY_IS_NOT_NULL(actionAndArgs.Args());
myArgs = actionAndArgs.Args().try_as<MovePaneArgs>();
myArgs = actionAndArgs.Args().try_as<SwapPaneArgs>();
VERIFY_IS_NOT_NULL(myArgs);
VERIFY_ARE_EQUAL(FocusDirection::Right, myArgs.Direction());
}
Expand Down
20 changes: 10 additions & 10 deletions src/cascadia/LocalTests_TerminalApp/TabTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ namespace TerminalAppLocalTests
TEST_METHOD(MoveFocusFromZoomedPane);
TEST_METHOD(CloseZoomedPane);

TEST_METHOD(MovePanes);
TEST_METHOD(SwapPanes);

TEST_METHOD(NextMRUTab);
TEST_METHOD(VerifyCommandPaletteTabSwitcherOrder);
Expand Down Expand Up @@ -821,7 +821,7 @@ namespace TerminalAppLocalTests
VERIFY_SUCCEEDED(result);
}

void TabTests::MovePanes()
void TabTests::SwapPanes()
{
auto page = _commonSetup();

Expand Down Expand Up @@ -914,10 +914,10 @@ namespace TerminalAppLocalTests
// -------------------
TestOnUIThread([&]() {
// Set up action
MovePaneArgs args{ FocusDirection::Left };
SwapPaneArgs args{ FocusDirection::Left };
ActionEventArgs eventArgs{ args };

page->_HandleMovePane(nullptr, eventArgs);
page->_HandleSwapPane(nullptr, eventArgs);
});

Sleep(250);
Expand Down Expand Up @@ -945,10 +945,10 @@ namespace TerminalAppLocalTests
// -------------------
TestOnUIThread([&]() {
// Set up action
MovePaneArgs args{ FocusDirection::Up };
SwapPaneArgs args{ FocusDirection::Up };
ActionEventArgs eventArgs{ args };

page->_HandleMovePane(nullptr, eventArgs);
page->_HandleSwapPane(nullptr, eventArgs);
});

Sleep(250);
Expand Down Expand Up @@ -976,10 +976,10 @@ namespace TerminalAppLocalTests
// -------------------
TestOnUIThread([&]() {
// Set up action
MovePaneArgs args{ FocusDirection::Right };
SwapPaneArgs args{ FocusDirection::Right };
ActionEventArgs eventArgs{ args };

page->_HandleMovePane(nullptr, eventArgs);
page->_HandleSwapPane(nullptr, eventArgs);
});

Sleep(250);
Expand Down Expand Up @@ -1007,10 +1007,10 @@ namespace TerminalAppLocalTests
// -------------------
TestOnUIThread([&]() {
// Set up action
MovePaneArgs args{ FocusDirection::Down };
SwapPaneArgs args{ FocusDirection::Down };
ActionEventArgs eventArgs{ args };

page->_HandleMovePane(nullptr, eventArgs);
page->_HandleSwapPane(nullptr, eventArgs);
});

Sleep(250);
Expand Down
20 changes: 17 additions & 3 deletions src/cascadia/TerminalApp/AppActionHandlers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,20 @@ namespace winrt::TerminalApp::implementation
}
}

void TerminalPage::_HandleMovePane(const IInspectable& /*sender*/,
const ActionEventArgs& args)
{
if (args == nullptr)
{
args.Handled(false);
}
else if (const auto& realArgs = args.ActionArgs().try_as<MovePaneArgs>())
{
auto moved = _MovePane(realArgs.TabIndex());
args.Handled(moved);
}
}

void TerminalPage::_HandleSplitPane(const IInspectable& /*sender*/,
const ActionEventArgs& args)
{
Expand Down Expand Up @@ -323,10 +337,10 @@ namespace winrt::TerminalApp::implementation
}
}

void TerminalPage::_HandleMovePane(const IInspectable& /*sender*/,
void TerminalPage::_HandleSwapPane(const IInspectable& /*sender*/,
const ActionEventArgs& args)
{
if (const auto& realArgs = args.ActionArgs().try_as<MovePaneArgs>())
if (const auto& realArgs = args.ActionArgs().try_as<SwapPaneArgs>())
{
if (realArgs.Direction() == FocusDirection::None)
{
Expand All @@ -335,7 +349,7 @@ namespace winrt::TerminalApp::implementation
}
else
{
_MovePane(realArgs.Direction());
_SwapPane(realArgs.Direction());
args.Handled(true);
}
}
Expand Down
Loading

0 comments on commit 9eb9bc9

Please sign in to comment.