Add action for closing all unfocused panes #13547

JerBast · 2022-07-20T14:17:35Z

This PR introduces a new action for closing all unfocused / other panes within a tab.

Edit (17-08-2022): Read-only panes are ignored when this action is being used (i.e., left open).

Validation Steps Performed

The action was manually tested by applying it to various 'compositions' of panes. This includes tests on read-only panes.

Closes #12216

JerBast · 2022-07-21T09:21:42Z

Is it desired to play the closing animation when this action is being used?

zadjii-msft · 2022-07-21T10:38:25Z

Is it desired to play the closing animation when this action is being used?

That might be cool, but I suspect that's Hard, so if you wanna do this without the animation at first, I'd be all for that ☺️ We can always add the animations back in post, if we figure out it's possible.

JerBast · 2022-07-21T12:38:11Z

if you wanna do this without the animation at first

Yes, that is the current state of the PR 😉. I'm not sure what would be a good way to perform the closing animation sequentially when multiple panes are being closed. My current guess would be to trigger the removal of the next pane after the animation of the previous pane is finished

doc/cascadia/profiles.schema.json

src/cascadia/TerminalApp/AppActionHandlers.cpp

src/cascadia/TerminalApp/TabManagement.cpp

…ead-only pane is kept

carlos-zamora

A few questions:

For read-only panes, you say it's similar to closing a single pane. So like, when you invoke the closeOtherPanes action, you immediately get the pop-up for all of them? Or do you get the pop-up when it tries to delete the read-only pane? I'm wondering if the expected behavior might be to skip over read-only panes?
For animations, this respects the "Pane animations" setting in Settings UI > Appearance right?

src/cascadia/TerminalApp/AppActionHandlers.cpp

src/cascadia/TerminalApp/TabManagement.cpp

JerBast · 2022-07-27T17:34:50Z

Thank you for reviewing @carlos-zamora. To answer your questions:

For read-only panes, you say it's similar to closing a single pane. So like, when you invoke the closeOtherPanes action, you immediately get the pop-up for all of them? Or do you get the pop-up when it tries to delete the read-only pane? I'm wondering if the expected behavior might be to skip over read-only panes?

The pop-up is only shown when trying to close the pane, not immediately after invoking the action. Skipping read-only panes might be a good solution, but you should probably let the user know why some panes were not closed (for clarity).

For animations, this respects the "Pane animations" setting in Settings UI > Appearance right?

Yes, that setting is respected because the existing closing animation that performs the check is reused.

…en there are no unfocused panes

carlos-zamora · 2022-07-28T23:38:28Z

Nice! I'm gonna add the Needs-Discussion tag just to discuss the read-only panes thing with the team at Monday's meeting. Want to make sure we have behavior we're comfortable with. 😊 Other than that, this PR is looking great! Thank you so much for doing this!

zadjii-msft · 2022-08-10T21:31:04Z

I dunno if this really needs discussion - this seems like it makes sense to me

zadjii-msft · 2022-08-15T21:47:49Z

Hey so we finally had team discussion today - consensus was that we've had enough issues in the past of context dialogs popping in the middle of something happening. We should just blanket skip all read-only panes, to just avoid the issue entirely. This sound good to you/?

JerBast · 2022-08-16T05:03:47Z

Hey, thanks for discussing this internally. Changing the behaviour for read-only panes sounds good to me and I will make sure to change it before the end of this week.

carlos-zamora

This looks great! Thanks for doing this! Sorry it took us a bit to get to it :)

zadjii-msft

Okay maybe I'm missing something. 90% of this looks really good.

I'm blocking over the one parent/readonly check in TerminalPage::_PaneConfirmCloseReadOnly. We should really bring that back.
The recursion in TerminalPage::_ClosePanes is maybe a bit tricky to follow. It's not clear to me (just from reading the code) how the loop is ever used...
- We add the callback to the pane we're going to close
- We call pane.close
- If the pane is the parent of the pane to be closed, it calls _CloseChild
- in there we trigger the callback
- we pop off the last id off the ids
- {we do this dance again}
- eventually we return out of the top _HandleClosePaneRequested, and now ids is empty and we exit the loop immediately.

If we can do it just with the loop, that seems easier than the recursion with the callback.

Everything else looks totally 👍 though!

(sorry for the delays 😞)

zadjii-msft · 2022-08-25T17:48:42Z

src/cascadia/TerminalApp/TabManagement.cpp

+                    pane->ClosedByParent([ids{ std::move(paneIds) }, weakThis{ get_weak() }, weakTab]() {
+                        if (auto strongThis{ weakThis.get() })
+                        {
+                            strongThis->_ClosePanes(weakTab, std::move(ids));
+                        }
+                    });


Why do we need both the while loop, and the recursion here? Seems like we move the ids into the callback, but then we shouldn't need the ids again, right? Since the callback now owns that vector...

Maybe I'm missing something.

The loop is used to deal with the case where one of the panes can no longer be found in the tree. Otherwise we cannot guarantee the removal of the other panes in case something like this araises.

The recursion is used to reduce code duplication for the chained removal of the panes (since the pane itself is removed from the tree after the animation finishes). This used to be a simple loop in 11bba27 where the pane was detached before before closing it, bypassing the animation.

(No need to apologise for the delay(s). It is perfectly understandable!)

zadjii-msft · 2022-08-25T17:48:46Z

src/cascadia/TerminalApp/TabManagement.cpp

+        if (const auto control{ pane->GetTerminalControl() })
+        {
+            if (control.ReadOnly())


This used to be a pane->ContainsReadOnly call, replaced now with checking if the actual control in the pane is read-only.

Technically, you can select a parent pane. So you could close a parent pane (and close all the leaves within it). I think with this change, we've lost the ability to check if any leaves in a pane are read-only.

A simple fix might be to just add an else (pane->ContainsReadOnly()) after the if (const auto control block, and repeat the walking we had before in that block. A little duplicated, but

As the 'response' to a read-only pane is the same in this case (for parents and leaf nodes), I replaced the existing read-only check with the pane->ContainsReadOnly check and changed the toggling accordingly to avoid duplication (f5a1347)

zadjii-msft

Yea I'm satisfied with that explanation, thanks!

ghost · 2022-08-26T17:27:00Z

Hello @zadjii-msft!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

ghost · 2022-09-13T17:06:26Z

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

Handy links:

Add action for closing all unfocused panes

11bba27

ghost added Area-UserInterface Issues pertaining to the user interface of the Console or Terminal Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels Jul 20, 2022

Play closing animation when other panes are being closed

f550bd5

This comment has been minimized.

Sign in to view

JerBast marked this pull request as ready for review July 25, 2022 08:30

lhecker reviewed Jul 25, 2022

View reviewed changes

doc/cascadia/profiles.schema.json Outdated Show resolved Hide resolved

src/cascadia/TerminalApp/AppActionHandlers.cpp Outdated Show resolved Hide resolved

src/cascadia/TerminalApp/TabManagement.cpp Outdated Show resolved Hide resolved

Add suggested changes and resolve other panes not being closed when r…

f405ce0

…ead-only pane is kept

This comment has been minimized.

Sign in to view

Merge branch 'main' of github.com:JerBast/terminal into 12216

48b7b03

carlos-zamora requested changes Jul 27, 2022

View reviewed changes

src/cascadia/TerminalApp/AppActionHandlers.cpp Outdated Show resolved Hide resolved

src/cascadia/TerminalApp/AppActionHandlers.cpp Outdated Show resolved Hide resolved

src/cascadia/TerminalApp/TabManagement.cpp Outdated Show resolved Hide resolved

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 27, 2022

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jul 27, 2022

Change _CloseUnfocusedPanes to _ClosePanes; mark args as unhandled wh…

f9ad9f2

…en there are no unfocused panes

carlos-zamora added the Needs-Discussion Something that requires a team discussion before we can proceed label Jul 28, 2022

zadjii-msft added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Discussion Something that requires a team discussion before we can proceed labels Aug 15, 2022

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 16, 2022

Ignore read-only panes while performing close other panes action

73f02ef

carlos-zamora approved these changes Aug 17, 2022

View reviewed changes

zadjii-msft requested changes Aug 25, 2022

View reviewed changes

ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 25, 2022

Replace single-level read-only check with multi-level read-only check

f5a1347

ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Aug 26, 2022

zadjii-msft approved these changes Aug 26, 2022

View reviewed changes

zadjii-msft added the AutoMerge Marked for automatic merge by the bot when requirements are met label Aug 26, 2022

ghost merged commit 0ca1356 into microsoft:main Aug 26, 2022

ghost mentioned this pull request Sep 13, 2022

Add "Close All Other Panes" Action to Settings/Actions #12216

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add action for closing all unfocused panes #13547

Add action for closing all unfocused panes #13547

JerBast commented Jul 20, 2022 •

edited

Loading

JerBast commented Jul 21, 2022

zadjii-msft commented Jul 21, 2022

JerBast commented Jul 21, 2022

This comment has been minimized.

This comment has been minimized.

carlos-zamora left a comment

JerBast commented Jul 27, 2022

carlos-zamora commented Jul 28, 2022

zadjii-msft commented Aug 10, 2022

zadjii-msft commented Aug 15, 2022

JerBast commented Aug 16, 2022

carlos-zamora left a comment

zadjii-msft left a comment

zadjii-msft Aug 25, 2022

JerBast Aug 26, 2022

zadjii-msft Aug 25, 2022

JerBast Aug 26, 2022

zadjii-msft left a comment

ghost commented Aug 26, 2022

ghost commented Sep 13, 2022

Add action for closing all unfocused panes #13547

Add action for closing all unfocused panes #13547

Conversation

JerBast commented Jul 20, 2022 • edited Loading

Validation Steps Performed

JerBast commented Jul 21, 2022

zadjii-msft commented Jul 21, 2022

JerBast commented Jul 21, 2022

This comment has been minimized.

This comment has been minimized.

carlos-zamora left a comment

Choose a reason for hiding this comment

JerBast commented Jul 27, 2022

carlos-zamora commented Jul 28, 2022

zadjii-msft commented Aug 10, 2022

zadjii-msft commented Aug 15, 2022

JerBast commented Aug 16, 2022

carlos-zamora left a comment

Choose a reason for hiding this comment

zadjii-msft left a comment

Choose a reason for hiding this comment

zadjii-msft Aug 25, 2022

Choose a reason for hiding this comment

JerBast Aug 26, 2022

Choose a reason for hiding this comment

zadjii-msft Aug 25, 2022

Choose a reason for hiding this comment

JerBast Aug 26, 2022

Choose a reason for hiding this comment

zadjii-msft left a comment

Choose a reason for hiding this comment

ghost commented Aug 26, 2022

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

ghost commented Sep 13, 2022

JerBast commented Jul 20, 2022 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.