Fixes #110 #134

Lplenka · 2017-04-02T02:16:45Z

There was no support for showing unread count in the Tray icon, I have added the functionality.

There is no support for overlay icon in Linux as of now please refer issue #9020.

This fix works fine.
Any suggestions are welcome 😄

…g of Tray icon zulip#110 [WIP]

sinwar · 2017-04-02T02:32:29Z

@Lplenka It'll be great if you use different branch than master to create pull request.

akashnimare · 2017-04-02T02:53:31Z

It'll be great if you use different branch than master to create pull request.
🙏

akashnimare · 2017-04-02T02:54:40Z

app/renderer/js/tray.js

+				window.tray = null;
+			}
+		} else {
+			window.createTray();


should be createTray()

Yes, will change it.

akashnimare · 2017-04-02T02:56:46Z

on macOs -

Lplenka · 2017-04-02T03:00:01Z

Size issue on different OS. Have to make the config OS specific.

Lplenka · 2017-04-02T08:20:12Z

Fixed the errors in Mac & Windows.
Everything looks & works perfect now. 😄

Mac OS:

Windows:

akashnimare · 2017-04-03T19:16:24Z

The one big problem with this PR is that we don't want to completely replace the tray icon with that canvas image. The ideal solution is to set that canvas on top of Zulip tray icon. Something like this -

So that it look like this -

You gotta use setOverlayIcon method for this. As of now this method only supports Windows as mentioned here.
More info -
electron/electron#4011
electron/electron#3148

Lplenka · 2017-04-03T19:20:40Z

Okay, Since there is no support to overlay tray in Linux & MacOS I thought this will work for the time being. Because we cannot implement something which electron doesn't have itself.

Lplenka · 2017-04-03T19:23:15Z

The original zulip Icon will always be there if no unread messages are present.
If new messages come canvas will appear with the specific unread count.
This is the best fix I could find until the electron actually supports overlay.
I hope I am making sense.

akashnimare · 2017-04-03T19:24:49Z

Wait on. Let me try to setup setOverlayIcon on windows and see if we still can get something like this (atleast on windows) -

akashnimare · 2017-04-03T19:26:57Z

The original zulip Icon will always be there if no unread messages are present.
If new messages come canvas will appear with the specific unread count.
This is the best fix I could find until the electron actually supports overlay.
I hope I am making sense.

Definitely, it makes sense. It's probably a good fix until electron supports this api on macOS + Linux 😃

Lplenka · 2017-04-03T19:28:18Z

Yes, we can handle this differently in different OS. If Windows has overlay we can use that, but then we will have a different implementation in different OS. Would that be fine?
I have seen other electron apps too, none of them has this feature of Overlay, so I could only come up with this idea or workaround till Electron supports Overlay in Tray. We can always implement that then. 😄

akashnimare · 2017-04-03T19:29:20Z

If Windows has overlay we can use that, but then we will have a different implementation in different OS. Would that be fine?

Yes.

timabbott · 2017-04-03T19:37:19Z

For the webapp, we just have a collection of 100 generated images that we use for favicons (if the number exceeds 100, we just show an infinity symbol). So one doesn't need overlay support -- just the ability to change the image -- to produce arbitrary numbers.

Lplenka · 2017-04-03T19:47:50Z

@timabbott yes I feel like that's the only one more possible way to deal with this Issue.

This approach is even easier to implement than the way I used i.e. generate new canvas every time a new message comes. But here is twist we have to make 100 icons for Windows, 100 for Linux and 100 for MacOS because tray Icon size differs in three of them

I couldn't make those icons for all OS. Found html canvas is the only way till Electron supports overlay for all OS.

akashnimare · 2017-04-03T20:47:08Z

I think we could get these favicons using -

	page.on('page-favicon-updated', (event, favicons) => {
		console.log(favicons.slice(-1)[0]);
	});

output -

➜  zulip-electron git:(master) ✗ npm start

> zulip@0.5.8 start /Users/akka/dev/zulipwork/zulip-electron
> electron ./app/main

Wait for app ready
Server StatusCode: 200
You are connected to: chat.zulip.org
https://chat.zulip.org/static/images/favicon/favicon-38.png
https://chat.zulip.org/static/images/favicon/favicon-93.png
https://chat.zulip.org/static/images/favicon/favicon-93.png
https://chat.zulip.org/static/images/favicon/favicon-93.png

I was just wondering if somehow we could set these favicon to our tray icon 🤔

Lplenka · 2017-04-03T21:06:20Z

I was just wondering if somehow we could set these favicon to our tray icon 🤔.

We can receive the image using http request to specific URL like

request
.get('https://chat.zulip.org/static/images/favicon/favicon-93.png')
.on('error', function(err) {
console.log(err)
})
.pipe(fs.createWriteStream('favicon-93.png'))'

But I think size issue will still remain for different OS 😕

akashnimare · 2017-04-05T14:35:05Z

Will try to run this PR on windows tonight plus I'll see if we could use setOverlayIcon.

Lplenka · 2017-04-05T14:57:46Z

Will try to run this PR on windows tonight plus I'll see if we could use setOverlayIcon.

Okay 👍
I have checked in windows 10 also. Hopefully, you won't find any issues with the PR. Sorry, I by mistake clicked the close PR button 😅

akashnimare · 2017-04-08T20:58:25Z

Will review it again on Monday.

akashnimare · 2017-04-16T21:57:32Z

Does it work on Windows 10? Can you paste a screenshot?

akashnimare · 2017-04-16T22:27:22Z

Please move this file -

app/renderer/js/tray.js to app/main/tray.js (tray.js should be in the browser process)

Lplenka · 2017-04-17T02:56:45Z

Does it work on Windows 10? Can you paste a screenshot?

Yes works perfectly fine, here are some screenshots.

Lplenka · 2017-04-17T03:39:36Z

@akashnimare Is this necessary?

Please move this file -
app/renderer/js/tray.js to app/main/tray.js (tray.js should be in the browser process)

Initially, I tried running this file in app/main/tray.js but things didn't work if tray.js was imported into app/main/index.js, the tray icon image was never rendered. So I shifted to Renderer process and everything worked as expected.

I have added every functionality of Tray icon using IpcMain and IpcRenderer for proper communication between Main and Renderer processes. The icon works perfectly well while switching servers, reloading page, logging in/out etc.

Also if Tray icon is in renderer process it will be helpful to add required functionalities for our "Multiple Server Support" feature.

akashnimare · 2017-04-17T18:28:04Z

Initially, I tried running this file in app/main/tray.js but things didn't work if tray.js was imported into app/main/index.js, the tray icon image was never rendered. So I shifted to Renderer process and everything worked as expected.

You can't import tray.js in Main process since it belongs to Renderer.

Also if Tray icon is in renderer process it will be helpful to add required functionalities for our "Multiple Server Support" feature.

Make sense.

akashnimare · 2017-04-17T18:36:26Z

@Lplenka why does it create multiple tray icon in some cases (ex. navigating to a new URL)? Although it gets disappeared after few seconds. Can we prevent this event?

Lplenka · 2017-04-17T18:37:15Z

You can't import tray.js in Main process since it belongs to Renderer.

I meant currently our app does that, we import app/main/tray.js to app/main/index.js and call function tray.create();.

While In this PR since I shifted tray.js file to renderer, I am just calling this file in app/main/preload.js so the file runs every time app's window is created.

Lplenka · 2017-04-17T18:40:03Z

@Lplenka why does it create multiple tray icon in some cases (ex. navigating to a new URL)? Although it gets disappeared after few seconds. Can we prevent this event?

Yes I specifically dealt with this problem in index.js, Added this,

     //  To destroy tray icon when navigate to a new URL

win.webContents.on('will-navigate', e => {
	if (e) {
		win.webContents.send('destroytray');
	}
});

This probably happens because preload.js is called while navigating to a new URL, so I added this code to destroy tray icon during navigation and new tray icon appears as the page loads.

@akashnimare I am sure this part of code dealt with the problem you mentioned because I don't find this problem anymore. I can share the gif if you want.

akashnimare · 2017-04-17T19:06:54Z

Cross tested this PR on all the platforms. Works great. This is probably the best solution to show unreadcounts in the tray icon. We could improve it later once electron officially support it with setOverlayicon etc. @Lplenka keep in mind that eventually we should add something like this
https://github.com/zulip/zulip-electron/pull/134#issuecomment-291244986

Note - Sorry for the delay. Thanks for all the work 🚀 ❤️

Lplenka · 2017-04-17T19:12:17Z

Cross tested this PR on all the platforms. Works great. This is probably the best solution to show unreadcounts in the tray icon. We could improve it later once electron officially support it with setOverlayicon etc. @Lplenka keep in mind that eventually we should add something like this
#134 (comment)
Note - Sorry for the delay. Thanks for all the work 🚀 ❤️

Thanks! 😄 and yes I will definitely work on that as soon as required feature is implemented in Electron.

Lplenka added 3 commits April 2, 2017 00:55

Added Tray.js to renderer js files

a7a80ce

Added communication b/w Main & Renderer process for proper functionin…

9ec62a7

…g of Tray icon zulip#110 [WIP]

Js Lint error fixed 🔧, Fixes zulip#110

460a647

zulipbot added the needs review label Apr 2, 2017

zulipbot assigned akashnimare Apr 2, 2017

zulipbot added reviewed and removed needs review labels Apr 2, 2017

akashnimare requested changes Apr 2, 2017

View reviewed changes

zulipbot added needs review and removed reviewed labels Apr 2, 2017

Lplenka added 5 commits April 2, 2017 09:33

Added function to set tray size according to OS, fixed toggle tray

0ec38ba

Fixed bugs in Windows zulip#110 [WIP]

d5e6184

Lint error fixed zulip#110 [WIP]

40f81af

Fixed bug in Mac OS zulip#110 [WIP]

6d45105

Fixed size bug in Mac OS zulip#110 [WIP]

ca078cb

akashnimare added Status: In Progress Type: Discussion labels Apr 5, 2017

Lplenka closed this Apr 5, 2017

Lplenka reopened this Apr 5, 2017

Lplenka added 3 commits April 9, 2017 10:02

Merge branch 'master' into master

d4448ba

Merge branch 'master' into master

184e1a5

🔧 Lint Error fixed

7efe90e

akashnimare approved these changes Apr 17, 2017

View reviewed changes

zulipbot added reviewed and removed needs review labels Apr 17, 2017

akashnimare merged commit f290732 into zulip:master Apr 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #110 #134

Fixes #110 #134

Lplenka commented Apr 2, 2017

sinwar commented Apr 2, 2017

akashnimare commented Apr 2, 2017

akashnimare Apr 2, 2017

Lplenka Apr 2, 2017

akashnimare commented Apr 2, 2017

Lplenka commented Apr 2, 2017 •

edited

Loading

Lplenka commented Apr 2, 2017 •

edited

Loading

akashnimare commented Apr 3, 2017

Lplenka commented Apr 3, 2017 •

edited

Loading

Lplenka commented Apr 3, 2017

akashnimare commented Apr 3, 2017

akashnimare commented Apr 3, 2017 •

edited

Loading

Lplenka commented Apr 3, 2017 •

edited

Loading

akashnimare commented Apr 3, 2017 •

edited

Loading

timabbott commented Apr 3, 2017

Lplenka commented Apr 3, 2017 •

edited

Loading

akashnimare commented Apr 3, 2017

Lplenka commented Apr 3, 2017 •

edited

Loading

akashnimare commented Apr 5, 2017

Lplenka commented Apr 5, 2017 •

edited

Loading

akashnimare commented Apr 8, 2017

akashnimare commented Apr 16, 2017 •

edited

Loading

akashnimare commented Apr 16, 2017 •

edited

Loading

Lplenka commented Apr 17, 2017

Lplenka commented Apr 17, 2017

akashnimare commented Apr 17, 2017

akashnimare commented Apr 17, 2017

Lplenka commented Apr 17, 2017

Lplenka commented Apr 17, 2017 •

edited

Loading

akashnimare commented Apr 17, 2017

Lplenka commented Apr 17, 2017

Fixes #110 #134

Fixes #110 #134

Conversation

Lplenka commented Apr 2, 2017

sinwar commented Apr 2, 2017

akashnimare commented Apr 2, 2017

akashnimare Apr 2, 2017

Choose a reason for hiding this comment

Lplenka Apr 2, 2017

Choose a reason for hiding this comment

akashnimare commented Apr 2, 2017

Lplenka commented Apr 2, 2017 • edited Loading

Lplenka commented Apr 2, 2017 • edited Loading

akashnimare commented Apr 3, 2017

Lplenka commented Apr 3, 2017 • edited Loading

Lplenka commented Apr 3, 2017

akashnimare commented Apr 3, 2017

akashnimare commented Apr 3, 2017 • edited Loading

Lplenka commented Apr 3, 2017 • edited Loading

akashnimare commented Apr 3, 2017 • edited Loading

timabbott commented Apr 3, 2017

Lplenka commented Apr 3, 2017 • edited Loading

akashnimare commented Apr 3, 2017

Lplenka commented Apr 3, 2017 • edited Loading

akashnimare commented Apr 5, 2017

Lplenka commented Apr 5, 2017 • edited Loading

akashnimare commented Apr 8, 2017

akashnimare commented Apr 16, 2017 • edited Loading

akashnimare commented Apr 16, 2017 • edited Loading

Lplenka commented Apr 17, 2017

Lplenka commented Apr 17, 2017

akashnimare commented Apr 17, 2017

akashnimare commented Apr 17, 2017

Lplenka commented Apr 17, 2017

Lplenka commented Apr 17, 2017 • edited Loading

akashnimare commented Apr 17, 2017

Lplenka commented Apr 17, 2017

Lplenka commented Apr 2, 2017 •

edited

Loading

Lplenka commented Apr 2, 2017 •

edited

Loading

Lplenka commented Apr 3, 2017 •

edited

Loading

akashnimare commented Apr 3, 2017 •

edited

Loading

Lplenka commented Apr 3, 2017 •

edited

Loading

akashnimare commented Apr 3, 2017 •

edited

Loading

Lplenka commented Apr 3, 2017 •

edited

Loading

Lplenka commented Apr 3, 2017 •

edited

Loading

Lplenka commented Apr 5, 2017 •

edited

Loading

akashnimare commented Apr 16, 2017 •

edited

Loading

akashnimare commented Apr 16, 2017 •

edited

Loading

Lplenka commented Apr 17, 2017 •

edited

Loading