Fix `ShowWindow(GetConsoleWindow())` by zadjii-msft · Pull Request #13118 · microsoft/terminal

zadjii-msft · 2022-05-17T18:25:12Z

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 [1.14] ShowWindow(SW_MINIMIZE) doesn't work anymore #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

GetAncestor(GetConsoleWindow(), GA_ROOTOWNER)

Reparenting

For #1256.

Although not tested in the scope of the Terminal, I'm fairly confident that SetWindowLongPtr will work for this.

In that screenshot, I reparented the black terminal to be owned by the blue one. API calls all seemed to confirm that worked, and the hidden pty window wasn't activated as a middle step.

Helper Scripts

Guys TIL how handy the Win32 interop of powershell is. Look at this. This is so much easier.

$NativeFunctions=@"
using System;
using System.Runtime.InteropServices;
public class Native {

    [DllImport("user32.dll")]
    public static extern bool ShowWindow(IntPtr hWnd, int nCmdShow);

    [DllImport("user32.dll")]
    public static extern IntPtr GetAncestor(IntPtr hWnd, uint gaFlags);

    [DllImport("user32.dll")]
    public static extern IntPtr GetParent(IntPtr hWnd);

    [DllImport("user32.dll")]
    public static extern IntPtr GetWindowLongPtr(IntPtr hWnd, int nIndex);

    [DllImport("user32.dll")]
    public static extern IntPtr SetWindowLongPtr(IntPtr hWnd, int nIndex, IntPtr dwNewLong);

    [DllImport("kernel32.dll", SetLastError = true)]
    public static extern IntPtr GetConsoleWindow();
}
"@
Add-Type -TypeDefinition $NativeFunctions


# [Native]::ShowWindow([Native]::GetConsoleWindow(), 0)

function hwnds {
  $console = [Native]::GetConsoleWindow()

  $GA_PARENT = [Native]::GetAncestor($console, 1) # GA_PARENT
  $GA_ROOT = [Native]::GetAncestor($console, 2) # GA_ROOT
  $GA_ROOTOWNER = [Native]::GetAncestor($console, 3) # GA_ROOTOWNER
  $Parent = [Native]::GetParent($console)
  $GWLP_HWNDPARENT = [Native]::GetWindowLongPtr($console, -8)

  Write-Host ("Console`t`t{0:x}`nGA_PARENT`t{1:x}`nGA_ROOT`t`t{2:x}`nGA_ROOTOWNER`t{3:x}`nParent`t`t{4:x}`nGWLP_HWNDPARENT`t{5:x}" -f $console,$GA_PARENT,$GA_ROOT,$GA_ROOTOWNER,$Parent,$GWLP_HWNDPARENT)

  if ($console -eq $GA_ROOTOWNER) { Write-Host "`e[31mThis is bad`e[m" }
  else {
    if ($console -eq $GA_ROOT) { Write-Host "`e[33mThis is okay`e[m - we told folks that they should use ROOT. They probably will need ROOTOWNER`e[m" }
    else { Write-Host "`e[32mThis is good`e[m" }
  }
}

(cherry picked from commit 0f256dc)

This does not fix doing a `[Native]::ShowWindow([Native]::GetConsoleWindow(), 0)` (0==`SW_HIDE`) immediately in a Terminal window. Presumably, this is because the conpty window already has that bit set?

DHowett · 2022-05-17T18:26:52Z

src/cascadia/TerminalControl/ControlCore.cpp

    {
-        auto showWindow = winrt::make_self<implementation::ShowWindowArgs>(showOrHide);
-        _ShowWindowChangedHandlers(*this, *showWindow);
+        if (_initializedTerminal)


Vague horror that we might be getting this callback before we have finished initialization...!

I would actually bet that code is vestigial at this point. I'd guess leftover from me experimenting with the debouncing. That being said, I thought that 0dc6e0d wasn't needed, and it totally was.

DHowett · 2022-05-17T18:30:11Z

src/host/VtIo.cpp

+//    * 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


any way to guarantee that this happens before LPW?

DHowett · 2022-05-17T18:30:57Z

@carlos-zamora FYI last blocker for 1.14!

DHowett · 2022-05-17T18:32:10Z

What horrors happen if a console app uses SW_NORMAL on its own window handle inside Terminal? Is there a way for us to suppress activation in the message loop?

zadjii-msft · 2022-05-17T19:23:13Z

What horrors happen if a console app uses SW_NORMAL on its own window handle inside Terminal? Is there a way for us to suppress activation in the message loop?

[Native]::ShowWindow([Native]::GetConsoleWindow(), 0) ; sleep 1 ; [Native]::ShowWindow([Native]::GetConsoleWindow(), 1) does seem to work correctly in the terminal, so I suppose this works fine?

zadjii-msft · 2022-05-17T19:25:47Z

🛑 converted to draft for a sec while I make sure that the parenting / ownership stuff didn't regress more than expected.

This reverts commit 0dc6e0d.

…066-SW_FLASH-2

src/cascadia/TerminalConnection/ConptyConnection.cpp

DHowett · 2022-05-18T22:04:55Z

src/cascadia/TerminalConnection/ConptyConnection.h

        hstring _startingDirectory{};
        hstring _startingTitle{};
-        bool _initialVisibility{ false };
+        bool _initialVisibility{ true };


I have to admit, I am VERY worried that I read two comments that say "conpty assumes it's hidden" and then this banger that makes ConptyConnection assume it is visible

i think it makes sense, it was just surprising

DHowett

Okay, let's do this. It's dark magic, sure, but so was what we had before. If you worked with @ekoschik on it I have to assume that it could take a bullet and keep going...

DHowett · 2022-05-18T22:05:33Z

src/cascadia/TerminalConnection/ConptyConnection.h

        hstring _startingDirectory{};
        hstring _startingTitle{};
-        bool _initialVisibility{ false };
+        bool _initialVisibility{ true };


i think it makes sense, it was just surprising

DHowett · 2022-05-19T00:21:32Z

This doesn't work with DefTerm until the window is minimized and restored once. :|

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) ``` (cherry picked from commit 77215d9) Service-Card-Id: 82170678 Service-Version: 1.14

zadjii-msft added 8 commits May 16, 2022 11:21

A bad merge, that actually revealed a horrible bug

3ab3bef

This doesn't help at all, unfortunately.

b89ed13

I thought this would help, it didn't

9525beb

(cherry picked from commit 0f256dc)

This fixes the flashing. However...

bbae235

This does not fix doing a `[Native]::ShowWindow([Native]::GetConsoleWindow(), 0)` (0==`SW_HIDE`) immediately in a Terminal window. Presumably, this is because the conpty window already has that bit set?

LOAD BEARING - DON'T ACTIVATE THE HIDDEN WINDOW!

ad5295a

This state of the world works.

50cd051

Merge branch 'main' into dev/migrie/b/13066-SW_FLASH-2

f04b892

These changes were not needed

0dc6e0d

DHowett reviewed May 17, 2022

View reviewed changes

This comment was marked as outdated.

Sign in to view

DHowett reviewed May 17, 2022

View reviewed changes

DHowett approved these changes May 17, 2022

View reviewed changes

zadjii-msft marked this pull request as draft May 17, 2022 19:23

zadjii-msft added 6 commits May 18, 2022 13:15

The WS_POPUP/GA_ROOTOWNER solution

478cb9f

notes

5f6ff34

egregious notes

33b8a20

Merge branch 'main' into dev/migrie/b/13066-ws_popup-route

ed5222c

Revert "These changes were not needed"

f1dda12

This reverts commit 0dc6e0d.

Merge branch 'dev/migrie/b/13066-ws_popup-route' into dev/migrie/b/13…

b3e2987

…066-SW_FLASH-2

This comment was marked as resolved.

Sign in to view

a bit of spel

60f391b

zadjii-msft marked this pull request as ready for review May 18, 2022 20:54

DHowett reviewed May 18, 2022

View reviewed changes

src/cascadia/TerminalConnection/ConptyConnection.cpp Outdated Show resolved Hide resolved

DHowett reviewed May 18, 2022

View reviewed changes

DHowett approved these changes May 18, 2022

View reviewed changes

Update src/cascadia/TerminalConnection/ConptyConnection.cpp

ee18932

lhecker approved these changes May 18, 2022

View reviewed changes

DHowett merged commit 77215d9 into main May 18, 2022

DHowett deleted the dev/migrie/b/13066-SW_FLASH-2 branch May 18, 2022 22:25

		// * 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

Conversation

zadjii-msft commented May 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DHowett May 17, 2022

Choose a reason for hiding this comment

Uh oh!

zadjii-msft May 18, 2022

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

DHowett May 17, 2022

Choose a reason for hiding this comment

Uh oh!

DHowett commented May 17, 2022

Uh oh!

DHowett commented May 17, 2022

Uh oh!

zadjii-msft commented May 17, 2022

Uh oh!

zadjii-msft commented May 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🛑 converted to draft for a sec while I make sure that the parenting / ownership stuff didn't regress more than expected.

Uh oh!

This comment was marked as resolved.

Uh oh!

DHowett May 18, 2022

Choose a reason for hiding this comment

Uh oh!

DHowett May 18, 2022

Choose a reason for hiding this comment

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

DHowett May 18, 2022

Choose a reason for hiding this comment

Uh oh!

DHowett commented May 19, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

zadjii-msft commented May 17, 2022 •

edited

Loading

zadjii-msft commented May 17, 2022 •

edited

Loading