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

Move Pane to Tab (GH7075) #10780

Merged
24 commits merged into from
Aug 12, 2021
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
0fb4b38
first very-much-not-working pass. Need to figure out how to manage al…
Rosefield Jul 20, 2021
4adfdee
GH7075 Move Pane to Tab
Rosefield Jul 24, 2021
e33b7aa
Remove control event handlers when pane is detached. Make sure event …
Rosefield Jul 25, 2021
64346fb
Appease the spelling bot
Rosefield Jul 25, 2021
d66d02e
Flesh out comments on new methods. run code formatter
Rosefield Jul 25, 2021
b8e95e3
Fix typos
Rosefield Jul 25, 2021
3bb24b2
Anticipate some code review refactoring. Make sure that we clean up e…
Rosefield Jul 29, 2021
4077160
Fix duplicated comment. Only say we handled an action if we actually …
Rosefield Jul 29, 2021
a7d7aec
box the event token so that we can pass a reference to it to the even…
Rosefield Aug 1, 2021
e7a9d21
Merge remote-tracking branch 'origin/main' into feature/gh7075-move-p…
Rosefield Aug 1, 2021
eea166c
Merge remote-tracking branch 'origin/main' into feature/gh7075-move-p…
Rosefield Aug 2, 2021
abe6a2b
Fix bug where border is lost when child is closed. Make it explicit w…
Rosefield Aug 2, 2021
530f658
spelling
Rosefield Aug 2, 2021
d1ed59c
Add action to keybindings in schema
Rosefield Aug 4, 2021
23b1ccf
Make separate DetachRoot function so that logic can be used separatel…
Rosefield Aug 4, 2021
ae1a84a
fix comment
Rosefield Aug 5, 2021
ef5089f
dont crash if we try to move a pane to a non-terminal tab
Rosefield Aug 6, 2021
2188f6b
Merge remote-tracking branch 'origin/main' into feature/gh7075-move-p…
Rosefield Aug 7, 2021
c6c9cc1
Rename move-pane -> swap-pane, move-pane-to-tab -> move-pane
Rosefield Aug 10, 2021
012747a
Merge remote-tracking branch 'origin/main' into feature/gh7075-move-p…
Rosefield Aug 10, 2021
4d9e571
formatting
Rosefield Aug 10, 2021
e91c69c
update tests to be swap-pane as well
Rosefield Aug 10, 2021
61a3cae
Make things that can be const, const
Rosefield Aug 12, 2021
1e048e2
Fix crash introduced with last merge; dont try to ask the root pane f…
Rosefield Aug 12, 2021
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/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
18 changes: 18 additions & 0 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",
"movePaneToTab",
"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" ]
},
"MovePaneToTabAction": {
zadjii-msft marked this conversation as resolved.
Show resolved Hide resolved
"description": "Arguments corresponding to a Move Pane To Tab Action",
"allOf": [
{ "$ref": "#/definitions/ShortcutAction" },
{
"properties": {
"action": { "type": "string", "pattern": "movePaneToTab" },
"index": {
"type": "integer",
"default": 0,
"description": "Which tab to move the pane to, with the first being 0"
}
}
}
Expand Down
14 changes: 14 additions & 0 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::_HandleMovePaneToTab(const IInspectable& /*sender*/,
const ActionEventArgs& args)
{
if (args == nullptr)
{
args.Handled(false);
}
else if (const auto& realArgs = args.ActionArgs().try_as<MovePaneToTabArgs>())
{
auto moved = _MovePaneToTab(realArgs.TabIndex());
args.Handled(moved);
}
}

void TerminalPage::_HandleSplitPane(const IInspectable& /*sender*/,
const ActionEventArgs& args)
{
Expand Down
41 changes: 41 additions & 0 deletions src/cascadia/TerminalApp/AppCommandlineArgs.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,7 @@ void AppCommandlineArgs::_buildParser()
// Subcommands
_buildNewTabParser();
_buildSplitPaneParser();
_buildMovePaneToTabParser();
_buildFocusTabParser();
_buildMoveFocusParser();
_buildMovePaneParser();
Expand Down Expand Up @@ -297,6 +298,43 @@ void AppCommandlineArgs::_buildSplitPaneParser()
setupSubcommand(_newPaneCommand);
setupSubcommand(_newPaneShort);
}
// Method Description:
// - Adds the `move-pane-to-tab` subcommand and related options to the commandline parser.
// - Additionally adds the `mptt` subcommand, which is just a shortened version of `move-pane-to-tab`
// Arguments:
// - <none>
// Return Value:
// - <none>
void AppCommandlineArgs::_buildMovePaneToTabParser()
{
_movePaneToTabCommand = _app.add_subcommand("move-pane-to-tab", RS_A(L"CmdMovePaneToTabDesc"));
_movePaneToTabShort = _app.add_subcommand("mptt", RS_A(L"CmdMPTTDesc"));
Copy link
Member

Choose a reason for hiding this comment

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

Alright so this is a wack idea that might need socializing with the rest of the team, but I wonder if it would make sense to just combine this functionality with the move-pane action? Thoughts on the matter:

  • mptt is kinda a black swan here - all the other short forms are two characters.
  • for something like mp -t 0, the long form could actually just be --tab 🤔
  • mp -t 0 left now is a little weird, "move the pane to tab 0 (to the left???)" That doesn't make sense. The two options (direction and --tab) would have to be exclusive
  • It's probably not that insane to have two actions (MovePane and MovePaneToTab) that are exposed through a single commandline interface.
  • (as a follow up idea:) we could do something like move-pane --tab 0 --parent 4 which would move the pane to be a sibling of pane 4 in tab 0.

Copy link
Contributor Author

@Rosefield Rosefield Aug 4, 2021

Choose a reason for hiding this comment

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

Ultimately, I don't have a strong preference either way since I currently don't plan on using the command line version at all. If there is a desired change to the command line parsing I can try to figure out how to implement it.

One of the other things I was thinking about while implementing this is, especially forward looking, that you could have a target pane, and a SplitStyle. E.g. if directional split and drag and drop is added, the underlying command might be like "move pane to tab 2, with target pane 5, split and place on the left side." That is at least a couple of features away, so I don't know how much it makes sense it build that in quite yet. As for the command line interface for such a feature, I'm not sure if it needs to be so involved, but I also just don't know the use case.

I think one of the possible issues is that the naming here is not quite consistent with intent (and I am at fault for that). move-pane actually is swap-panes in that it switches positions of two things, whereas move-pane-to-tab takes the current pane and puts it somewhere else, and otherwise doesn't disrupt the positioning of other panes.

For a last thought, I am comfortable with vim and its notion of commands and movements being separate. Here, commands would be like move-focus, move-pane, move-pane-to-tab, etc, and the movements/targets would be like left or previous or pane with id 1 on tab 3. If everything were to be rearchitected in this style, we could have more of the "command" style things just take Panes as arguments, and then hypothetically a command like move-focus could gain new abilities if we added a first pane or neighbor of parent "movement" (or just be removed outright in favor of focus-pane). Such a change is of course highly subject to opinion and would require more discussion.

Copy link
Member

Choose a reason for hiding this comment

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

You've definitely got some good points here. I think the

move-pane actually is swap-panes in that it switches positions of two things

bit is definitely a specifically good argument. Almost seems like we should rename the existing move-pane to swap-pane, but then that wouldn't be able to have a two-letter short form, so that's annoying.

However, in the team discussion we had, I think the general consensus was to have this be exposed as move-pane --tab <id>. The commandline is kinda it's own UX, and the consensus was that move-pane-to-tab didn't really feel ergonomic. That, and we were thinking about maybe in the future of something like move-pane --window 2 or something like move-pane -w new. (admittedly, that's all waiting on #1256 and #5000). Those commands seemed pretty clear in what they were trying to do, and simpler than the equivalent move-pane-to-window -t 2.

Lemme double check with the tmux man pages. They have pretty good precedent for all this usually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would it be fine to just merge the command line portion and have that call the separate actions, or should there be a higher level of integration?

Copy link
Member

Choose a reason for hiding this comment

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

Okay so taking a look at the man pages:

tmux man page selection
     join-pane [-bdfhv] [-l size] [-s src-pane] [-t dst-pane]
                   (alias: joinp)
             Like split-window, but instead of splitting dst-pane and
             creating a new pane, split it and move src-pane into the
             space.  This can be used to reverse break-pane.  The -b
             option causes src-pane to be joined to left of or above
             dst-pane.
     move-pane [-bdfhv] [-l size] [-s src-pane] [-t dst-pane]
                   (alias: movep)
             Does the same as join-pane.
     select-pane [-DdeLlMmRUZ] [-T title] [-t target-pane]
                   (alias: selectp)
             Make pane target-pane the active pane in its window.  If
             one of -D, -L, -R, or -U is used, respectively the pane
             below, to the left, to the right, or above the target pane
             is used...
     swap-pane [-dDUZ] [-s src-pane] [-t dst-pane]
                   (alias: swapp)
             Swap two panes.  If -U is used and no source pane is
             specified with -s, dst-pane is swapped with the previous
             pane (before it numerically); -D swaps with the next pane
             (after it numerically).

So they definitely don't allow move-pane -t 4 left, because that's nonsensical. "move the pane to tab 4, then move it left"?

Maybe we should follow their lead on this one.

  • swap-pane [up|down|left|right|prev|nextInOrder|prevInOrder]|[-t,--target tgt-pane]
    • This is what we've already got as move-pane, so this would be a rename, but without a short form (sad, but okay)
    • obviously, we'd add the nextInOrder (etc) values after Allow moving through panes in ID order #10909
    • and we'd add the -t argument in another PR, as an exclusive arg with the direction
  • move-pane [-t,--tab index]: move the pane to the given tab. If index is new, then moves it to a new tab.
    • in the future we'd add [-w,--window id] for moving to another window.
    • we could add additional params for like, [-d,--destination pane-id]: split the given destination pane rather than the active pane in the tab

and I suppose with both we could ass a [-s,--source pane-id] arg as well.

I might just be openly brainstorming at this point. What do you think? we haven't shipped move-pane yet, so we're definitely still free to change it if we feel there's a better name

Copy link
Member

Choose a reason for hiding this comment

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

Would it be fine to just merge the command line portion and have that call the separate actions, or should there be a higher level of integration?

Yea we definitely thought it was okay to do that. Having two actions internally for one subcommand is kinda weird, but is definitely easier to deal with logically than trying to combine them into one args blob with optional properties and all that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm happy to rename move-pane to swap-pane and move-pane-to-tab to move-pane, or to just make it all move-pane just let me know. Bit annoying to dig through all the resource files again but its not too bad.

Copy link
Member

Choose a reason for hiding this comment

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

FWIW, the team did feel that we should just do it all on move-pane, I've just been brainstorming myself over here since re-reading this thread this morning.

I'm trying to think of all the moving things we want to do with panes, and coming up with a single command that looks like... this?

move-pane(mp) [--window,-w window-id] [--source,-s src-pane] [--tab,-t index]|[up|down|left|right|prev|nextInOrder|prevInOrder]|[--destination,-d dest-pane]
  • we want to move panes to other tabs
  • we want to move panes to other windows
  • we want to swap a pane with another pane, by ID
  • we want to move a pane in a given direction (we want to swap it in a direction)
  • we want to be able to move the active pane, or a specific pane in a tab

and I think all that together means that we want to

  1. swap with a (source)? and (a tgt or a direction)
  2. move with a (source)? and (a tab)? and (a tgt pane)?

and if these were all on one command, then I don't think it works. We'd need tab and direction to be exclusive to differentiate between 1 and 2. But then direction and dest would need to be exclusive for swap, but not exclusive for move, and now the rules don't work.

So in conclusion, yea:

rename move-pane to swap-pane and move-pane-to-tab to move-pane

(sorry for the runaround on all this!)


auto setupSubcommand = [this](auto* subcommand) {
subcommand->add_option("-t,--target",
_movePaneToTabIndex,
RS_A(L"CmdMovePaneToTabTargetArgDesc"));

// When ParseCommand is called, if this subcommand was provided, this
// callback function will be triggered on the same thread. We can be sure
// that `this` will still be safe - this function just lets us know this
// command was parsed.
subcommand->callback([&, this]() {
// Build the action from the values we've parsed on the commandline.
ActionAndArgs movePaneToTabAction{};

if (_movePaneToTabIndex >= 0)
{
movePaneToTabAction.Action(ShortcutAction::MovePaneToTab);
MovePaneToTabArgs args{ static_cast<unsigned int>(_movePaneToTabIndex) };
movePaneToTabAction.Args(args);
_startupActions.push_back(movePaneToTabAction);
}
});
};
setupSubcommand(_movePaneToTabCommand);
setupSubcommand(_movePaneToTabShort);
}

// Method Description:
// - Adds the `focus-tab` subcommand and related options to the commandline parser.
Expand Down Expand Up @@ -619,6 +657,8 @@ bool AppCommandlineArgs::_noCommandsProvided()
{
return !(*_newTabCommand.subcommand ||
*_newTabShort.subcommand ||
*_movePaneToTabCommand ||
*_movePaneToTabShort ||
*_focusTabCommand ||
*_focusTabShort ||
*_moveFocusCommand ||
Expand Down Expand Up @@ -653,6 +693,7 @@ void AppCommandlineArgs::_resetStateToDefault()
_splitPaneSize = 0.5f;
_splitDuplicate = false;

_movePaneToTabIndex = -1;
_focusTabIndex = -1;
_focusNextTab = false;
_focusPrevTab = false;
Expand Down
4 changes: 4 additions & 0 deletions src/cascadia/TerminalApp/AppCommandlineArgs.h
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,8 @@ class TerminalApp::AppCommandlineArgs final
NewTerminalSubcommand _newTabShort;
NewPaneSubcommand _newPaneCommand;
NewPaneSubcommand _newPaneShort;
CLI::App* _movePaneToTabCommand;
CLI::App* _movePaneToTabShort;
CLI::App* _focusTabCommand;
CLI::App* _focusTabShort;
CLI::App* _moveFocusCommand;
Expand Down Expand Up @@ -107,6 +109,7 @@ class TerminalApp::AppCommandlineArgs final
bool _splitDuplicate{ false };
float _splitPaneSize{ 0.5f };

int _movePaneToTabIndex{ -1 };
int _focusTabIndex{ -1 };
bool _focusNextTab{ false };
bool _focusPrevTab{ false };
Expand All @@ -128,6 +131,7 @@ class TerminalApp::AppCommandlineArgs final
void _addNewTerminalArgs(NewTerminalSubcommand& subcommand);
void _buildParser();
void _buildNewTabParser();
void _buildMovePaneToTabParser();
void _buildSplitPaneParser();
void _buildFocusTabParser();
void _buildMoveFocusParser();
Expand Down
120 changes: 92 additions & 28 deletions src/cascadia/TerminalApp/Pane.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1011,6 +1011,84 @@ void Pane::UpdateSettings(const TerminalSettingsCreateResult& settings, const GU
}
}

// Method Description:
// - Attempts to add the provided pane as a split of the current pane.
// Arguments:
// - pane: the new pane to add
// Return Value:
// - the new reference to the child created from the current pane.
std::shared_ptr<Pane> Pane::AttachPane(std::shared_ptr<Pane> pane, SplitState splitType)
{
// Splice the new pane into the tree
auto [first, _] = _Split(splitType, .5, pane);
Rosefield marked this conversation as resolved.
Show resolved Hide resolved

// If the new pane has a child that was the focus, re-focus it
// to steal focus from the currently focused pane.
if (pane->_HasFocusedChild())
{
pane->WalkTree([](auto p) {
if (p->_lastActive)
{
p->_FocusFirstChild();
return true;
}
return false;
});
}

return first;
}

// Method Description:
// - Attempts to find one of our children matching `id,` if found remove the
// pane from the tree and return it.
// - If the removed pane was (or contained the focus) the first sibling will
// gain focus
// Arguments:
// - The id of the pane to close
// Return Value:
// - The removed pane, if found.
std::shared_ptr<Pane> Pane::DetachPane(uint32_t id)
{
// We can't remove a pane if we only have a reference to a leaf, even if we
// match the id.
if (_IsLeaf())
{
return nullptr;
}

// Check if either of our children matches the search
auto isFirstChild = _firstChild->_IsLeaf() && _firstChild->_id == id;
auto isSecondChild = _secondChild->_IsLeaf() && _secondChild->_id == id;

if (isFirstChild || isSecondChild)
{
// Keep a reference to the child we are removing
auto detached = isFirstChild ? _firstChild : _secondChild;
// Remove the child from the tree, replace the current node with the
// other child.
_CloseChild(isFirstChild);

detached->_borders = Borders::None;
detached->_UpdateBorders();

// Trigger the detached event on each child
detached->WalkTree([](auto pane) {
pane->_PaneDetachedHandlers(pane);
return false;
});

return detached;
}

if (auto detached = !_firstChild->_IsLeaf() ? _firstChild->DetachPane(id) : nullptr)
{
return detached;
}

return !_secondChild->_IsLeaf() ? _secondChild->DetachPane(id) : nullptr;
}

// Method Description:
// - Closes one of our children. In doing so, takes the control from the other
// child, and makes this pane a leaf node again.
Expand Down Expand Up @@ -1073,12 +1151,10 @@ void Pane::_CloseChild(const bool closeFirst)
// them.
_lastActive = _firstChild->_lastActive || _secondChild->_lastActive;

// Remove all the ui elements of our children. This'll make sure we can
// re-attach the TermControl to our Grid.
_firstChild->_root.Children().Clear();
_secondChild->_root.Children().Clear();
_firstChild->_border.Child(nullptr);
_secondChild->_border.Child(nullptr);
// Remove all the ui elements of the remaining child. This'll make sure
// we can re-attach the TermControl to our Grid.
remainingChild->_root.Children().Clear();
remainingChild->_border.Child(nullptr);

// Reset our UI:
_root.Children().Clear();
Expand Down Expand Up @@ -1125,17 +1201,8 @@ void Pane::_CloseChild(const bool closeFirst)
}
else
{
// Determine which border flag we gave to the child when we first split
// it, so that we can take just that flag away from them.
Borders clearBorderFlag = Borders::None;
if (_splitState == SplitState::Horizontal)
{
clearBorderFlag = closeFirst ? Borders::Top : Borders::Bottom;
}
else if (_splitState == SplitState::Vertical)
{
clearBorderFlag = closeFirst ? Borders::Left : Borders::Right;
}
// Find what borders need to persist after we close the child
auto remainingBorders = _GetCommonBorders();

// First stash away references to the old panes and their tokens
const auto oldFirstToken = _firstClosedToken;
Expand Down Expand Up @@ -1192,13 +1259,9 @@ void Pane::_CloseChild(const bool closeFirst)
_root.Children().Append(_firstChild->GetRootElement());
_root.Children().Append(_secondChild->GetRootElement());

// Take the flag away from the children that they inherited from their
// parent, and update their borders to visually match
WI_ClearAllFlags(_firstChild->_borders, clearBorderFlag);
WI_ClearAllFlags(_secondChild->_borders, clearBorderFlag);
_UpdateBorders();
_firstChild->_UpdateBorders();
_secondChild->_UpdateBorders();
// Propagate the new borders down to the children.
_borders = remainingBorders;
_ApplySplitDefinitions();

// If the closed child was focused, transfer the focus to it's first sibling.
if (closedChild->_lastActive)
Expand Down Expand Up @@ -1757,7 +1820,8 @@ std::pair<std::shared_ptr<Pane>, std::shared_ptr<Pane>> Pane::Split(SplitState s
return { nullptr, nullptr };
}

return _Split(splitType, splitSize, profile, control);
auto newPane = std::make_shared<Pane>(profile, control);
return _Split(splitType, splitSize, newPane);
}

// Method Description:
Expand Down Expand Up @@ -1833,8 +1897,7 @@ SplitState Pane::_convertAutomaticSplitState(const SplitState& splitType) const
// - The two newly created Panes
std::pair<std::shared_ptr<Pane>, std::shared_ptr<Pane>> Pane::_Split(SplitState splitType,
const float splitSize,
const GUID& profile,
const TermControl& control)
std::shared_ptr<Pane> newPane)
{
if (splitType == SplitState::None)
{
Expand Down Expand Up @@ -1874,7 +1937,7 @@ std::pair<std::shared_ptr<Pane>, std::shared_ptr<Pane>> Pane::_Split(SplitState
_firstChild->_connectionState = std::exchange(_connectionState, ConnectionState::NotConnected);
_profile = std::nullopt;
_control = { nullptr };
_secondChild = std::make_shared<Pane>(profile, control);
_secondChild = newPane;

_CreateRowColDefinitions();

Expand Down Expand Up @@ -2551,3 +2614,4 @@ bool Pane::ContainsReadOnly() const
DEFINE_EVENT(Pane, GotFocus, _GotFocusHandlers, winrt::delegate<std::shared_ptr<Pane>>);
DEFINE_EVENT(Pane, LostFocus, _LostFocusHandlers, winrt::delegate<std::shared_ptr<Pane>>);
DEFINE_EVENT(Pane, PaneRaiseBell, _PaneRaiseBellHandlers, winrt::Windows::Foundation::EventHandler<bool>);
DEFINE_EVENT(Pane, Detached, _PaneDetachedHandlers, winrt::delegate<std::shared_ptr<Pane>>);
Loading