Resources improvements for native macOS app #5169
Conversation
terrakok
left a comment
terrakok
left a comment
There was a problem hiding this comment.
Tests for the new logic are required
| val pathSuffix = if (target.isMacTarget()) "/Resources" else "" | ||
| File("$builtProductsDir/$contentsFolderPath$pathSuffix/$IOS_COMPOSE_RESOURCES_ROOT_DIR").canonicalPath |
There was a problem hiding this comment.
I don't like the idea to hardcode a mac specific path here.
It is better to make new function for mac targets and support cocoapods as well then.
There was a problem hiding this comment.
Fixed the path by changing CONTENTS_FOLDER_PATH to UNLOCALIZED_RESOURCES_FOLDER_PATH. Now it is the same for iOS and macOS.
There was a problem hiding this comment.
Also added a test for cocoapods (copied from iOS with minor changes), so I think all should work fine.
Thomas-Vos
force-pushed
the
resources-improvements-macos
branch
2 times, most recently
from
4111be3 to
d5c4dc5
Compare
…n to existing executable support
Thomas-Vos
force-pushed
the
resources-improvements-macos
branch
from
d5c4dc5 to
8f07e0e
Compare
|
@terrakok thanks a lot for your review! Thanks for helping me with reviewing some fixes for macOS issues so I can start using it in my app. I added tests to the PR as you asked. However, the tests are failing on CI because they use an outdated snapshot of the resources library. For the tests to pass, they must use the latest version of the resources library. This is because there are changes in the resources library in order to support macOS. When forcing a local build of the library, it passes on my own mac:
Is there any way to fix this? Or is this expected to happen until the next compose snapshot is released with the changes of this PR? |
| private fun getPathOnDisk(path: String): String { | ||
| val fm = NSFileManager.defaultManager() | ||
| val currentDirectoryPath = fm.currentDirectoryPath | ||
| val pathFix = path.removePrefix("composeResources/").substringAfter("/") |
There was a problem hiding this comment.
I find this line too dangerous. It mixes removePrefix and substringAfter that is hard to predict a final result:
"composeResources/foo/bar/baz" -> "bar/baz"
"/foo/bar/baz" -> "foo/bar/baz"
"/composeResources/foo/bar/baz" -> "composeResources/foo/bar/baz"
Could you please refactor it to use Path API and add a comment here what and why was fixed, please.
There was a problem hiding this comment.
@terrakok Thanks, I understand this is dangerous. However, as far as I know there is no path API that can be used for this on macOS. I did however move this into a separate function with some comments. Also improved the code a bit so now it checks that no slash prefix is present and it only uses substringAfter. What do you think about this?
Remember that this code only fixes existing implementation when running macOS app using executable binary, and is temporary until resources are actually bundled in the executable binary. I guess this was added in the past for easy debugging. This code is not required for the framework binary.
|
We will merge the changes a bit later (at the moment we stabilize for 1.8.0). |
|
I reverted the commit because there is a problem with Skiko publication https://youtrack.jetbrains.com/issue/CMP-3859 |
|
This PR was reverted, see new PR: #5219 |
See old PR for details: #5169 ~~Requires skiko update (JetBrains/compose-multiplatform-core#1804
This PR fixes compose resources for native macOS app (see currently broken chat app using runDebugExecutableMacosArm64). This PR also adds support for embedding resources into native macOS framework. Similar to how it works on iOS. This allows using the macOS compose code in an Xcode project. Just like iOS, using kotlin code as framwork. Then call the main function from the Xcode project to run the compose app. #5219 #5169 ## Testing - For executable: run chat app using runDebugExecutableMacosArm64 - For framework: Tested by adding macOS support to the iOS Xcode project in chat app (not in this PR). Can add the sample app if needed. There were also tests added to verify this. ## Release Notes N/A
2 participants
First commit fixes compose resources for native macOS app (see currently broken chat app using
runDebugExecutableMacosArm64).Second commit adds support for embedding resources into native macOS framework. Similar to how it works on iOS. This allows using the macOS compose code in an Xcode project. Just like iOS, using kotlin code as framwork. Then call the main function from the Xcode project to run the compose app.
Testing
First commit: run chat app using
runDebugExecutableMacosArm64Second commit: Tested by adding macOS support to the iOS Xcode project in chat app (not in this PR). Can add the sample app if needed.