Add Minimize to Tray and Tray Icon #10368

leonMSFT · 2021-06-08T19:22:39Z

A brief summary of the behavior of the tray icon:

There will only ever be one tray icon representing all windows.
Left-Click on a Tray Icon brings up the MRU window.
Right-Click on a Tray Icon brings up a Context Menu:

Focus Terminal
----------------
Windows --> Window ID 1 - <unnamed window>
            Named Window
            Named Window Again

Focus Terminal will bring up the MRU window.
Clicking on any of the Window "names" in the submenu will summon the window.

Settings Changes

Two new global settings are introduced: alwaysShowTrayIcon and minimizeToTray. Here's a chart explaining the behavior with the two settings.

	`alwaysShowTrayIcon:true`	`alwaysShowTrayIcon:false`
`minimizeToTray:true`	tray icon is always shown. minimize button will hide the window.	tray icon is always shown. minimize button will hide the window.
`minimizeToTray:false`	tray icon is always shown.	tray icon is not shown ever.

Closes #5727

References

Spec for Minimize to Tray
Docs PR - MicrosoftDocs/terminal#352
#10448 - My list of TODOs

The text was updated successfully, but these errors were encountered:

This comment has been hidden.

Sign in to view

This comment has been hidden.

Sign in to view

This comment has been hidden.

Sign in to view

This comment has been hidden.

Sign in to view

This comment has been hidden.

Sign in to view

This comment has been hidden.

Sign in to view

This comment has been hidden.

Sign in to view

This comment has been hidden.

Sign in to view

This comment has been hidden.

Sign in to view

This comment has been hidden.

Sign in to view

This comment has been hidden.

Sign in to view

zadjii-msft · 2021-07-21T15:46:43Z

I've introduced a new keybinding also named minimizeToTray for convenience in case users perfer to minimize with a keybinding instead of having to click on the minimize button. Upon activation, the currently focused window will become hidden.

wait uh, I don't think we need this. Win+down will minimize a window

zadjii-msft

blocking for discussion on the minimizeToTray action and the #if TIL_FEATURE thing

src/cascadia/WindowsTerminal/main.cpp

zadjii-msft · 2021-07-21T16:18:29Z

src/cascadia/TerminalApp/Resources/en-US/Resources.resw

@@ -637,6 +637,17 @@
  <data name="CommandPaletteMenuItem" xml:space="preserve">
    <value>Command Palette</value>
  </data>
+  <data name="AppName" xml:space="preserve">
+    <value>Windows Terminal</value>


wait uh, if you're gonna pull it from the CascadiaPackage resources, do we need this string still?

src/cascadia/TerminalApp/AppLogic.cpp

leonMSFT · 2021-07-21T18:18:00Z

wait uh, I don't think we need this. Win+down will minimize a window

Ah crap I didn't even know about this shortcut 😅
Ok so with this shortcut, the user can minimize to tray with the keyboard as long as minimizeToTray = true.
What about users who have minimizeToTray = false, alwaysShowTrayIcon = true? They may not want all minimize actions to minimizeToTray, but they'd likely want some way of sending the window to the tray considering they want the icon there. Perhaps it should be a system menu item? Or maybe it should be a tray icon menu item? 🤔

zadjii-msft · 2021-07-21T18:54:32Z

They may not want all minimize actions to minimizeToTray, but they'd likely want some way of sending the window to the tray considering they want the icon there.

Hmm. Interesting. Okay, I'm convinced. I kinda think it should be a separate PR, but it's already here so ¯\_(ツ)_/¯

zadjii-msft

Okay most of these are just notes to self as I read the code. Mostly just indicators of where I might have been confused as I was following the logic.

I think at this point I'm mostly blocking on the "add a try/catch here" one.

I don't know how I feel about the minimize to tray action, but I don't feel like blocking that element of it.

This is ∞% better without all the ifdefs, thanks for that ☺️

zadjii-msft · 2021-08-03T17:09:46Z

src/cascadia/TerminalApp/AppActionHandlers.cpp

+                                             const ActionEventArgs& args)
+    {
+#if TIL_FEATURE_TRAYICON_ENABLED
+        if (_settings.GlobalSettings().MinimizeToTray() || _settings.GlobalSettings().AlwaysShowTrayIcon())


Wait so if they don't have either of these features on, then this will do nothing. I thought the point of this action was to allow minimizing a window to the tray even when neither setting is on?

right okay so this action only works when there is a tray icon. Right, cause otherwise the window would just disappear into the void. And we're not doing any sort of per-window tracking of "this window wanted to be able to minimize to the tray, so we must have a tray icon".

IDK I still feel weird about this action and almost wish it was a separate PR to review/discuss

Yup, pretty much I don't want to let the user do be able to minimize if there won't be a tray icon. The idea was to be able to minimize when minimizeToTray = false, alwaysShowTrayIcon = true.

The per-window tracking would definitely be useful especially for big sweeping actions like "summon all" - I'll add it to the list.

I'll also split this off into its own PR and we could discuss some more there - makes the PR smaller too 😉

src/cascadia/WindowsTerminal/AppHost.cpp

zadjii-msft · 2021-08-03T17:09:46Z

src/cascadia/WindowsTerminal/AppHost.cpp

-        // work properly.
-        nid.uVersion = NOTIFYICON_VERSION_4;
-        Shell_NotifyIcon(NIM_SETVERSION, &nid);
+void AppHost::_ShowTrayIconRequested()


Don't we need to make sure to hop onto the BG thread at the top of this (and _HideTrayIconRequested)? Usually the COM x-proc calls end up throwing exceptions/gracefully doing nothing when called on the main thread.

Is this really never called on the main thread?

_IsQuakeWindowChanged - pretty sure that can be on the main thread... unsure tho

_windowManager.ShowTrayIconRequested okay this one's on the bg thread

Hmm I've never observed it doing weird stuff, I'll throw it on a resume_background in the beginning of the functions just to be safe.

src/cascadia/WindowsTerminal/AppHost.cpp

src/cascadia/WindowsTerminal/IslandWindow.cpp

src/cascadia/WindowsTerminal/TrayIcon.cpp

zadjii-msft · 2021-08-03T17:09:46Z

src/cascadia/Remoting/Monarch.cpp

+        {
+            SummonWindowBehavior args{};
+            args.ToggleVisibility(false);
+            peasant.Summon(args);


wrap this boy up in a try/catch

I'm actually doing the _forAllPeasantsIgnoringTheDead thing for this little snippet, that has error handling in it so I should be good right?

Oh yep, you're good

zadjii-msft · 2021-08-03T17:09:46Z

src/cascadia/WindowsTerminal/TrayIcon.cpp

+// - <none>
+void TrayIcon::DestroyTrayIcon()
+{
+    Shell_NotifyIcon(NIM_DELETE, &_trayIconData);


Should we also do a

_trayIconData.reset();

here, like we used to?

I was intending for this function to be more of a RemoveIconFromTray instead of a "destroy" (I'll rename). I wanted the actual "destruction" of the tray icon to be when AppHost sets its _trayIcon ptr to null, and if it ever wanted to get a new tray icon, it should instantiate a new one.

Okeydokey, good enough for me

…ult because I think it looks nicer

zadjii-msft

lets do it

msftbot · 2021-08-11T23:27:10Z

Hello @leonMSFT!

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.

carlos-zamora

I think we should be using if constexpr (Feature_XXX::IsEnabled()) when possible
the gsl::narrow vs gsl::narrow_cast thing is a bit concerning, so I just want to double check that
the one copyright header that's missing

doc/cascadia/profiles.schema.json

src/cascadia/TerminalApp/AppLogic.cpp

src/cascadia/Remoting/WindowManager.cpp

src/cascadia/WindowsTerminal/TrayIcon.cpp

src/cascadia/WindowsTerminal/AppHost.cpp

DHowett · 2021-08-12T22:08:14Z

src/cascadia/WindowsTerminal/AppHost.cpp

    {
-        Shell_NotifyIcon(NIM_DELETE, &_trayIconData.value());
-        _trayIconData.reset();
+        _windowManager.RequestHideTrayIcon();


btw there's no guarantee that this destructs 😄

argh I used the wrong one here it should be AppHost::_HideTrayIconRequested

just kidding that's not the right one either

DHowett · 2021-08-12T22:08:14Z

src/cascadia/WindowsTerminal/AppHost.cpp

@@ -662,6 +655,20 @@ void AppHost::_BecomeMonarch(const winrt::Windows::Foundation::IInspectable& /*s
                             const winrt::Windows::Foundation::IInspectable& /*args*/)
 {
    _setupGlobalHotkeys();
+
+    // The monarch is just going to be THE listener for inbound connections.
+    _listenForInboundConnections();


this doesn't break the defapp server?

oh shit good catch

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.

initial

150efbb

leonMSFT added 2 commits May 20, 2021

give it a fresh look

87238e1

scaffolding for MinimizeToTray action

b3ae922

scoping to quek only

7964b5a

cleanup

eee447c

using the correct term

a83686a

not sure if necessary, but try not to add every single time

e8462e0

remove when no longer quake

b2b76ca

pr comments, spelling

ce22c0c

leonMSFT added 8 commits May 26, 2021

ok ok ok fine

8fa4718

saving progress

bdfef54

run it back

5f84958

merge conflicts

2707769

add a hwnd check

f432539

merge conflicts'

604c204

Merge branch 'main' into dev/lelian/notifyicon/yougetoneyougetone

bd1a8a7

remove this

b90f0aa

smooth like butter

f82cffa

bring out your dead

46af976

zadjii-msft requested changes Jul 21, 2021

View changes

msftbot bot added Needs-Author-Feedback and removed Needs-Author-Feedback labels Jul 21, 2021

leonMSFT added 2 commits Jul 21, 2021

cleaning up so eyes don't bleed

d827e6b

a bit of cleanup

bc62fef

zadjii-msft requested changes Aug 3, 2021

View changes

msftbot bot added Needs-Author-Feedback and removed Needs-Author-Feedback labels Aug 3, 2021

leonMSFT added 5 commits Aug 5, 2021

smalls

af2ee79

other smalls

a90d2ec

PR comments, merge from main

50ffa27

rename from destroy tray icon to remove, put the if statement in defa…

ac8513e

…ult because I think it looks nicer

move the windows to whence they came

1c731a1

zadjii-msft approved these changes Aug 11, 2021

View changes

zadjii-msft requested a review from carlos-zamora Aug 11, 2021

Merge branch 'main' into dev/lelian/notifyicon/yougetoneyougetone

df559c1

leonMSFT added the AutoMerge label Aug 11, 2021

carlos-zamora reviewed Aug 12, 2021

View changes

use constexpr for feature flag, copyright header, narrow_cast

cbf08aa

carlos-zamora approved these changes Aug 12, 2021

View changes

msftbot bot merged commit a0edb12 into main Aug 12, 2021
9 checks passed

msftbot bot deleted the dev/lelian/notifyicon/yougetoneyougetone branch Aug 12, 2021

DHowett reviewed Aug 12, 2021

View changes

leonMSFT mentioned this pull request Aug 12, 2021

Tray Icon PR followup #10938

Merged

rashil2000 mentioned this pull request Sep 15, 2021

Add a minimizeToNotificationArea action #11229

Open

Dec	JAN	Feb
	28
2021	2022	2023

microsoft / terminal Public

Add Minimize to Tray and Tray Icon #10368

Add Minimize to Tray and Tray Icon #10368

leonMSFT commented Jun 8, 2021 •

edited

This comment has been hidden.

This comment has been hidden.

This comment has been hidden.

This comment has been hidden.

This comment has been hidden.

This comment has been hidden.

This comment has been hidden.

This comment has been hidden.

This comment has been hidden.

This comment has been hidden.

This comment has been hidden.

zadjii-msft commented Jul 21, 2021

zadjii-msft left a comment

zadjii-msft Jul 21, 2021

leonMSFT commented Jul 21, 2021

zadjii-msft commented Jul 21, 2021

zadjii-msft left a comment

zadjii-msft Aug 3, 2021

zadjii-msft Aug 3, 2021

leonMSFT Aug 4, 2021 •

edited

zadjii-msft Aug 3, 2021

leonMSFT Aug 6, 2021

zadjii-msft Aug 3, 2021

leonMSFT Aug 5, 2021

zadjii-msft Aug 11, 2021

zadjii-msft Aug 3, 2021

leonMSFT Aug 9, 2021

zadjii-msft Aug 11, 2021

zadjii-msft left a comment

msftbot bot commented Aug 11, 2021

carlos-zamora left a comment

DHowett Aug 12, 2021

leonMSFT Aug 12, 2021

leonMSFT Aug 12, 2021

DHowett Aug 12, 2021

leonMSFT Aug 12, 2021

microsoft / terminal Public

Add Minimize to Tray and Tray Icon #10368

Add Minimize to Tray and Tray Icon #10368

Conversation

leonMSFT commented Jun 8, 2021 • edited

Settings Changes

References

This comment has been hidden.

This comment has been hidden.

This comment has been hidden.

This comment has been hidden.

This comment has been hidden.

This comment has been hidden.

This comment has been hidden.

This comment has been hidden.

This comment has been hidden.

This comment has been hidden.

This comment has been hidden.

zadjii-msft commented Jul 21, 2021

zadjii-msft left a comment

Choose a reason for hiding this comment

leonMSFT commented Jul 21, 2021

zadjii-msft commented Jul 21, 2021

zadjii-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leonMSFT Aug 4, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zadjii-msft left a comment

msftbot bot commented Aug 11, 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.

carlos-zamora left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leonMSFT commented Jun 8, 2021 •

edited

leonMSFT Aug 4, 2021 •

edited

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.