-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix ShowWindow(GetConsoleWindow())
#13118
Conversation
(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?
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Vague horror that we might be getting this callback before we have finished initialization...!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
This comment was marked as outdated.
This comment was marked as outdated.
// * 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any way to guarantee that this happens before LPW?
@carlos-zamora FYI last blocker for 1.14! |
What horrors happen if a console app uses |
|
🛑 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.
This comment was marked as resolved.
This comment was marked as resolved.
@@ -73,7 +73,7 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation | |||
hstring _commandline{}; | |||
hstring _startingDirectory{}; | |||
hstring _startingTitle{}; | |||
bool _initialVisibility{ false }; | |||
bool _initialVisibility{ true }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think it makes sense, it was just surprising
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
@@ -73,7 +73,7 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation | |||
hstring _commandline{}; | |||
hstring _startingDirectory{}; | |||
hstring _startingTitle{}; | |||
bool _initialVisibility{ false }; | |||
bool _initialVisibility{ true }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think it makes sense, it was just surprising
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
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
inInteractivityFactory
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.ShowWindow(SW_MINIMIZE)
doesn't work anymore #13066.gci | ogv
still works rightGetAncestor
s.t. it returns the Terminal's hwnd in Terminal, and the console's in ConhostThe
SW_SHOWNOACTIVATE
change is also quite load bearing. With justSW_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 isWS_VISIBLE
will cause the OS to hide the window, make it a child window, then callSW_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 meansGA_ROOT
can no longer be used to find the owner's hwnd. For even more insanity, withoutWS_POPUP
, none of the values ofGetAncestor
will actually get the terminal HWND. So, now we also needWS_POPUP
on the pty hwnd. To get at the Terminal hwnd, you'll needGetAncestor(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.