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

libappindicator support #2862

Closed
wants to merge 12 commits into from
Closed

Conversation

purejava
Copy link
Contributor

This PR changes the tray icon support for Linux away from AWT towards the libappindicator library as suggested in #1645.

It displays SVG versions of the Cryptomator icon, that scale and should look ok on all desktop environments. On GNOME 43 and KDE 5.24.7 they look like this:

Bildschirmfoto 2023-04-11 um 18 43 00

Bildschirmfoto 2023-04-13 um 07 31 45

Some notes on requirements to use libappindicator:

  • libappindicator is a native library, that needs to be installed on the system. On Ubuntu, the package is named libappindicator3-1, on Arch linux it's called libappindicator-gtk3
  • recent GNOME desktop environments by default do not allow the SystemTray icon to be shown. It's necessary to install the Appindicator Support plugin to show libappindicator icons at all
  • of course, tray icon support needs to be enabled in the Cryptomator settings:
  "uiOrientation": "LEFT_TO_RIGHT",
  "keychainProvider": "org.cryptomator.linux.keychain.SecretServiceKeychainAccess",
  "useKeychain": true,
  "licenseKey": "",
  "showMinimizeButton": false,
  "showTrayIcon": true,
  "windowXPosition": 0,
  "windowYPosition": 0,
  "windowWidth": 0,
  "windowHeight": 0,
  "displayConfiguration": "displayId: 0, 1440.0x772.0;"

Some notes from the development perspective:

  • I've choosen to use libappindicator to show icons on Linux in case the native lib is available without a fallback to AWT, as AWT doesn't work well on Linux any more
  • the tray icon files themselves needed to be placed in the resources directly (not in the img folder like the other files) as their full path needs to be determined
  • the integrations-api was capable of handling PNG files (as byte[] streams), but needed to be widened to handle SVG as well

@purejava
Copy link
Contributor Author

Depends on cryptomator/integrations-api#17

@tobihagemann
Copy link
Member

Incredible contribution! 🚀 Even though I'm not qualified to give much feedback regarding Linux systems, I just wanted to let you know that we already have a monochrome icon (e.g., we're using it for the menu bar icon on macOS). I've sent the SVG file via Slack, maybe you can use that (and fix anything in the SVG if necessary)?

@infeo
Copy link
Member

infeo commented Apr 14, 2023

@purejava Cool! However, this is the wrong place. Since this is Linux specifc, the implementation needs to be placed in https://github.com/cryptomator/integrations-linux.

Create a new package like for other implemented interfaces (as can be seen in https://github.com/cryptomator/integrations-win/tree/develop/src/main/java/org/cryptomator/windows) and after the implementation is done, create a service provider entry.

Regarding the selection of the service implementation: This can be controlled by the @Priority and @OperatingSystem annotations.

@infeo infeo closed this Apr 14, 2023
@infeo infeo added type:enhancement Improvements on an existing feature os:linux labels Apr 14, 2023
Copy link
Member

@overheadhunter overheadhunter left a comment

Choose a reason for hiding this comment

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

While I agree this needs to be done in integrations-linux, I want to leave some general remarks already.

@@ -31,12 +32,13 @@
requires com.tobiasdiez.easybind;
requires dagger;
requires io.github.coffeelibs.tinyoauth2client;
requires libappindicator.gtk3.minimal.java;
Copy link
Member

Choose a reason for hiding this comment

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

I would strongly suggest to align the module name with the package name and make sure that both follow reverse domain name notation for a domain that you control. Otherwise we might have to deal with split packages again.


@CheckAvailability
public static boolean isAvailable() {
return SystemUtils.IS_OS_LINUX && MemoryAllocator.isLoadedNativeLib() && SystemTray.isSupported();
Copy link
Member

Choose a reason for hiding this comment

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

SystemTray.isSupported() should not be needed, since this only determines whether the AWT system tray is supported, which might not be the case while libappindicator is still available.

@@ -55,6 +55,7 @@
<slf4j.version>2.0.6</slf4j.version>
<tinyoauth2.version>0.5.1</tinyoauth2.version>
<zxcvbn.version>1.7.0</zxcvbn.version>
<appindicator.version>1.0.0-SNAPSHOT</appindicator.version>
Copy link
Member

Choose a reason for hiding this comment

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

This requires JDK 19 (not >= 19 but == 19), which is fine atm but we're in the process of switching to JDK 20 which will break support, as this is compiled with --enable-preview.

Not really a problem right now, but as long as Panama is a preview feature, this might force us to temporarily drop support for this tray icon implementation again, when updating the JDK.

I would suggest that you use JDK 20 already, since it is only a matter of days - weeks for us to make the switch.

@purejava
Copy link
Contributor Author

Thanks for the quick and thorough feedback @tobihagemann, @infeo and @overheadhunter!

I'll change the package names and the other recommendations as well and will move the stuff to integrations-linux.
And use Java 20. It's still not released on Arch though.

@purejava
Copy link
Contributor Author

purejava commented Apr 16, 2023

@purejava Cool! However, this is the wrong place. Since this is Linux specifc, the implementation needs to be placed in https://github.com/cryptomator/integrations-linux.

@infeo I thought about this. In principle, I am used to the integrations concept from writing keychain implementations.

The TrayIntegrationProvider does not provide anything I need here for appindicator. All the relevant stuff is happening in the TrayMenuController.

So what should be placed in integrations-linux from your point of view? What sort of provider and which methods should it provide?

@infeo
Copy link
Member

infeo commented Apr 17, 2023

To cite the doc of the TrayIntegrationsProvider:

Allows to perform OS-specific tasks when the app gets minimized to or restored from a tray icon.

For an example, see https://github.com/cryptomator/integrations-mac/blob/develop/src/main/java/org/cryptomator/macos/tray/MacTrayIntegrationProvider.java.

When you do not need this interface, you can ignore it. From my point of view, it has a bad naming. The package contains two different services. We should rework the name scheme.


tl;dr: Just use the TrayMenuController.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
os:linux type:enhancement Improvements on an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants