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

Add iOS targets #102

Closed
wants to merge 1 commit into from
Closed

Add iOS targets #102

wants to merge 1 commit into from

Conversation

Vichy97
Copy link
Contributor

@Vichy97 Vichy97 commented Sep 13, 2024

Add support for iOS targets

Add support for iOS targets
Comment on lines +26 to +28
iosArm64()
iosX64()
iosSimulatorArm64()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not familiar with publishing KMP libraries for iOS. I think we may need the equivalent to publishLibraryVariants but for iOS?

Copy link
Owner

Choose a reason for hiding this comment

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

Yea I'm not sure either. Let's try merging this and seeing if a SNAPSHOT version gets published correctly for iOS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any ideas on the failures on this PR? The error mentions a missing android target, but I did not modify that 🤔

Copy link
Owner

Choose a reason for hiding this comment

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

Sorry I'm just seeing this. Let me try building the project on my machine.

Comment on lines +26 to +28
iosArm64()
iosX64()
iosSimulatorArm64()
Copy link
Owner

Choose a reason for hiding this comment

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

Yea I'm not sure either. Let's try merging this and seeing if a SNAPSHOT version gets published correctly for iOS.

@wang222222222
Copy link

Hi any update here ?

@saket
Copy link
Owner

saket commented Nov 25, 2024

Closing this PR as I ended up merging the commit manually. Git wasn't letting me force-push to your branch @Vichy97.

@saket saket closed this Nov 25, 2024
@saket
Copy link
Owner

saket commented Nov 28, 2024

@wang222222222 @Vichy97 iOS targets are now available with 0.15.0-SNAPSHOT. Would either of you be able to try it out?

@Vichy97
Copy link
Contributor Author

Vichy97 commented Nov 28, 2024

@saket https://mvnrepository.com/artifact/me.saket.telephoto/zoomable I don't the snapshot version on Maven.

@saket
Copy link
Owner

saket commented Nov 28, 2024

Snapshot builds are available in the snapshot repository:

maven("https://oss.sonatype.org/content/repositories/snapshots/")

@Vichy97
Copy link
Contributor Author

Vichy97 commented Nov 28, 2024

Hmm it doesn't appear to have ios artifacts. was able to use it in Android using the snapshot though.

@Vichy97
Copy link
Contributor Author

Vichy97 commented Nov 28, 2024

I think you maybe need more changes than just adding the iOS target that I had in my pr, but I'm honestly not sure what else you need for the iOS targets to be uploaded.

@saket
Copy link
Owner

saket commented Nov 28, 2024

Interesting. It looks like telephoto will need to enable this option: https://kotlinlang.org/docs/multiplatform-publish-lib.html#host-requirements

@Vichy97
Copy link
Contributor Author

Vichy97 commented Nov 28, 2024

Ah that makes sense. I'm happy to try the next snapshot if you add that change! I could also try to make a PR soon

vanniktech added a commit to vanniktech/telephoto that referenced this pull request Dec 3, 2024
saket pushed a commit that referenced this pull request Dec 6, 2024
@saket
Copy link
Owner

saket commented Dec 7, 2024

@vanniktech
Copy link
Contributor

Yup works for me on iOS.

@vanniktech
Copy link
Contributor

Weirdly when I change:

-telephoto = "me.saket.telephoto:zoomable:0.15.0-SNAPSHOT"
+telephoto = "me.saket.telephoto:zoomable-image-coil3:0.15.0-SNAPSHOT"

Android Studio sync throws up:

org.gradle.api.internal.artifacts.ivyservice.TypedResolveException: Could not resolve all files for configuration ':composeApp:iosArm64CompilationDependenciesMetadata'.

So the zoomable module works on iOS, the zoomable-image-coil3, does build but Gradle Sync is broken. Is the iosArm64 variant missing there?

@saket
Copy link
Owner

saket commented Dec 8, 2024

Yep, SubSamplingImage() needs more work to make it multiplatform :(

@Vichy97
Copy link
Contributor Author

Vichy97 commented Dec 8, 2024

@saket everything seems to be working now. Thanks for doing that!

@vanniktech
Copy link
Contributor

Is SubSamplingImage required for ZoomableAsyncImage? Or would it be possible to split it up in two modules so that ZoomableAsyncImage can also be used on JVM.

@saket
Copy link
Owner

saket commented Dec 16, 2024

Yea this should be doable now. Since Coil 3.x restricts the maximum size of bitmaps, large images will not cause a crash even if they can't be sub-sampled. Time to start moving zoomable-image:core to shared code.

@vanniktech
Copy link
Contributor

I've ended up using the Modifier.zoomable() so I'm all good for now with the SNAPSHOT.

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.

4 participants