Conversation
leonMSFT
added
Issue-Bug
ghost
requested review from
DHowett,
PankajBhojwani,
carlos-zamora,
lhecker and
zadjii-msft
| } | ||
| else if (_window->IsQuakeWindow()) | ||
| { | ||
| _HideTrayIconRequested(); |
There was a problem hiding this comment.
This is a bad idea; _HideTrayIconRequested is a coroutine that takes an implicit reference to this, and you're dispatching it out of the destructor. By the time the resume_backround resumes you, this will have been destroyed.
This needs to be restructured such that (1) the destructor never uses anything that requires a living this to persist after the destructor returns and (2) it is as small as possible 😄
(Also, there's no guarantee that AppHost will be destructed.)
There was a problem hiding this comment.
Also, there's no guarantee that AppHost will be destructed.
It's a regular instance in wWinMain. It should be destroyed properly unless the app abort()s, right?
We could make a _HideTrayIconRequestedAsync and have _HideTrayIconRequested run synchronously.
BTW _HideTrayIconRequested accesses settings from a background thread which is a race condition... I think I should add thread asserts to CascadiaSettings. 🤔
There was a problem hiding this comment.
It's a regular instance in
wWinMain. It should be destroyed properly unless the appabort()s, right?
Yes. That does assume that main exits normally. If we were more like a standard UWP Xaml application, we would actually ExitProcess/TerminateProcess when the window was destroyed. The fact that we do not makes us uncommon in the world of Xaml, and is the cause for some of our "this object was already disposed of!" error traces on shutdown in debug mode. 😄
(Because of that, we've entertained the idea multiple times of switching -- it would clear up almost all of our on-exit crashes that originate deep inside Windows.UI.Xaml.dll!)
There was a problem hiding this comment.
Hmm then I guess an alternative might be to do this particular Tray Icon cleanup in something like LastTabClosed?
ghost
added
Needs-Author-Feedback
| _setupGlobalHotkeys(); | ||
|
|
||
| // The monarch is just going to be THE listener for inbound connections. | ||
| _listenForInboundConnections(); |
There was a problem hiding this comment.
I don't understand this change. Otherwise I'd approve this PR.
There was a problem hiding this comment.
Oh this was originally removed as part of this defapp bug fix #10823, and my bad merge left it in 😥. In short, the fix made it so that instead of making the monarch receive default terminal handoffs by default, the monarch should first figure out which peasant (or itself) should handle a default terminal handoff and tell that window to receive the connection.
| @@ -324,6 +319,15 @@ void AppHost::AppTitleChanged(const winrt::Windows::Foundation::IInspectable& /* | |||
| // - <none> | |||
| void AppHost::LastTabClosed(const winrt::Windows::Foundation::IInspectable& /*sender*/, const winrt::TerminalApp::LastTabClosedEventArgs& /*args*/) | |||
There was a problem hiding this comment.
you might hate me for asking . . . but does this fire when you use the [x] on the window? I know that we try to kill all the tabs, but this might only be a side effect... unless it is the main effect 😀
There was a problem hiding this comment.
It does in fact go off on the X button 🙂
|
We might want to throw an |
|
(just right at the top, but inside the |
…a little closer to the x-proc call
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 (
|
|
🎉 Handy links: |
5 participants
Some followups to #10368: