Move Pane to Tab (GH7075)

[Rosefield]

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.

References

#4587

PR Checklist

• Closes Allow moving panes to other tabs #7075
• CLA signed. If not, go over here and sign the CLA
• Tests added/passed
• Documentation updated. If checked, please file a pull request on our docs repo and link it here: #xxx
• 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

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.

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.

[zadjii-msft]

fyi the team probably won't get around to reviewing PRs this week. I don't want you to get the impression we're ignoring you 😝

[Rosefield]

merged in main last night which fixed all of the CI errors. This should be fully ready now.

[github-actions]

@check-spelling-bot Report

Unrecognized words, please review:

• Propogate

Previously acknowledged words that are now absent

SPACEBAR Unregister

To accept these unrecognized words as correct (and remove the previously acknowledged and now absent words), run the following commands

... in a clone of the git@github.com:Rosefield/terminal.git repository
on the feature/gh7075-move-pane-to-tab branch:

update_files() {
perl -e '
my @expect_files=qw('".github/actions/spelling/expect/alphabet.txt
.github/actions/spelling/expect/expect.txt
.github/actions/spelling/expect/web.txt"');
@ARGV=@expect_files;
my @stale=qw('"$patch_remove"');
my $re=join "|", @stale;
my $suffix=".".time();
my $previous="";
sub maybe_unlink { unlink($_[0]) if $_[0]; }
while (<>) {
if ($ARGV ne $old_argv) { maybe_unlink($previous); $previous="$ARGV$suffix"; rename($ARGV, $previous); open(ARGV_OUT, ">$ARGV"); select(ARGV_OUT); $old_argv = $ARGV; }
next if /^(?:$re)(?:(?:\r|\n)*$| .*)/; print;
}; maybe_unlink($previous);'
perl -e '
my $new_expect_file=".github/actions/spelling/expect/e7108332f72a3c14c72a65484d091c4e79ab28b2.txt";
use File::Path qw(make_path);
use File::Basename qw(dirname);
make_path (dirname($new_expect_file));
open FILE, q{<}, $new_expect_file; chomp(my @words = <FILE>); close FILE;
my @add=qw('"$patch_add"');
my %items; @items{@words} = @words x (1); @items{@add} = @add x (1);
@words = sort {lc($a)."-".$a cmp lc($b)."-".$b} keys %items;
open FILE, q{>}, $new_expect_file; for my $word (@words) { print FILE "$word\n" if $word =~ /\w/; };
close FILE;
system("git", "add", $new_expect_file);
'
}

comment_json=$(mktemp)
curl -L -s -S \
  --header "Content-Type: application/json" \
  "https://api.github.com/repos/microsoft/terminal/issues/comments/891356151" > "$comment_json"
comment_body=$(mktemp)
jq -r .body < "$comment_json" > $comment_body
rm $comment_json

patch_remove=$(perl -ne 'next unless s{^</summary>(.*)</details>$}{$1}; print' < "$comment_body")
  

patch_add=$(perl -e '$/=undef;
$_=<>;
s{<details>.*}{}s;
s{^#.*}{};
s{\n##.*}{};
s{(?:^|\n)\s*\*}{}g;
s{\s+}{ }g;
print' < "$comment_body")
  
update_files
rm $comment_body
git add -u

✏️ Contributor please read this

By default the command suggestion will generate a file named based on your commit. That's generally ok as long as you add the file to your commit. Someone can reorganize it later.

⚠️ The command is written for posix shells. You can copy the contents of each perl command excluding the outer ' marks and dropping any '"/"' quotation mark pairs into a file and then run perl file.pl from the root of the repository to run the code. Alternatively, you can manually insert the items...

If the listed items are:

• ... misspelled, then please correct them instead of using the command.
• ... names, please add them to .github/actions/spelling/allow/names.txt.
• ... APIs, you can add them to a file in .github/actions/spelling/allow/.
• ... just things you're using, please add them to an appropriate file in .github/actions/spelling/expect/.
• ... tokens you only need in one place and shouldn't generally be used, you can add an item in an appropriate file in .github/actions/spelling/patterns/.

See the README.md in each directory for more information.

🔬 You can test your commits without appending to a PR by creating a new branch with that extra change and pushing it to your fork. The check-spelling action will run in response to your push -- it doesn't require an open pull request. By using such a branch, you can limit the number of typos your peers see you make. 😉

🗜️ If you see a bunch of garbage

If it relates to a ...

well-formed pattern

See if there's a pattern that would match it.

If not, try writing one and adding it to a patterns/{file}.txt.

Patterns are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your lines.

Note that patterns can't match multiline strings.

binary-ish string

Please add a file path to the excludes.txt file instead of just accepting the garbage.

File paths are Perl 5 Regular Expressions - you can test yours before committing to verify it will match your files.

^ refers to the file's path from the root of the repository, so ^README\.md$ would exclude README.md (on whichever branch you're using).

[zadjii-msft]

Okay, I think this looks great. I need to socialize the commandline here a bit with the rest of the team to see what they think. Overall, I think this is a great PR and sets us up well for some really great features in the future!

(only blocking for the team discussion, usually mondays)

[zadjii-msft]

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.

[Rosefield]

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.

[zadjii-msft]

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.

[Rosefield]

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?

[zadjii-msft]

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

[zadjii-msft]

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.

[Rosefield]

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.

[zadjii-msft]

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!)

[zadjii-msft]

this is very clever, I love it

[Rosefield]

Out of curiosity, what is the stabilization process like? I know that releases happen every 6 weeks, with the next one in ETA ~20 days, but I don't know how soon things need to be merged/how long they need to sit on main before they can be included. I would love to see this included in the next preview build so I can stop running my own fork.

[zadjii-msft]

Yep, my bad 😋

[carlos-zamora]

This looks great! Thanks!

There's a few const that can be sprinkled around, but that's all I found.

[carlos-zamora]

Oh! Don't forget to update the docs too! https://github.com/MicrosoftDocs/terminal

[Rosefield]

Created documentation updates for all of the features I worked on.

[Rosefield]

I believe this is ready to merge?

[carlos-zamora]

@msftbot merge this in 5 minutes

[ghost]

Hello @carlos-zamora!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

• I won't merge this pull request until after the UTC date Thu, 12 Aug 2021 16:40:58 GMT, which is in 5 minutes

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

[DHowett]

Just wanted to say that this is really, really clever. I love it. Well done and thank you!

[ghost]

🎉Windows Terminal Preview v1.11.2421.0 has been released which incorporates this pull request.:tada:

Handy links:

• Release Notes
• Store Download