diff --git a/src/cascadia/TerminalApp/Pane.cpp b/src/cascadia/TerminalApp/Pane.cpp index fd195d8d42a..899e67e14ec 100644 --- a/src/cascadia/TerminalApp/Pane.cpp +++ b/src/cascadia/TerminalApp/Pane.cpp @@ -467,7 +467,7 @@ std::shared_ptr Pane::NextPane(const std::shared_ptr targetPane) std::shared_ptr nextPane = nullptr; auto foundTarget = false; - auto foundNext = WalkTree([&](auto pane) { + auto foundNext = WalkTree([&](const auto& pane) { // If we are a parent pane we don't want to move to one of our children if (foundTarget && targetPane->_HasChild(pane)) { @@ -985,6 +985,17 @@ 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; // Fire our Closed event to tell our parent that we should be removed. Closed.raise(nullptr, nullptr); } @@ -1341,7 +1352,7 @@ std::shared_ptr Pane::DetachPane(std::shared_ptr pane) detached->_ApplySplitDefinitions(); // Trigger the detached event on each child - detached->WalkTree([](auto pane) { + detached->WalkTree([](const auto& pane) { pane->Detached.raise(pane); }); @@ -1542,7 +1553,7 @@ void Pane::_CloseChild(const bool closeFirst) { // update our path to our first remaining leaf _parentChildPath = _firstChild; - _firstChild->WalkTree([](auto p) { + _firstChild->WalkTree([](const auto& p) { if (p->_IsLeaf()) { return true; @@ -2398,7 +2409,7 @@ void Pane::Id(uint32_t id) noexcept bool Pane::FocusPane(const uint32_t id) { // Always clear the parent child path if we are focusing a leaf - return WalkTree([=](auto p) { + return WalkTree([=](const auto& p) { p->_parentChildPath.reset(); if (p->_id == id) { @@ -2421,7 +2432,7 @@ bool Pane::FocusPane(const uint32_t id) // - true if focus was set bool Pane::FocusPane(const std::shared_ptr pane) { - return WalkTree([&](auto p) { + return WalkTree([&](const auto& p) { if (p == pane) { p->_Focus(); diff --git a/src/cascadia/TerminalApp/Pane.h b/src/cascadia/TerminalApp/Pane.h index f25a7b0834a..86757ad36f9 100644 --- a/src/cascadia/TerminalApp/Pane.h +++ b/src/cascadia/TerminalApp/Pane.h @@ -248,7 +248,7 @@ class Pane : public std::enable_shared_from_this std::optional _id; std::weak_ptr _parentChildPath{}; - + bool _closed{ false }; bool _lastActive{ false }; winrt::event_token _firstClosedToken{ 0 }; winrt::event_token _secondClosedToken{ 0 }; diff --git a/src/cascadia/TerminalApp/TabManagement.cpp b/src/cascadia/TerminalApp/TabManagement.cpp index 3053d746df2..8ef5a36584e 100644 --- a/src/cascadia/TerminalApp/TabManagement.cpp +++ b/src/cascadia/TerminalApp/TabManagement.cpp @@ -739,7 +739,7 @@ namespace winrt::TerminalApp::implementation } // Clean read-only mode to prevent additional prompt if closing the pane triggers closing of a hosting tab - pane->WalkTree([](auto p) { + pane->WalkTree([](const auto& p) { if (const auto control{ p->GetTerminalControl() }) { if (control.ReadOnly()) diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index ca86bb80a37..54a3e4efe09 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -3810,7 +3810,7 @@ namespace winrt::TerminalApp::implementation // recipe for disaster. We won't ever open up a tab in this window. newTerminalArgs.Elevate(false); const auto newPane = _MakePane(newTerminalArgs, nullptr, connection); - newPane->WalkTree([](auto pane) { + newPane->WalkTree([](const auto& pane) { pane->FinalizeConfigurationGivenDefault(); }); _CreateNewTabFromPane(newPane); diff --git a/src/cascadia/TerminalApp/TerminalTab.cpp b/src/cascadia/TerminalApp/TerminalTab.cpp index d3e52d4037d..6e1be17e38b 100644 --- a/src/cascadia/TerminalApp/TerminalTab.cpp +++ b/src/cascadia/TerminalApp/TerminalTab.cpp @@ -41,7 +41,7 @@ namespace winrt::TerminalApp::implementation auto firstId = _nextPaneId; - _rootPane->WalkTree([&](std::shared_ptr pane) { + _rootPane->WalkTree([&](const auto& pane) { // update the IDs on each pane if (pane->_IsLeaf()) { @@ -203,7 +203,7 @@ namespace winrt::TerminalApp::implementation { ASSERT_UI_THREAD(); - _rootPane->WalkTree([&](std::shared_ptr pane) { + _rootPane->WalkTree([&](const auto& pane) { // Attach event handlers to each new pane _AttachEventHandlersToPane(pane); if (auto content = pane->GetContent()) @@ -275,7 +275,7 @@ namespace winrt::TerminalApp::implementation _UpdateHeaderControlMaxWidth(); // Update the settings on all our panes. - _rootPane->WalkTree([&](auto pane) { + _rootPane->WalkTree([&](const auto& pane) { pane->UpdateSettings(settings); return false; }); @@ -534,7 +534,7 @@ namespace winrt::TerminalApp::implementation // Add the new event handlers to the new pane(s) // and update their ids. - pane->WalkTree([&](auto p) { + pane->WalkTree([&](const auto& p) { _AttachEventHandlersToPane(p); if (p->_IsLeaf()) { @@ -624,7 +624,7 @@ namespace winrt::TerminalApp::implementation // manually. _rootPane->Closed(_rootClosedToken); auto p = _rootPane; - p->WalkTree([](auto pane) { + p->WalkTree([](const auto& pane) { pane->Detached.raise(pane); }); @@ -650,7 +650,7 @@ namespace winrt::TerminalApp::implementation // Add the new event handlers to the new pane(s) // and update their ids. - pane->WalkTree([&](auto p) { + pane->WalkTree([&](const auto& p) { _AttachEventHandlersToPane(p); if (p->_IsLeaf()) { @@ -949,26 +949,20 @@ namespace winrt::TerminalApp::implementation events.CloseRequested = content.CloseRequested( winrt::auto_revoke, - [dispatcher, weakThis](auto sender, auto&&) -> winrt::fire_and_forget { - // Don't forget! this ^^^^^^^^ sender can't be a reference, this is a async callback. - - // The lambda lives in the `std::function`-style container owned by `control`. That is, when the - // `control` gets destroyed the lambda struct also gets destroyed. In other words, we need to - // copy `weakThis` onto the stack, because that's the only thing that gets captured in coroutines. - // See: https://devblogs.microsoft.com/oldnewthing/20211103-00/?p=105870 - const auto weakThisCopy = weakThis; - co_await wil::resume_foreground(dispatcher); - // Check if Tab's lifetime has expired - if (auto tab{ weakThisCopy.get() }) + [this](auto&& sender, auto&&) { + if (const auto content{ sender.try_as() }) { - 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) { - tab->_rootPane->WalkTree([content](std::shared_ptr pane) { - if (pane->GetContent() == content) - { - pane->Close(); - } - }); + pane->Close(); } } });