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.

Sign up for GitHub

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

Jump to bottom

appindicator support #18

Merged
merged 12 commits into from
May 10, 2023
Merged

appindicator support #18

merged 12 commits into from
May 10, 2023

Conversation

purejava
Copy link
Contributor

@purejava purejava commented May 1, 2023

I changed integrations-linux from Java 19 to 20 already, but can revert that, if it's not liked.

@purejava purejava mentioned this pull request May 1, 2023
Merged
@overheadhunter overheadhunter self-requested a review May 1, 2023 14:47
overheadhunter
overheadhunter requested changes May 1, 2023
View reviewed changes
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.

Thanks, this looks really promising! I still have some concerns, but we'll work this out! 🙂

src/main/java/org/cryptomator/linux/tray/ActionItemCallback.java Outdated Show resolved Hide resolved
src/main/java/module-info.java
requires org.freedesktop.dbus;
requires org.purejava.appindicator;
requires org.purejava.kwallet;
requires secret.service;
Copy link
Member

Choose a reason for hiding this comment

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

I still hope for an updated version of this lib as well, as it can easily become fully modular too (needs issues 37 and 38 to be worked on).

I hope this is now the only lib using org.freedesktop.* classes, otherwise we might run into a problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hope this is now the only lib using org.freedesktop.* classes, otherwise we might run into a problem.

I hope the same. Fingers crossed. 🤞🏻

src/main/java/module-info.java Outdated Show resolved Hide resolved
src/main/java/org/cryptomator/linux/tray/AppindicatorTrayMenuController.java Outdated Show resolved Hide resolved
src/main/java/org/cryptomator/linux/tray/AppindicatorTrayMenuController.java Outdated Show resolved Hide resolved
src/main/java/org/cryptomator/linux/tray/AppindicatorTrayMenuController.java Outdated Show resolved Hide resolved
src/main/java/org/cryptomator/linux/tray/LinuxTrayIntegrationsProvider.java Outdated Show resolved Hide resolved
src/main/resources/META-INF/services/org.cryptomator.integrations.tray.TrayIntegrationProvider Outdated Show resolved Hide resolved
src/main/java/org/cryptomator/linux/tray/AppindicatorTrayMenuController.java Outdated
Comment on lines 93 to 105
private String getAbsolutePath(String iconName) {
var res = getClass().getClassLoader().getResource(iconName);
if (null == res) {
throw new IllegalArgumentException("Icon '" + iconName + "' cannot be found in resource folder");
}
File file = null;
try {
file = Paths.get(res.toURI()).toFile();
} catch (URISyntaxException e) {
throw new IllegalArgumentException("Icon '" + iconName + "' cannot be converted to file", e);
}
return file.getAbsolutePath();
}
Copy link
Member

Choose a reason for hiding this comment

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

Does this really work? After all the icon is bundled within a jar. Is it really a "file" that can be read from within GTK?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this really work? After all the icon is bundled within a jar.

It worked in the IDE. The final application is probably different than the compiled code in the IDE. Needs more testing.

Is it really a "file" that can be read from within GTK?

No, the method takes the "icon_name" which is either the (short-) name of an icon installed on the system or its absolute path (what I am using here). The solution might be to use app_indicator_set_icon_theme_path. libappindicator does not offer to much opportunities, though.

Copy link
Member

Choose a reason for hiding this comment

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

It worked in the IDE. The final application is probably different than the compiled code in the IDE. Needs more testing.

That's what I thought. In the IDE it is a "normal" file, it is not yet packed into a .jar archive. So either we need to unpack it from the jar to a temporary directory or bundle the icon with the installer in a way such that the icon will be installed as a regular file that we know the path of.

Copy link
Member

@overheadhunter overheadhunter May 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Do you know how to resolve an icon "by name"? How do you register icon files? Will GTK use this freedesktop specification? In that case we might just use the icon named org.cryptomator.Cryptomator. The desktop menu makes use of this icon name, too.

Our .deb file (ppa), the Flatpak package as well as our AppImage adhere to this standard.

@SailReal How about our AUR release? The AUR does the same. 🎉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The getAbsolutePath(...) stuff was removed. As not needed any more.

We need to add a "org.cryptomator.Cryptomator-unlocked" SVG to our releases, so that it gets installed as an icon on Linux. The file is already in the repo as src/main/resources/tray_icon_unlocked.svg and can be removed afterwards, together with tray_icon.svg (which is already available as "org.cryptomator.Cryptomator").

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done as well. For ppa, appimage and flatpak.

Copy link
Member

Choose a reason for hiding this comment

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

Just for the record: Did you confirm, this works? Or did you find any source confirming that libappindicator and libayatana refer to this freedesktop spec with this "icon name"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the first link to the debian wiki you provided the lib usees the freedesktop icon spec and libayatana is the same and that's what I did during testing all the time. So: yes.

Please note, I didn't build the ppa, nor the appimage or the flatpak app. The flatpak app requires the icon in the downloaded package that wasn't included at that time.

@overheadhunter
Copy link
Member

Oh, the GitHub Actions need to be updated to JDK 20 as well. 😊

@overheadhunter overheadhunter mentioned this pull request May 2, 2023
Merged
@purejava
Copy link
Contributor Author

purejava commented May 3, 2023

Oh, the GitHub Actions need to be updated to JDK 20 as well. 😊

How good you are so thorough. 😃

purejava added a commit to purejava/cryptomator that referenced this pull request May 6, 2023
@purejava
Code improvements continued
948a62b 
Discussion: cryptomator/integrations-linux#18
@purejava
Copy link
Contributor Author

purejava commented May 6, 2023

The try (var arena = MemoryArena.openConfined()) { isn't implemented yet. I wanted to see first, ih the code compiles so far.

@purejava
Copy link
Contributor Author

purejava commented May 6, 2023

All recommendations are implemented now for all appindicator PRs.

@infeo infeo merged commit eef050b into cryptomator:develop May 10, 2023
2 checks passed
@infeo
Copy link
Member

infeo commented May 10, 2023

@purejava Thank you for this amazing contribution!

@purejava
Copy link
Contributor Author

@infeo You are very welcome! 🤗

@purejava purejava deleted the libappindicator branch June 2, 2023 09:17
@infeo infeo mentioned this pull request Jul 26, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcimadamore mcimadamore mcimadamore left review comments

@overheadhunter overheadhunter overheadhunter approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@purejava @overheadhunter @infeo @mcimadamore