diff --git a/src/cascadia/TerminalApp/Pane.cpp b/src/cascadia/TerminalApp/Pane.cpp index 899e67e14e..29a4e0b67e 100644 --- a/src/cascadia/TerminalApp/Pane.cpp +++ b/src/cascadia/TerminalApp/Pane.cpp @@ -27,10 +27,10 @@ static const int CombinedPaneBorderSize = 2 * PaneBorderSize; static const int AnimationDurationInMilliseconds = 200; static const Duration AnimationDuration = DurationHelper::FromTimeSpan(winrt::Windows::Foundation::TimeSpan(std::chrono::milliseconds(AnimationDurationInMilliseconds))); -Pane::Pane(const IPaneContent& content, const bool lastFocused) : - _content{ content }, +Pane::Pane(IPaneContent content, const bool lastFocused) : _lastActive{ lastFocused } { + _setPaneContent(std::move(content)); _root.Children().Append(_borderFirst); const auto& control{ _content.GetRoot() }; @@ -985,17 +985,7 @@ void Pane::_ContentLostFocusHandler(const winrt::Windows::Foundation::IInspectab // - void Pane::Close() { - // Pane has two events, CloseRequested and Closed. CloseRequested is raised by the content asking to be closed, - // but also by the window who owns the tab when it's closing. The event is then caught by the TerminalTab which - // calls Close() which then raises the Closed event. Now, if this is the last pane in the window, this will result - // in the window raising CloseRequested again which leads to infinite recursion, so we need to guard against that. - // Ideally we would have just a single event in the future. - if (_closed) - { - return; - } - - _closed = true; + _setPaneContent(nullptr); // Fire our Closed event to tell our parent that we should be removed. Closed.raise(nullptr, nullptr); } @@ -1007,7 +997,7 @@ void Pane::Shutdown() { if (_IsLeaf()) { - _content.Close(); + _setPaneContent(nullptr); } else { @@ -1411,7 +1401,7 @@ void Pane::_CloseChild(const bool closeFirst) _borders = _GetCommonBorders(); // take the control, profile, id and isDefTermSession of the pane that _wasn't_ closed. - _content = remainingChild->_content; + _setPaneContent(std::move(remainingChild->_content)); _id = remainingChild->Id(); // Revoke the old event handlers. Remove both the handlers for the panes @@ -1716,6 +1706,25 @@ void Pane::_SetupChildCloseHandlers() }); } +void Pane::_setPaneContent(IPaneContent content) +{ + // The IPaneContent::Close() implementation may be buggy and raise the CloseRequested event again. + // To avoid this we unbind the event handlers before calling Close(). + _closeRequestedRevoker.revoke(); + + if (_content) + { + _content.Close(); + } + + _content = std::move(content); + + if (_content) + { + _closeRequestedRevoker = _content.CloseRequested(winrt::auto_revoke, [this](auto&&, auto&&) { Close(); }); + } +} + // Method Description: // - Sets up row/column definitions for this pane. There are three total // row/cols. The middle one is for the separator. The first and third are for @@ -2266,8 +2275,8 @@ std::pair, std::shared_ptr> Pane::_Split(SplitDirect else { // Move our control, guid, isDefTermSession into the first one. - _firstChild = std::make_shared(_content); - _content = nullptr; + _firstChild = std::make_shared(std::move(_content)); + _setPaneContent(nullptr); _firstChild->_broadcastEnabled = _broadcastEnabled; } @@ -2462,6 +2471,11 @@ bool Pane::_HasChild(const std::shared_ptr child) }); } +winrt::TerminalApp::TerminalPaneContent Pane::_getTerminalContent() const +{ + return _IsLeaf() ? _content.try_as() : nullptr; +} + // Method Description: // - Recursive function that finds a pane with the given ID // Arguments: diff --git a/src/cascadia/TerminalApp/Pane.h b/src/cascadia/TerminalApp/Pane.h index 86757ad36f..32c40e16e9 100644 --- a/src/cascadia/TerminalApp/Pane.h +++ b/src/cascadia/TerminalApp/Pane.h @@ -62,7 +62,7 @@ struct PaneResources class Pane : public std::enable_shared_from_this { public: - Pane(const winrt::TerminalApp::IPaneContent& content, + Pane(winrt::TerminalApp::IPaneContent content, const bool lastFocused = false); Pane(std::shared_ptr first, @@ -248,13 +248,13 @@ private: std::optional _id; std::weak_ptr _parentChildPath{}; - bool _closed{ false }; bool _lastActive{ false }; winrt::event_token _firstClosedToken{ 0 }; winrt::event_token _secondClosedToken{ 0 }; winrt::Windows::UI::Xaml::UIElement::GotFocus_revoker _gotFocusRevoker; winrt::Windows::UI::Xaml::UIElement::LostFocus_revoker _lostFocusRevoker; + winrt::TerminalApp::IPaneContent::CloseRequested_revoker _closeRequestedRevoker; Borders _borders{ Borders::None }; @@ -264,11 +264,9 @@ private: bool _IsLeaf() const noexcept; bool _HasFocusedChild() const noexcept; void _SetupChildCloseHandlers(); + void _setPaneContent(winrt::TerminalApp::IPaneContent content); bool _HasChild(const std::shared_ptr child); - winrt::TerminalApp::TerminalPaneContent _getTerminalContent() const - { - return _IsLeaf() ? _content.try_as() : nullptr; - } + winrt::TerminalApp::TerminalPaneContent _getTerminalContent() const; std::pair, std::shared_ptr> _Split(winrt::Microsoft::Terminal::Settings::Model::SplitDirection splitType, const float splitSize, diff --git a/src/cascadia/TerminalApp/ScratchpadContent.cpp b/src/cascadia/TerminalApp/ScratchpadContent.cpp index 16346c9b03..b1a05c6d38 100644 --- a/src/cascadia/TerminalApp/ScratchpadContent.cpp +++ b/src/cascadia/TerminalApp/ScratchpadContent.cpp @@ -45,7 +45,6 @@ namespace winrt::TerminalApp::implementation } void ScratchpadContent::Close() { - CloseRequested.raise(*this, nullptr); } INewContentArgs ScratchpadContent::GetNewTerminalArgs(const BuildStartupKind /* kind */) const diff --git a/src/cascadia/TerminalApp/SettingsPaneContent.cpp b/src/cascadia/TerminalApp/SettingsPaneContent.cpp index 7e48ae9f6b..9d514e1600 100644 --- a/src/cascadia/TerminalApp/SettingsPaneContent.cpp +++ b/src/cascadia/TerminalApp/SettingsPaneContent.cpp @@ -47,7 +47,6 @@ namespace winrt::TerminalApp::implementation } void SettingsPaneContent::Close() { - CloseRequested.raise(*this, nullptr); } INewContentArgs SettingsPaneContent::GetNewTerminalArgs(const BuildStartupKind /*kind*/) const diff --git a/src/cascadia/TerminalApp/TerminalPaneContent.cpp b/src/cascadia/TerminalApp/TerminalPaneContent.cpp index 652486e441..5ecdb4bf56 100644 --- a/src/cascadia/TerminalApp/TerminalPaneContent.cpp +++ b/src/cascadia/TerminalApp/TerminalPaneContent.cpp @@ -78,8 +78,6 @@ namespace winrt::TerminalApp::implementation _bellPlayer = nullptr; _bellPlayerCreated = false; } - - CloseRequested.raise(*this, nullptr); } winrt::hstring TerminalPaneContent::Icon() const @@ -239,19 +237,20 @@ namespace winrt::TerminalApp::implementation if (_profile) { - if (_isDefTermSession && _profile.CloseOnExit() == CloseOnExitMode::Automatic) - { - // For 'automatic', we only care about the connection state if we were launched by Terminal - // Since we were launched via defterm, ignore the connection state (i.e. we treat the - // close on exit mode as 'always', see GH #13325 for discussion) - Close(); - } - const auto mode = _profile.CloseOnExit(); - if ((mode == CloseOnExitMode::Always) || - ((mode == CloseOnExitMode::Graceful || mode == CloseOnExitMode::Automatic) && newConnectionState == ConnectionState::Closed)) + + if ( + // This one is obvious: If the user asked for "always" we do just that. + (mode == CloseOnExitMode::Always) || + // Otherwise, and unless the user asked for the opposite of "always", + // close the pane when the connection closed gracefully (not failed). + (mode != CloseOnExitMode::Never && newConnectionState == ConnectionState::Closed) || + // However, defterm handoff can result in Windows Terminal randomly opening which may be annoying, + // so by default we should at least always close the pane, even if the command failed. + // See GH #13325 for discussion. + (mode == CloseOnExitMode::Automatic && _isDefTermSession)) { - Close(); + CloseRequested.raise(nullptr, nullptr); } } } @@ -331,7 +330,7 @@ namespace winrt::TerminalApp::implementation void TerminalPaneContent::_closeTerminalRequestedHandler(const winrt::Windows::Foundation::IInspectable& /*sender*/, const winrt::Windows::Foundation::IInspectable& /*args*/) { - Close(); + CloseRequested.raise(nullptr, nullptr); } void TerminalPaneContent::_restartTerminalRequestedHandler(const winrt::Windows::Foundation::IInspectable& /*sender*/, diff --git a/src/cascadia/TerminalApp/TerminalTab.cpp b/src/cascadia/TerminalApp/TerminalTab.cpp index 0f0303e323..72995e29ba 100644 --- a/src/cascadia/TerminalApp/TerminalTab.cpp +++ b/src/cascadia/TerminalApp/TerminalTab.cpp @@ -947,26 +947,6 @@ namespace winrt::TerminalApp::implementation auto dispatcher = TabViewItem().Dispatcher(); ContentEventTokens events{}; - events.CloseRequested = content.CloseRequested( - winrt::auto_revoke, - [this](auto&& sender, auto&&) { - if (const auto content{ sender.try_as() }) - { - // Calling Close() while walking the tree is not safe, because Close() mutates the tree. - const auto pane = _rootPane->_FindPane([&](const auto& p) -> std::shared_ptr { - if (p->GetContent() == content) - { - return p; - } - return {}; - }); - if (pane) - { - pane->Close(); - } - } - }); - events.TitleChanged = content.TitleChanged( winrt::auto_revoke, [dispatcher, weakThis](auto&&, auto&&) -> winrt::fire_and_forget { diff --git a/src/cascadia/TerminalApp/TerminalTab.h b/src/cascadia/TerminalApp/TerminalTab.h index 2f395f0361..f38895a3da 100644 --- a/src/cascadia/TerminalApp/TerminalTab.h +++ b/src/cascadia/TerminalApp/TerminalTab.h @@ -134,7 +134,6 @@ namespace winrt::TerminalApp::implementation winrt::TerminalApp::IPaneContent::ConnectionStateChanged_revoker ConnectionStateChanged; winrt::TerminalApp::IPaneContent::ReadOnlyChanged_revoker ReadOnlyChanged; winrt::TerminalApp::IPaneContent::FocusRequested_revoker FocusRequested; - winrt::TerminalApp::IPaneContent::CloseRequested_revoker CloseRequested; // These events literally only apply if the content is a TermControl. winrt::Microsoft::Terminal::Control::TermControl::KeySent_revoker KeySent;