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

Show icons for flatpacked app #22

Merged
merged 9 commits into from
Jun 16, 2023

Conversation

purejava
Copy link
Contributor

@purejava purejava commented Jun 3, 2023

This handles the different Linux flavors of Cryptomator.

There is no way to set an icon theme with app_indicator_new_with_path for a flatpacked app. This is the reason for this code change.

Copy link
Member

@infeo infeo left a comment

Choose a reason for hiding this comment

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

Can you point me to the libappindicator api documentation?

Because, instead of using a property set downstream, we could first try app_indicator_new_with_path(...) and just fallback to app_indicator_new(...) in the error case. But that is only possible if there is some sort of error handling.

@purejava
Copy link
Contributor Author

purejava commented Jun 6, 2023

Can you point me to the libappindicator api documentation?

Because, instead of using a property set downstream, we could first try app_indicator_new_with_path(...) and just fallback to app_indicator_new(...) in the error case. But that is only possible if there is some sort of error handling.

I couldn't find any api documentation, only the code of the libs:
https://bazaar.launchpad.net/~indicator-applet-developers/libappindicator/trunk/view/head:/src/app-indicator.c
https://github.com/AyatanaIndicators/libayatana-appindicator/blob/master/src/app-indicator.c

@purejava
Copy link
Contributor Author

purejava commented Jun 8, 2023

The last two commits 6b482da and 36b1ee4 reduce the log messages of the bindings to info (keep out the stack trace), in case a native library could not be loaded and enable saving vault passwords in kdewallets.

Copy link
Member

@infeo infeo left a comment

Choose a reason for hiding this comment

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

I have a new idea/approach for the "if flatpak, then this, else check APPDIR"-thing. Let's introduce a new JVM property and document this property in the README (like done in https://github.com/cryptomator/integrations-win#config):

  • org.cryptomator.integrationsLinux.appIndicator.svgSource - specifies the path from which the svg images are loaded

If the property is not specified (== null), the flatpak approach via app_indicator_new(...) is used. If the property is specified, we are using its value without validation in the app_indicator_new_with_path(...) function.

Then the downstream library consumer can control, where it is loaded from and in the starting script/ build process we can define it appropiately for the target distribution.

I would also split the KDE-Wallet changes into a seperate PR. (or for sake of brevity i'll cherry pick'em).

@purejava
Copy link
Contributor Author

I have a new idea/approach for the "if flatpak, then this, else check APPDIR"-thing. Let's introduce a new JVM property and document this property in the README (like done in https://github.com/cryptomator/integrations-win#config):

* `org.cryptomator.integrationsLinux.appIndicator.svgSource` - specifies the path from which the svg images are loaded

If the property is not specified (== null), the flatpak approach via app_indicator_new(...) is used. If the property is specified, we are using its value without validation in the app_indicator_new_with_path(...) function.

Then the downstream library consumer can control, where it is loaded from and in the starting script/ build process we can define it appropiately for the target distribution.

I would also split the KDE-Wallet changes into a seperate PR. (or for sake of brevity i'll cherry pick'em).

Thanks for that good suggestion @infeo. Please note, that for the AppImage it will still be necessary to evaluate System.getenv("APPDIR"), as we need to now, where the AppImage was mounted, because this determines where to find the icon theme. The last parameter of app_indicator_new_with_path is the icon theme path.

So in the end, the code will look pretty much the same. I'll go on and implement your suggestion!

@infeo
Copy link
Member

infeo commented Jun 13, 2023

Please note, that for the AppImage it will still be necessary to evaluate System.getenv("APPDIR"), as we need to now, where the AppImage was mounted, because this determines where to find the icon theme.

Yes, but we can't we evaluate it in the AppImage start script?

@purejava
Copy link
Contributor Author

Yes, but we can't we evaluate it in the AppImage start script?

Sure. The JVM property would be set in the script/ build process, modified in the AppImage start script and used in the AppindicatorTrayMenuController. I'll do it that way.

@purejava
Copy link
Contributor Author

purejava commented Jun 14, 2023

Yes, but we can't we evaluate it in the AppImage start script?

I am afraid this is not possible. Java system properties are only accessible by the process they are added to.

Edit: @infeo so the two commits above implement it as far as possible. The code is tested and working for the AppImage. If you could provide a new deb in the ppa, I would test that too.

Copy link
Member

@infeo infeo left a comment

Choose a reason for hiding this comment

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

So, i talked with @overheadhunter about this matter and he mentioned an interesting preview feature of JDK 21: String templates. We already thought about changing JVM properties at runtime to adjust to different system envs and have a plan now (will link it later edit: see cryptomator/cryptomator#2957). With string templates we can do it in a regulated way and already using a new JDK feature.

@purejava What does this mean for this PR? Drop evaluation of APPDIR and only use the JVM property, like I initially proposed. I'll make changes in Cryptomator such that the property will have the correct value when calling showTrayIconWithSVG()

README.md Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
purejava added a commit to purejava/cryptomator that referenced this pull request Jun 15, 2023
@purejava
Copy link
Contributor Author

So, i talked with @overheadhunter about this matter and he mentioned an interesting preview feature of JDK 21: String templates. We already thought about changing JVM properties at runtime to adjust to different system envs and have a plan now (will link it later). With string templates we can do it in a regulated way and already using a new JDK feature.

@purejava What does this mean for this PR? Drop evaluation of APPDIR and only use the JVM property, like I initially proposed. I'll make changes in Cryptomator such that the property will have the correct value when calling showTrayIconWithSVG()

Thanks for the innovative suggestion! Changes are done. Please note my comment above, regarding kdewallet.

purejava added a commit to purejava/integrations-linux that referenced this pull request Jun 15, 2023
Copy link
Member

@infeo infeo left a comment

Choose a reason for hiding this comment

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

The last request for change, i promise! 😇

src/main/java/module-info.java Outdated Show resolved Hide resolved
This reverts commit 36b1ee4.

Changes regarding kdewallet will be included in a separate PR
@infeo infeo merged commit 8de5733 into cryptomator:release/1.3.0 Jun 16, 2023
1 check passed
@purejava purejava deleted the release/1.3.0 branch June 16, 2023 16:48
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.

None yet

2 participants