-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Added hide to tray option #6915
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
base: development
Are you sure you want to change the base?
Conversation
Head branch was pushed to by a user without write access
Please fix the build and revert all unrelated changes such as the dependency additions, there is no need to manually extract anything from the asar file and the translations are overkill at the moment (especially pulling in multiple dependencies just to translate 2 strings). Even after the corrections there are still open questions such as how to handle multiple windows. |
Head branch was pushed to by a user without write access
Head branch was pushed to by a user without write access
Thanks for the review, I reverted the new dependencies (I didn't know you could read asar directly from node fs! It's much better now). About the i18n, if needed maybe we could send the translated text via IPC and recreate the tray. As for how to handle multiple windows, I didn't know you could have more than one, is there some kind of setting for that? |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Head branch was pushed to by a user without write access
Conflicts have been resolved. A maintainer will review the pull request shortly. |
Looks like you may have mistakenly used npm instead of yarn to install the dependencies and pushed a package-lock.json file, additionally there seem to be changes to the yarn.lock file which shouldn't really be there as the dependencies are no longer changing in this PR. |
Head branch was pushed to by a user without write access
Head branch was pushed to by a user without write access
Co-authored-by: efb4f5ff-1298-471a-8973-3d47447115dc <73130443+efb4f5ff-1298-471a-8973-3d47447115dc@users.noreply.github.com>
Head branch was pushed to by a user without write access
Head branch was pushed to by a user without write access
I'm not able to reproduce that behaviour in my Ubuntu, when I click the window it stays there. It's weird too because, as far as I know, there's no handler for that type of events, so it should behave normally. Does it happen in previous builds too?
There was a check in createWindow from the very first commit, that would destroy the tray if it already existed. Now that logic is handled in manageTray, so I had to remove it from createWindow because it was destroying the tray when it shouldn't.
You mean like listing the things we've been testing so far? |
Taken from our recently made changes to the PR template: Please provide instructions so that others can ensure that your pull request would produce correct results. For examples see, #5743, #7349, #5125, #7338 |
Its weird i can reproduce this on the very first commit you made. I'll spin up my other vm's to check if this is consistent |
Created a fresh ubuntu vm and its still happening |
@efb4f5ff-1298-471a-8973-3d47447115dc I'm afraid you'll have to add some traces again. Please add this in the minimize handler (under line 976 in src/main/index.js):
That should be the only place where the tray logic is called, so let's start by checking if it's invoking the handler. The message should pop once when you click the minimize button (that's expected), and then let's see what happens. I've updated the testing section of the PR too. |
@Devenor something to note is that when i minimize you can see on the left hand side that it thinks a new window has been opened. So when i minimized the first time it showed 2 dots indicating that there were 2 windows. This will be more apparent later in the video VirtualBoxVM_5pISWqdcc1.mp4 |
The two dots seem ok, one is the main window and the other one is the dialog, but the four dots and multiple windows in the tray in the end is not... let's focus on why the handler is being invoked, please try again after moving the dialog above line 976 and disabling the tray setting. |
I mean like this, and disabling the Minimize to system tray option to see if it continues invoking the handler:
|
@Devenor i dont see any difference to what you have provided vs the actual code? |
It's in the showMessageBoxSync placement, in the code you tested it should be after the if (trayOnMinimize) {, now i'd like to try moving it outside the if statement, to check if the message appears even when the setting is disabled. |
VirtualBoxVM_FMpJTxsZc3.mp4 |
After doing that, if you activate the setting again and click the title bar, does it breaks or it keeps working? |
Behavior still happening VirtualBoxVM_HohBNUc96s.mp4 |
That's an interesting test though, I thought the problem might be when starting the app, but now we know that it seems to happen precisely when hiding or showing the window. There's a couple of things we can try, let's start by removing some stuff and see if there's any difference, please do:
Gotta sleep, I'll check it out tomorrow. |
Do you also want me to revert the changes i made previously? |
Ok, then the lines to comment will be 982 to 988, line 710 remains the same. |
will do have a good night! |
Same results VirtualBoxVM_Xxuf7AG0Dg.mp4 |
Let's test two more things:
The second test won't minimize the window to the tray, but will let us now if there's a problem with the hide method. |
VirtualBoxVM_JKGvFM4dbu.mp4
Seems promising VirtualBoxVM_ZEwK4XOdik.mp4 |
Mmm... but this means it's happening when calling Electron's hide method, that's unsettling. I'd like to know if it started happening after some of the recent changes or if it's always been there and we didn't notice, could you test it in one of the first commits ? Maybe eb34595? |
@Devenor didnt i already answer your question here?
|
Yeah sorry, that's what I needed to know. If there's an issue with the hide method we have a problem, because that's essential for the tray to work. I've managed to reproduce the issue in a VM, but it's too clunky to do anything, I'll try to set it up in a better way and see if I can find a workaround. |
Added hide to tray option
Pull Request Type
Related issue
closes #59
Description
Adds optional hide to system tray behaviour on closing and/or minimizing the main window. The settings have an option to enable or disable it in the general section. The tray icon has a small menu for closing the app or opening the window again, and it can be opened by clicking on the icon too.
Screenshots
https://imgur.com/a/1TVWBJp

https://imgur.com/a/xvBEqFe

Testing
Desktop
Additional context