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

WIP: dark/ligh system tray icons for Linux #2395

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

paulatz
Copy link

@paulatz paulatz commented Apr 19, 2022

This PR implements automatic dark/light system tray for Linux using nativeTheme.shouldUseDarkColors. It also requires '@electron/remote', which reduces process isolation, but should not be a problem as it is used elsewhere in mailspring.

It also includes a set of full/empty icons which blend well in my system tray (kde 5.x), lines are sharp and pixel-aligned at 22x22 resolution and multiples. SVG files are provided for future reference, but because of nativeImage limitations, what is used are rendered PNG files over-sampled at 88x88 to scale gracefully.

Known limitation:

  1. shouldUseDarkColors is not actually supported by all Linux desktop flavors. However, at least KDE seems to be able to automatically invert the color of the icon if it is dark and mostly monochomatic.
  2. I did not implement a callback for the "updated" method, which means that when switching system-wide from light/dark theme, the icon will not change until it is triggered elsewhere. Which happens fairly often.
  3. I understand that my icon design my not be appreciated by everyone, please return feedback.

…dapt

the color of the tray icon accordingly.

ISSUE: I have tried a few ways to get the desktop colo theme, but none
seems to work. Probably because I do not know electron:

    //const { systemPreferences } = require('electron')
    //const darkMode = systemPreferences.isDarkMode()
    //let darkMode = Electron.nativeTheme.shouldUseDarkColors
    let darkMode = false;
    //Electron.systemPreferences.isDarkMode();

IMPORTANT: this commit breaks the tray icon for windows and macosx. It
is only ment as a test!
…re not actually used in the code, png images are generated with inkscape
…pdate when theme is changed (icon theme updated next time it is changed)

I tried to keep the structure very close to the original file, by putting Linux stuff in a separate function, this causes some duplication
…e not in the main process. This seems to work at the moment, may break in future versions of electron if more process sandboxing is enforced (but my impression is the Mailspring is quite lax with it)
@foundry376-bot
Copy link

This pull request has been mentioned on Mailspring Community. There might be relevant details there:

https://community.getmailspring.com/t/implement-a-dark-tray-icon-for-light-themes/700/21

@Phylu
Copy link
Contributor

Phylu commented Apr 20, 2022

@paulatz Thanks a lot for the pull request! Looks really good for a first draft.

  • Looking at the code: I think it can be cleaned up and simplified a bit and I will try to provide some pointers in the code directly.
  • Regarding the icon: I tested this on my Ubuntu 21.10 and the new icon seems to have a different size and is less legible in my try compared to the old one. I would suggest to stay with the original icon and just invert the color for the light tray, similar how it is (automatically) adjusted on MacOS.

Comparison new (left) / old (right) icon on GNOME:
mailspring_icon_gnome

Comparison dark / light tray on MacOS:
macos_dark_tray macos_light_tray

@@ -76,6 +74,14 @@ class SystemTrayIconStore {
};

_updateIcon = () => {
if (platform == 'linux'){
Copy link
Contributor

@Phylu Phylu Apr 20, 2022

Choose a reason for hiding this comment

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

Instead of having a complete separate logic for linux within the same method, I suggest to create a new helper function:

_selectThemedIcon(ligthIcon: boolean, isInboxZero: boolean, showUnread: boolean) {
 // Select correct image based on the original and your updated logic
return icon
}

Then you can use the _updateIcon method to configure the prerequisites to chose the icon and call your additional helper function to select it based on the variables:

if (platform == 'linux' && nativeTheme.shouldUseDarkColors) {
  lightIcon = true
} else {
  lightIcon = false
}

if (this._windowBackgrounded && unread !== 0) {
 showUnread = true
} else {
  showUnread = false
}

icon = _selectThemedIcon(ligthIcon, isInboxZero, showUnread)

This will probably be a little bit more code, but the structure should be easier to read by humans. Additionally, you don't repeat code like setting the unread counter, etc.

I hope this helps. :)

@Phylu
Copy link
Contributor

Phylu commented Nov 19, 2023

@paulatz I have created some logic regarding the dark mode for Windows systems here: #2476

If you want to pick this up again, this might be a good base. I suggest to stick to the icons that we have instead of creating new color schemes. It might need further checks, as e.g. the default Ubuntu/Gnome tray has a black background even if the color scheme is the light one. So this might be KDE specific.

@bengotow bengotow force-pushed the master branch 2 times, most recently from 77d1cc7 to e2e0f88 Compare November 21, 2023 18:05
@bengotow
Copy link
Collaborator

Hey folks - thanks for contributing this @paulatz! I merged @Phylu's work referenced above - if you want to add the light/dark linux icons, rebasing on master and using the same "_dark" check he added would be awesome. I agree we want to keep the colored icons the same colors generally, since folks get used to them.

One question I do have is if there's a good way to detect the color of the status bar on linux well enough to do this in a general way. I wonder if we ship this in a way that's just for KDE?

@foundry376-bot
Copy link

This pull request has been mentioned on Mailspring Community. There might be relevant details there:

https://community.getmailspring.com/t/dark-gray-icon-on-windows-dark-mode/7337/8

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.

4 participants