Fix ShowWindow(GetConsoleWindow()) (#13118)

A bad merge, that actually revealed a horrible bug.

There was a secret conflict between the code in #12526 and #12515. 69b77ca was a bad merge that hid just how bad the issue was. Fixing the one line `nullptr`->`this` in `InteractivityFactory` resulted in a window that would flash uncontrollably, as it minimized and restored itself in a loop. Great. 

This can seemingly be fixed by making sure that the conpty window is initially created with the owner already set, rather than relying on a `SetParent` call in post. This does pose some complications for the #1256 future we're approaching. However, this is a blocking bug _now_, and we can figure out the tearout/`SetParent` thing in post. 

* fixes #13066.
* Tested with the script in that issue.
* Window doesn't flash uncontrollably.
* `gci | ogv` still works right
* I work here.
* Opening a new tab doesn't spontaneously cause the window to minimize
* Restoring from minimized doesn't yeet focus to an invisible window
* Opening a new tab doesn't yeet focus to an invisible window
* There _is_ a viable way to call `GetAncestor` s.t. it returns the Terminal's hwnd in Terminal, and the console's in Conhost


The `SW_SHOWNOACTIVATE` change is also quite load bearing. With just `SW_NORMAL`, the pseudo window (which is invisible!) gets activated whenever the terminal window is restored from minimized. That's BAD.


There's actually more to this as well. 


Calling `SetParent` on a window that is `WS_VISIBLE` will cause the OS to hide the window, make it a _child_ window, then call `SW_SHOW` on the window to re-show it. `SW_SHOW`, however, will cause the OS to also set that window as the _foreground_ window, which would result in the pty's hwnd stealing the foreground away from the owning terminal window. That's bad.

`SetWindowLongPtr` seems to do the job of changing who the window owner is, without all the other side effects of reparenting the window. 

Without `SetParent`, however, the pty HWND is no longer a descendant of the Terminal HWND, so that means `GA_ROOT` can no longer be used to find the owner's hwnd. For even more insanity, without `WS_POPUP`, none of the values of `GetAncestor` will actually get the terminal HWND. So, now we also need `WS_POPUP` on the pty hwnd. To get at the Terminal hwnd, you'll need

```c++
GetAncestor(GetConsoleWindow(), GA_ROOTOWNER)
```

Author: zadjii-msft

src/cascadia/TerminalApp/TerminalPage.cpp

@@ -2431,7 +2431,10 @@ namespace winrt::TerminalApp::implementation
         TermControl term{ settings.DefaultSettings(), settings.UnfocusedSettings(), connection };
 
         // GH#12515: ConPTY assumes it's hidden at the start. If we're not, let it know now.
-        term.WindowVisibilityChanged(_visible);
+        if (_visible)
+        {
+            term.WindowVisibilityChanged(_visible);
+        }
 
         if (_hostingHwnd.has_value())
         {

src/cascadia/TerminalConnection/ConptyConnection.cpp

@@ -311,13 +311,17 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
 
             THROW_IF_FAILED(_CreatePseudoConsoleAndPipes(dimensions, flags, &_inPipe, &_outPipe, &_hPC));
 
-            // GH#12515: The conpty assumes it's hidden at the start. If we're visible, let it know now.
-            THROW_IF_FAILED(ConptyShowHidePseudoConsole(_hPC.get(), _initialVisibility));
             if (_initialParentHwnd != 0)
             {
                 THROW_IF_FAILED(ConptyReparentPseudoConsole(_hPC.get(), reinterpret_cast<HWND>(_initialParentHwnd)));
             }
 
+            // GH#12515: The conpty assumes it's hidden at the start. If we're visible, let it know now.
+            if (_initialVisibility)
+            {
+                THROW_IF_FAILED(ConptyShowHidePseudoConsole(_hPC.get(), _initialVisibility));
+            }
+
             THROW_IF_FAILED(_LaunchAttachedClient());
         }
         // But if it was an inbound handoff... attempt to synchronize the size of it with what our connection

src/cascadia/TerminalConnection/ConptyConnection.h

@@ -73,7 +73,7 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
         hstring _commandline{};
         hstring _startingDirectory{};
         hstring _startingTitle{};
-        bool _initialVisibility{ false };
+        bool _initialVisibility{ true };
         Windows::Foundation::Collections::ValueSet _environment{ nullptr };
         guid _guid{}; // A unique session identifier for connected client
         hstring _clientName{}; // The name of the process hosted by this ConPTY connection (as of launch).

src/cascadia/TerminalControl/ControlCore.cpp

@@ -1196,8 +1196,11 @@ namespace winrt::Microsoft::Terminal::Control::implementation
 
     void ControlCore::_terminalShowWindowChanged(bool showOrHide)
     {
-        auto showWindow = winrt::make_self<implementation::ShowWindowArgs>(showOrHide);
-        _ShowWindowChangedHandlers(*this, *showWindow);
+        if (_initializedTerminal)
+        {
+            auto showWindow = winrt::make_self<implementation::ShowWindowArgs>(showOrHide);
+            _ShowWindowChangedHandlers(*this, *showWindow);
+        }
     }
 
     bool ControlCore::HasSelection() const
@@ -1703,10 +1706,13 @@ namespace winrt::Microsoft::Terminal::Control::implementation
     // - <none>
     void ControlCore::WindowVisibilityChanged(const bool showOrHide)
     {
-        // show is true, hide is false
-        if (auto conpty{ _connection.try_as<TerminalConnection::ConptyConnection>() })
+        if (_initializedTerminal)
         {
-            conpty.ShowHide(showOrHide);
+            // show is true, hide is false
+            if (auto conpty{ _connection.try_as<TerminalConnection::ConptyConnection>() })
+            {
+                conpty.ShowHide(showOrHide);
+            }
         }
     }
 

src/host/PtySignalInputThread.cpp

@@ -75,11 +75,20 @@ void PtySignalInputThread::ConnectConsole() noexcept
         _DoShowHide(_initialShowHide->show);
     }
 
-    // If we were given a owner HWND, then manually start the pseudo window now.
-    if (_earlyReparent)
-    {
-        _DoSetWindowParent(*_earlyReparent);
-    }
+    // We should have successfully used the _earlyReparent message in CreatePseudoWindow.
+}
+
+// Method Description:
+// - Create our pseudo window. We're doing this here, instead of in
+//   ConnectConsole, because the window is created in
+//   ConsoleInputThreadProcWin32, before ConnectConsole is first called. Doing
+//   this here ensures that the window is first created with the initial owner
+//   set up (if so specified).
+// - Refer to GH#13066 for details.
+void PtySignalInputThread::CreatePseudoWindow()
+{
+    HWND owner = _earlyReparent.has_value() ? reinterpret_cast<HWND>((*_earlyReparent).handle) : HWND_DESKTOP;
+    ServiceLocator::LocatePseudoWindow(owner);
 }
 
 // Method Description:
@@ -227,15 +236,28 @@ void PtySignalInputThread::_DoShowHide(const bool show)
 // - <none>
 void PtySignalInputThread::_DoSetWindowParent(const SetParentData& data)
 {
+    const auto owner{ reinterpret_cast<HWND>(data.handle) };
     // This will initialize s_interactivityFactory for us. It will also
     // conveniently return 0 when we're on OneCore.
     //
     // If the window hasn't been created yet, by some other call to
     // LocatePseudoWindow, then this will also initialize the owner of the
     // window.
-    if (const auto pseudoHwnd{ ServiceLocator::LocatePseudoWindow(reinterpret_cast<HWND>(data.handle)) })
+    if (const auto pseudoHwnd{ ServiceLocator::LocatePseudoWindow(owner) })
     {
-        LOG_LAST_ERROR_IF_NULL(::SetParent(pseudoHwnd, reinterpret_cast<HWND>(data.handle)));
+        // DO NOT USE SetParent HERE!
+        //
+        // Calling SetParent on a window that is WS_VISIBLE will cause the OS to
+        // hide the window, make it a _child_ window, then call SW_SHOW on the
+        // window to re-show it. SW_SHOW, however, will cause the OS to also set
+        // that window as the _foreground_ window, which would result in the
+        // pty's hwnd stealing the foreground away from the owning terminal
+        // window. That's bad.
+        //
+        // SetWindowLongPtr seems to do the job of changing who the window owner
+        // is, without all the other side effects of reparenting the window.
+        // See #13066
+        ::SetWindowLongPtr(pseudoHwnd, GWLP_HWNDPARENT, reinterpret_cast<LONG_PTR>(owner));
     }
 }
 

src/host/PtySignalInputThread.hpp

@@ -33,6 +33,7 @@ namespace Microsoft::Console
         PtySignalInputThread& operator=(const PtySignalInputThread&) = delete;
 
         void ConnectConsole() noexcept;
+        void CreatePseudoWindow();
 
     private:
         enum class PtySignal : unsigned short

src/host/VtIo.cpp

@@ -300,18 +300,34 @@ bool VtIo::IsUsingVt() const
 
     if (_pPtySignalInputThread)
     {
-        // IMPORTANT! Start the pseudo window on this thread. This thread has a
-        // message pump. If you DON'T, then a DPI change in the owning hwnd will
-        // cause us to get a dpi change as well, which we'll never deque and
-        // handle, effectively HANGING THE OWNER HWND.
+        // Let the signal thread know that the console is connected.
         //
-        // Let the signal thread know that the console is connected
+        // By this point, the pseudo window should have already been created, by
+        // ConsoleInputThreadProcWin32. That thread has a message pump, which is
+        // needed to ensure that DPI change messages to the owning terminal
+        // window don't end up hanging because the pty didn't also process it.
         _pPtySignalInputThread->ConnectConsole();
     }
 
     return S_OK;
 }
 
+// Method Description:
+// - Create our pseudo window. This is exclusively called by
+//   ConsoleInputThreadProcWin32 on the console input thread.
+//    * It needs to be called on that thread, before any other calls to
+//      LocatePseudoWindow, to make sure that the input thread is the HWND's
+//      message thread.
+//    * It needs to be plumbed through the signal thread, because the signal
+//      thread knows if someone should be marked as the window's owner. It's
+//      VERY IMPORTANT that any initial owners are set up when the window is
+//      first created.
+// - Refer to GH#13066 for details.
+void VtIo::CreatePseudoWindow()
+{
+    _pPtySignalInputThread->CreatePseudoWindow();
+}
+
 // Method Description:
 // - Create and start the signal thread. The signal thread can be created
 //      independent of the i/o threads, and doesn't require a client first

src/host/VtIo.hpp

@@ -52,6 +52,8 @@ namespace Microsoft::Console::VirtualTerminal
 
         [[nodiscard]] HRESULT ManuallyClearScrollback() const noexcept;
 
+        void CreatePseudoWindow();
+
     private:
         // After CreateIoHandlers is called, these will be invalid.
         wil::unique_hfile _hInput;

src/host/outputStream.cpp

@@ -285,7 +285,7 @@ void ConhostInternalGetSet::ShowWindow(bool showOrHide)
 {
     const auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
     const auto hwnd = gci.IsInVtIoMode() ? ServiceLocator::LocatePseudoWindow() : ServiceLocator::LocateConsoleWindow()->GetWindowHandle();
-    ::ShowWindow(hwnd, showOrHide ? SW_NORMAL : SW_MINIMIZE);
+    ::ShowWindow(hwnd, showOrHide ? SW_SHOWNOACTIVATE : SW_MINIMIZE);
 }
 
 // Routine Description:

src/interactivity/base/InteractivityFactory.cpp

@@ -320,8 +320,14 @@ using namespace Microsoft::Console::Interactivity;
                 // as far as the difference between parent/child and owner/owned
                 // windows). Evan K said we should do it this way, and he
                 // definitely knows.
-                const auto windowStyle = WS_OVERLAPPEDWINDOW;
-                const auto exStyles = WS_EX_TOOLWINDOW | WS_EX_TRANSPARENT | WS_EX_LAYERED;
+                //
+                // GH#13066: Load-bearing: Make sure to set WS_POPUP. If you
+                // don't, then GetAncestor(GetConsoleWindow(), GA_ROOTOWNER)
+                // will return the console handle again, not the owning
+                // terminal's handle. It's not entirely clear why, but WS_POPUP
+                // is absolutely vital for this to work correctly.
+                const auto windowStyle = WS_OVERLAPPEDWINDOW | WS_POPUP;
+                const auto exStyles = WS_EX_TOOLWINDOW | WS_EX_TRANSPARENT | WS_EX_LAYERED | WS_EX_NOACTIVATE;
 
                 // Attempt to create window.
                 hwnd = CreateWindowExW(exStyles,
@@ -335,7 +341,7 @@ using namespace Microsoft::Console::Interactivity;
                                        owner,
                                        nullptr,
                                        nullptr,
-                                       nullptr);
+                                       this);
 
                 if (hwnd == nullptr)
                 {

src/interactivity/win32/windowio.cpp

@@ -1036,9 +1036,18 @@ DWORD WINAPI ConsoleInputThreadProcWin32(LPVOID /*lpParameter*/)
         // If we are headless (because we're a pseudo console), we
         // will still need a window handle in the win32 environment
         // in case anyone sends messages at that HWND (vim.exe is an example.)
-        // We have to CreateWindow on the same thread that will pump the messages
-        // which is this thread.
-        ServiceLocator::LocatePseudoWindow();
+        //
+        // IMPORTANT! We have to CreateWindow on the same thread that will pump
+        // the messages, which is this thread. If you DON'T, then a DPI change
+        // in the owning hwnd will cause us to get a dpi change as well, which
+        // we'll never deque and handle, effectively HANGING THE OWNER HWND.
+        // ServiceLocator::LocatePseudoWindow();
+        //
+        // Instead of just calling LocatePseudoWindow, make sure to go through
+        // VtIo's CreatePseudoWindow, which will make sure that the window is
+        // successfully created with the owner configured when the window is
+        // first created. See GH#13066 for details.
+        ServiceLocator::LocateGlobals().getConsoleInformation().GetVtIo()->CreatePseudoWindow();
     }
 
     UnlockConsole();

src/terminal/adapter/InteractDispatch.cpp

@@ -199,11 +199,19 @@ bool InteractDispatch::FocusChanged(const bool focused) const
             {
                 // They want focus, we found a pseudo hwnd.
 
-                // Note: ::GetParent(pseudoHwnd) will return 0. GetAncestor works though.
-                // GA_PARENT and GA_ROOT seemingly return the same thing for
-                // Terminal. We're going with GA_ROOT since it seems
-                // semantically more correct here.
-                if (const auto ownerHwnd{ ::GetAncestor(pseudoHwnd, GA_ROOT) })
+                // BODGY
+                //
+                // This needs to be GA_ROOTOWNER here. Not GA_ROOT, GA_PARENT,
+                // or GetParent. The ConPTY hwnd is an owned, top-level, popup,
+                // non-parented window. It does not have a parent set. It does
+                // have an owner set. It is not a WS_CHILD window. This
+                // combination of things allows us to find the owning window
+                // with GA_ROOTOWNER. GA_ROOT will get us ourselves, and
+                // GA_PARENT will return the desktop HWND.
+                //
+                // See GH#13066
+
+                if (const auto ownerHwnd{ ::GetAncestor(pseudoHwnd, GA_ROOTOWNER) })
                 {
                     // We have an owner from a previous call to ReparentWindow