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

File associations #4957

Merged
merged 10 commits into from
Jul 2, 2024
Merged

File associations #4957

merged 10 commits into from
Jul 2, 2024

Conversation

zhelenskiy
Copy link
Member

@zhelenskiy zhelenskiy commented Jun 11, 2024 •
edited
Loading

Add file associations support to Compose Desktop

Fixes #773

Testing

Tested on the sample project.
Behaviours per OSs:

  • MacOS Sonoma: associations work for distributables.
  • Windows 11: associations work after the installation of the MSI.
  • Kubuntu: associations do not work, but everything else works fine. However, IDEA also does not have associations there, so I assume this is fine.

I didn't write any unit tests because I don’t know which of them you are expecting me to write. So, I'm looking forward to your feedback and suggestions.

This should be tested by QA

Release Notes

Highlight - Desktop

  • Introduction of the new DSL function in nativeDistributions block: 
    fun fileAssociation(mimeType: String, extension: String, description: String): Unit

@zhelenskiy
Add file associations support
fee7af9 
[Desktop: Add support for file associations](JetBrains#773)
@zhelenskiy
Fix Linux error
e4a35b4
@igordmn igordmn self-requested a review June 12, 2024 00:44
@zhelenskiy
fixup! Add file associations support
574c12b 
[Desktop: Add support for file associations](JetBrains#773)
@igordmn
Copy link
Collaborator

igordmn commented Jun 19, 2024

@terrakok, could you also review the new API in this PR? I am also reviewing it and the implementation.

@igordmn igordmn requested a review from terrakok June 19, 2024 10:30
igordmn
igordmn reviewed Jun 19, 2024
View reviewed changes
Copy link
Collaborator

@igordmn igordmn left a comment

Choose a reason for hiding this comment

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

Thanks!

A few changes needed, an API review from @terrakok, and it is good to merge.

gradle-plugins/compose/src/test/test-projects/application/macOptions/Expected-Info.plist Show resolved Hide resolved
...pose/src/main/kotlin/org/jetbrains/compose/desktop/application/tasks/AbstractJPackageTask.kt Outdated Show resolved Hide resolved
...src/main/kotlin/org/jetbrains/compose/desktop/application/dsl/JvmApplicationDistributions.kt Outdated Show resolved Hide resolved
...src/main/kotlin/org/jetbrains/compose/desktop/application/dsl/JvmApplicationDistributions.kt Outdated Show resolved Hide resolved
@zhelenskiy
fixup! Add file associations support
8e6c08e 
[Desktop: Add support for file associations](JetBrains#773)
terrakok
terrakok requested changes Jun 21, 2024
View reviewed changes
...ins/compose/src/main/kotlin/org/jetbrains/compose/desktop/application/dsl/FileAssociation.kt Outdated Show resolved Hide resolved
...src/main/kotlin/org/jetbrains/compose/desktop/application/dsl/JvmApplicationDistributions.kt Outdated Show resolved Hide resolved
...mpose/src/main/kotlin/org/jetbrains/compose/desktop/application/internal/InfoPlistBuilder.kt Outdated Show resolved Hide resolved
...mpose/src/main/kotlin/org/jetbrains/compose/desktop/application/internal/InfoPlistBuilder.kt Outdated Show resolved Hide resolved
...mpose/src/main/kotlin/org/jetbrains/compose/desktop/application/internal/InfoPlistBuilder.kt Outdated Show resolved Hide resolved
...mpose/src/main/kotlin/org/jetbrains/compose/desktop/application/internal/InfoPlistBuilder.kt Outdated Show resolved Hide resolved
...mpose/src/main/kotlin/org/jetbrains/compose/desktop/application/internal/InfoPlistBuilder.kt Outdated Show resolved Hide resolved
...rc/main/kotlin/org/jetbrains/compose/desktop/application/internal/configureJvmApplication.kt Outdated Show resolved Hide resolved
gradle-plugins/compose/src/test/test-projects/application/macOptions/Expected-Info.plist Show resolved Hide resolved
@zhelenskiy
fixup! Add file associations support
100aeef 
[Desktop: Add support for file associations](JetBrains#773)
@zhelenskiy
fixup! Add file associations support
811739d 
[Desktop: Add support for file associations](JetBrains#773)
@zhelenskiy
Copy link
Member Author

zhelenskiy commented Jun 21, 2024 •
edited
Loading

Except for DSL test and docs, everything else is fixed. Can you close the comment, @terrakok? It is not closable.

What do you think about the API? It supports:

  • Ability to make platform-specific file associations.
  • Ability to make platform-independent associations without copying.
  • Ability to provide or not an icon.
  • For platform-specific association icons you need to specify only one property iconFile while for universal you need to specify windowsIconFile, etc.

The demo project is also updated according to the new API.

PS. This version is only tested on MacOS for now. I'll check the rest a bit later.

igordmn
igordmn approved these changes Jun 21, 2024
View reviewed changes
Copy link
Collaborator

@igordmn igordmn left a comment

Choose a reason for hiding this comment

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

Looks good to me after resolving 2 my open comments.

The end review and merging I delegate to @terrakok.

...src/main/kotlin/org/jetbrains/compose/desktop/application/dsl/JvmApplicationDistributions.kt Outdated Show resolved Hide resolved
@zhelenskiy
fixup! Add file associations support
0fd4ebe 
[Desktop: Add support for file associations](JetBrains#773)
@zhelenskiy
Copy link
Member Author

zhelenskiy commented Jun 22, 2024 •
edited
Loading

I had to change resource name generation to make it stable for testing.

@zhelenskiy
Copy link
Member Author

I tested the current version on Windows, MacOS, Ubuntu, Kubuntu. On Windows, MacOS everything is ok, while on both Linux distributives there are problems (in Kubuntu there is no icon, in Ubuntu, association doesn't work), but the same thing is with idea as well.

@zhelenskiy zhelenskiy requested a review from terrakok June 22, 2024 17:46
@zhelenskiy
fixup! Add file associations support
a435833 
[Desktop: Add support for file associations](JetBrains#773)
@zhelenskiy
Copy link
Member Author

Everything seems fixed now.

terrakok
terrakok reviewed Jul 1, 2024
View reviewed changes
Copy link
Member

@terrakok terrakok left a comment

Choose a reason for hiding this comment

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

val fileAssociationFiles = fileAssociations.orNull.orEmpty()

do we need it iyet?

@zhelenskiy
fixup! Add file associations support
df432b0 
[Desktop: Add support for file associations](JetBrains#773)
@zhelenskiy
fixup! Add file associations support
01f0eb8 
[Desktop: Add support for file associations](JetBrains#773)
@igordmn
Copy link
Collaborator

igordmn commented Jul 1, 2024

Looks good to me, thanks!

@terrakok, please merge, if everything is okay to you too.

zhelenskiy added a commit to zhelenskiy/compose-multiplatform that referenced this pull request Jul 1, 2024
@zhelenskiy
Add file associations support documentation
fe6dd1e 
JetBrains#4957

Fixes JetBrains#773
@zhelenskiy zhelenskiy mentioned this pull request Jul 1, 2024
Merged
@terrakok terrakok merged commit dea37a0 into JetBrains:master Jul 2, 2024
12 checks passed
igordmn pushed a commit that referenced this pull request Jul 2, 2024
@zhelenskiy @zamulla
Add file associations support documentation (#5054)
810745f 
Docs for file associations support
(#4957) which
fixes #773.

---------

Co-authored-by: Aleksey Zamulla <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@igordmn igordmn igordmn approved these changes

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

Desktop: Add support for file associations
3 participants
@zhelenskiy @igordmn @terrakok