Add restore recently closed panes #11471

Rosefield · 2021-10-09T19:59:42Z

Summary of the Pull Request

Add an action to restore the last closed pane or tab. When all you have is restoring last sessions everything looks like a std::vector<ActionAndArgs>.

References

#9800

PR Checklist

Partially closes Feature Request - Restore closed tab #960
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

Keep a buffer of panes/tabs that were closed recently in the form of a list
of actions to remake it.
To restore the pane or tab just run the list of actions.
This (deliberately) does not restore the exact visual state as focus could
have changed / new panes might have been created in the mean time. Mostly
this means that restoring a pane will just attach to the currently focused
pane instead of whatever its old neighbor was.
Buffer is limited to 100 entries which might as well be "infinite" by most reasonable
standards, but prevents complaints about there being memory leaks in long running
instances.
The action name could be potentially changed, but it felt unwieldy as "restoreLastClosedPaneOrTab".
This does not handle restoring the actual running contents of a pane.

Validation Steps Performed

- Keep a buffer of panes/tabs that were closed recently in the form of a list of actions to remake it. - To restore the pane or tab just run the list of actions. - This (deliberately) does not restore the exact visual state as focus could have changed / new panes might have been created in the mean time. Mostly this means that restoring a pane will just attach to the currently focused pane instead of whatever its old neighbor was.

Rosefield · 2021-10-09T20:00:13Z

2021-10-09.15-27-45.mp4

Rosefield · 2021-10-09T20:03:10Z

Also I understand that the next release is probably 2021-10-15 and this probably won't get into 1.12

PankajBhojwani · 2021-10-27T17:14:21Z

src/cascadia/TerminalApp/TabManagement.cpp

@@ -474,6 +474,23 @@ namespace winrt::TerminalApp::implementation
                co_return;
            }
        }
+
+        if (const auto terminalTab = _GetTerminalTabImpl(tab))


Why don't we check the size of _previouslyClosedPanesAndTabs in this function? (like how we do in _CloseFocusedPane)

I probably just forgot. I added that as an afterthought right before pushing. It probably isn't even necessary overall since the vector in most reasonable cases won't be more than 10s to 100s of KB of data, but I'm curious what everyone's thoughts are on that.

…-last-pane-or-tab

zadjii-msft

Honestly, this is just a nit and a question. I'm cool signing off on this, but I wanna make sure we've considered the parent pane situation first.

The code looks great, the implementation is simple and straightforward, thanks!

zadjii-msft · 2021-12-02T14:30:23Z

src/cascadia/TerminalApp/TabManagement.cpp

+
+            _AddPreviouslyClosedPaneOrTab(std::move(actions));
+        }
+        else if (tab.try_as<SettingsTab>())


this almost seems like TabBase should have a virtual method like BuildStartupActions that both TerminalTab and SettingsTab override with their specific implementations...

that seems reasonable.

src/cascadia/TerminalApp/TabManagement.cpp

zadjii-msft

yea I'm cool with this

…ype.

Rosefield · 2021-12-13T17:38:25Z

Ping on this. Not sure if people are out for the holiday season yet.

Rosefield · 2022-01-10T17:58:56Z

new year ping.

zadjii-msft · 2022-01-10T18:27:51Z

@DHowett that means you. There was a reason you had me assign you last month, I'm sure of it (but I can't remember why)

miniksa · 2022-01-10T18:58:19Z

src/cascadia/TerminalApp/TabManagement.cpp

+        if (const auto size = _previouslyClosedPanesAndTabs.size(); size > 150)
+        {
+            const auto it = _previouslyClosedPanesAndTabs.begin();
+            // delete 50 at a time so that we don't have to do an erase
+            // of the buffer every time when at capacity.
+            _previouslyClosedPanesAndTabs.erase(it, it + (size - 100));
+        }
+


Out of curiosity... when this has like 149 actions in it and you restore it... does the Terminal UI wildly flicker as it plays all the actions back until it settles? I'm concerned that either someone will interact with it in some way while it's playing back or that it will look horrendous on a slower system as it recreates a bunch of stuff.

To disambiguate, _previouslyClosedPanesAndTabs itself contains vectors of actions, and when you restore the last closed thing it will pop one vector and run all of those. The size limit, whatever it ends up being, shouldn't meaningfully affect the performance of the UI. Technically this erase call will end up calling a memcpy and some destructors, but that should be fast.

To answer your intended question, in the video above I show what it looks like to restore a tab with 7 panes in it. It does take a second for it to resolve everything since it is spawning new processes/terminal controls for each one. Of course that is in a debug build so it might be faster in a release build.

I did another test just quickly (still a debug build, with debugger attached), restoring a tab that had 20 powershell panes in it took about ~6 seconds and the UI was unresponsive during that time. That corresponds to ~40 actions being executed: 20 split panes and some move focus to put the panes in the right spots.

Huh, down the line, we should display a message like "setting up your workspace..." or something like that rather than being unresponsive. That'd be some nice polish instead of sitting with an unresponsive UI haha.

Yeah ok. I'm fine with taking it for the sake of getting the useful feature in, but Carlos is right that we should probably add some small polish to it in the future, if someone complains or we feel like it.

For the case of restoring a single pane, it won't be any slower than if the user did a split pane manually. It is only on the case where someone restores a tab with a bunch of panes on it that it takes some time. There is the benefit of the user (roughly) knowing what they are recreating, so they shouldn't be too surprised if it takes a second.

That being said, this is a fairly naive implementation so there is probably be room for improvement,

miniksa

I've talked with @DHowett and he's fine with me being second on this. Let's do it and see how it rolls.

ghost · 2022-01-11T20:08:02Z

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.

Rosefield · 2022-01-11T20:09:26Z

I'm free~~~.

A number of the items on 9800 can probably be updated as well since they have been fixed / are stale.

DHowett · 2022-01-11T20:26:31Z

Thanks so much for doing this 😄

ghost · 2022-02-03T18:27:45Z

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

Handy links:

PankajBhojwani reviewed Oct 27, 2021

View reviewed changes

Rosefield added 2 commits December 1, 2021 13:27

Merge remote-tracking branch 'origin/main' into feature/gh960-restore…

a35ecd2

…-last-pane-or-tab

Make cleanup consistent between tab/pane close

15bf2e6

zadjii-msft reviewed Dec 2, 2021

View reviewed changes

zadjii-msft assigned Rosefield Dec 2, 2021

zadjii-msft approved these changes Dec 2, 2021

View reviewed changes

zadjii-msft unassigned Rosefield Dec 2, 2021

zadjii-msft added the Needs-Second It's a PR that needs another sign-off label Dec 2, 2021

ghost requested review from miniksa, carlos-zamora, DHowett, leonMSFT and lhecker December 2, 2021 16:08

Code review: use dynamic dispatch intead of statically checking tab t…

a55d56a

…ype.

zadjii-msft assigned DHowett Dec 13, 2021

zadjii-msft changed the title ~~GH960 Add restore recently closed panes~~ Add restore recently closed panes Jan 10, 2022

miniksa reviewed Jan 10, 2022

View reviewed changes

miniksa approved these changes Jan 11, 2022

View reviewed changes

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

ghost merged commit ec226ee into microsoft:main Jan 11, 2022

Rosefield deleted the feature/gh960-restore-last-pane-or-tab branch January 11, 2022 20:08

ghost mentioned this pull request Feb 3, 2022

Feature Request - Restore closed tab #960

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add restore recently closed panes #11471

Add restore recently closed panes #11471

Rosefield commented Oct 9, 2021 •

edited

Loading

Rosefield commented Oct 9, 2021

Rosefield commented Oct 9, 2021

PankajBhojwani Oct 27, 2021

Rosefield Oct 28, 2021

zadjii-msft left a comment

zadjii-msft Dec 2, 2021

Rosefield Dec 2, 2021

zadjii-msft left a comment

Rosefield commented Dec 13, 2021

Rosefield commented Jan 10, 2022

zadjii-msft commented Jan 10, 2022

miniksa Jan 10, 2022

Rosefield Jan 10, 2022 •

edited

Loading

carlos-zamora Jan 11, 2022

miniksa Jan 11, 2022

Rosefield Jan 11, 2022

miniksa left a comment

ghost commented Jan 11, 2022

Rosefield commented Jan 11, 2022

DHowett commented Jan 11, 2022

ghost commented Feb 3, 2022

Add restore recently closed panes #11471

Add restore recently closed panes #11471

Conversation

Rosefield commented Oct 9, 2021 • edited Loading

Summary of the Pull Request

References

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Rosefield commented Oct 9, 2021

Rosefield commented Oct 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zadjii-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zadjii-msft left a comment

Choose a reason for hiding this comment

Rosefield commented Dec 13, 2021

Rosefield commented Jan 10, 2022

zadjii-msft commented Jan 10, 2022

Choose a reason for hiding this comment

Rosefield Jan 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miniksa left a comment

Choose a reason for hiding this comment

ghost commented Jan 11, 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.

Rosefield commented Jan 11, 2022

DHowett commented Jan 11, 2022

ghost commented Feb 3, 2022

Rosefield commented Oct 9, 2021 •

edited

Loading

Rosefield Jan 10, 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.