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: monochrome tray icon on Windows & Linux #2159

Merged
merged 2 commits into from
Aug 31, 2022
Merged

feat: monochrome tray icon on Windows & Linux #2159

merged 2 commits into from
Aug 31, 2022

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented May 26, 2022

This addresses #956 by adding monochrome tray icons for the remaining platforms (Windows and Linux). This way, we are consistent cross-platform and consistent with the platforms themselves.

Background, or, why did we not do this before?:

  1. On Linux, I don't think we've ever had the conversation of supporting both light and dark themes. Therefore, it was just something we missed.
  2. On Windows, there is a little caveat: Windows has both application and system themes and they are separate. Electron is only equipped to detect application mode. However, recent versions of Windows (11+) do not separate both themes anymore! So I think it is safe to assume that Electron gives us the correct theme to use.
  3. On macOS we don't really need different icons. We just give a "template" icon and macOS uses it as alpha channel, adapting to whatever theme the user has on their system ✨

Screenshots on Windows:

light-mode

dark-mode

@lidel @SgtPooki I would not consider this a priority. However, I would like to ask you to test this on any Linux platform you have in hand to check if it behaves correctly. If not, we can keep the colored icons for the time being.

@hacdias hacdias requested review from SgtPooki and lidel May 26, 2022 10:30
@hacdias hacdias marked this pull request as ready for review May 26, 2022 10:30
lidel
lidel previously requested changes Jun 6, 2022
Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

On Linux, I don't think we've ever had the conversation of supporting both light and dark themes. Therefore, it was just something we missed.

We did not touch this on Linux because (afaik) there is no standard API for this that works across Linux ecosystem. Linux people use different DE and using the colored icon is safer.

As a data point, the icon is invisible on old school black i3bar:

2022-06-06_19-12

@hacdias mind restoring old behavior (colored icon) on Linux?

@SgtPooki
Copy link
Member

SgtPooki commented Jun 8, 2022

@hacdias mind restoring old behavior (colored icon) on Linux?

Instead, can we include a setting for this and default linux to the colored icon?

Copy link
Member

@SgtPooki SgtPooki left a comment

Choose a reason for hiding this comment

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

Code LGTM but we should resolve the linux tray icon discussion

@hacdias
Copy link
Member Author

hacdias commented Jun 9, 2022

@hacdias mind restoring old behavior (colored icon) on Linux?

Instead, can we include a setting for this and default linux to the colored icon?

I think this is also a great idea: just give users a simple option to either use colored or B&W.

@SgtPooki
Copy link
Member

@lidel do you agree with us? #2159 (comment)

Do you see any problems with this?

@lidel
Copy link
Member

lidel commented Jul 24, 2022

I don't feel super strong about this, my default mindset is to avoid adding niche features that increase maintenance cost, but this is small enough that it should be fine.

SgtPooki
SgtPooki previously approved these changes Aug 24, 2022
@SgtPooki SgtPooki dismissed their stale review August 24, 2022 22:33

need to wait for change: Instead, can we include a setting for this and default linux to the colored icon?

@SgtPooki
Copy link
Member

@hacdias can you confirm if you were planning on implementing the

setting for this[monochrome icon] and default linux to the colored icon

@hacdias hacdias marked this pull request as draft August 30, 2022 18:01
@hacdias hacdias force-pushed the feat/tray-icon branch 2 times, most recently from 786e92a to e605016 Compare August 30, 2022 18:15
@hacdias
Copy link
Member Author

hacdias commented Aug 30, 2022

@SgtPooki I updated the PR such that a new option shows under Settings on Windows and Linux. By default, we keep the current behavior (colored icons), but users are free to choose if they want monochromatic icons or not.

Having a different default for Windows and Linux would involve further changes, to which I don't have bandwidth right now.

Tested on macOS (to check if the behavior is kept) and Windows (it works fine).

You can see the last commit for this changes only.

image

@hacdias hacdias marked this pull request as ready for review August 30, 2022 18:40
@hacdias hacdias requested a review from SgtPooki August 30, 2022 19:00
@hacdias hacdias enabled auto-merge (squash) August 31, 2022 13:55
@hacdias hacdias requested a review from lidel August 31, 2022 13:55
@SgtPooki SgtPooki dismissed lidel’s stale review August 31, 2022 16:56

the requested changes were made

@hacdias hacdias merged commit a4d3e90 into main Aug 31, 2022
@hacdias hacdias deleted the feat/tray-icon branch August 31, 2022 16:56
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.

3 participants