Fix panes being dropped when tearing off tabs #18627

lhecker · 2025-02-25T15:41:33Z

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 ✅

lhecker · 2025-02-25T15:42:31Z

src/cascadia/TerminalApp/AppActionHandlers.cpp

            {
                actionArgs.Handled(true);
-                ProcessStartupActions(actions, false);
+                ProcessStartupActions(std::move(actions), false);


Why use IVector, when everything from start to finish in this stack talks in std::vector already?

thank you ugh

lhecker · 2025-02-25T15:43:14Z

src/cascadia/TerminalApp/TerminalPage.cpp

@@ -375,7 +374,7 @@ namespace winrt::TerminalApp::implementation
    // - <none>
    void TerminalPage::HandoffToElevated(const CascadiaSettings& settings)
    {
-        if (!_startupActions)
+        if (_startupActions.empty())


Pretty sure this was a bug. Handing off to elevated with 0 tabs doesn't seem useful to me.

lhecker · 2025-02-25T15:43:31Z

src/cascadia/TerminalApp/TerminalPage.cpp

-
-        // Handle it on a subsequent pass of the UI thread.
-        co_await wil::resume_foreground(Dispatcher(), CoreDispatcherPriority::Normal);
+        const auto strong = get_strong();


Simplified lifetime management.

lhecker · 2025-02-25T15:44:49Z

src/cascadia/TerminalApp/TerminalPage.cpp

-        {
-            // We didn't actually need to change the virtual CWD, so we don't
-            // need to restore it
-            restoreCwd.release();


Subjectively speaking, I found this highly confusing. Rewrote it to be a literal translation of the comment above.
(= "If the caller provided a CWD, [...] switch back once we're done" = if (!cwd.empty()))

lhecker · 2025-02-25T15:45:18Z

src/cascadia/TerminalApp/TerminalPage.cpp

+                // 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);


Bug fix. If the rest is too much, I'll extract this.

lhecker · 2025-02-25T15:46:14Z

src/cascadia/TerminalApp/TerminalWindow.cpp

@@ -1200,7 +1196,7 @@ namespace winrt::TerminalApp::implementation
    {
        try
        {
-            const auto& args = ActionAndArgs::Deserialize(content);
+            const auto args = ActionAndArgs::Deserialize(content);


C++ is a whack language so be careful when binding return values to references. Doing so incorrectly breaks return-value-optimization (as it did here)!

/cc @carlos-zamora this is an excellent example of where const auto& on a local goes wrong!

(Double-checking) So what's the lesson here? Don't store returned IVectors (or any winrt collection) to a const auto&? Does it still work correctly, but it just gets rid of the optimization?

When you have a function that returns something by-value and you bind it to a reference here's what actually happens:

const auto __hidden_variable__ = ActionAndArgs::Deserialize(content); const auto& args = __hidden_variable__; // ... return args;

As you can see in the example, you return a reference at the end, not the original object. This forces C++ to make a copy of the object. If you don't bind it to a reference, then you'll also return the original object at the end and "return value optimization" will kick in.

The problem is when you have a function that returns a reference, but you bind it to a value. Because then you'll also make a copy:

const auto args = return_a_reference_to_arguments(); // ^ ^ // | Because `args` is a value type, but the function returns a reference, // | this invokes the `vector(const vector& other)` copy constructor. // | // "auto" always infers a value type, never refences, etc. // So, in this case it may be `std::vector`, not `std::vector&`.

C++ has a fix for this con job of supposed type inference and it's this:

decltype(auto) args = return_a_reference_to_arguments(); // ^ // This always infers the exact type, including references, constness, pointers, etc.

It couldn't have been just let or var or something. 😐
In any case, if you use Resharper (currently free to use via the so-called EAP), you get little inlay hints that tell you when an invisible copy constructor is called. This allows me to quickly spot these issues.

carlos-zamora

Nice work! :)

carlos-zamora · 2025-02-25T18:44:54Z

src/cascadia/TerminalApp/TerminalPage.h

-        Windows::Foundation::Collections::IVector<Microsoft::Terminal::Settings::Model::ActionAndArgs> _startupActions;
+        std::vector<Microsoft::Terminal::Settings::Model::ActionAndArgs> _startupActions;


Ever since you told me to use std::vector, it's been great btw. Love to actually be able to see what's inside a vector. Big fan of this change!

carlos-zamora · 2025-02-25T18:52:41Z

src/cascadia/TerminalApp/TerminalWindow.cpp

@@ -1200,7 +1196,7 @@ namespace winrt::TerminalApp::implementation
    {
        try
        {
-            const auto& args = ActionAndArgs::Deserialize(content);
+            const auto args = ActionAndArgs::Deserialize(content);


(Double-checking) So what's the lesson here? Don't store returned IVectors (or any winrt collection) to a const auto&? Does it still work correctly, but it just gets rid of the optimization?

carlos-zamora

Nice work! :)

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 ✅ (cherry picked from commit e1b28e7) Service-Card-Id: PVTI_lADOAF3p4s4AxadtzgXTsJY PVTI_lADOAF3p4s4AxadtzgXsQ6w Service-Version: 1.23

microsoft-github-policy-service bot added Issue-Bug Area-Windowing Priority-1 Product-Terminal labels Feb 25, 2025

lhecker commented Feb 25, 2025

View reviewed changes

lhecker force-pushed the dev/lhecker/18572-tab-tearoff branch from 176beba to 3ee9efd Compare February 25, 2025 15:49

Fix panes being dropped when tearing off tabs

472e608

lhecker force-pushed the dev/lhecker/18572-tab-tearoff branch from 3ee9efd to 472e608 Compare February 25, 2025 15:49

Fix test compilation

d69c852

carlos-zamora approved these changes Feb 25, 2025

View reviewed changes

lhecker enabled auto-merge (squash) February 26, 2025 18:57

DHowett approved these changes Feb 26, 2025

View reviewed changes

lhecker merged commit e1b28e7 into main Feb 26, 2025
19 checks passed

lhecker deleted the dev/lhecker/18572-tab-tearoff branch February 26, 2025 18:58

lhecker mentioned this pull request Feb 27, 2025

Terminal Preview Hanging On Startup #18610

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix panes being dropped when tearing off tabs #18627

Fix panes being dropped when tearing off tabs #18627

lhecker commented Feb 25, 2025

lhecker Feb 25, 2025

DHowett Feb 26, 2025

lhecker Feb 25, 2025

lhecker Feb 25, 2025

lhecker Feb 25, 2025

lhecker Feb 25, 2025

lhecker Feb 25, 2025 •

edited

Loading

DHowett Feb 25, 2025

carlos-zamora Feb 25, 2025

lhecker Feb 25, 2025

carlos-zamora left a comment

carlos-zamora Feb 25, 2025

carlos-zamora Feb 25, 2025

carlos-zamora left a comment

		Windows::Foundation::Collections::IVector<Microsoft::Terminal::Settings::Model::ActionAndArgs> _startupActions;
		std::vector<Microsoft::Terminal::Settings::Model::ActionAndArgs> _startupActions;

Fix panes being dropped when tearing off tabs #18627

Fix panes being dropped when tearing off tabs #18627

Conversation

lhecker commented Feb 25, 2025

Validation Steps Performed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhecker Feb 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlos-zamora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlos-zamora left a comment

Choose a reason for hiding this comment

lhecker Feb 25, 2025 •

edited

Loading