Hide the window from DWM until we're finished with initialization #12979
Conversation
ghost
added
Area-UserInterface
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
| // so that didn't actually resolve any issues. | ||
|
|
||
| // Capture calling context. | ||
| winrt::apartment_context ui_thread; |
There was a problem hiding this comment.
yea - I wanted to just co_await ui_thread, but that doesn't let me await it with low priority 🤷
There was a problem hiding this comment.
We should try to avoid using winrt::apartment_context as it's inherently flawed unfortunately. 😥
As Raymond Chen wrote:
Unfortunately, it's true. co_await'ing an
apartment_contextis blocking [the background thread]. That's because it's built out of COM'sIContextCallback, which supports only synchronous transfer.
[...]
Note that it's even worse: Even after you get to the other side, the side you came *from* is still blocked until you finish doing your other-side work (which could take the form of another co_await).
[...]
If the UI thread is hung, this will lead to threadpool starvation and eventually a deadlock.
_GetWindowLayoutAsync() in particular is quite dangerous because it uses an apartment_context to switch back to a background thread and blocks the UI thread for that duration...
|
Okay, after discussing in team sync - we totally can just defer the actual first call to
and now we get the little shwoop animation back too. Thanks for the idea @lhecker! |
| if (auto self{ weak.get() }) | ||
| { | ||
| // Then enqueue the rest of this function for after the UI thread settles. | ||
| co_await wil::resume_foreground(self->Dispatcher(), CoreDispatcherPriority::Low); |
There was a problem hiding this comment.
I'd suggest just storing the dispatcher in the beginning as
const auto dispatcher = Dispatcher();Seems like a more hassle free solution to me.
Also: Do you mind removing the winrt::apartment_context instantiation?
Finally there's the option to drop all of the coroutine stuff and write this:
Dispatcher().RunAsync(CoreDispatcherPriority::Low, [weak = get_weak()]() {
if (auto self = weak.get())
{
self->_InitializedHandlers(*self, nullptr);
}
});There was a problem hiding this comment.
Also: Do you mind removing the
winrt::apartment_contextinstantiation?
frick deleted locally, forgot to save
I'd suggest just storing the dispatcher in the beginning
I thought it best to not stash an owning reference to the dispatcher, as to decrease the chances that the coroutine kept the Dispatcher alive despite the Page getting deref'ed.
zadjii-msft
changed the title
|
|
| // visible. | ||
|
|
||
| LaunchMode launchMode{}; | ||
| _initialResizeAndRepositionWindow(_window->GetHandle(), _proposedRect, launchMode); |
There was a problem hiding this comment.
Worried a bit about the resize - how often will we start up at the wrong size?
There was a problem hiding this comment.
lmao well this was the issue now 😑
Yep, pretty slick honestly
The foreground window changes for a second, but the new Terminal window never actually gets drawn.
Seems to work well enough
You got me, in that specific case, a frame flashes and immediately goes away. |
DHowett
added
the
AutoMerge
|
Hello @DHowett! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
ghost
merged commit 
…2979) When we start up, our window is initially just a frame with a transparent content area. We're gonna do all this startup init on the UI thread, so the UI won't actually paint till it's all done. This results in a few frames where the frame is visible, before the page paints for the first time, before any tabs appears, etc. To mitigate this, we're gonna wait for the UI thread to finish everything it's gotta do for the initial init, and _then_ fire our Initialized event. By waiting for everything else to finish (`CoreDispatcherPriority::Low`), we let all the tabs and panes actually get created. In the window layer, we're gonna ~cloak~ just not show the window till this event is fired, so we don't actually see this frame until we're actually all ready to go. **This will result in the window seemingly not loading as fast**, but it will actually take exactly the same amount of time before it's usable. I also experimented with drawing a solid BG color before the initialization is finished. However, there are still a few frames after the frame is displayed before the XAML content first draws, so that didn't actually resolve any issues. * [x] Closes #11561 * [x] Tested manually * [x] I work here. * [x] Accidentally also closes #9053. By switching the initial call from `ShowWindow(SW_SHOW)` to `ShowWindow(SW_SHOWDEFAULT)`, we actually obey the startup info now. (cherry picked from commit 14098d7) Service-Card-Id: 81599073 Service-Version: 1.14
## Summary of the Pull Request For some reason, the PGO tests (specifically the `RunMakeKillTabs` test) started to fail after #12979 merged. After closer inspection, the test was actually improperly written. We should be using <kbd>ctrl+shift+t</kbd> to open new tabs, not <kbd>alt+shift+t</kbd>. Presumably, the <kbd>alt</kbd> was copied over from the previous test, because they look _very_ similar. So I went ahead and fixed the test, and it now (1) tests what it's intended to test and (2) doesn't fail. Why did #12979 cause the tests to fail? idk, but it works now. ## References #10071 - Introduce PGO Tests ## Validation Steps Performed Ran PGO tests locally and confirmed that it works. Ran PGO pipeline and confirmed that it works.
## Summary of the Pull Request For some reason, the PGO tests (specifically the `RunMakeKillTabs` test) started to fail after #12979 merged. After closer inspection, the test was actually improperly written. We should be using <kbd>ctrl+shift+t</kbd> to open new tabs, not <kbd>alt+shift+t</kbd>. Presumably, the <kbd>alt</kbd> was copied over from the previous test, because they look _very_ similar. So I went ahead and fixed the test, and it now (1) tests what it's intended to test and (2) doesn't fail. Why did #12979 cause the tests to fail? idk, but it works now. ## References #10071 - Introduce PGO Tests ## Validation Steps Performed Ran PGO tests locally and confirmed that it works. Ran PGO pipeline and confirmed that it works. (cherry picked from commit 17d1e24) Service-Card-Id: 81863976 Service-Version: 1.14
|
we definitely need to back this one out, I think it's causing my persisted windows (on my secondary monitor) to shrink a little each time. |
…tion (#12979)" (#13098) This reverts commit 14098d7. ## Summary of the Pull Request @zadjii-msft found that this is causing persisted windows on a secondary monitor to shrink a little each time. We're choosing to revert this commit until that gets resolved. ## References #12979
…tion (#12979)" (#13098) This reverts commit 14098d7. ## Summary of the Pull Request @zadjii-msft found that this is causing persisted windows on a secondary monitor to shrink a little each time. We're choosing to revert this commit until that gets resolved. ## References #12979 (cherry picked from commit 6ffc3dc) Service-Card-Id: 81910920 Service-Version: 1.14
…alization" (#13811) 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. * fixes #11561 * tested manually * I work here
…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]>
When we start up, our window is initially just a frame with a transparent content area. We're gonna do all this startup init on the UI thread, so the UI won't actually paint till it's all done. This results in a few frames where the frame is visible, before the page paints for the first time, before any tabs appears, etc.
To mitigate this, we're gonna wait for the UI thread to finish everything it's gotta do for the initial init, and then fire our Initialized event. By waiting for everything else to finish (
CoreDispatcherPriority::Low), we let all the tabs and panes actually get created. In the window layer, we're gonnacloakjust not show the window till this event is fired, so we don't actually see this frame until we're actually all ready to go. This will result in the window seemingly not loading as fast, but it will actually take exactly the same amount of time before it's usable.I also experimented with drawing a solid BG color before the initialization is finished. However, there are still a few frames after the frame is displayed before the XAML content first draws, so that didn't actually resolve any issues.
ShowWindow(SW_SHOW)toShowWindow(SW_SHOWDEFAULT), we actually obey the startup info now.