Separate between Close Tab Requested and Tab Closed flows #9574
Conversation
|
When I read #9572, I dearly hoped that you were going to do a Close/Requested split. This is amazing. I can't wait to read it on Monday. |
|
Hey we have a conflict here😅 Wonder if msftbot can send notifications when someone’s PR has conflicts with target branch. I’ve seen other boys doing this. |
| @@ -120,8 +120,6 @@ class Pane : public std::enable_shared_from_this<Pane> | |||
|
|
|||
| Borders _borders{ Borders::None }; | |||
|
|
|||
| std::atomic<bool> _isClosing{ false }; | |||
There was a problem hiding this comment.
Is there another way that we're tracking a pane entering the closing state? Is it no longer necessary?
There was a problem hiding this comment.
The reason I added it initially doesn't exist anymore, hence I removed it. We can also leave it, as it doesn't harm the correctness (closing is irreversible). I am fine with both 😊
| @@ -1794,7 +1813,7 @@ namespace winrt::TerminalApp::implementation | |||
| { | |||
| for (auto& tab : tabs) | |||
| { | |||
| co_await _RemoveTab(tab); | |||
| co_await _HandleCloseTabRequested(tab); | |||
There was a problem hiding this comment.
Is this one correct? This is an explicit action for removing all the tabs. What's it hooked up to, and is that thing expecting that it simply request closure?
There was a problem hiding this comment.
What is the user experience here like? Do we get multiple dialogs? "Yes, close all tabs. YES, I understand this tab is read-only."
There was a problem hiding this comment.
Yes. This is the behavior we have today, that for each read-only tab asks if it should be closed or not.
| @@ -788,7 +788,7 @@ namespace winrt::TerminalApp::implementation | |||
| closeTabMenuItem.Click([weakThis](auto&&, auto&&) { | |||
There was a problem hiding this comment.
do we need to do the same for Settings
There was a problem hiding this comment.
Settings inherit it from TabBase that already uses the correct event 😊
ghost
added
Needs-Author-Feedback
|
I suspect that Mike moving all of the tab management stuff into TabManagement.cpp just exploded most of your changes (thus the conflict). Argh! Sorry! |
|
(sorry) |
|
I'll resolve the conflict while I'm here. Just to get this merged. 😄 |
|
Manually merged (UGH) by replicating the changes in the new files. Build is running |
|
I've built and tested this, and it appears to act the same. |
DHowett
added
the
AutoMerge
|
Hello @DHowett! Because this pull request has the 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 (
|
ghost
merged commit
|
🎉 Handy links: |
ghost
mentioned this pull request
Summary of the Pull Request
Currently, both when the tab is already closed, and when there is a
request to close a tab (might be rejected), we go through the same flow
in TerminalPage.
This might leave the system in inconsistent state, as the side-effects
of closing will persist even if the closing was aborted.
This PR separates between the two flows, by introducing a CloseRequested
event to the TabBase.
This event is used to inform the upper tier (the terminal page) about
the request and to trigger the same logic that happens when the tab is
closed directly from the terminal page (e.g., by clicking close on the
tab view).
The Closed event will be used only to handle the actual closing of the
tab. It will ensure that the tab gets removed from the terminal page if
required.
As a result, it a read-only pane will be closed non-interactively (aka
connection exits), the tab closed flow will be invoked, and no user
prompt will be shown.
References
PR Checklist