-
Notifications
You must be signed in to change notification settings - Fork 1.4k
feat: Add support for macOS layered icons #5451
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
base: master
Are you sure you want to change the base?
Conversation
|
Hi, thank you for the contribution! |
|
Thanks again for creating the related YouTrack issue – it’s been really helpful! I just wanted to gently follow up on this PR – would you have a moment to review it when you get a chance? Let me know if I can clarify anything to make it easier! No rush at all, appreciate your help! |
freelife4850-netizen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like a good idea to try 🙂
kropp
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for a long wait, could you please take a look at comments and also rebase the branch to the latest version?
...pose/src/main/kotlin/org/jetbrains/compose/desktop/application/tasks/AbstractJPackageTask.kt
Outdated
Show resolved
Hide resolved
...mpose/src/test/kotlin/org/jetbrains/compose/test/tests/integration/DesktopApplicationTest.kt
Outdated
Show resolved
Hide resolved
gradle-plugins/compose/src/test/test-projects/application/macLayeredIcon/build.gradle.kts
Outdated
Show resolved
Hide resolved
.../compose/src/main/kotlin/org/jetbrains/compose/desktop/application/internal/MacAssetsTool.kt
Outdated
Show resolved
Hide resolved
This commit introduces support for using macOS layered icons (`.icon` directory) for both JVM and native application packaging. - A new `layeredIconDir` property is added to the `compose.desktop.application.nativeDistributions.macOS` and `compose.nativeApplication.macOS` DSLs. - The `actool` from Xcode Command Line Tools is used to compile the `.icon` directory into an `Assets.car` file. - The `Assets.car` file is included in the `.app` bundle resources. - The `Info.plist` is updated with `CFBundleIconName` to reference the compiled asset. - A check for a supported `actool` version is performed before compilation. - New integration tests (`testMacLayeredIcon`, `testMacLayeredIconRemove`) are added to verify the functionality for both JVM and native targets.
The `macAssetsDir` property in `AbstractJPackageTask` was unused and has been removed.
- Deleted the `Expected-Info.plist` which verified now-removed properties. - Removed `extraInfoPlistKeys` and file association configurations from the test's `build.gradle.kts`.
Updated the `macLayeredIcon` test project to use `COMPOSE_VERSION_PLACEHOLDER` for Compose dependency versions. This makes the test configuration more flexible and easier to maintain.
This commit refactors the `actool` version parsing logic to use the `plutil` command-line tool instead of a manual XML parser. This change simplifies the code and improves robustness. - Replaced the XML parsing implementation in `MacAssetsTool` with a call to `plutil -extract`. - Updated `try`/`catch` blocks to handle exceptions from the tool execution. - Added a `plutil` file definition to `MacUtils`. - Integration tests for macOS layered icons (`testMacLayeredIcon`, `testMacLayeredIconRemove`) are now skipped if the `actool` version is less than 26, ensuring they run only on supported environments.
6fe432f to
f6d1291
Compare
|
@kropp All the changes you mentioned that needed to be made have already been completed. |
|
@igordmn requesting second review as it adds new public API in Gradle plugin |
PierreVieira
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simple comments. Nice addition!
.../compose/src/main/kotlin/org/jetbrains/compose/desktop/application/internal/MacAssetsTool.kt
Outdated
Show resolved
Hide resolved
...plugins/compose/src/test/test-projects/application/macLayeredIcon/src/jvmMain/kotlin/main.kt
Outdated
Show resolved
Hide resolved
...ugins/compose/src/test/test-projects/application/macLayeredIcon/src/macosMain/kotlin/main.kt
Outdated
Show resolved
Hide resolved
...pose/src/test/test-projects/application/macLayeredIcon/subdir/kotlin_icon_big.icon/icon.json
Outdated
Show resolved
Hide resolved
gradle-plugins/compose/src/test/test-projects/application/macLayeredIcon/build.gradle.kts
Outdated
Show resolved
Hide resolved
gradle-plugins/compose/src/test/test-projects/application/macLayeredIcon/gradle.properties
Outdated
Show resolved
Hide resolved
gradle-plugins/compose/src/test/test-projects/application/macLayeredIcon/settings.gradle.kts
Outdated
Show resolved
Hide resolved
Co-authored-by: Pierre Vieira <pierrevieiraggg@gmail.com>
igordmn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you and sorry for more waiting time. After we resolve the new comments, it should be good to merge.
| var dmgPackageBuildVersion: String? = null | ||
| var appCategory: String? = null | ||
| var minimumSystemVersion: String? = null | ||
| var layeredIconDir: DirectoryProperty = objects.directoryProperty() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we override the icon defined in iconFile by defining this property and packaging it on the new macOs/Xcode? Is this icon supported on the oldest OS, because it is compiled into backward-compatible assets?
If so, it is better to reuse the existing iconFile instead of adding a new property and check that if it is a directory ending with ".icon" then we should use the layeredIcon logic.
Reasoning:
iconFile,layeredIconDirchange one thing (the application icon) and differ only in implementations- it is similar to using different extensions, just more unusual
- we might need to support it in
fileAssociation
| var dmgPackageBuildVersion: String? = null | ||
| var appCategory: String? = null | ||
| var minimumSystemVersion: String? = null | ||
| var layeredIconDir: DirectoryProperty = objects.directoryProperty() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we support it for fileAssociation?:
compose.desktop {
application {
nativeDistributions {
macOS {
fileAssociation(
"text/kotlin",
"kott",
"Kotlin Source File2",
project.file("subdir/kotlin_icon_big.icon"),
)
| var dmgPackageBuildVersion: String? = null | ||
| var appCategory: String? = null | ||
| var minimumSystemVersion: String? = null | ||
| var layeredIconDir: DirectoryProperty = objects.directoryProperty() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a link to the example (gradle-plugins/compose/src/test/test-projects/application/macLayeredIcon/subdir/kotlin_icon_big.icon) in the PR description, near the snippet and change the path in the snippet to subdir/kotlin_icon_big.icon
| var dmgPackageBuildVersion: String? = null | ||
| var appCategory: String? = null | ||
| var minimumSystemVersion: String? = null | ||
| var layeredIconDir: DirectoryProperty = objects.directoryProperty() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to describe necessary changes required in the documentation.
I suggest to just do it in the PR:
Documentation changes needed
.icns files or .icon directory ([example](gradle-plugins/compose/src/test/test-projects/application/macLayeredIcon/subdir/kotlin_icon_big.icon)) for macOS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the root icons not used? If so, let's delete.
|
|
||
| if (macAppStore.orNull == true) { | ||
| val systemVersion = macMinimumSystemVersion.orNull ?: "10.13" | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| "--platform", "macosx", | ||
| "--enable-icon-stack-fallback-generation=disabled", | ||
| "--include-all-app-icons", | ||
| "--minimum-deployment-target", minimumSystemVersion ?: "10.13", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| "--minimum-deployment-target", minimumSystemVersion ?: "10.13", | |
| "--minimum-deployment-target", minimumSystemVersion, |
Better to pass a non-null parameter to not have "10.13" in multiple places. We seems can do that, by changing one of the calling code to minimumSystemVersion.getOrElse(KOTLIN_NATIVE_MIN_SUPPORTED_MAC_OS)!!
|
|
||
| @get:InputDirectory | ||
| @get:Optional | ||
| internal val macLayeredIcons: DirectoryProperty = objects.directoryProperty() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| internal val macLayeredIcons: DirectoryProperty = objects.directoryProperty() | |
| internal val layeredIconDir: DirectoryProperty = objects.directoryProperty() |
to be consistent with the public property (if we still decide to keep it after this)
|
|
||
| fun compileAssets(iconDir: File, workingDir: File, minimumSystemVersion: String?): File { | ||
| val toolVersion = checkAssetsToolVersion() | ||
| logger.info("compile mac assets is starting, supported actool version:$toolVersion") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| logger.info("compile mac assets is starting, supported actool version:$toolVersion") | |
| logger.info("Compilation of mac assets is starting, actool version: $toolVersion") |
Support for using macOS layered icons (
.icondirectory) for both JVM and native application packaging.layeredIconDirproperty is added to thecompose.desktop.application.nativeDistributions.macOSandcompose.nativeApplication.macOSDSLs.actoolfrom Xcode Command Line Tools is used to compile the.icondirectory into anAssets.carfile.Assets.carfile is included in the.appbundle resources.Info.plistis updated withCFBundleIconNameto reference the compiled asset.actoolversion is performed before compilation.testMacLayeredIcon,testMacLayeredIconRemove) are added to verify the functionality for both JVM and native targets.Example
compose.desktop { application { nativeDistributions { macOS { layeredIconDir.set("Icon Path") } } } }Testing
Only verify the task behavior: compose:test-Gradle(9.2.0)-Agp(8.12.3) --tests org.jetbrains.compose.test.tests.integration.DesktopApplicationTest.testMacLayeredIcon
Only verify the task behavior: compose:test-Gradle(9.2.0)-Agp(8.12.3) --tests org.jetbrains.compose.test.tests.integration.DesktopApplicationTest.testMacLayeredIconRemove
This should be tested by QA
Tickets
CMP-9083 Support macOS 26 layered icons
Release Notes
Features - Gradle Plugin
.icondirectory) for both JVM and native application packaging.