maxmustermann/terminal
1
0
Fork 0
Code Issues Pull requests Projects Releases Wiki Activity

Separate between Close Tab Requested and Tab Closed flows (#9574)

Browse source 
## 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

<!-- Please review the items on the PR checklist before submitting-->
## PR Checklist
* [x] Closes https://github.com/microsoft/terminal/issues/9572
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [ ] Tests added/passed
* [ ] Documentation updated. 
* [ ] Schema updated.
* [ ] I've discussed this with core contributors already.
This commit is contained in:
Don-Vito 2021-03-30 18:58:35 +03:00 committed by GitHub
parent 19fb9b21da
commit c585a93fc9
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
9 changed files with 65 additions and 42 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

4
src/cascadia/TerminalApp/AppActionHandlers.cpp
View file

@ -498,7 +498,7 @@ namespace winrt::TerminalApp::implementation
return;
}
// Since _RemoveTab is asynchronous, create a snapshot of the tabs we want to remove
// Since _RemoveTabs is asynchronous, create a snapshot of the tabs we want to remove
std::vector<winrt::TerminalApp::TabBase> tabsToRemove;
if (index > 0)
{
@ -537,7 +537,7 @@ namespace winrt::TerminalApp::implementation
return;
}
// Since _RemoveTab is asynchronous, create a snapshot of the tabs we want to remove
// Since _RemoveTabs is asynchronous, create a snapshot of the tabs we want to remove
std::vector<winrt::TerminalApp::TabBase> tabsToRemove;
std::copy(begin(_tabs) + index + 1, end(_tabs), std::back_inserter(tabsToRemove));
_RemoveTabs(tabsToRemove);

7
src/cascadia/TerminalApp/Pane.cpp
View file

@ -416,11 +416,8 @@ void Pane::_ControlLostFocusHandler(winrt::Windows::Foundation::IInspectable con
// - <none>
void Pane::Close()
{
if (!_isClosing.exchange(true))
{
// Fire our Closed event to tell our parent that we should be removed.
_ClosedHandlers(nullptr, nullptr);
}
// Fire our Closed event to tell our parent that we should be removed.
_ClosedHandlers(nullptr, nullptr);
}
// Method Description:

2
src/cascadia/TerminalApp/Pane.h
View file

@ -120,8 +120,6 @@ private:
Borders _borders{ Borders::None };
std::atomic<bool> _isClosing{ false };
bool _zoomed{ false };
bool _IsLeaf() const noexcept;

2
src/cascadia/TerminalApp/TabBase.cpp
View file

@ -54,7 +54,7 @@ namespace winrt::TerminalApp::implementation
closeTabMenuItem.Click([weakThis](auto&&, auto&&) {
if (auto tab{ weakThis.get() })
{
tab->_ClosedHandlers(nullptr, nullptr);
tab->_CloseRequestedHandlers(nullptr, nullptr);
}
});
closeTabMenuItem.Text(RS_(L"TabClose"));

1
src/cascadia/TerminalApp/TabBase.h
View file

@ -26,6 +26,7 @@ namespace winrt::TerminalApp::implementation
void SetKeyMap(const Microsoft::Terminal::Settings::Model::KeyMapping& keymap);
WINRT_CALLBACK(Closed, winrt::Windows::Foundation::EventHandler<winrt::Windows::Foundation::IInspectable>);
WINRT_CALLBACK(CloseRequested, winrt::Windows::Foundation::EventHandler<winrt::Windows::Foundation::IInspectable>);
WINRT_CALLBACK(PropertyChanged, Windows::UI::Xaml::Data::PropertyChangedEventHandler);
// The TabViewIndex is the index this Tab object resides in TerminalPage's _tabs vector.

62
src/cascadia/TerminalApp/TabManagement.cpp
View file

@ -318,26 +318,10 @@ namespace winrt::TerminalApp::implementation
}
// Method Description:
// - Look for the index of the input tabView in the tabs vector,
// and call _RemoveTab
// Arguments:
// - tabViewItem: the TabViewItem in the TabView that is being removed.
void TerminalPage::_RemoveTabViewItem(const MUX::Controls::TabViewItem& tabViewItem)
{
uint32_t tabIndexFromControl = 0;
if (_tabView.TabItems().IndexOf(tabViewItem, tabIndexFromControl))
{
// If IndexOf returns true, we've actually got an index
auto tab{ _tabs.GetAt(tabIndexFromControl) };
_RemoveTab(tab);
}
}
// Method Description:
// - Removes the tab (both TerminalControl and XAML)
// - Removes the tab (both TerminalControl and XAML) after prompting for approval
// Arguments:
// - tab: the tab to remove
winrt::Windows::Foundation::IAsyncAction TerminalPage::_RemoveTab(winrt::TerminalApp::TabBase tab)
winrt::Windows::Foundation::IAsyncAction TerminalPage::_HandleCloseTabRequested(winrt::TerminalApp::TabBase tab)
{
if (tab.ReadOnly())
{
@ -349,12 +333,20 @@ namespace winrt::TerminalApp::implementation
co_return;
}
}
_RemoveTab(tab);
}
// Method Description:
// - Removes the tab (both TerminalControl and XAML)
// Arguments:
// - tab: the tab to remove
void TerminalPage::_RemoveTab(const winrt::TerminalApp::TabBase& tab)
{
uint32_t tabIndex{};
if (!_tabs.IndexOf(tab, tabIndex))
{
// The tab is already removed
co_return;
return;
}
// We use _removing flag to suppress _OnTabSelectionChanged events
@ -438,8 +430,6 @@ namespace winrt::TerminalApp::implementation
_rearrangeFrom = std::nullopt;
_rearrangeTo = std::nullopt;
}
co_return;
}
// Method Description:
@ -534,7 +524,7 @@ namespace winrt::TerminalApp::implementation
}
// Method Description:
// - returns a com_ptr to the currently focused tab. This might return null,
// - returns the currently focused tab. This might return null,
// so make sure to check the result!
winrt::TerminalApp::TabBase TerminalPage::_GetFocusedTab() const noexcept
{
@ -557,6 +547,20 @@ namespace winrt::TerminalApp::implementation
return nullptr;
}
// Method Description:
// - returns a tab corresponding to a view item. This might return null,
// so make sure to check the result!
winrt::TerminalApp::TabBase TerminalPage::_GetTabByTabViewItem(const Microsoft::UI::Xaml::Controls::TabViewItem& tabViewItem) const noexcept
{
uint32_t tabIndexFromControl{};
if (_tabView.TabItems().IndexOf(tabViewItem, tabIndexFromControl))
{
// If IndexOf returns true, we've actually got an index
return _tabs.GetAt(tabIndexFromControl);
}
return nullptr;
}
// Method Description:
// - An async method for changing the focused tab on the UI thread. This
// method will _only_ set the selected item of the TabView, which will
@ -589,7 +593,7 @@ namespace winrt::TerminalApp::implementation
if (auto index{ _GetFocusedTabIndex() })
{
auto tab{ _tabs.GetAt(*index) };
_RemoveTab(tab);
_HandleCloseTabRequested(tab);
}
}
@ -635,7 +639,7 @@ namespace winrt::TerminalApp::implementation
const auto tab{ _tabs.GetAt(*index) };
if (tab.try_as<TerminalApp::SettingsTab>())
{
_RemoveTab(tab);
_HandleCloseTabRequested(tab);
}
}
}
@ -648,7 +652,7 @@ namespace winrt::TerminalApp::implementation
{
for (auto& tab : tabs)
{
co_await _RemoveTab(tab);
co_await _HandleCloseTabRequested(tab);
}
}
// Method Description:
@ -689,7 +693,11 @@ namespace winrt::TerminalApp::implementation
{
if (eventArgs.GetCurrentPoint(*this).Properties().IsMiddleButtonPressed())
{
_RemoveTabViewItem(sender.as<MUX::Controls::TabViewItem>());
const auto tabViewItem = sender.try_as<MUX::Controls::TabViewItem>();
if (auto tab{ _GetTabByTabViewItem(tabViewItem) })
{
_HandleCloseTabRequested(tab);
}
eventArgs.Handled(true);
}
else if (eventArgs.GetCurrentPoint(*this).Properties().IsRightButtonPressed())
@ -882,7 +890,7 @@ namespace winrt::TerminalApp::implementation
void TerminalPage::_RemoveAllTabs()
{
// Since _RemoveTab is asynchronous, create a snapshot of the tabs we want to remove
// Since _RemoveTabs is asynchronous, create a snapshot of the tabs we want to remove
std::vector<winrt::TerminalApp::TabBase> tabsToRemove;
std::copy(begin(_tabs), end(_tabs), std::back_inserter(tabsToRemove));
_RemoveTabs(tabsToRemove);

21
src/cascadia/TerminalApp/TerminalPage.cpp
View file

@ -716,7 +716,10 @@ namespace winrt::TerminalApp::implementation
{
co_await winrt::resume_foreground(page->_tabView.Dispatcher());
page->_RemoveTabViewItem(tabViewItem);
if (auto tab{ _GetTabByTabViewItem(tabViewItem) })
{
_RemoveTab(tab);
}
}
// Method Description:
@ -1799,7 +1802,10 @@ namespace winrt::TerminalApp::implementation
void TerminalPage::_OnTabCloseRequested(const IInspectable& /*sender*/, const MUX::Controls::TabViewTabCloseRequestedEventArgs& eventArgs)
{
const auto tabViewItem = eventArgs.Tab();
_RemoveTabViewItem(tabViewItem);
if (auto tab{ _GetTabByTabViewItem(tabViewItem) })
{
_HandleCloseTabRequested(tab);
}
}
TermControl TerminalPage::_InitControl(const TerminalSettings& settings, const ITerminalConnection& connection)
@ -2353,6 +2359,17 @@ namespace winrt::TerminalApp::implementation
tabViewItem.PointerPressed({ this, &TerminalPage::_OnTabClick });
// When the tab requests close, try to close it (prompt for approval, if required)
newTabImpl->CloseRequested([weakTab, weakThis{ get_weak() }](auto&& /*s*/, auto&& /*e*/) {
auto page{ weakThis.get() };
auto tab{ weakTab.get() };
if (page && tab)
{
page->_HandleCloseTabRequested(*tab);
}
});
// When the tab is closed, remove it from our list of tabs.
newTabImpl->Closed([tabViewItem, weakThis{ get_weak() }](auto&& /*s*/, auto&& /*e*/) {
if (auto page{ weakThis.get() })

6
src/cascadia/TerminalApp/TerminalPage.h
View file

@ -178,8 +178,9 @@ namespace winrt::TerminalApp::implementation
void _DuplicateFocusedTab();
void _DuplicateTab(const TerminalTab& tab);
void _RemoveTabViewItem(const Microsoft::UI::Xaml::Controls::TabViewItem& tabViewItem);
winrt::Windows::Foundation::IAsyncAction _RemoveTab(winrt::TerminalApp::TabBase tab);
winrt::Windows::Foundation::IAsyncAction _HandleCloseTabRequested(winrt::TerminalApp::TabBase tab);
void _RemoveTab(const winrt::TerminalApp::TabBase& tab);
winrt::fire_and_forget _RemoveTabs(const std::vector<winrt::TerminalApp::TabBase> tabs);
void _RegisterTerminalEvents(Microsoft::Terminal::Control::TermControl term, TerminalTab& hostingTab);
@ -198,6 +199,7 @@ namespace winrt::TerminalApp::implementation
std::optional<uint32_t> _GetFocusedTabIndex() const noexcept;
TerminalApp::TabBase _GetFocusedTab() const noexcept;
winrt::com_ptr<TerminalTab> _GetFocusedTabImpl() const noexcept;
TerminalApp::TabBase _GetTabByTabViewItem(const Microsoft::UI::Xaml::Controls::TabViewItem& tabViewItem) const noexcept;
winrt::fire_and_forget _SetFocusedTabIndex(const uint32_t tabIndex);
void _CloseFocusedTab();

2
src/cascadia/TerminalApp/TerminalTab.cpp
View file

@ -788,7 +788,7 @@ namespace winrt::TerminalApp::implementation
closeTabMenuItem.Click([weakThis](auto&&, auto&&) {
if (auto tab{ weakThis.get() })
{
tab->_ClosedHandlers(nullptr, nullptr);
tab->_CloseRequestedHandlers(nullptr, nullptr);
}
});
closeTabMenuItem.Text(RS_(L"TabClose"));