Skip to content
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

A hypothetical fix for hidden windows #15213

Merged
merged 1 commit into from
Apr 25, 2023

Conversation

zadjii-msft
Copy link
Member

We had a report in a mail thread that someone's Terminal windows were getting created hidden, and never showing themselves.

As a theory, I'm guessing that dwFlags didn't say that we should actually use wShowWindow. So, to be more correct, let's actually obey that.

I'm gonna send this package to them to see if it fixes them.

Related to #14957.

Likely regressed in #13838.

  We had a report in a mail thread that someone's Terminal windows were getting created hidden, and never showing themselves.

  As a theory, I'm guessing that dwFlags didn't say that we should actually use `wShowWindow`. So, to be more correct, let's actually obey that.

  I'm gonna send this package to them to see if it fixes them.

  Related to #14957.

  Likely regressed in #13838.
@DHowett
Copy link
Member

DHowett commented Apr 20, 2023

Do you want to wait for validation before reviews or merge?

@@ -79,7 +79,7 @@ bool WindowEmperor::HandleCommandlineArgs()
// so we can open a new window with the same state.
STARTUPINFOW si;
GetStartupInfoW(&si);
const auto showWindow = si.wShowWindow;
const uint32_t showWindow = WI_IsFlagSet(si.dwFlags, STARTF_USESHOWWINDOW) ? si.wShowWindow : SW_SHOW;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we may want to prefer SW_SHOWDEFAULT. FYI, SW_SHOWDEFAULT actually.. wait

wait

SW_SHOWDEFAULT is supposed to do this entire thing (check the flag, use wShowWindow if it's set, etc.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now i'm curious, why was SHOWDEFAULT not working?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Presumably, because we'd collect the wShowWindow here, and eventually plumb it into the ShowWindow call in AppHost::_WindowInitializedHandler, regardless of what the actual dwFlags said. So if the wShowWindow was set to 0, then we'd hide the window even if the flags didn't call for it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I've got a couple other questions about this whole kit and/or kaboodle.

So, okay. This happens when a commandline comes in.

Can you walk me through what happens in these scenarios?

  1. Terminal is not running, and you run wt
  2. Terminal is not running, and you run start /min wt
  3. Terminal is running, and you run wt
  4. Terminal is running, and you run start /min wt
  5. Terminal is running in Attach to most recent window mode, and you run start /min wt

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i.e. whose nShowCmd/wShowWindow are we using? The one from the receiving process, or the sending process?

  1. Terminal is not running, and you run start /min wt, and then you run start /max wt -w -1 or just start wt -w -1
    • Would the new window get the incoming wShowWindow, or would it get the one of the original WT instance started with /min?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Terminal boots. Looks at its combined si.dwFlags and si.wShowWindow, sticks that into the Remoting::CommandlineArgs. That's given to the AppHost, who uses that in the post-init call to ShowWindow (unless launchMode is set to *maximized*)
  2. Same deal as 1. Here, si.dwFlags+si.wShowWindow --> SW_SHOWMINIMIZED
  3. Terminal boots. Looks at its combined si.dwFlags and si.wShowWindow, sticks that into the Remoting::CommandlineArgs. Those get remoted to the monarch. They hand them off to the AppHost created for this window. That AppHost, uses that in the post-init call to ShowWindow, just like in 1.
  4. Same deal. We just remote SW_SHOWMINIMIZED to the monarch who gives that to the eventual window that gets created
  5. Terminal boots. Looks at its combined si.dwFlags and si.wShowWindow. Finds SW_SHOWMINIMIZED. Sticks that in the CommandlineArgs. ProposeCommandline's that to the Monarch, who passes it to an existing window. That existing window finds a commandline of "wt". Treats that as a new-tab subcommand. A new tab is opened. The state of the window is unaffected.
  6. Same as 2 ; and then we remote a SW_MAXIMIZE in the CommandlineArgs for the new window, and create a second window with a different state.
    • yes, the new window gets the startupinfo from the process that initiated the creation of this window.

Sound right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SGTM

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Apr 20, 2023
Copy link
Member

@lhecker lhecker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is easily reproducible if you launch Windows Terminal through Nvidia Nsight Systems. The PR fixes the issue for me. 👍

@zadjii-msft
Copy link
Member Author

I'm building a Release signed msix for our internal friend to try out, but Leonard's comment is promising

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VER I FIED

@zadjii-msft zadjii-msft merged commit 4ebc383 into main Apr 25, 2023
@zadjii-msft zadjii-msft deleted the dev/migrie/b/theoretical-treit-fix branch April 25, 2023 21:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants