Add Minimize to Tray and Tray Icon by leonMSFT · Pull Request #10368 · microsoft/terminal

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

src/cascadia/WindowsTerminal/icon.cpp

src/cascadia/WindowsTerminal/AppHost.cpp

carlos-zamora · 2021-06-09T21:29:51Z

src/cascadia/WindowsTerminal/AppHost.cpp

+        // TODO: Other useful options may include:
+        // - Summon All
+        // - Summon MRU or Monarch
+        //   - Though that's technically already available with a left click
+        //     but may be a reasonable request to also put it explicitly in the
+        //     context menu
+        // - Quit All


Don't forget to tag up this bare TODO

src/cascadia/WindowsTerminal/AppHost.cpp

src/cascadia/Remoting/WindowManager.cpp

src/cascadia/Remoting/Peasant.cpp

src/cascadia/Remoting/Monarch.cpp

WSLUser · 2021-06-15T13:06:22Z

Make an "AlwaysShowTrayIcon" option so people can control if they want it in their tray or not by default.

This should be a requirement. So many applications provide this option that it should be considered a defacto standard (like Xterm).

(I also personally like seeing all my tray icons. If I don't want to see one, I'll close the application instead, usually from left clicking on the icon, if it's supported, which it usually is.)

zadjii-msft · 2021-06-15T14:07:30Z

This should be a requirement. So many applications provide this option that it should considered a defacto standard (like Xterm).

Yep, there's already a whole spec with a section devoted to this idea.

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:13:59Z

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

    <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-07-22T13:44:27Z

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-03T15:44:13Z

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-03T16:27:55Z

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-03T16:42:32Z

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

ghost · 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-12T20:05:53Z

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-12T20:06:24Z

src/cascadia/WindowsTerminal/AppHost.cpp

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

leonMSFT added 23 commits May 20, 2021 00:02

initial

150efbb

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

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

using menucommand instead

ef3230d

bringing things in line

9006092

ok....

3aebbbd

i missed you, spellchk

e519aa4

carlos-zamora reviewed Jun 9, 2021

View reviewed changes

zadjii-msft requested changes Jul 21, 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 Jul 21, 2021

leonMSFT added 2 commits July 21, 2021 12:29

cleaning up so eyes don't bleed

d827e6b

a bit of cleanup

bc62fef

zadjii-msft requested changes Aug 3, 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 3, 2021

leonMSFT added 5 commits August 5, 2021 14:50

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 reviewed changes

zadjii-msft requested a review from carlos-zamora August 11, 2021 13:27

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

df559c1

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

carlos-zamora reviewed Aug 12, 2021

View reviewed changes

use constexpr for feature flag, copyright header, narrow_cast

cbf08aa

carlos-zamora approved these changes Aug 12, 2021

View reviewed changes

ghost merged commit a0edb12 into main Aug 12, 2021

ghost deleted the dev/lelian/notifyicon/yougetoneyougetone branch August 12, 2021 19:54

DHowett reviewed Aug 12, 2021

View reviewed 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

This pull request was closed.

Conversation

leonMSFT commented Jun 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Settings Changes

References

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

WSLUser commented Jun 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zadjii-msft commented Jun 15, 2021

Uh oh!

zadjii-msft commented Jul 21, 2021

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

leonMSFT commented Jul 21, 2021

Uh oh!

zadjii-msft commented Jul 21, 2021

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leonMSFT Aug 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zadjii-msft left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

carlos-zamora left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

leonMSFT commented Jun 8, 2021 •

edited

Loading

WSLUser commented Jun 15, 2021 •

edited

Loading

leonMSFT Aug 4, 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.