PRE-MERGE microsoft#17358 Fix two panes being closed when just one is

Author: zadjii-msft

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() };
@@ -1172,17 +1172,7 @@ void Pane::_ContentLostFocusHandler(const winrt::Windows::Foundation::IInspectab
 // - <none>
 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);
 }
@@ -1194,7 +1184,7 @@ void Pane::Shutdown()
 {
     if (_IsLeaf())
     {
-        _content.Close();
+        _setPaneContent(nullptr);
     }
     else
     {
@@ -1598,7 +1588,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(remainingChild->_takePaneContent());
         _id = remainingChild->Id();
 
         // Revoke the old event handlers. Remove both the handlers for the panes
@@ -1903,6 +1893,34 @@ void Pane::_SetupChildCloseHandlers()
     });
 }
 
+// With this method you take ownership of the control from this Pane.
+// Assign it to another Pane with _setPaneContent() or Close() it.
+IPaneContent Pane::_takePaneContent()
+{
+    _closeRequestedRevoker.revoke();
+    return std::move(_content);
+}
+
+// This method safely sets the content of the Pane. It'll ensure to revoke and
+// assign event handlers, and to Close() the existing content if there's any.
+// The new content can be nullptr to remove any content.
+void Pane::_setPaneContent(IPaneContent content)
+{
+    // The IPaneContent::Close() implementation may be buggy and raise the CloseRequested event again.
+    // _takePaneContent() avoids this as it revokes the event handler.
+    if (const auto c = _takePaneContent())
+    {
+        c.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
@@ -2470,8 +2488,7 @@ std::pair<std::shared_ptr<Pane>, std::shared_ptr<Pane>> Pane::_Split(SplitDirect
     else
     {
         //   Move our control, guid, isDefTermSession into the first one.
-        _firstChild = std::make_shared<Pane>(_content);
-        _content = nullptr;
+        _firstChild = std::make_shared<Pane>(_takePaneContent());
         _firstChild->_broadcastEnabled = _broadcastEnabled;
     }
 
@@ -2669,6 +2686,11 @@ bool Pane::_HasChild(const std::shared_ptr<Pane> child)
     });
 }
 
+winrt::TerminalApp::TerminalPaneContent Pane::_getTerminalContent() const
+{
+    return _IsLeaf() ? _content.try_as<winrt::TerminalApp::TerminalPaneContent>() : nullptr;
+}
+
 // Method Description:
 // - Recursive function that finds a pane with the given ID
 // Arguments:

src/cascadia/TerminalApp/Pane.h

@@ -62,7 +62,7 @@ struct PaneResources
 class Pane : public std::enable_shared_from_this<Pane>
 {
 public:
-    Pane(const winrt::TerminalApp::IPaneContent& content,
+    Pane(winrt::TerminalApp::IPaneContent content,
          const bool lastFocused = false);
 
     Pane(std::shared_ptr<Pane> first,
@@ -250,7 +250,6 @@ class Pane : public std::enable_shared_from_this<Pane>
 
     std::optional<uint32_t> _id;
     std::weak_ptr<Pane> _parentChildPath{};
-    bool _closed{ false };
     bool _lastActive{ false };
     winrt::event_token _firstClosedToken{ 0 };
     winrt::event_token _secondClosedToken{ 0 };
@@ -260,10 +259,13 @@ class Pane : public std::enable_shared_from_this<Pane>
 
     winrt::Windows::UI::Xaml::UIElement::GotFocus_revoker _gotFocusRevoker;
     winrt::Windows::UI::Xaml::UIElement::LostFocus_revoker _lostFocusRevoker;
+
     winrt::Windows::UI::Xaml::UIElement::ManipulationDelta_revoker _manipulationDeltaRevoker;
     winrt::Windows::UI::Xaml::UIElement::ManipulationStarted_revoker _manipulationStartedRevoker;
     bool _shouldManipulate{ false };
 
+    winrt::TerminalApp::IPaneContent::CloseRequested_revoker _closeRequestedRevoker;
+
     Borders _borders{ Borders::None };
 
     bool _zoomed{ false };
@@ -272,11 +274,10 @@ class Pane : public std::enable_shared_from_this<Pane>
     bool _IsLeaf() const noexcept;
     bool _HasFocusedChild() const noexcept;
     void _SetupChildCloseHandlers();
+    winrt::TerminalApp::IPaneContent _takePaneContent();
+    void _setPaneContent(winrt::TerminalApp::IPaneContent content);
     bool _HasChild(const std::shared_ptr<Pane> child);
-    winrt::TerminalApp::TerminalPaneContent _getTerminalContent() const
-    {
-        return _IsLeaf() ? _content.try_as<winrt::TerminalApp::TerminalPaneContent>() : nullptr;
-    }
+    winrt::TerminalApp::TerminalPaneContent _getTerminalContent() const;
 
     std::pair<std::shared_ptr<Pane>, std::shared_ptr<Pane>> _Split(winrt::Microsoft::Terminal::Settings::Model::SplitDirection splitType,
                                                                    const float splitSize,

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

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

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*/,

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<TerminalApp::IPaneContent>() })
-                {
-                    // 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<Pane> {
-                        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 {

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;

src/cascadia/TerminalConnection/ConptyConnection.cpp

@@ -446,12 +446,10 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
         try
         {
             // GH#11556 - make sure to format the error code to this string as an UNSIGNED int
-            winrt::hstring exitText{ fmt::format(std::wstring_view{ RS_(L"ProcessExited") }, fmt::format(_errorFormat, status)) };
-            TerminalOutput.raise(L"\r\n");
-            TerminalOutput.raise(exitText);
-            TerminalOutput.raise(L"\r\n");
-            TerminalOutput.raise(RS_(L"CtrlDToClose"));
-            TerminalOutput.raise(L"\r\n");
+            const auto msg1 = fmt::format(std::wstring_view{ RS_(L"ProcessExited") }, fmt::format(_errorFormat, status));
+            const auto msg2 = RS_(L"CtrlDToClose");
+            const auto msg = fmt::format(FMT_COMPILE(L"\r\n{}\r\n{}\r\n"), msg1, msg2);
+            TerminalOutput.raise(msg);
         }
         CATCH_LOG();
     }