Tray Icon PR followup #10938

leonMSFT · 2021-08-12T22:49:03Z

Some followups to #10368:

Accidentally reverted a defapp change where the Monarch should not by default register itself as a handoff server.
Destroy the tray icon if we're a monarch otherwise if we're a quake window we request the monarch to hide the icon.

DHowett · 2021-08-13T18:27:57Z

src/cascadia/WindowsTerminal/AppHost.cpp

+    }
+    else if (_window->IsQuakeWindow())
+    {
+        _HideTrayIconRequested();


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.)

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. 🤔

It's a regular instance in wWinMain. It should be destroyed properly unless the app abort()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!)

Hmm then I guess an alternative might be to do this particular Tray Icon cleanup in something like LastTabClosed?

DHowett

as noted

zadjii-msft

lgtm

lhecker · 2021-08-16T15:41:42Z

src/cascadia/WindowsTerminal/AppHost.cpp

@@ -656,9 +660,6 @@ void AppHost::_BecomeMonarch(const winrt::Windows::Foundation::IInspectable& /*s
 {
    _setupGlobalHotkeys();

-    // The monarch is just going to be THE listener for inbound connections.
-    _listenForInboundConnections();


I don't understand this change. Otherwise I'd approve this PR.

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.

DHowett · 2021-08-16T21:41:56Z

src/cascadia/WindowsTerminal/AppHost.cpp

@@ -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*/)


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 😀

It does in fact go off on the X button 🙂

DHowett · 2021-08-16T21:43:14Z

We might want to throw an auto strongThis{ get_strong() }; in _HideTrayIconRequested, which will guarantee that it stays alive across the coroutine boundary. Otherwise, things could still get dicey.

DHowett · 2021-08-16T21:43:32Z

(just right at the top, but inside the if constexpr)

…a little closer to the x-proc call

ghost · 2021-08-19T17:37:59Z

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

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 (`@msftbot`) and give me an instruction to get started! Learn more here.

ghost · 2021-08-31T17:15:00Z

🎉Windows Terminal Preview v1.11.2421.0 has been released which incorporates this pull request.:tada:

Handy links:

accidentally kept monarch as defapp server, destroy tray icon if monarch

2c2a880

leonMSFT added Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. Area-User Interface Issues pertaining to the user interface of the Console or Terminal labels Aug 12, 2021

miniksa approved these changes Aug 13, 2021

View reviewed changes

leonMSFT added the Needs-Second It's a PR that needs another sign-off label Aug 13, 2021

ghost requested review from zadjii-msft, carlos-zamora, DHowett, PankajBhojwani and lhecker August 13, 2021 18:17

DHowett reviewed Aug 13, 2021

View reviewed changes

DHowett requested changes Aug 13, 2021

View reviewed changes

ghost 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 Aug 13, 2021

move tray icon cleanup to somewhere that's not the destructor

fdd14b3

zadjii-msft approved these changes Aug 16, 2021

View reviewed changes

lhecker reviewed Aug 16, 2021

View reviewed changes

lhecker approved these changes Aug 16, 2021

View reviewed changes

DHowett reviewed Aug 16, 2021

View reviewed changes

leonMSFT added 2 commits August 16, 2021 16:46

ensure we're not dead when we want to cleanup, move to the bg thread …

b94e199

…a little closer to the x-proc call

formatting

dd8d07e

DHowett approved these changes Aug 19, 2021

View reviewed changes

DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Aug 19, 2021

ghost merged commit 482dcec into main Aug 19, 2021

ghost deleted the dev/lelian/notifyicon/followup branch August 19, 2021 17:38

carlos-zamora mentioned this pull request Sep 7, 2021

Add the ability to quit all terminal instances #11143

Merged

3 tasks

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tray Icon PR followup #10938

Tray Icon PR followup #10938

leonMSFT commented Aug 12, 2021

DHowett Aug 13, 2021

lhecker Aug 13, 2021 •

edited

Loading

DHowett Aug 13, 2021

leonMSFT Aug 13, 2021

DHowett left a comment

zadjii-msft left a comment

lhecker Aug 16, 2021

leonMSFT Aug 16, 2021

DHowett Aug 16, 2021

leonMSFT Aug 16, 2021 •

edited

Loading

DHowett commented Aug 16, 2021

DHowett commented Aug 16, 2021

ghost commented Aug 19, 2021

ghost commented Aug 31, 2021

		@@ -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/)

Tray Icon PR followup #10938

Tray Icon PR followup #10938

Conversation

leonMSFT commented Aug 12, 2021

DHowett Aug 13, 2021

Choose a reason for hiding this comment

lhecker Aug 13, 2021 • edited Loading

Choose a reason for hiding this comment

DHowett Aug 13, 2021

Choose a reason for hiding this comment

leonMSFT Aug 13, 2021

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

zadjii-msft left a comment

Choose a reason for hiding this comment

lhecker Aug 16, 2021

Choose a reason for hiding this comment

leonMSFT Aug 16, 2021

Choose a reason for hiding this comment

DHowett Aug 16, 2021

Choose a reason for hiding this comment

leonMSFT Aug 16, 2021 • edited Loading

Choose a reason for hiding this comment

DHowett commented Aug 16, 2021

DHowett commented Aug 16, 2021

ghost commented Aug 19, 2021

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 (@msftbot) and give me an instruction to get started! Learn more here.

ghost commented Aug 31, 2021

lhecker Aug 13, 2021 •

edited

Loading

leonMSFT Aug 16, 2021 •

edited

Loading

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 (`@msftbot`) and give me an instruction to get started! Learn more here.