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

Revert the revert of "Hide the window until we're finished with initialization" #13811

Merged
merged 37 commits into from
Apr 6, 2023

Conversation

zadjii-msft
Copy link
Member

@zadjii-msft zadjii-msft commented Aug 22, 2022

This is a revert of the revert of #12979. We mainly reverted that PR because of an issue where restored windows would grow/shrink slightly on external displays. It was too close to the ship date for that release, so we backed it out wholesale (in #13098). I think I've found the real root of the problem, and fixed it here.

The money diff here from the original PR: 4c08b9a...c34495d. Basically, I had put the part where we actually handle the creation of the window into _AppInitializedHandler, when we should have left the window to be created in _HandleCreateWindow We create it there, hidden, and then should only show it in _AppInitializedHandler.

I'm NOT incorporating the change for #9053. I reverted that bit in 1fac403. I am too worried about that messing with the phwnd that I wanted to get that reviewed and committed atomically, separately.

  This hooks up the defterm interface to pass more info along in the startup. It's notably not the actual link title, but it does work from a plumbing standpoint

  This is for #9458
@ghost ghost added Area-User Interface Issues pertaining to the user interface of the Console or Terminal Area-Windowing Window frame, quake mode, tearout Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal. labels Aug 22, 2022
src/cascadia/TerminalApp/TerminalPage.cpp Outdated Show resolved Hide resolved
@zadjii-msft zadjii-msft marked this pull request as draft August 26, 2022 17:04
@zadjii-msft zadjii-msft removed the AutoMerge Marked for automatic merge by the bot when requirements are met label Dec 1, 2022
@zadjii-msft
Copy link
Member Author

Huh, I never added real notes as to why we held this one out. IIRC it was something like the fact that this would consistently work for the first window I'd make but not subsequent ones, which is w e i r d

And now that I'm 6 months removed from that code, that makes me thing it might have something to do with summoning peasant windows after creation, that the monarch wouldn't do. That's at least something that would be obviously different between the N=0 and N>=1 windows.

@zadjii-msft
Copy link
Member Author

@nirgranth if you'd like to help out, I'd start here. Or possibly even further back in the past, to the original version of this PR, #12979.

The original idea was that we shouldn't call ShowWindow in our WM_CREATE handler, we should instead wait until the XAML content is fuly initialized to show the window.

That original PR had some issue about getting the initial sizing just a little wrong, so we reverted that. This PR attempted to fix that sizing issue (and definitely did figure the sizing thing out).

But somewhere between there (#12979) and now, this PR doesn't seem to work at all anymore. Probably an incoming merge from main, but there's months of commits here now. It might just be best to start from the original idea fresh again.

Anyways, that's my notes. I'd doubt I'll have time to loop back on this one soon, so any help would be appreciated. I'm guessing there's a rogue ShowWindow call that's happening, but I don't see anything obvious to blame.

# Conflicts:
#	src/cascadia/TerminalApp/AppLogic.h
#	src/cascadia/TerminalApp/AppLogic.idl
#	src/cascadia/TerminalApp/TerminalPage.cpp
#	src/cascadia/WindowsTerminal/AppHost.cpp
#	src/cascadia/WindowsTerminal/AppHost.h
@zadjii-msft
Copy link
Member Author

Magically, this works again after 6a99f25. We needed another jiggle the handle to let XAML paint

@zadjii-msft zadjii-msft marked this pull request as ready for review April 5, 2023 21:27
@zadjii-msft zadjii-msft enabled auto-merge (squash) April 6, 2023 17:38
@zadjii-msft zadjii-msft merged commit 083fc64 into main Apr 6, 2023
@zadjii-msft zadjii-msft deleted the dev/migrie/b/9053-attempt-2 branch April 6, 2023 18:03
// If you summon here, the resulting code will call ShowWindow(SW_SHOW) on
// the Terminal window, making it visible BEFORE the XAML island is actually
// ready to be drawn. We want to wait till the app's Initialized event
// before we make the window visible.
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit worried that (1) we used to Summon from here and (2) now we aren't. Will this result in new terminal windows not coming to the front when they are the second window in the process? Will this result in us missing some activations somehow?

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.

Could this PR be the cause of #15139...somehow?

microsoft-github-policy-service bot pushed a commit that referenced this pull request Apr 18, 2023
…ute (#13838)

Original description, pre-process model v3:

> This is just the `SHOWDEFAULT` bit from #12979. This seems to also
work now, but I'm PR'ing it separately so it can be a separate revert
from #13811, if it is problematic.

More accurately: 
This PR enables terminal windows to use the `wShowCmd` from the
STARTUPINFO passed to `windowsterminal.exe` to set the initial
visibility of the window. We can't just use `SW_SHOWDEFAULT`, because
all the windows are running in the initial process! After the first
window, the subsequent ones would ignore any params passed to their
originating `windowsterminal.exe` processes. To mitigate, we pass that
`wShowCmd` info from the source process, to the actual running terminal
process. That accounts for most of the delta here.

Closes #9053


This doesn't do the same for defterm-initiated connections. This is
because we don't need to! Defterm very explicitly rejects handoff for
minimized console apps. This is probably for the best! I put an attempt
in 66f8b25 before I forgot that it was filtered long before the
Terminal. NOT doing this for /min saves us all sorts of "what happens if
`start /min cmd` tries to glom?" or "what if someone does `start /min
cmd && start /max cmd` and they glom together?".

<hr>

Also closes #15193, which was introduced as a part of this.

---------

Co-authored-by: Dustin L. Howett <[email protected]>
microsoft-github-policy-service bot pushed a commit that referenced this pull request May 2, 2023
Apparently, `ShowWindow` also sends a `WM_MOVE`, which we then turn
around and use to dismiss open dialogs.

Closes #15170

Regressed in #13811
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-User Interface Issues pertaining to the user interface of the Console or Terminal Area-Windowing Window frame, quake mode, tearout Automerge-Not-Compatible Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-3 A description (P3) Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Starting WT momentarily shows a black titlebar
3 participants