Skip to content
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

feat(windowManager): added app tray, users can minimize, maximize and quit polar from system tray #842

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AdamuAbba
Copy link

@AdamuAbba AdamuAbba commented Mar 5, 2024

Closes #693

Description

This PR contains changes to specifically the electron/windowManager.ts file which addresses issue #693 requesting a feature that enables the user to minimize the app to the system tray. my implementation so far leverages the Tray class from the electron package and enables the user to hide the BrowserWindow object while keeping the app alive in the background.

Quick notes: I found this comment helpful in getting the tray icon to display on macOS

Steps to Test

1 To test on Ubuntu, Windows, and macOS machines, run the app in dev mode

 yarn dev

2 Click on the Tray icon on the top right-hand corner of the operating system's panel
3 Click on any option from the context menu to either hide, show, or Quit the Polar app.

Screenshots

Ubuntu
Screenshot from 2024-03-05 20-01-40

macOS
Screenshot 2024-03-05 at 8 23 29 PM

@AdamuAbba AdamuAbba marked this pull request as ready for review March 5, 2024 20:01
@codecov-commenter
Copy link

codecov-commenter commented Mar 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (f6b4a57) to head (af67e69).
Report is 30 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##            master      #842   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          139       139           
  Lines         4401      4402    +1     
  Branches       858       858           
=========================================
+ Hits          4401      4402    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jamaljsr
Copy link
Owner

jamaljsr commented Mar 6, 2024

Hey @AdamuAbba, thanks so much for the PR. I tested it locally and it works great.

I have some feedback but it's pretty much all cosmetic:

  1. Tray icons are typically white or black depending on if the user is using dark mode or not in their OS, so I wouldn't want to use the colored Polar logo for this. I'm not sure how this is typically handled through Electron and what format the images need to be in, but I think this is something we should do.
  2. It would also be cool if the dropdown menu options had icons next to each option.
  3. Just to minimize accidental quitting, can you add a separator above the "Quit Polar" option?
  4. Very minor but I think the menu item labels should be fully capitalized.
    1. "Minimize to tray" becomes "Minimize to Tray"
    2. "Show window" becomes "Show Window"

Let me know if you need assistance with creating the logo image or icons and I'll try to help create them.

@AdamuAbba
Copy link
Author

Hey @jamaljsr thank you so much for the review. yes I would need help with creating the logo images or icons i'd really appreciate that thanks. let me know when it's done and I'll hop on integrating them ASAP

@jamaljsr
Copy link
Owner

jamaljsr commented Mar 6, 2024

Ok, I'll take a look into this over the weekend.

@AdamuAbba
Copy link
Author

Alright, thanks

@AdamuAbba
Copy link
Author

Hey @jamaljsr so I went and got some icons and was able to get the light and dark theme icon switch working on macOS but I've struggled with getting it to work on Windows and Ubuntu.

for some reason, the context menu doesn't refresh to reflect the updates once you've switched between dark theme and light theme on your computer. I'm a bit frustrated as to why this is not working tho I've been trying to get it to work since the day you dropped your most recent comment.

@jamaljsr
Copy link
Owner

Hi @AdamuAbba, that sounds like good progress. Can you push your code so I can give it a try. I'll be able to try it out tomorrow.

@AdamuAbba AdamuAbba force-pushed the feat/minimize-to-system-tray branch from 79a7c05 to 172a0ba Compare March 12, 2024 18:34
@AdamuAbba
Copy link
Author

Hey @jamaljsr sorry it's been a long couple of hours on my end I had to reach out for some help and was eventually able to get the icons to swap out on both light and dark theme through the help of @NonsoAmadi10 a fellow and more experienced polar contributor.

Dark Theme
Screenshot from 2024-03-12 19-59-52

Light theme
Screenshot from 2024-03-12 20-00-12

I also went ahead to squash some commits together not sure it was exactly how I planned it to be but gladly the updates are in the most recent commit and in the PR.

is everything working fine on your end?

@jamaljsr
Copy link
Owner

Thank you for figuring out how to get this working. It's looking very good now on Mac. I'll do some more testing on Linux and windows by tomorrow.

  1. What is special about the 16x16Template.png files that are used on Mac?
  2. I noticed a small bug on Mac. If I close the main Polar window, then click Minimize or Show Window in the tray menu, I get this error.
    image

@AdamuAbba
Copy link
Author

Hey @jamaljsr thanks for the review, really appreciate it. to answer your question:

  • the 16x16Template.png images ensure that the icons are compatible with the macOS's light and dark theme docs and the 16x16 image size is more or less convenient to display on the mac as extremely large icons don't render on the macOS tray

  • As for the bug, I believe this may have been the cause

 onMainClosed() {
    this.mainWindow = null;
  }

  onAllClosed() {
    if (process.platform !== 'darwin') {
      app.quit();
    }
  }

so I've gone ahead to refactor a bit

onMainClosed() {
    this.mainWindow = null;
    this.tray?.destroy();
    app.quit();
  }

  onAllClosed() {
    this.tray?.destroy();
    app.quit();
  }

This ensures that the tray is destroyed and the app is killed when the main window is closed which is the expected behavior on macOS. my recent commit implements this.

@jamaljsr
Copy link
Owner

Thanks, for pointing me to the docs on the image. I tested this on Mac and Linux. My Windows machine is giving me problems but I'll hopefully get that working soon.

This looks pretty much good to go. Can you rebase this branch on master for one final round of testing?

@jamaljsr
Copy link
Owner

I finally got my Windows machine back in order.

Unfortunately, it looks like the icons are the wrong size here.
image

@AdamuAbba
Copy link
Author

oh goodness, I'll get on it and push an update. thank you

@AdamuAbba AdamuAbba force-pushed the feat/minimize-to-system-tray branch from af67e69 to 2e325ba Compare April 3, 2024 09:10
@AdamuAbba
Copy link
Author

hey @jamaljsr sorry it took a while, been a crazy couple of weeks. Amazing polar maintainer session at BTrust the other day, I learned a lot from your time with us.

  • I've added 16x16 icons for both light and dark modes that conditionally swap out on Windows and linux while macOS maintains its initial 16x16Template template images that work just fine.
  • I also squashed all my commits into one so the history is clean, synced my fork of the repo, and rebased the branch on Master

Dark mode (Windows)
Screenshot (4)

Light mode (Windows)
Screenshot (5)

let me know if I missed anything and I'll get on it thank you.

@jamaljsr
Copy link
Owner

jamaljsr commented Apr 3, 2024

No worries on the timing. Thanks for fixing this remaining issue. I'll test again on Windows as soon as I have a chance.

@jamaljsr jamaljsr self-requested a review April 7, 2024 00:22
Copy link
Owner

@jamaljsr jamaljsr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for the delay in review. I've been traveling a bunch this month and didn't have my windows machine with me.

I noticed a couple of remaining issues.

  1. On a laptop I have running native Windows 10, I still see the large icons that I mentioned before.
  2. On a Windows 11 VM, I see the small menu icons but the Polar icon in the tray is showing as white in light mode.
    image

@AdamuAbba
Copy link
Author

Hey @jamaljsr been a while, glad you're back. so for the first issue with the icons not showing the proper size on a native Windows 10 machine I might need more details to reproduce it on my end because the icons do show up fine on my native Windows 10 machine. the screenshots I shared were actually from thesame machine 🤔

As for the second issue I'll get to work on fixing that as well.

Thanks for the review.

@jamaljsr
Copy link
Owner

Oh my bad, I tried again on my Windows 10 machine and I realized I didn't pull the latest branch the last time I tested. I can confirm that the icons are the correct size now.

@AdamuAbba
Copy link
Author

alright perfect @jamaljsr I'll focus on fixing the second issue and update the PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: Minimize to system tray
3 participants