Skip to content

libappindicator support#17

Merged
infeo merged 3 commits into
cryptomator:developfrom
purejava:libappindicator
Apr 26, 2023
Merged

libappindicator support#17
infeo merged 3 commits into
cryptomator:developfrom
purejava:libappindicator

Conversation

@purejava

Copy link
Copy Markdown
Contributor

No description provided.

@infeo infeo 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.

Instead of adding an additional parameter, i suggest we replace parameter byte [] imageData by URI imageUri. This way, implementations or specifications do not need to handle the case, if both parameters are set to non-null values.

URI's support direct data transportation via the data scheme, file paths can be specified by the file scheme, and depending on the implementation, other schemes (even custom ones, e.g. freedesktop) can be supported too.

@overheadhunter

Copy link
Copy Markdown
Member

Instead of adding an additional parameter, i suggest we replace parameter byte [] imageData by URI imageUri. This way, implementations or specifications do not need to handle the case, if both parameters are set to non-null values.

URI's support direct data transportation via the data scheme, file paths can be specified by the file scheme, and depending on the implementation, other schemes (even custom ones, e.g. freedesktop) can be supported too.

I'm not sure about this suggestion. Adding parameters is backwards compatible, replacing them is not. Furthermore, data:// URIs are verbose due to base64 encoding overhead and we can not assume they are directly supported by implementors, so they might require custom code for parsing and decoding. file:// URIs should work fine for actual files, but not for resources embedded in jars. iirc, loading resources from a different jar is prohibited by the module system, so we can not pass URIs to resources from the cryptomator main jar to the integrations-* jar.

@overheadhunter

Copy link
Copy Markdown
Member

I stand corrected, getClass().getRessource('path/without/preceeding/slash.png') should work (but we better confirm this experimentally).

@purejava

Copy link
Copy Markdown
Contributor Author

I stand corrected, getClass().getRessource('path/without/preceeding/slash.png') should work (but we better confirm this experimentally).

It does work and there wasn't too much additional code necessary. You'll find the change here.
The new implementation of appindicator which includes all your recommendations from the old Cryptomator PR and this one is progressing well.

@infeo infeo modified the milestones: 1.3.0, 2.0.0 Apr 26, 2023
@infeo infeo merged commit 90ad49c into cryptomator:develop Apr 26, 2023
@infeo

infeo commented Apr 26, 2023

Copy link
Copy Markdown
Member

@purejava Thanks for picking up this topic. An new version of integrations-api will be released today. Due to the breaking api, it will be a major bump, namely 2.0.0

@purejava

Copy link
Copy Markdown
Contributor Author

@infeo Thanks for merging my PR. I left you a private message on Slack regarding the question, whether the required changes for the Cryptomator code should be based on Java 19 or on Java 20.

@overheadhunter

Copy link
Copy Markdown
Member

After this discussion, I believe we need to reevaluate this change.

The attempt to kill two birds with one stone with the introduction of URIs for both binary data (data://) as well as file paths (file://) was a mistake. It turns out, that file paths simply don't work for libappindicator (as the file must be an actual file, not something bundled within a .jar) and the data workaround for the AWT implementation was rather ugly anyway.

So in my opinion the original proposal (3e53d06) is actually preferrable. It is not a breaking change, so we can continue publishing 1.x releases and instead adds an alternative icon name, which can be used to locate an icon. It is of course debatable, whether this is the best approach in regards of further additions.

@overheadhunter

Copy link
Copy Markdown
Member

@purejava Can you see if #19 would work for you? It is still API-breaking, so it would stay on 2.x, but it allows further additions and is more flexible than trying to stuff everything into URI.

@overheadhunter overheadhunter modified the milestones: 2.0.0, 1.3.0 May 7, 2023
@overheadhunter

Copy link
Copy Markdown
Member

Since this API is an @ApiStatus.Experimental, API consumers are expecting it to still evolve. There is no need for a 2.0.0. I therefore rescheduled this.

@purejava purejava deleted the libappindicator branch April 7, 2024 15:22
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.

3 participants