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

Hide the window from DWM until we're finished with initialization #12979

Merged
15 commits merged into from
May 6, 2022
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/actions/spelling/allow/apis.txt
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ DERR
dlldata
DONTADDTORECENT
DWORDLONG
DWMWA
endfor
enumset
environstrings
Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalApp/AppLogic.h
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,7 @@ namespace winrt::TerminalApp::implementation
FORWARDED_TYPED_EVENT(FullscreenChanged, winrt::Windows::Foundation::IInspectable, winrt::Windows::Foundation::IInspectable, _root, FullscreenChanged);
FORWARDED_TYPED_EVENT(ChangeMaximizeRequested, winrt::Windows::Foundation::IInspectable, winrt::Windows::Foundation::IInspectable, _root, ChangeMaximizeRequested);
FORWARDED_TYPED_EVENT(AlwaysOnTopChanged, winrt::Windows::Foundation::IInspectable, winrt::Windows::Foundation::IInspectable, _root, AlwaysOnTopChanged);
FORWARDED_TYPED_EVENT(Initialized, winrt::Windows::Foundation::IInspectable, winrt::Windows::Foundation::IInspectable, _root, Initialized);
FORWARDED_TYPED_EVENT(RaiseVisualBell, winrt::Windows::Foundation::IInspectable, winrt::Windows::Foundation::IInspectable, _root, RaiseVisualBell);
FORWARDED_TYPED_EVENT(SetTaskbarProgress, winrt::Windows::Foundation::IInspectable, winrt::Windows::Foundation::IInspectable, _root, SetTaskbarProgress);
FORWARDED_TYPED_EVENT(IdentifyWindowsRequested, Windows::Foundation::IInspectable, Windows::Foundation::IInspectable, _root, IdentifyWindowsRequested);
Expand Down
1 change: 1 addition & 0 deletions src/cascadia/TerminalApp/AppLogic.idl
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,7 @@ namespace TerminalApp
event Windows.Foundation.TypedEventHandler<Object, Object> FullscreenChanged;
event Windows.Foundation.TypedEventHandler<Object, Object> ChangeMaximizeRequested;
event Windows.Foundation.TypedEventHandler<Object, Object> AlwaysOnTopChanged;
event Windows.Foundation.TypedEventHandler<Object, Object> Initialized;
event Windows.Foundation.TypedEventHandler<Object, Object> RaiseVisualBell;
event Windows.Foundation.TypedEventHandler<Object, Object> SetTaskbarProgress;
event Windows.Foundation.TypedEventHandler<Object, Object> IdentifyWindowsRequested;
Expand Down
42 changes: 40 additions & 2 deletions src/cascadia/TerminalApp/TerminalPage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -658,7 +658,7 @@ namespace winrt::TerminalApp::implementation
// - <none>
// Return Value:
// - <none>
void TerminalPage::_CompleteInitialization()
winrt::fire_and_forget TerminalPage::_CompleteInitialization()
{
_startupState = StartupState::Initialized;

Expand All @@ -678,10 +678,48 @@ namespace winrt::TerminalApp::implementation
if (_tabs.Size() == 0 && !(_shouldStartInboundListener || _isEmbeddingInboundListener))
{
_LastTabClosedHandlers(*this, nullptr);
co_return;
}
else
{
_InitializedHandlers(*this, nullptr);
// GH#11561: 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 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.
//
// We 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.

// Capture calling context.
winrt::apartment_context ui_thread;
Copy link
Member

Choose a reason for hiding this comment

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

Unused?

Copy link
Member Author

Choose a reason for hiding this comment

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

yea - I wanted to just co_await ui_thread, but that doesn't let me await it with low priority 🤷

Copy link
Member

@lhecker lhecker Apr 26, 2022

Choose a reason for hiding this comment

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

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_context is blocking [the background thread]. That's because it's built out of COM's IContextCallback, 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...

auto weak{ get_weak() };

// Switch to the BG thread -
co_await winrt::resume_background();

// Then enqueue the rest of this function for after the UI thread settles.
co_await wil::resume_foreground(Dispatcher(), CoreDispatcherPriority::Low);

if (auto self{ weak.get() })
{
_InitializedHandlers(*self, nullptr);
}
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/cascadia/TerminalApp/TerminalPage.h
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ namespace winrt::TerminalApp::implementation
TYPED_EVENT(AlwaysOnTopChanged, IInspectable, IInspectable);
TYPED_EVENT(RaiseVisualBell, IInspectable, IInspectable);
TYPED_EVENT(SetTaskbarProgress, IInspectable, IInspectable);
TYPED_EVENT(Initialized, IInspectable, winrt::Windows::UI::Xaml::RoutedEventArgs);
TYPED_EVENT(Initialized, IInspectable, IInspectable);
TYPED_EVENT(IdentifyWindowsRequested, IInspectable, IInspectable);
TYPED_EVENT(RenameWindowRequested, Windows::Foundation::IInspectable, winrt::TerminalApp::RenameWindowRequestedArgs);
TYPED_EVENT(IsQuakeWindowChanged, IInspectable, IInspectable);
Expand Down Expand Up @@ -385,7 +385,7 @@ namespace winrt::TerminalApp::implementation

void _StartInboundListener();

void _CompleteInitialization();
winrt::fire_and_forget _CompleteInitialization();

void _FocusActiveControl(IInspectable sender, IInspectable eventArgs);

Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalApp/TerminalPage.idl
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ namespace TerminalApp
event Windows.Foundation.TypedEventHandler<Object, Object> FocusModeChanged;
event Windows.Foundation.TypedEventHandler<Object, Object> FullscreenChanged;
event Windows.Foundation.TypedEventHandler<Object, Object> AlwaysOnTopChanged;
event Windows.Foundation.TypedEventHandler<Object, Windows.UI.Xaml.RoutedEventArgs> Initialized;
event Windows.Foundation.TypedEventHandler<Object, Object> Initialized;
event Windows.Foundation.TypedEventHandler<Object, Object> SetTaskbarProgress;
event Windows.Foundation.TypedEventHandler<Object, Object> IdentifyWindowsRequested;
event Windows.Foundation.TypedEventHandler<Object, RenameWindowRequestedArgs> RenameWindowRequested;
Expand Down
21 changes: 21 additions & 0 deletions src/cascadia/WindowsTerminal/AppHost.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -377,6 +377,7 @@ void AppHost::Initialize()
_revokers.FullscreenChanged = _logic.FullscreenChanged(winrt::auto_revoke, { this, &AppHost::_FullscreenChanged });
_revokers.FocusModeChanged = _logic.FocusModeChanged(winrt::auto_revoke, { this, &AppHost::_FocusModeChanged });
_revokers.AlwaysOnTopChanged = _logic.AlwaysOnTopChanged(winrt::auto_revoke, { this, &AppHost::_AlwaysOnTopChanged });
_revokers.Initialized = _logic.Initialized(winrt::auto_revoke, { this, &AppHost::_AppInitializedHandler });
_revokers.RaiseVisualBell = _logic.RaiseVisualBell(winrt::auto_revoke, { this, &AppHost::_RaiseVisualBell });
_revokers.SystemMenuChangeRequested = _logic.SystemMenuChangeRequested(winrt::auto_revoke, { this, &AppHost::_SystemMenuChangeRequested });
_revokers.ChangeMaximizeRequested = _logic.ChangeMaximizeRequested(winrt::auto_revoke, { this, &AppHost::_ChangeMaximizeRequested });
Expand Down Expand Up @@ -533,6 +534,14 @@ LaunchPosition AppHost::_GetWindowLaunchPosition()
// - None
void AppHost::_HandleCreateWindow(const HWND hwnd, RECT proposedRect, LaunchMode& launchMode)
{
// GH#11561: Hide the window until we're totally done being initialized.
// More commentary in TerminalPage::_CompleteInitialization
BOOL cloak = TRUE;
LOG_IF_FAILED(DwmSetWindowAttribute(hwnd,
DWMWA_CLOAK,
&cloak,
sizeof(cloak)));

launchMode = _logic.GetLaunchMode();

// Acquire the actual initial position
Expand Down Expand Up @@ -1547,3 +1556,15 @@ void AppHost::_CloseRequested(const winrt::Windows::Foundation::IInspectable& /*
const auto pos = _GetWindowLaunchPosition();
_logic.CloseWindow(pos);
}

void AppHost::_AppInitializedHandler(const winrt::Windows::Foundation::IInspectable& /*sender*/,
const winrt::Windows::Foundation::IInspectable& /*arg*/)
{
// GH#11561: We're totally done being initialized. Uncloak the window, making it visible.
// More commentary in TerminalPage::_CompleteInitialization
BOOL cloak = FALSE;
LOG_IF_FAILED(DwmSetWindowAttribute(_window->GetHandle(),
DWMWA_CLOAK,
&cloak,
sizeof(cloak)));
}
4 changes: 4 additions & 0 deletions src/cascadia/WindowsTerminal/AppHost.h
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,9 @@ class AppHost
const winrt::Windows::Foundation::IInspectable& arg);
void _AlwaysOnTopChanged(const winrt::Windows::Foundation::IInspectable& sender,
const winrt::Windows::Foundation::IInspectable& arg);
void _AppInitializedHandler(const winrt::Windows::Foundation::IInspectable& sender,
const winrt::Windows::Foundation::IInspectable& arg);

void _RaiseVisualBell(const winrt::Windows::Foundation::IInspectable& sender,
const winrt::Windows::Foundation::IInspectable& arg);
void _WindowMouseWheeled(const til::point coord, const int32_t delta);
Expand Down Expand Up @@ -144,6 +147,7 @@ class AppHost
winrt::TerminalApp::AppLogic::FullscreenChanged_revoker FullscreenChanged;
winrt::TerminalApp::AppLogic::FocusModeChanged_revoker FocusModeChanged;
winrt::TerminalApp::AppLogic::AlwaysOnTopChanged_revoker AlwaysOnTopChanged;
winrt::TerminalApp::AppLogic::Initialized_revoker Initialized;
winrt::TerminalApp::AppLogic::RaiseVisualBell_revoker RaiseVisualBell;
winrt::TerminalApp::AppLogic::SystemMenuChangeRequested_revoker SystemMenuChangeRequested;
winrt::TerminalApp::AppLogic::ChangeMaximizeRequested_revoker ChangeMaximizeRequested;
Expand Down