borgbase / vorta

Desktop Backup Client for Borg Backup
https://vorta.borgbase.com
GNU General Public License v3.0
1.97k stars 130 forks source link

Add white icon for menu bar MacOS #1220

Open ph00lt0 opened 2 years ago

ph00lt0 commented 2 years ago

Describe the bug Please add a white icon to conform with the design standard of menu bar icons in Monterey / Big Sur

Environment (please complete the following information):

real-yfprojects commented 2 years ago

@m3nu You are more familiar with MacOS.

m3nu commented 2 years ago

Currently it uses an icon that works on dark and light themes. So the issue isn't urgent.

Will see if there is a simple way to update the icon when a scheme change is detected. Like we do with other icons.

stale[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Brawl345 commented 1 year ago

Currently it uses an icon that works on dark and light themes

image

This does not look good though :/

diivi commented 1 year ago

I made two alternatives. image I prefer the second one, which one do you think might be better? @m3nu Also, the docstring for this function:

  def set_tray_icon(self, active=False):
      """
      Use white tray icon, when on Gnome or in dark mode. Otherwise use dark icon.
      """
      icon_name = f"icons/hdd-o{'-active' if active else ''}.png"
      icon = QIcon(get_asset(icon_name))
      self.setIcon(icon)

mentions that it uses white tray icon when on gnome/in dark mode, otherwise it should use a dark icon. However, the icon being selected is either the active/non-active icon. Should I just update the hdd-o.png file to use a transparent body instead of a black one and update this docstring, or should I add code to check the current theme preferences too (don't know how to do this yet)?

m3nu commented 1 year ago

4 icons are needed in total: dark/light x normal/active

I'm also not sure if Qt gets the signal regarding the theme change, if it's in the background only. That's why I didn't implement this and went for a simple icon that works for all cases. If you can make it work, it would be a nice addition though.

diivi commented 1 year ago

4 icons are needed in total: dark/light x normal/active I'm also not sure if Qt gets the signal regarding the theme change, if it's in the background only.

I am on a Linux machine running Ubuntu 22.04 so I won't be able to test how Vorta responds to changes to the system's theme on MacOS. But I feel like a transparent body for the hdd icon might be more suitable than the current black body.

real-yfprojects commented 1 year ago

The other icons use solid shapes instead of contours. So maybe your first option is better @diivi.

Brawl345 commented 1 year ago

I would also prefer the first option.

i1sm3ky commented 1 year ago

I found a way to check which theme is being used by using darkdetect. I wanted to ask is there a good way of checking if we are running GNOME or not?

Edit: NVM, Found a solution on the tray_menu.py file itself.

diivi commented 1 year ago

Feel free to use the designs I shared, just request access so I can allow you to export the icons!

real-yfprojects commented 1 year ago

I wondered why detecting via QPalette doesn't work. Or maybe it does...

i1sm3ky commented 1 year ago

I wondered why detecting via QPalette doesn't work. Or maybe it does...

It does! I'm using self.app.paletteChanged.connect(self.set_tray_icon) along with darkdetect to detect and change the icon for light or dark mode.

But I'm having problems importing the module, It says it's installed but when run it gives a module not found error. It runs fine on another adjacent python files.

real-yfprojects commented 1 year ago

Can you enclose relevant output including the error message?

i1sm3ky commented 1 year ago

Can you enclose relevant output including the error message?

It's a normal module not found error:

2023-04-01 13:37:38,971 - root - CRITICAL - Uncaught exception, file a report at https://github.com/borgbase/vorta/issues/new/choose
Traceback (most recent call last):
  File "/Library/Frameworks/Python.framework/Versions/3.9/bin/vorta", line 8, in <module>
    sys.exit(main())
  File "/Users/1tsm3ky/Desktop/Vorta/vorta/src/vorta/__main__.py", line 69, in main
    from vorta.application import VortaApp
  File "/Users/1tsm3ky/Desktop/Vorta/vorta/src/vorta/application.py", line 20, in <module>
    from vorta.tray_menu import TrayMenu
  File "/Users/1tsm3ky/Desktop/Vorta/vorta/src/vorta/tray_menu.py", line 3, in <module>
    import darkdetect
ModuleNotFoundError: No module named 'darkdetect'

But as suggested by @m3nu, adding a new dependency just for correcting the icon colour isn't something we are willing to do. So I think a white icon for both light and dark modes on GNOME and MacOS and rest could have the original black/dark one?

real-yfprojects commented 1 year ago

What about https://github.com/borgbase/vorta/blob/e0fe766051e521396d7d58f973d1f8002ff749ad/src/vorta/utils.py#L374-L379

Can this be used to determine the colour of the tray icon?

m3nu commented 1 year ago

Can this be used to determine the colour of the tray icon?

It should be used. It may not change the icon right away, but eventually. Not sure if it triggers with the app in the BG or no window open.

i1sm3ky commented 1 year ago

What about https://github.com/borgbase/vorta/blob/e0fe766051e521396d7d58f973d1f8002ff749ad/src/vorta/utils.py#L374-L379

Can this be used to determine the colour of the tray icon?

I guess, Yes. Let me check if this works. BTW thanks for the snippet.

i1sm3ky commented 1 year ago

https://github.com/borgbase/vorta/blob/e0fe766051e521396d7d58f973d1f8002ff749ad/src/vorta/utils.py#L374-L379

I tried this, but it didn't worked reliably, It returns False even when in dark mode.

For now I've implemented the white icons for GNOME and MacOS(I think the white one fits better with the rest of MacOS's solid icons) and for rest it uses the previous black ones.

I've opened a PR #1676 for this as well, please someone verify the implementation.

real-yfprojects commented 1 year ago

I tried this, but it didn't worked reliably, It returns False even when in dark mode.

Which attribute of the MacOS palette is the cause of this?

i1sm3ky commented 1 year ago

Which attribute of the MacOS palette is the cause of this?

The uses_dark_modes works on the principle of checking the contrast between text colour and window colour but on MacOS when used inside the tray_menu.py it returns the same lightness values for both the theme modes. But when used on a separate file it outputs the correct values, I think it only works with the default palette of PyQt.

real-yfprojects commented 1 year ago

when used inside the tray_menu.py it returns the same lightness values for both the theme modes

Did you call it after QApplication was initialized completely?

i1sm3ky commented 1 year ago

Did you call it after QApplication was initialized completely?

Yes, it was called after QApplication was completely initialized.

real-yfprojects commented 1 year ago

I just found these two things that are worth testing for solving this issue. The background being the template feature of NSImage that native apps use to display their icon in the correct colour.

https://doc.qt.io/qt-6/qicon.html#setIsMask

https://stackoverflow.com/questions/38039262/creating-a-template-image-dynamically-for-osx-menu-bar

The system will automatically mark an image loaded from a file as a template image if its filename-minus-extension ends with "Template". So, for example, fooTemplate.png or barTemplate.pdf.

flying-x commented 3 months ago

image

When I use vorta on gnome with the "blur my shell" extension, the icon isn't transparent as you can see above. I am using manjaro/arch linux and should be on the latest version. Is this the correct place to report this issue?