-
Notifications
You must be signed in to change notification settings - Fork 407
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add system tray support #886
base: master
Are you sure you want to change the base?
Conversation
Hi! I've done this kind of thing before. I unfortunately this feature, and not finished. Your version looks cool! But will it run on OS Windows? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay in reviewing this. I have dropped some comments/questions. Please let me know what you think.
"pack": "gulp build && build --dir", | ||
"dist": "gulp build && build -mwl --x64" | ||
"pack": "electron-builder --dir", | ||
"dist": "electron-builder" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you explain the reason for this change? Why are we now running the command this way?
"snap", | ||
"AppImage", | ||
"zip", | ||
"deb" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the reason for removing the build config?
@@ -118,11 +162,12 @@ app.setAppUserModelId('com.ifedapoolarewaju.desktop.igdm') | |||
|
|||
app.on('ready', () => { | |||
createWindow(); | |||
createTray(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this functionality should probably only apply to a windows machine
|
||
mainWindow.on('minimize', function (event) { | ||
event.preventDefault(); | ||
mainWindow.hide(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this functionality should probably only apply to a windows machine. Also, what is the difference between "minimize" and "hide"? 🤔
@@ -45,7 +48,19 @@ function createWindow () { | |||
})) | |||
}) | |||
|
|||
mainWindow.on('closed', () => mainWindow = null) | |||
mainWindow.on('close', function (event) { | |||
if (!app.isQuiting) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can just use this clause instead, or not?
@ifedapoolarewaju Hello, please mind that this is just a sketchy pull request and it could had been an enhancement request with some code. I've only tested it on debian stable (i3-gaps latest, polybar latest) and it works fine. I did some modifications to package.json for my own testing purposes (more specifically I had issues with gulp and hence I tried to avoid it) and I also removed some targets from the building process in order to speed up my tests. Of course master's configs won't need any change on an upcoming merge. I also added this dependency "electron-json-config" (yes I know that adding a dependency just for 2 lines of code hurts but I may be useful later, although it depends on the design and the features it wants to provide) to save the color of the icon the user had before exiting, cause I didn't want to mess with cookies. I'm pretty sure the pro version uses something similar for the dark theme. If you are into the idea, please let me know, so that I can remove any conflicts from the configs. But you have to take some decisions : 1) Which icons will be used for the notification tray (I used the ones that where shipped with the app but iirc I did some simple conversions and transformations on them)? 2) How user's preferences will be retrieved? |
@rand0x0m so this tray support applies to windows and linux systems? |
@ifedapoolarewaju Yes it does. Sorry for the late reply but I've been really busy lately. |
3f2ddfb
to
6b32139
Compare
Isn't the tray thing slowly passing out? 🤔 |
c614180
to
f09f454
Compare
This is more like a blueprint. It is not ready to merge. Decisions about the system tray image should be taken. And package.json should be changed accordingly. I built it for personal use and it works fine with some custom icons.
It would be nice to save user preference on 'light' or 'dark' icon on exit. A design decision should be taken here.
Lastly, shutdown hook needs to be checked again.