Skip to content

libappindicator support#2862

Closed
purejava wants to merge 12 commits into
cryptomator:developfrom
purejava:libappindicator
Closed

libappindicator support#2862
purejava wants to merge 12 commits into
cryptomator:developfrom
purejava:libappindicator

Conversation

@purejava

@purejava purejava commented Apr 14, 2023

Copy link
Copy Markdown
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

purejava commented Apr 14, 2023

Copy link
Copy Markdown
Contributor Author

Depends on cryptomator/integrations-api#17

@tobihagemann

tobihagemann commented Apr 14, 2023

Copy link
Copy Markdown
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

infeo commented Apr 14, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

@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
overheadhunter
overheadhunter reviewed Apr 14, 2023
View reviewed changes

@overheadhunter overheadhunter left a comment

Copy link
Copy Markdown
Member

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.

Comment thread src/main/java/module-info.java
requires com.tobiasdiez.easybind;
requires dagger;
requires io.github.coffeelibs.tinyoauth2client;
requires libappindicator.gtk3.minimal.java;

@overheadhunter overheadhunter Apr 14, 2023

Copy link
Copy Markdown
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.

Comment thread src/main/java/org/cryptomator/ui/traymenu/AppindicatorTrayMenuController.java

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

@overheadhunter overheadhunter Apr 14, 2023

Copy link
Copy Markdown
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.

Comment thread pom.xml
<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>

@overheadhunter overheadhunter Apr 14, 2023

Copy link
Copy Markdown
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

purejava commented Apr 14, 2023

Copy link
Copy Markdown
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

purejava commented Apr 16, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@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

infeo commented Apr 17, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

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.

@purejava purejava mentioned this pull request Apr 27, 2023
Merged
@overheadhunter overheadhunter mentioned this pull request Apr 29, 2023
Closed
@infeo infeo mentioned this pull request Jul 18, 2023
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@overheadhunter overheadhunter overheadhunter left review comments

Assignees

No one assigned

Labels

os:linux

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@purejava @tobihagemann @infeo @overheadhunter