From 0df82681fe057d292f64c424d493e05cb1bfe121 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Tue, 25 Feb 2025 11:50:06 -0800 Subject: [PATCH 1/5] Reduce log spam on conhost exit (#18629) When the server handle gets closed on conhost (= terminal is gone), and e.g. PowerShell is used, we would previously log 6 error messages. This PR reduces it to zero, by removing the 3 biggest offenders. --- src/host/srvinit.cpp | 2 +- src/server/ApiMessage.cpp | 12 ++++-------- src/server/ApiMessage.h | 2 +- src/server/ConDrvDeviceComm.cpp | 17 +++++++++-------- src/server/WaitBlock.cpp | 6 ++++-- 5 files changed, 19 insertions(+), 20 deletions(-) diff --git a/src/host/srvinit.cpp b/src/host/srvinit.cpp index 97bcd8f2767..a6627bde993 100644 --- a/src/host/srvinit.cpp +++ b/src/host/srvinit.cpp @@ -991,7 +991,7 @@ DWORD WINAPI ConsoleIoThread(LPVOID lpParameter) { if (ReplyMsg != nullptr) { - LOG_IF_FAILED(ReplyMsg->ReleaseMessageBuffers()); + ReplyMsg->ReleaseMessageBuffers(); } // TODO: 9115192 correct mixed NTSTATUS/HRESULT diff --git a/src/server/ApiMessage.cpp b/src/server/ApiMessage.cpp index 2603ba0a62b..abb2a3edd3f 100644 --- a/src/server/ApiMessage.cpp +++ b/src/server/ApiMessage.cpp @@ -180,12 +180,8 @@ CATCH_RETURN(); // (if any) to the message. // Arguments: // - -// Return Value: -// - HRESULT indicating if the payload was successfully written if applicable. -[[nodiscard]] HRESULT _CONSOLE_API_MSG::ReleaseMessageBuffers() +void _CONSOLE_API_MSG::ReleaseMessageBuffers() { - auto hr = S_OK; - if (State.InputBuffer != nullptr) { _inputBuffer.clear(); @@ -203,15 +199,15 @@ CATCH_RETURN(); IoOperation.Buffer.Data = State.OutputBuffer; IoOperation.Buffer.Size = (ULONG)Complete.IoStatus.Information; - LOG_IF_FAILED(_pDeviceComm->WriteOutput(&IoOperation)); + // This call fails when the server pipe is closed on us, + // which results in log spam in practice. + std::ignore = _pDeviceComm->WriteOutput(&IoOperation); } _outputBuffer.clear(); State.OutputBuffer = nullptr; State.OutputBufferSize = 0; } - - return hr; } void _CONSOLE_API_MSG::SetReplyStatus(const NTSTATUS Status) diff --git a/src/server/ApiMessage.h b/src/server/ApiMessage.h index 6c1809aa9d7..a990ef5481a 100644 --- a/src/server/ApiMessage.h +++ b/src/server/ApiMessage.h @@ -42,7 +42,7 @@ typedef struct _CONSOLE_API_MSG [[nodiscard]] HRESULT GetOutputBuffer(_Outptr_result_bytebuffer_(*pcbSize) void** const ppvBuffer, _Out_ ULONG* const pcbSize); [[nodiscard]] HRESULT GetInputBuffer(_Outptr_result_bytebuffer_(*pcbSize) void** const ppvBuffer, _Out_ ULONG* const pcbSize); - [[nodiscard]] HRESULT ReleaseMessageBuffers(); + void ReleaseMessageBuffers(); void SetReplyStatus(const NTSTATUS Status); void SetReplyInformation(const ULONG_PTR pInformation); diff --git a/src/server/ConDrvDeviceComm.cpp b/src/server/ConDrvDeviceComm.cpp index 2bad88d605c..3d9ddba52b1 100644 --- a/src/server/ConDrvDeviceComm.cpp +++ b/src/server/ConDrvDeviceComm.cpp @@ -137,14 +137,15 @@ ConDrvDeviceComm::~ConDrvDeviceComm() = default; // See: https://msdn.microsoft.com/en-us/library/windows/desktop/aa363216(v=vs.85).aspx // Written is unused but cannot be nullptr because we aren't using overlapped. DWORD cbWritten = 0; - RETURN_IF_WIN32_BOOL_FALSE(DeviceIoControl(_Server.get(), - dwIoControlCode, - pInBuffer, - cbInBufferSize, - pOutBuffer, - cbOutBufferSize, - &cbWritten, - nullptr)); + RETURN_IF_WIN32_BOOL_FALSE_EXPECTED(DeviceIoControl( + _Server.get(), + dwIoControlCode, + pInBuffer, + cbInBufferSize, + pOutBuffer, + cbOutBufferSize, + &cbWritten, + nullptr)); return S_OK; } diff --git a/src/server/WaitBlock.cpp b/src/server/WaitBlock.cpp index 49a4663f1cd..b26dffcb58e 100644 --- a/src/server/WaitBlock.cpp +++ b/src/server/WaitBlock.cpp @@ -223,9 +223,11 @@ bool ConsoleWaitBlock::Notify(const WaitTerminationReason TerminationReason) a->NumBytes = gsl::narrow(NumBytes); } - LOG_IF_FAILED(_WaitReplyMessage.ReleaseMessageBuffers()); + _WaitReplyMessage.ReleaseMessageBuffers(); - LOG_IF_FAILED(Microsoft::Console::Interactivity::ServiceLocator::LocateGlobals().pDeviceComm->CompleteIo(&_WaitReplyMessage.Complete)); + // This call fails when the server pipe is closed on us, + // which results in log spam in practice. + std::ignore = Microsoft::Console::Interactivity::ServiceLocator::LocateGlobals().pDeviceComm->CompleteIo(&_WaitReplyMessage.Complete); fRetVal = true; } From ff9664d2d45349193ac2a17ffedf9d26128f8699 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Tue, 25 Feb 2025 11:50:25 -0800 Subject: [PATCH 2/5] Fix two sources of runtime exceptions (#18628) * `_ApplyLanguageSettingChange` calls `PrimaryLanguageOverride` (the WinRT API function) and we would call it every time a new window is created. Now it's only called on settings load. * `_RegisterTabEvents` would listen for "Content" changes which can be null. `IVector::Append` throws if a null object is given. In our case, it's null if the content got erased with nothing. Additionally, this fixes a bug where we wouldn't call `_ProcessLazySettingsChanges` on startup. This is important if the settings file was changed while Windows Terminal wasn't running. Lastly, there's a lifetime fix in this PR, which is a one-line change and I didn't want to make a separate PR for that. --- src/cascadia/TerminalApp/AppLogic.cpp | 8 +++----- src/cascadia/TerminalApp/TabPaletteItem.cpp | 6 ++++-- src/cascadia/TerminalApp/TerminalPage.cpp | 14 ++++++++++---- 3 files changed, 17 insertions(+), 11 deletions(-) diff --git a/src/cascadia/TerminalApp/AppLogic.cpp b/src/cascadia/TerminalApp/AppLogic.cpp index a022be1e6a2..b892c10cb55 100644 --- a/src/cascadia/TerminalApp/AppLogic.cpp +++ b/src/cascadia/TerminalApp/AppLogic.cpp @@ -182,8 +182,6 @@ namespace winrt::TerminalApp::implementation // this as a MTA, before the app is Create()'d WINRT_ASSERT(_loadedInitialSettings); - _ApplyLanguageSettingChange(); - TraceLoggingWrite( g_hTerminalAppProvider, "AppCreated", @@ -399,6 +397,9 @@ namespace winrt::TerminalApp::implementation _settings.LogSettingChanges(true); } + _ApplyLanguageSettingChange(); + _ProcessLazySettingsChanges(); + if (initialLoad) { // Register for directory change notification. @@ -409,9 +410,6 @@ namespace winrt::TerminalApp::implementation // Here, we successfully reloaded the settings, and created a new // TerminalSettings object. - _ApplyLanguageSettingChange(); - _ProcessLazySettingsChanges(); - auto warnings{ winrt::multi_threaded_vector() }; for (auto&& warn : _warnings) { diff --git a/src/cascadia/TerminalApp/TabPaletteItem.cpp b/src/cascadia/TerminalApp/TabPaletteItem.cpp index e3c550206d6..98c447033a3 100644 --- a/src/cascadia/TerminalApp/TabPaletteItem.cpp +++ b/src/cascadia/TerminalApp/TabPaletteItem.cpp @@ -51,8 +51,10 @@ namespace winrt::TerminalApp::implementation _tabStatusChangedRevoker = status.PropertyChanged(winrt::auto_revoke, [weakThis{ get_weak() }](auto& /*sender*/, auto& /*e*/) { // Sometimes nested bindings do not get updated, // thus let's notify property changed on TabStatus when one of its properties changes - auto item{ weakThis.get() }; - item->PropertyChanged.raise(*item, Windows::UI::Xaml::Data::PropertyChangedEventArgs{ L"TabStatus" }); + if (auto item{ weakThis.get() }) + { + item->PropertyChanged.raise(*item, Windows::UI::Xaml::Data::PropertyChangedEventArgs{ L"TabStatus" }); + } }); } } diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index 7604b4d7d56..cb4c0403d97 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -1783,16 +1783,22 @@ namespace winrt::TerminalApp::implementation auto tab{ weakTab.get() }; if (page && tab) { - if (args.PropertyName() == L"Title") + const auto propertyName = args.PropertyName(); + if (propertyName == L"Title") { page->_UpdateTitle(*tab); } - else if (args.PropertyName() == L"Content") + else if (propertyName == L"Content") { if (*tab == page->_GetFocusedTab()) { - page->_tabContent.Children().Clear(); - page->_tabContent.Children().Append(tab->Content()); + const auto children = page->_tabContent.Children(); + + children.Clear(); + if (auto content = tab->Content()) + { + page->_tabContent.Children().Append(std::move(content)); + } tab->Focus(FocusState::Programmatic); } From e5b972a828cf8749a55a9822b95eea39e4fdd389 Mon Sep 17 00:00:00 2001 From: Carlos Zamora Date: Wed, 26 Feb 2025 10:51:10 -0800 Subject: [PATCH 3/5] Bugfix: don't round to nearest cell for mouse movements and VT mouse mode (#18602) Missed a few `_getTerminalPosition()` on the first run. Disabled rounding for pointer movements and mouse wheel events (which are used for hyperlink hover detection and vt mouse mode). The only time we round now is... - `SetEndSelectionPoint()` --> because we're updating a selection - `ControlCore->LeftClickOnTerminal()` --> where all paths are used for selection* *the only path that doesn't is `RepositionCursorWithMouse` being enabled, which also makes sense based on clicking around Notepad with a large font size. ## References and Relevant Issues Follow-up for #18486 Closes #18595 ## Validation Steps Performed In large font size, play around with midnight commander and hover over hyperlink edges. --- src/cascadia/TerminalControl/ControlInteractivity.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/cascadia/TerminalControl/ControlInteractivity.cpp b/src/cascadia/TerminalControl/ControlInteractivity.cpp index 0fd2840a583..6587f9777b6 100644 --- a/src/cascadia/TerminalControl/ControlInteractivity.cpp +++ b/src/cascadia/TerminalControl/ControlInteractivity.cpp @@ -340,7 +340,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation const Core::Point pixelPosition, const bool pointerPressedInBounds) { - const auto terminalPosition = _getTerminalPosition(til::point{ pixelPosition }, true); + const auto terminalPosition = _getTerminalPosition(til::point{ pixelPosition }, false); // Returning true from this function indicates that the caller should do no further processing of this movement. bool handledCompletely = false; @@ -489,7 +489,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation const Core::Point pixelPosition, const Control::MouseButtonState buttonState) { - const auto terminalPosition = _getTerminalPosition(til::point{ pixelPosition }, true); + const auto terminalPosition = _getTerminalPosition(til::point{ pixelPosition }, false); // Short-circuit isReadOnly check to avoid warning dialog. // From e1b28e72b39bf1adae9e02b550c4810a5f4e221c Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Wed, 26 Feb 2025 10:58:34 -0800 Subject: [PATCH 4/5] Fix panes being dropped when tearing off tabs (#18627) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I don't actually know why this is happening, because it doesn't happen with startup actions specified in the settings file. In any case, it's fixed with more delays. Closes #18572 ## Validation Steps Performed * Create a tab with 2 panes * Tear it off into a new window * New window has 1 tab with 2 panes ✅ --- .../LocalTests_TerminalApp/TabTests.cpp | 2 +- .../TerminalApp/AppActionHandlers.cpp | 8 +- src/cascadia/TerminalApp/TerminalPage.cpp | 109 +++++++----------- src/cascadia/TerminalApp/TerminalPage.h | 6 +- src/cascadia/TerminalApp/TerminalWindow.cpp | 16 +-- 5 files changed, 53 insertions(+), 88 deletions(-) diff --git a/src/cascadia/LocalTests_TerminalApp/TabTests.cpp b/src/cascadia/LocalTests_TerminalApp/TabTests.cpp index 477e6dcf0ea..941d5fa06fe 100644 --- a/src/cascadia/LocalTests_TerminalApp/TabTests.cpp +++ b/src/cascadia/LocalTests_TerminalApp/TabTests.cpp @@ -287,7 +287,7 @@ namespace TerminalAppLocalTests NewTabArgs args{ newTerminalArgs }; ActionAndArgs newTabAction{ ShortcutAction::NewTab, args }; // push the arg onto the front - page->_startupActions.Append(newTabAction); + page->_startupActions.push_back(std::move(newTabAction)); Log::Comment(L"Added a single newTab action"); auto app = ::winrt::Windows::UI::Xaml::Application::Current(); diff --git a/src/cascadia/TerminalApp/AppActionHandlers.cpp b/src/cascadia/TerminalApp/AppActionHandlers.cpp index b2ae698205a..69648929637 100644 --- a/src/cascadia/TerminalApp/AppActionHandlers.cpp +++ b/src/cascadia/TerminalApp/AppActionHandlers.cpp @@ -752,13 +752,11 @@ namespace winrt::TerminalApp::implementation { if (const auto& realArgs = actionArgs.ActionArgs().try_as()) { - auto actions = winrt::single_threaded_vector( - TerminalPage::ConvertExecuteCommandlineToActions(realArgs)); - - if (actions.Size() != 0) + auto actions = ConvertExecuteCommandlineToActions(realArgs); + if (!actions.empty()) { actionArgs.Handled(true); - ProcessStartupActions(actions, false); + ProcessStartupActions(std::move(actions), false); } } } diff --git a/src/cascadia/TerminalApp/TerminalPage.cpp b/src/cascadia/TerminalApp/TerminalPage.cpp index cb4c0403d97..fb061e548cc 100644 --- a/src/cascadia/TerminalApp/TerminalPage.cpp +++ b/src/cascadia/TerminalApp/TerminalPage.cpp @@ -62,7 +62,6 @@ namespace winrt::TerminalApp::implementation TerminalPage::TerminalPage(TerminalApp::WindowProperties properties, const TerminalApp::ContentManager& manager) : _tabs{ winrt::single_threaded_observable_vector() }, _mruTabs{ winrt::single_threaded_observable_vector() }, - _startupActions{ winrt::single_threaded_vector() }, _manager{ manager }, _hostingHwnd{}, _WindowProperties{ std::move(properties) } @@ -297,7 +296,7 @@ namespace winrt::TerminalApp::implementation // GH#12267: Don't forget about defterm handoff here. If we're being // created for embedding, then _yea_, we don't need to handoff to an // elevated window. - if (!_startupActions || IsRunningElevated() || _shouldStartInboundListener || _startupActions.Size() == 0) + if (_startupActions.empty() || IsRunningElevated() || _shouldStartInboundListener) { // there aren't startup actions, or we're elevated. In that case, go for it. return false; @@ -375,7 +374,7 @@ namespace winrt::TerminalApp::implementation // - void TerminalPage::HandoffToElevated(const CascadiaSettings& settings) { - if (!_startupActions) + if (_startupActions.empty()) { return; } @@ -489,7 +488,7 @@ namespace winrt::TerminalApp::implementation { _startupState = StartupState::InStartup; - ProcessStartupActions(_startupActions, true); + ProcessStartupActions(std::move(_startupActions), true); // If we were told that the COM server needs to be started to listen for incoming // default application connections, start it now. @@ -546,80 +545,56 @@ namespace winrt::TerminalApp::implementation // nt -d .` from inside another directory to work as expected. // Return Value: // - - safe_void_coroutine TerminalPage::ProcessStartupActions(Windows::Foundation::Collections::IVector actions, - const bool initial, - const winrt::hstring cwd, - const winrt::hstring env) + safe_void_coroutine TerminalPage::ProcessStartupActions(std::vector actions, const bool initial, const winrt::hstring cwd, const winrt::hstring env) { - auto weakThis{ get_weak() }; - - // Handle it on a subsequent pass of the UI thread. - co_await wil::resume_foreground(Dispatcher(), CoreDispatcherPriority::Normal); + const auto strong = get_strong(); // If the caller provided a CWD, "switch" to that directory, then switch - // back once we're done. This looks weird though, because we have to set - // up the scope_exit _first_. We'll release the scope_exit if we don't - // actually need it. - + // back once we're done. auto originalVirtualCwd{ _WindowProperties.VirtualWorkingDirectory() }; - auto restoreCwd = wil::scope_exit([&originalVirtualCwd, this]() { - // ignore errors, we'll just power on through. We'd rather do - // something rather than fail silently if the directory doesn't - // actually exist. - _WindowProperties.VirtualWorkingDirectory(originalVirtualCwd); - }); - - // Literally the same thing with env vars too auto originalVirtualEnv{ _WindowProperties.VirtualEnvVars() }; - auto restoreEnv = wil::scope_exit([&originalVirtualEnv, this]() { - _WindowProperties.VirtualEnvVars(originalVirtualEnv); + auto restoreCwd = wil::scope_exit([&]() { + if (!cwd.empty()) + { + // ignore errors, we'll just power on through. We'd rather do + // something rather than fail silently if the directory doesn't + // actually exist. + _WindowProperties.VirtualWorkingDirectory(originalVirtualCwd); + _WindowProperties.VirtualEnvVars(originalVirtualEnv); + } }); + _WindowProperties.VirtualWorkingDirectory(cwd); + _WindowProperties.VirtualEnvVars(env); - if (cwd.empty()) + for (size_t i = 0; i < actions.size(); ++i) { - // We didn't actually need to change the virtual CWD, so we don't - // need to restore it - restoreCwd.release(); - } - else - { - _WindowProperties.VirtualWorkingDirectory(cwd); - } + if (i != 0) + { + // Each action may rely on the XAML layout of a preceding action. + // Most importantly, this is the case for the combination of NewTab + SplitPane, + // as the former appears to only have a layout size after at least 1 resume_foreground, + // while the latter relies on that information. This is also why it uses Low priority. + // + // Curiously, this does not seem to be required when using startupActions, but only when + // tearing out a tab (this currently creates a new window with injected startup actions). + // This indicates that this is really more of an architectural issue and not a fundamental one. + co_await wil::resume_foreground(Dispatcher(), CoreDispatcherPriority::Low); + } - if (env.empty()) - { - restoreEnv.release(); - } - else - { - _WindowProperties.VirtualEnvVars(env); + _actionDispatch->DoAction(actions[i]); } - if (auto page{ weakThis.get() }) + // GH#6586: now that we're done processing all startup commands, + // focus the active control. This will work as expected for both + // commandline invocations and for `wt` action invocations. + if (const auto& terminalTab{ _GetFocusedTabImpl() }) { - for (const auto& action : actions) + if (const auto& content{ terminalTab->GetActiveContent() }) { - if (auto page{ weakThis.get() }) - { - _actionDispatch->DoAction(action); - } - else - { - co_return; - } - } - - // GH#6586: now that we're done processing all startup commands, - // focus the active control. This will work as expected for both - // commandline invocations and for `wt` action invocations. - if (const auto& terminalTab{ _GetFocusedTabImpl() }) - { - if (const auto& content{ terminalTab->GetActiveContent() }) - { - content.Focus(FocusState::Programmatic); - } + content.Focus(FocusState::Programmatic); } } + if (initial) { _CompleteInitialization(); @@ -3670,13 +3645,9 @@ namespace winrt::TerminalApp::implementation // - actions: a list of Actions to process on startup. // Return Value: // - - void TerminalPage::SetStartupActions(std::vector& actions) + void TerminalPage::SetStartupActions(std::vector actions) { - // The fastest way to copy all the actions out of the std::vector and - // put them into a winrt::IVector is by making a copy, then moving the - // copy into the winrt vector ctor. - auto listCopy = actions; - _startupActions = winrt::single_threaded_vector(std::move(listCopy)); + _startupActions = std::move(actions); } // Routine Description: diff --git a/src/cascadia/TerminalApp/TerminalPage.h b/src/cascadia/TerminalApp/TerminalPage.h index d094e07f0fe..9837f9fe618 100644 --- a/src/cascadia/TerminalApp/TerminalPage.h +++ b/src/cascadia/TerminalApp/TerminalPage.h @@ -128,7 +128,7 @@ namespace winrt::TerminalApp::implementation void Maximized(bool newMaximized); void RequestSetMaximized(bool newMaximized); - void SetStartupActions(std::vector& actions); + void SetStartupActions(std::vector actions); void SetInboundListener(bool isEmbedding); static std::vector ConvertExecuteCommandlineToActions(const Microsoft::Terminal::Settings::Model::ExecuteCommandlineArgs& args); @@ -146,7 +146,7 @@ namespace winrt::TerminalApp::implementation void ActionSaveFailed(winrt::hstring message); void ShowTerminalWorkingDirectory(); - safe_void_coroutine ProcessStartupActions(Windows::Foundation::Collections::IVector actions, + safe_void_coroutine ProcessStartupActions(std::vector actions, const bool initial, const winrt::hstring cwd = winrt::hstring{}, const winrt::hstring env = winrt::hstring{}); @@ -255,7 +255,7 @@ namespace winrt::TerminalApp::implementation winrt::Windows::UI::Xaml::Controls::Grid::LayoutUpdated_revoker _layoutUpdatedRevoker; StartupState _startupState{ StartupState::NotInitialized }; - Windows::Foundation::Collections::IVector _startupActions; + std::vector _startupActions; bool _shouldStartInboundListener{ false }; bool _isEmbeddingInboundListener{ false }; diff --git a/src/cascadia/TerminalApp/TerminalWindow.cpp b/src/cascadia/TerminalApp/TerminalWindow.cpp index 055d6123ccf..4d644666e1c 100644 --- a/src/cascadia/TerminalApp/TerminalWindow.cpp +++ b/src/cascadia/TerminalApp/TerminalWindow.cpp @@ -1054,12 +1054,8 @@ namespace winrt::TerminalApp::implementation { _contentBounds = bounds; - const auto& args = _contentStringToActions(content, true); - - for (const auto& action : args) - { - _initialContentArgs.push_back(action); - } + const auto args = _contentStringToActions(content, true); + _initialContentArgs = wil::to_vector(args); } // Method Description: @@ -1085,7 +1081,7 @@ namespace winrt::TerminalApp::implementation if (_appArgs->ExitCode() == 0) { auto& parsedArgs = _appArgs->ParsedArgs(); - auto actions = winrt::single_threaded_vector(std::move(parsedArgs.GetStartupActions())); + auto& actions = parsedArgs.GetStartupActions(); _root->ProcessStartupActions(actions, false, _appArgs->CurrentDirectory(), _appArgs->CurrentEnvironment()); @@ -1200,7 +1196,7 @@ namespace winrt::TerminalApp::implementation { try { - const auto& args = ActionAndArgs::Deserialize(content); + const auto args = ActionAndArgs::Deserialize(content); if (args == nullptr || args.Size() == 0) { @@ -1244,9 +1240,9 @@ namespace winrt::TerminalApp::implementation const bool replaceFirstWithNewTab = tabIndex >= _root->NumberOfTabs(); - const auto& args = _contentStringToActions(content, replaceFirstWithNewTab); + auto args = _contentStringToActions(content, replaceFirstWithNewTab); - _root->AttachContent(args, tabIndex); + _root->AttachContent(std::move(args), tabIndex); } } void TerminalWindow::SendContentToOther(winrt::TerminalApp::RequestReceiveContentArgs args) From e1be2f4c73b8a8d55e07a9499a72d7b943ac3fe7 Mon Sep 17 00:00:00 2001 From: Leonard Hecker Date: Wed, 26 Feb 2025 11:05:59 -0800 Subject: [PATCH 5/5] Fix persistence of the last closed window (#18623) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Does what it says on the tin. Closes #18525 ## Validation Steps Performed * Enable persistence * Close the last window * Persisted ✅ --- .../WindowsTerminal/WindowEmperor.cpp | 49 ++++++++++++++----- src/cascadia/WindowsTerminal/WindowEmperor.h | 1 + 2 files changed, 39 insertions(+), 11 deletions(-) diff --git a/src/cascadia/WindowsTerminal/WindowEmperor.cpp b/src/cascadia/WindowsTerminal/WindowEmperor.cpp index f5a646e991f..6a25256af6f 100644 --- a/src/cascadia/WindowsTerminal/WindowEmperor.cpp +++ b/src/cascadia/WindowsTerminal/WindowEmperor.cpp @@ -354,7 +354,10 @@ void WindowEmperor::HandleCommandlineArgs(int nCmdShow) } // If we created no windows, e.g. because the args are "/?" we can just exit now. - _postQuitMessageIfNeeded(); + if (_windows.empty()) + { + _postQuitMessageIfNeeded(); + } } // ALWAYS change the _real_ CWD of the Terminal to system32, @@ -735,9 +738,30 @@ void WindowEmperor::_createMessageWindow(const wchar_t* className) StringCchCopy(_notificationIcon.szTip, ARRAYSIZE(_notificationIcon.szTip), appNameLoc.c_str()); } +// Counterpart to _postQuitMessageIfNeeded: +// If it returns true, don't close that last window, if any. +// This ensures we persist the last window. +bool WindowEmperor::_shouldSkipClosingWindows() const +{ + const auto globalSettings = _app.Logic().Settings().GlobalSettings(); + const size_t windowLimit = globalSettings.ShouldUsePersistedLayout() ? 1 : 0; + return _windows.size() <= windowLimit; +} + +// Posts a WM_QUIT as soon as we have no reason to exist anymore. +// That basically means no windows [^1] and no message boxes. +// +// [^1] Unless: +// * We've been asked to persist the last remaining window +// in which case we exit with 1 remaining window. +// * We're allowed to be headless +// in which case we never exit. void WindowEmperor::_postQuitMessageIfNeeded() const { - if (_messageBoxCount <= 0 && _windows.empty() && !_app.Logic().Settings().GlobalSettings().AllowHeadless()) + const auto globalSettings = _app.Logic().Settings().GlobalSettings(); + const size_t windowLimit = globalSettings.ShouldUsePersistedLayout() ? 1 : 0; + + if (_messageBoxCount <= 0 && _windows.size() <= windowLimit && !globalSettings.AllowHeadless()) { PostQuitMessage(0); } @@ -771,17 +795,20 @@ LRESULT WindowEmperor::_messageHandler(HWND window, UINT const message, WPARAM c { case WM_CLOSE_TERMINAL_WINDOW: { - const auto host = reinterpret_cast(lParam); - auto it = _windows.begin(); - const auto end = _windows.end(); - - for (; it != end; ++it) + if (!_shouldSkipClosingWindows()) { - if (host == it->get()) + const auto host = reinterpret_cast(lParam); + auto it = _windows.begin(); + const auto end = _windows.end(); + + for (; it != end; ++it) { - host->Close(); - _windows.erase(it); - break; + if (host == it->get()) + { + host->Close(); + _windows.erase(it); + break; + } } } diff --git a/src/cascadia/WindowsTerminal/WindowEmperor.h b/src/cascadia/WindowsTerminal/WindowEmperor.h index ca30159ac2d..7012a7d66ee 100644 --- a/src/cascadia/WindowsTerminal/WindowEmperor.h +++ b/src/cascadia/WindowsTerminal/WindowEmperor.h @@ -55,6 +55,7 @@ class WindowEmperor safe_void_coroutine _dispatchCommandlineCurrentDesktop(winrt::TerminalApp::CommandlineArgs args); LRESULT _messageHandler(HWND window, UINT message, WPARAM wParam, LPARAM lParam) noexcept; void _createMessageWindow(const wchar_t* className); + bool _shouldSkipClosingWindows() const; void _postQuitMessageIfNeeded() const; safe_void_coroutine _showMessageBox(winrt::hstring message, bool error); void _notificationAreaMenuRequested(WPARAM wParam);