Merge remote-tracking branch 'origin/main' into dev/lhecker/18598-shu…

…tdown-crash
microsoft · Feb 27, 2025 · 01b1777 · 01b1777
2 parents 1487b2a + e1be2f4
commit 01b1777
Show file tree

Hide file tree

Showing 15 changed files with 130 additions and 132 deletions.
diff --git 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
@@ -752,13 +752,11 @@ namespace winrt::TerminalApp::implementation
     {
         if (const auto& realArgs = actionArgs.ActionArgs().try_as<ExecuteCommandlineArgs>())
         {
-            auto actions = winrt::single_threaded_vector<ActionAndArgs>(
-                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/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<SettingsLoadWarnings>() };
         for (auto&& warn : _warnings)
         {

diff --git 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
@@ -62,7 +62,6 @@ namespace winrt::TerminalApp::implementation
     TerminalPage::TerminalPage(TerminalApp::WindowProperties properties, const TerminalApp::ContentManager& manager) :
         _tabs{ winrt::single_threaded_observable_vector<TerminalApp::TabBase>() },
         _mruTabs{ winrt::single_threaded_observable_vector<TerminalApp::TabBase>() },
-        _startupActions{ winrt::single_threaded_vector<ActionAndArgs>() },
         _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
     // - <none>
     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:
     // - <none>
-    safe_void_coroutine TerminalPage::ProcessStartupActions(Windows::Foundation::Collections::IVector<ActionAndArgs> actions,
-                                                            const bool initial,
-                                                            const winrt::hstring cwd,
-                                                            const winrt::hstring env)
+    safe_void_coroutine TerminalPage::ProcessStartupActions(std::vector<ActionAndArgs> 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();
@@ -1783,16 +1758,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);
                     }
@@ -3664,13 +3645,9 @@ namespace winrt::TerminalApp::implementation
     // - actions: a list of Actions to process on startup.
     // Return Value:
     // - <none>
-    void TerminalPage::SetStartupActions(std::vector<ActionAndArgs>& actions)
+    void TerminalPage::SetStartupActions(std::vector<ActionAndArgs> 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<ActionAndArgs>(std::move(listCopy));
+        _startupActions = std::move(actions);
     }
 
     // Routine Description:

diff --git 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<Microsoft::Terminal::Settings::Model::ActionAndArgs>& actions);
+        void SetStartupActions(std::vector<Microsoft::Terminal::Settings::Model::ActionAndArgs> actions);
 
         void SetInboundListener(bool isEmbedding);
         static std::vector<Microsoft::Terminal::Settings::Model::ActionAndArgs> 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<Microsoft::Terminal::Settings::Model::ActionAndArgs> actions,
+        safe_void_coroutine ProcessStartupActions(std::vector<Microsoft::Terminal::Settings::Model::ActionAndArgs> 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<Microsoft::Terminal::Settings::Model::ActionAndArgs> _startupActions;
+        std::vector<Microsoft::Terminal::Settings::Model::ActionAndArgs> _startupActions;
         bool _shouldStartInboundListener{ false };
         bool _isEmbeddingInboundListener{ false };
 

diff --git 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<ActionAndArgs>(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)

diff --git 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.
         //