Add a tray icon to the application #1676

m-pilia · 2017-11-02T11:34:40Z

This commit adds a tray icon to the application, shown in the system
tray bar, that can be used to minimise the application window. This
is a common feature on most desktop messaging apps (e.g. Telegram
Desktop or Slack) and allows to save space in the system task bar.

The tray icon provides a context menu that contains a button to
show/hide the application window, and a button to quit the
application. When the tray icon is clicked, the visibility of the
window is toggled. When the close (x) button of the window is
pressed, the application is not terminated but minimised to the tray
icon instead (it can be terminated by using the "Quit" entry in the
File menu or in the context menu of the tray icon).

The tray icon is disabled by default, and two command line arguments
are available to enable it:

--use-tray-icon: enables the tray icon
--start-in-tray: enables the tray icon and the application starts minimised in the tray bar

Resolves: #1480

First time contributor checklist

I have read the README and Contributor Guidelines
I have signed the Contributor Licence Agreement

Contributor checklist

My contribution is fully baked and ready to be merged as is
My changes are rebased on the latest master branch
My commits are in nice logical chunks
I have followed the best practices in my commit messages
I have tested my contribution on these platforms:

Arch Linux, Linux 4.13.9, Chromium 62.0.3202.75

My changes pass all the local tests 100% (actually a test fails, but it was also failing before my commit)
I have considered whether my changes need additional tests, and in the case they do, I have written them

Description

breznak

Looks great! Was waiting for this feature, thank you 👏

breznak · 2017-11-02T13:02:24Z

main.js

+ {id: 'quit', label: locale.messages.quit.message, click: () => {app.quit();}}
+ ]);
+
+ // NOTE: a function equivalent to the following will become part of the


Maybe open a "remind this" Issue here to remove it when electron time comes?

Yes, an issue to track this, linked in the code, would be great. That issue can link to the places we can go to see if it's available yet.

adisib · 2017-11-02T17:51:59Z

So this still needs to be hooked up to settings?

m-pilia · 2017-11-02T18:51:38Z

Yes, I thought it was safer to have it tested by different devs on different platforms first, if it works and there are no other problems, adding a couple switches to the settings later should be straightforward (and maybe add other features, like a different icon when there are unread messages).

I think keeping a command line switch may be useful, it could be used to launch the app with a specific behaviour regardless of the settings (e.g. when launching the program from some autostart script). I would find it useful, but I guess this may be a matter of taste.

scottnonnenberg

Quite a lot of work you put into this, thanks! And I really appreciate the test strategy you've come up with here, enabling it for advance testing first.

I've left a number of comments and requests for changes. But a higher level thing to consider is how much of his can be extracted into its own file under /app. main.js is getting really big at this point. The way our top-level menus are generated might be a good example.

scottnonnenberg · 2017-11-09T01:38:15Z

main.js

@@ -167,7 +176,7 @@ function createWindow () {

 // Emitted when the window is about to be closed.
 mainWindow.on('close', function (e) {
- if (process.platform === 'darwin' && !windowState.shouldQuit() && config.environment !== 'test') {
+ if ((usingTrayIcon || process.platform === 'darwin') && !windowState.shouldQuit() && config.environment !== 'test') {


We should probably break this up into explanatory variables, now that it's gotten so long. Can you do that?

I broke it in two meaningful if statements, hope it looks better now.

scottnonnenberg · 2017-11-09T01:38:40Z

main.js

+function createTrayIcon() {
+ logger.info('Creating tray icon');
+
+ // TODO: better to not use a hardcoded icon file


What would you recommend here instead of a hardcoded path?

Sorry, I should have detailed this better. Having multiple icon styles (light, dark, colour) for the tray and allowing the user to choose in the options would be great for theme integration. But probably this is not the most urgent issue.

scottnonnenberg · 2017-11-09T01:40:59Z

main.js

+
+ trayContextMenu = Menu.buildFromTemplate([
+ {id: 'show', label: locale.messages.show.message, visible: !mainWindow.isVisible(), click: () => {showWindow();}},
+ {id: 'hide', label: locale.messages.hide.message, visible: mainWindow.isVisible(), click: () => {hideWindow();}},


I'm seeing a lot of () => { something(); } syntax in this PR, but that can be simplfied to something, no surrounding function required.

And in cases where it's something like app.quit() inside that function, we can change that to app.quit.bind(app). That's a common pattern in this codebase.

That was really silly of me, I moved the body of the lambdas to named functions, but I left the lambdas there...

scottnonnenberg · 2017-11-09T01:42:26Z

main.js

+ {id: 'quit', label: locale.messages.quit.message, click: () => {app.quit();}}
+ ]);
+
+ // NOTE: a function equivalent to the following will become part of the


Yes, an issue to track this, linked in the code, would be great. That issue can link to the places we can go to see if it's available yet.

scottnonnenberg · 2017-11-09T01:44:11Z

main.js

+ // https://github.com/electron/electron/releases/tag/v1.8.1
+ // https://github.com/electron/electron/pull/10618
+ trayContextMenu.getMenuItemById = function (id) {
+ return this.items.find(item => item.id === id) || null;


The || null here seems unecessary. Below, where this is used, there's no check before setting its visible property, so it will crash. Maybe the right thing to do here is to throw if we can't find any menu item with the provided id?

I think throwing an exception there would be the cleanest way. But the method implemented in electron does not do that, it just returns null, so if we want to be able to replace this function with it we may need to do null checks (that I added where needed, thank you for pointing it out).
Otherwise we can do differently and not rely on the (future) electron's API for it (after all, this is a very short piece of code and it is called just twice).
This is the method in electron 1.8.1: https://github.com/electron/electron/blob/ba754cf5c3f36d66a3b23f409613a348bfa6b308/lib/browser/api/menu.js#L84

scottnonnenberg · 2017-11-09T01:44:44Z

main.js

+
+ tray.setContextMenu(trayContextMenu);
+
+ tray.on('clicked', () => {toggleWindowVisibility();});


What's the difference between the 'clicked' and 'click' events?

Also, another situation where we can dispense with the surrounding function.

Whoops. 'click' is the event. 'clicked' was an experiment I made, and somehow I missed to remove that line later.

scottnonnenberg · 2017-11-09T01:45:10Z

main.js

+ tray.on('clicked', () => {toggleWindowVisibility();});
+ tray.on('click', () => {toggleWindowVisibility();});
+
+ tray.setToolTip('Signal Desktop');


This should be localized. Let's pull this string from locale.

scottnonnenberg · 2017-11-09T01:46:28Z

main.js

+ if (mainWindow) {
+ if (mainWindow.isVisible()) {
+ hideWindow();
+ }


A minor issue, but we generally use } else { brace style in this project.

scottnonnenberg · 2017-11-09T01:49:51Z

main.js

 function showWindow() {
 if (mainWindow) {
 mainWindow.show();
+
+ // trick to bring above other windows and get focus


Tell me more about this trick. What platforms has it been tested on (we've had particular difficulty with window placement/focus on windows) and where did it come from?

Also, note the 'show-window' IPC handler just a few lines above this. The right thing here might be to extract that out into a focusWindow method which shows the window if it isn't already showing. Then the tray handlers can use that instead of the simple showWindow method.

I moved this in a specific method for the tray.
Sorry, I should definitely have given more detail here. I was aware that on some versions of GNOME there were problems when using this API, and the window may not be restored on top if it was not focused when it was hidden (cfr. also this issue on an app that uses the same API: https://github.com/Enrico204/Whatsapp-Desktop/issues/29 ), so I included it to be on the safe side (tested on Linux, KDE Plasma 5.11 and GNOME 3.26), even if I suspect it may be unnecessary.

m-pilia · 2017-11-12T16:43:13Z

Thanks @scottnonnenberg for the review. I am sorry for so much sloppiness in my code, I had to implement this in very little time and probably I should not have submitted it so early.

I moved the tray related code in a new module in the app folder and followed your suggestions for the other issues. I am still not satisfied with the setTrayContextMenuItemsVisibility method, but I saw no other cleaner solution to update the visibility of the show/hide items in the context menu of the tray icon (apart from less elegant solutions, like always having both the elements visible, or having a single "show/hide" element).

scottnonnenberg · 2017-11-14T22:07:14Z

Note: I just moved this to target the development branch, because this will need to go out in a Beta build before hitting production. It looks like one merge conflict popped up.

scottnonnenberg

Thanks for the changes. We're almost there!

scottnonnenberg · 2017-11-17T00:41:47Z

app/tray_icon.js

+
+function createTrayIcon(mainWindow, messages) {
+
+ // TODO: for better desktop theme integration, it would be good to provide


If you want to put a TODO in the code, please create an issue and link to it directly in the code next to the TODO.

scottnonnenberg · 2017-11-17T00:47:07Z

main.js

@@ -273,6 +298,11 @@ app.on('ready', function() {

 createWindow();

+ if (usingTrayIcon) {
+ const createTrayIcon = require("./app/tray_icon");
+ tray = createTrayIcon(mainWindow, locale.messages);


I really like how everything has been pulled into a different file! However, there's one thing to consider: there can be multiple values of mainWindow over the lifetime of the application. I think, instead of passing mainWindow directly here, a function getMainWindow() should be passed instead.

scottnonnenberg · 2017-11-17T00:49:12Z

app/tray_icon.js

+
+ // TODO: for better desktop theme integration, it would be good to provide
+ // multiple tray icons (light, dark, colour) and allow selection from the options
+ tray = new Tray(path.join(__dirname, '../images/icon_48.png'));


How about path.join(__dirname, '..', 'images', 'icon_48.png') instead? Keep those pesky linux-y / characters out of a cross-platform codebase? :0)

m-pilia · 2017-11-19T12:15:08Z

I apologise for being so slow to reply, unfortunately I can work on this only during the weekend.
I followed your suggestions, removing *nix file path delimiters (sorry for my bias on this...) and fixing the problem of the changing mainWindow object.
I also tried to make the code for the tray context menu simpler and more clear. However I put that in a separate commit, so if it does not work (or you do not like it) it is easy to restore the previous approach.

scottnonnenberg · 2017-11-20T19:20:21Z

@m-pilia We're looking good on these changes (nice simplification!), but it looks like there's still a merge conflict to deal with. I think we're ready to merge as soon as that is addressed.

m-pilia · 2017-11-21T06:28:49Z

Right, I didn't look at it. Now it should be ok I think!

scottnonnenberg · 2017-11-21T16:20:41Z

@m-pilia That is one huge commit, that merge from the development branch. Are you familiar with the rebase command in Git? Please rebase on development branch and take this PR down to one or two clean commits.

m-pilia · 2017-11-21T16:58:00Z

@scottnonnenberg sorry, this was embarassing... I'm familiar with rebase, but apparently I'm definitely not familiar with GitHub. I tried to use the web interface to solve the conflict, but I didn't realise it was also going to merge the entire development branch in my feature branch...

scottnonnenberg · 2017-11-22T21:00:32Z

Okay, I just tested this - looking good! Fix that merge conflict and I'll merge this PR. If we can get this done in the next couple hours, it will go out in the next beta build. :0)

This commit adds a tray icon to the application, shown in the system tray bar, that can be used to minimise the application window. This is a common feature on most desktop messaging apps (e.g. Telegram Desktop or Slack) and allows to save space in the system task bar. The tray icon provides a context menu that contains a button to show/hide the application window, and a button to quit the application. When the tray icon is clicked, the visibility of the window is toggled. When the close (x) button of the window is pressed, the application is not terminated but minimised to the tray icon instead (it can be terminated by using the "Quit" entry in the File menu or in the context menu of the tray icon). The tray icon is disabled by default, and two command line arguments are available to enable it: --use-tray-icon: enables the tray icon --start-in-tray: enables the tray icon and the application starts minimised in the tray bar Resolves: signalapp#1480

m-pilia · 2017-11-23T08:26:33Z

Sorry, I could not do it in time, apparently we are in fairly different time zones. I do not know exactly where this conflict came from, so I reset, synced my branch with upstream and then re-applied my changes, now hopefully it should be clean. Thank you for your patience, I have done a mess with git!

peterthomassen · 2017-11-30T11:19:15Z

Thanks so much!! <3

@cornerman

Add proxy support based on environment variables (#1855) Fix issue where window would not show new message alerts on some Linux systesm - thanks @cornerman (#1820) Add a tray icon to the application behind command-line argument - thanks @m-pilia (#1676) Emoji: - Fix issue where clicks in emoji panel wouldn't do anything (#1849) - Add support for Emoji 5 - thanks @liliakai (#1797) - Eliminate unused emoji images from production package (#1849) Deployment: - aptly.sh: support for current and previous ubuntu versions (#1856) - deployment: update electron-publisher-s3 to match builder update (17f0bb4) Dev: - Update development branch to include everything up to v1.0.40 (f013eed and 3ac29a4) - Update readme.md to use signal.org URLs - thanks @scienmind (#1814)

morrow95 · 2017-12-08T05:13:58Z

Any timeframe on when the next build will be released? As in not one that needs compiled manually.

scottnonnenberg · 2017-12-08T18:36:12Z

The tray is in current beta builds. You can install the beta like this:

Mac: install file in 'path' at https://updates.signal.org/desktop/beta-mac.yml
Windows: install file in 'path' at https://updates.signal.org/desktop/beta.yml
Debian-like linux: same setup, then apt-get install signal-desktop-beta

GuardianMajor · 2017-12-08T21:24:01Z

Some feedback on this, since I just got a chance to test it.

1. The start in tray works great, the icon shows there just fine.

However, it doesn't actually load the app fully and sync messages until you click on it and it opens.

2. The minimize still goes to taskbar but closing it will keep the icon in the tray, great, I can live with that to some extent.

But if we can either remove that minimize menu option, or make it tied to a setting that -> if min to tray = true then minimize sends it back to tray and = false then minimize to taskbar like now, whichever. Current implementation seems a bit disconnect in behavior.

3. Minimizing to tray feature is great but current implementation loses great usefulness

In that when you get a new message, there is no notification or anything to let you know, this makes it quite counterproductive since you have no idea unless you keep randomly checking which is greatly reducing the productivity and usefulness of this. A desktop alert/toast would be useful or an option that once a message comes in, open it up from the tray (less desirable as it would be disruptive if you are working on something and suddenly the window pops up), whichever would be greatly needed here.

again, thank you for doing this, it is a great start and hope we can improve on it.

scottnonnenberg · 2017-12-08T22:30:16Z

@GuardianMajor I think it's time to take some of this behavior to other issues. This pull request is long-dead. In particular, your item 3 is definitely a bug. Notifications are supposed to happen when the window is minimized. We'll need more information: platform details, log, etc. as usual.

I'm locking this discussion.

m-pilia force-pushed the tray-icon branch 2 times, most recently from 9b07f0b to 518e270 Compare November 2, 2017 13:00

breznak approved these changes Nov 2, 2017

View reviewed changes

scottnonnenberg suggested changes Nov 9, 2017

View reviewed changes

scottnonnenberg changed the base branch from master to development November 14, 2017 22:06

scottnonnenberg suggested changes Nov 17, 2017

View reviewed changes

scottnonnenberg mentioned this pull request Nov 17, 2017

Added option to minimize to tray #1788

Closed

9 tasks

scottnonnenberg approved these changes Nov 20, 2017

View reviewed changes

m-pilia force-pushed the tray-icon branch 2 times, most recently from bf9708b to 9d7cab1 Compare November 21, 2017 16:55

m-pilia force-pushed the tray-icon branch from 9d7cab1 to 080a905 Compare November 22, 2017 11:47

m-pilia force-pushed the tray-icon branch from 080a905 to 141eaef Compare November 23, 2017 08:12

scottnonnenberg merged commit cd50fe3 into signalapp:development Nov 27, 2017

This was referenced Nov 28, 2017

Minimise to tray #1825

Closed

In the standalone Windows version, there is no 'hide to tray' or something #1837

Closed

scottnonnenberg mentioned this pull request Dec 2, 2017

feature request :desktop app (Win) minimize to taskbar #1847

Closed

mpsdskd mentioned this pull request Dec 7, 2017

[beta] Can't minimize to tray on Ubuntu 17.10 #1875

Open

1 task

signalapp locked and limited conversation to collaborators Dec 8, 2017


		tray.setContextMenu(trayContextMenu);

		tray.on('clicked', () => {toggleWindowVisibility();});


		function createTrayIcon(mainWindow, messages) {

		// TODO: for better desktop theme integration, it would be good to provide

Add a tray icon to the application #1676

Add a tray icon to the application #1676

Conversation

m-pilia commented Nov 2, 2017

First time contributor checklist

Contributor checklist

Description

breznak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adisib commented Nov 2, 2017

m-pilia commented Nov 2, 2017

scottnonnenberg 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

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

m-pilia Nov 12, 2017 • edited Loading

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

m-pilia Nov 12, 2017 • edited Loading

Choose a reason for hiding this comment

m-pilia commented Nov 12, 2017

scottnonnenberg commented Nov 14, 2017

scottnonnenberg 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

m-pilia commented Nov 19, 2017 • edited Loading

scottnonnenberg commented Nov 20, 2017

m-pilia commented Nov 21, 2017

scottnonnenberg commented Nov 21, 2017

m-pilia commented Nov 21, 2017

scottnonnenberg commented Nov 22, 2017

m-pilia commented Nov 23, 2017 • edited Loading

peterthomassen commented Nov 30, 2017

morrow95 commented Dec 8, 2017

scottnonnenberg commented Dec 8, 2017

GuardianMajor commented Dec 8, 2017

1. The start in tray works great, the icon shows there just fine.

2. The minimize still goes to taskbar but closing it will keep the icon in the tray, great, I can live with that to some extent.

3. Minimizing to tray feature is great but current implementation loses great usefulness

scottnonnenberg commented Dec 8, 2017 • edited Loading

m-pilia Nov 12, 2017 •

edited

Loading

m-pilia Nov 12, 2017 •

edited

Loading

m-pilia commented Nov 19, 2017 •

edited

Loading

m-pilia commented Nov 23, 2017 •

edited

Loading

scottnonnenberg commented Dec 8, 2017 •

edited

Loading