From 82f302b7142e116c52f5c143761febb76b34a9b1 Mon Sep 17 00:00:00 2001 From: "Dustin L. Howett (MSFT)" Date: Thu, 23 Jan 2020 14:12:20 -0800 Subject: [PATCH] Shut down all controls under a tab before we remove it from the list (#4337) This commit introduces a new recursive pane shutdown that will give all controls under a tab a chance to clean up their state before beign detached from the UI. It also reorders the call to LastTabClosed() so that the application does not exit before the final connections are terminated. It also teaches TSFInputControl how to shut down to avoid a dramatic platform bug. Fixes #4159. Fixes #4336. ## PR Checklist * [x] CLA signed * [x] I've discussed this with core contributors already. ## Validation Steps Performed Validated through manual terminal teardown within and without the debugger, given a crazy number of panes and tabs. --- src/cascadia/TerminalApp/Pane.cpp | 19 +++++++++++++ src/cascadia/TerminalApp/Pane.h | 1 + src/cascadia/TerminalApp/Tab.cpp | 7 +++++ src/cascadia/TerminalApp/Tab.h | 1 + src/cascadia/TerminalApp/TerminalPage.cpp | 14 ++++++---- .../TerminalControl/TSFInputControl.cpp | 27 ++++++++++++------- .../TerminalControl/TSFInputControl.h | 12 +++++++++ .../TerminalControl/TSFInputControl.idl | 2 ++ src/cascadia/TerminalControl/TermControl.cpp | 2 ++ 9 files changed, 71 insertions(+), 14 deletions(-) diff --git a/src/cascadia/TerminalApp/Pane.cpp b/src/cascadia/TerminalApp/Pane.cpp index aaa42789104..0d54c0b9f78 100644 --- a/src/cascadia/TerminalApp/Pane.cpp +++ b/src/cascadia/TerminalApp/Pane.cpp @@ -358,6 +358,25 @@ void Pane::Close() _ClosedHandlers(nullptr, nullptr); } +// Method Description: +// - Prepare this pane to be removed from the UI hierarchy by closing all controls +// and connections beneath it. +void Pane::Shutdown() +{ + // Lock the create/close lock so that another operation won't concurrently + // modify our tree + std::unique_lock lock{ _createCloseLock }; + if (_IsLeaf()) + { + _control.Close(); + } + else + { + _firstChild->Shutdown(); + _secondChild->Shutdown(); + } +} + // Method Description: // - Get the root UIElement of this pane. There may be a single TermControl as a // child, or an entire tree of grids and panes as children of this element. diff --git a/src/cascadia/TerminalApp/Pane.h b/src/cascadia/TerminalApp/Pane.h index cbc7072764a..d702634e28f 100644 --- a/src/cascadia/TerminalApp/Pane.h +++ b/src/cascadia/TerminalApp/Pane.h @@ -64,6 +64,7 @@ class Pane : public std::enable_shared_from_this const winrt::Microsoft::Terminal::TerminalControl::TermControl& control); float CalcSnappedDimension(const bool widthOrHeight, const float dimension) const; + void Shutdown(); void Close(); WINRT_CALLBACK(Closed, winrt::Windows::Foundation::EventHandler); diff --git a/src/cascadia/TerminalApp/Tab.cpp b/src/cascadia/TerminalApp/Tab.cpp index cea9db3470c..3e5e0cd39b9 100644 --- a/src/cascadia/TerminalApp/Tab.cpp +++ b/src/cascadia/TerminalApp/Tab.cpp @@ -298,6 +298,13 @@ void Tab::NavigateFocus(const winrt::TerminalApp::Direction& direction) _rootPane->NavigateFocus(direction); } +// Method Description: +// - Prepares this tab for being removed from the UI hierarchy by shutting down all active connections. +void Tab::Shutdown() +{ + _rootPane->Shutdown(); +} + // Method Description: // - Closes the currently focused pane in this tab. If it's the last pane in // this tab, our Closed event will be fired (at a later time) for anyone diff --git a/src/cascadia/TerminalApp/Tab.h b/src/cascadia/TerminalApp/Tab.h index 56a97f75653..2ed003a05eb 100644 --- a/src/cascadia/TerminalApp/Tab.h +++ b/src/cascadia/TerminalApp/Tab.h @@ -37,6 +37,7 @@ class Tab : public std::enable_shared_from_this winrt::hstring GetActiveTitle() const; winrt::fire_and_forget SetTabText(const winrt::hstring text); + void Shutdown(); void ClosePane(); WINRT_CALLBACK(Closed, winrt::Windows::Foundation::EventHandler); diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index 70e98b95676..6e23596ded2 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -753,16 +753,20 @@ namespace winrt::TerminalApp::implementation // - tabIndex: the index of the tab to be removed void TerminalPage::_RemoveTabViewItemByIndex(uint32_t tabIndex) { + // Removing the tab from the collection should destroy its control and disconnect its connection, + // but it doesn't always do so. The UI tree may still be holding the control and preventing its destruction. + auto iterator = _tabs.begin() + tabIndex; + (*iterator)->Shutdown(); + + _tabs.erase(iterator); + _tabView.TabItems().RemoveAt(tabIndex); + // To close the window here, we need to close the hosting window. - if (_tabs.size() == 1) + if (_tabs.size() == 0) { _lastTabClosedHandlers(*this, nullptr); } - // Removing the tab from the collection will destroy its control and disconnect its connection. - _tabs.erase(_tabs.begin() + tabIndex); - _tabView.TabItems().RemoveAt(tabIndex); - auto focusedTabIndex = _GetFocusedTabIndex(); if (gsl::narrow_cast(tabIndex) == focusedTabIndex) { diff --git a/src/cascadia/TerminalControl/TSFInputControl.cpp b/src/cascadia/TerminalControl/TSFInputControl.cpp index 5fc6bafa3c2..9ebce09a47d 100644 --- a/src/cascadia/TerminalControl/TSFInputControl.cpp +++ b/src/cascadia/TerminalControl/TSFInputControl.cpp @@ -59,23 +59,32 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation // set the input scope to Text because this control is for any text. _editContext.InputScope(Core::CoreTextInputScope::Text); - _editContext.TextRequested({ this, &TSFInputControl::_textRequestedHandler }); + _textRequestedRevoker = _editContext.TextRequested(winrt::auto_revoke, { this, &TSFInputControl::_textRequestedHandler }); - _editContext.SelectionRequested({ this, &TSFInputControl::_selectionRequestedHandler }); + _selectionRequestedRevoker = _editContext.SelectionRequested(winrt::auto_revoke, { this, &TSFInputControl::_selectionRequestedHandler }); - _editContext.FocusRemoved({ this, &TSFInputControl::_focusRemovedHandler }); + _focusRemovedRevoker = _editContext.FocusRemoved(winrt::auto_revoke, { this, &TSFInputControl::_focusRemovedHandler }); - _editContext.TextUpdating({ this, &TSFInputControl::_textUpdatingHandler }); + _textUpdatingRevoker = _editContext.TextUpdating(winrt::auto_revoke, { this, &TSFInputControl::_textUpdatingHandler }); - _editContext.SelectionUpdating({ this, &TSFInputControl::_selectionUpdatingHandler }); + _selectionUpdatingRevoker = _editContext.SelectionUpdating(winrt::auto_revoke, { this, &TSFInputControl::_selectionUpdatingHandler }); - _editContext.FormatUpdating({ this, &TSFInputControl::_formatUpdatingHandler }); + _formatUpdatingRevoker = _editContext.FormatUpdating(winrt::auto_revoke, { this, &TSFInputControl::_formatUpdatingHandler }); - _editContext.LayoutRequested({ this, &TSFInputControl::_layoutRequestedHandler }); + _layoutRequestedRevoker = _editContext.LayoutRequested(winrt::auto_revoke, { this, &TSFInputControl::_layoutRequestedHandler }); - _editContext.CompositionStarted({ this, &TSFInputControl::_compositionStartedHandler }); + _compositionStartedRevoker = _editContext.CompositionStarted(winrt::auto_revoke, { this, &TSFInputControl::_compositionStartedHandler }); - _editContext.CompositionCompleted({ this, &TSFInputControl::_compositionCompletedHandler }); + _compositionCompletedRevoker = _editContext.CompositionCompleted(winrt::auto_revoke, { this, &TSFInputControl::_compositionCompletedHandler }); + } + + // Method Description: + // - Prepares this TSFInputControl to be removed from the UI hierarchy. + void TSFInputControl::Close() + { + // Explicitly disconnect the LayoutRequested handler -- it can cause problems during application teardown. + // See GH#4159 for more info. + _layoutRequestedRevoker.revoke(); } // Method Description: diff --git a/src/cascadia/TerminalControl/TSFInputControl.h b/src/cascadia/TerminalControl/TSFInputControl.h index 91d7c59c759..d6e7b2501dd 100644 --- a/src/cascadia/TerminalControl/TSFInputControl.h +++ b/src/cascadia/TerminalControl/TSFInputControl.h @@ -37,6 +37,8 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation void NotifyFocusEnter(); void NotifyFocusLeave(); + void Close(); + static void OnCompositionChanged(Windows::UI::Xaml::DependencyObject const&, Windows::UI::Xaml::DependencyPropertyChangedEventArgs const&); // -------------------------------- WinRT Events --------------------------------- @@ -55,6 +57,16 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation void _textUpdatingHandler(winrt::Windows::UI::Text::Core::CoreTextEditContext sender, winrt::Windows::UI::Text::Core::CoreTextTextUpdatingEventArgs const& args); void _formatUpdatingHandler(winrt::Windows::UI::Text::Core::CoreTextEditContext sender, winrt::Windows::UI::Text::Core::CoreTextFormatUpdatingEventArgs const& args); + winrt::Windows::UI::Text::Core::CoreTextEditContext::TextRequested_revoker _textRequestedRevoker; + winrt::Windows::UI::Text::Core::CoreTextEditContext::SelectionRequested_revoker _selectionRequestedRevoker; + winrt::Windows::UI::Text::Core::CoreTextEditContext::FocusRemoved_revoker _focusRemovedRevoker; + winrt::Windows::UI::Text::Core::CoreTextEditContext::TextUpdating_revoker _textUpdatingRevoker; + winrt::Windows::UI::Text::Core::CoreTextEditContext::SelectionUpdating_revoker _selectionUpdatingRevoker; + winrt::Windows::UI::Text::Core::CoreTextEditContext::FormatUpdating_revoker _formatUpdatingRevoker; + winrt::Windows::UI::Text::Core::CoreTextEditContext::LayoutRequested_revoker _layoutRequestedRevoker; + winrt::Windows::UI::Text::Core::CoreTextEditContext::CompositionStarted_revoker _compositionStartedRevoker; + winrt::Windows::UI::Text::Core::CoreTextEditContext::CompositionCompleted_revoker _compositionCompletedRevoker; + Windows::UI::Xaml::Controls::Canvas _canvas; Windows::UI::Xaml::Controls::TextBlock _textBlock; diff --git a/src/cascadia/TerminalControl/TSFInputControl.idl b/src/cascadia/TerminalControl/TSFInputControl.idl index f2eaa9a5f55..d97ebcf20ba 100644 --- a/src/cascadia/TerminalControl/TSFInputControl.idl +++ b/src/cascadia/TerminalControl/TSFInputControl.idl @@ -27,5 +27,7 @@ namespace Microsoft.Terminal.TerminalControl void NotifyFocusEnter(); void NotifyFocusLeave(); + + void Close(); } } diff --git a/src/cascadia/TerminalControl/TermControl.cpp b/src/cascadia/TerminalControl/TermControl.cpp index 6d5703210c0..07705cea884 100644 --- a/src/cascadia/TerminalControl/TermControl.cpp +++ b/src/cascadia/TerminalControl/TermControl.cpp @@ -1734,6 +1734,8 @@ namespace winrt::Microsoft::Terminal::TerminalControl::implementation _connection.TerminalOutput(_connectionOutputEventToken); _connectionStateChangedRevoker.revoke(); + _tsfInputControl.Close(); // Disconnect the TSF input control so it doesn't receive EditContext events. + if (auto localConnection{ std::exchange(_connection, nullptr) }) { localConnection.Close();