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 and MacOS support #3

Merged
merged 11 commits into from
Aug 10, 2024
Merged

Add iOS and MacOS support #3

merged 11 commits into from
Aug 10, 2024

Conversation

Nek-12
Copy link
Contributor

@Nek-12 Nek-12 commented Aug 8, 2024

  • Android app builds
  • iOS app builds
  • desktop app builds
  • screenshot
  • publishToMavenLocal passes
    ❌ published to remote - undocumented how to provide publishing credentials
    ❌ CI passes - unable to run CI from a fork without setting up the entire pipeline
Screenshot of iOS 17 device

Simulator Screenshot - iPhone 14 - 2024-08-08 at 17 19 00

Notes:

  • The project wouldn't sync because of jvmToolchain misconfiguration. The project doesn't need toolchains. I had to replace toolchains with explicit target spec to use my jdk 22 installation.
  • I added CI configuration blindly, without testing it. I cannot guarantee CI will pass first try, it only has a basic setup.
  • The project wouldn't build until I bumped AGP to a newer version and gradle to 8.9 (to use jvm 22)
  • I added some TODOs about some problems in the code base
  • I removed the native icon resource expect/actual since I didn't know what its purpose was (it wasn't documented) and I didn't know how to implement it on iOS.
  • I haven't tested macOS support as I'm not sure how to run the sample app on macOS yet, however it should work the same as iOS does.
  • I can provide basic support for maintaining the native target, but you will not be able to build or run the native target on your local machine, which will lead to the build failing on CI only. Assign an issue to me or mention me and I will take a look, but I cannot promise continued support or quick response times.

@tclement0922
Copy link
Owner

Thanks for your contribution! I just have a question: is it possible to move the isoApp directory inside testapp? I would rather separate the test app source from the rest of the code. Apart from that everything looks good to me.

About your notes:

The project wouldn't sync because of jvmToolchain misconfiguration.

I wasn't actually sure this was still needed, this was just something I added at the beginning of the project and forgot to remove/look into.

I added CI configuration blindly, without testing it.

No problems with that, everything should still work.

The project wouldn't build until I bumped AGP to a newer version and gradle to 8.9 (to use jvm 22)

That's pretty strange because everything was working correctly in the CI. Maybe this is a MacOS specific issue? AGP was just kept at an older version because IntelliJ didn't support higher that that, but this seems to no longer be the case so that's not an issue anymore.

I added some TODOs about some problems in the code base

  • Use caching or disable downloads each time the project is configured.

    I actually forgot to add the option that prevents the download if the file already exists, thanks for pointing that out!

  • Define jvm target in a single place per entire project and use here

    I don't really understand what you mean here, do you mean adding a property in gradle.properties? If so that's easily fixable.

  • remove logging from production code and use kmp-compatible logging library (e.g. kermit)

    I don't really like the idea of adding an additional dependency. Now that I looked at the (very low amount of) usages of the class, I would rather just replace it with something like expect fun printWarning.

I removed the native icon resource expect/actual since I didn't know what its purpose was

It was just a leftover of before 1.2.0, when the library didn't support Compose Resources and needed a different implementation for each platform.

I haven't tested macOS support as I'm not sure how to run the sample app on macOS yet

There is no reason it wouldn't work since you haven't added any iOS/MacOS specific function. Every platform implementation except Android's uses Skiko as the backend, so as long as Skiko behaves the same on every platform everything should work.

I can provide basic support for maintaining the native target

Thanks!

I cannot promise continued support or quick response times.

No problem with that, that is completely understandable. I'll just add a warning to the README saying that the native targets aren't as tested as the others.

@Nek-12
Copy link
Contributor Author

Nek-12 commented Aug 9, 2024

is it possible to move the isoApp directory inside testapp? I would rather separate the test app source from the rest of the code

Usually it's not done this way as it may break running the iOS app from AS, but I will try to move the dir there and see if it works. We can't get rid of the subdir because it's opened in Xcode

Maybe this is a MacOS specific issue?

It's an issue with my setup - I use a java version that was not supported by your gradle / agp versions. My problem, but a dep update wouldn't hurt I thought. I can revert that but it will involve installing another Jdk instance

I don't really understand what you mean here, do you mean adding a property in gradle.properties? If so that's easily fixable.

Yes, I was not sure how you do these kinds of things and did not want to impose my solutions (I usually create a singleton in buildSrc folder). Please set that up in any way you like and I will follow that example in the future.

I don't really like the idea of adding an additional dependency. Now that I looked at the (very low amount of) usages of the class, I would rather just replace it with something like expect fun printWarning

I do not like the idea of logging something in production and in a library. If logging is necessary, introduce a callback or an interface users can implement. By hiding logging in the internal API, you do not let any user of the library override this behavior. I am sorry for being unclear, I did not suggest adding a library to prod executable, I assumed that logging was only needed for your debugging, so suggested to use that temporarily and remove the logging calls when publishing to prod (kermit allows to do that).

It was just a leftover of before 1.2.0, when the library didn't support Compose Resources

That was also my assumption, so I hope it's not a problem I removed it.

There is no reason it wouldn't work since you haven't added any iOS/MacOS specific function

I agree, all should work fine, our team has already tested the fork in a prod-ready application and it passed qa with flying colors with the resolution of #2 especially.

@Nek-12
Copy link
Contributor Author

Nek-12 commented Aug 9, 2024

I moved the iOS app inside the /testapp. That indeed resulted in non-standard configuration, but I only had to change some xcode scripts for that to work, so it should be fine given that everyone uses provided run configuration I pushed.

@tclement0922
Copy link
Owner

I can revert that but it will involve installing another Jdk instance

No it's all good, as I said AGP was outdated because of IntelliJ restrictions, but it seems they fixed it on the latest.

I do not like the idea of logging something in production and in a library.

Most of the logging is just warnings addressed at the developers, for example when an icon doesn't exist, so it shouldn't happen in a prod build. I can remove the logging entirely if that's really bothering you.

That was also my assumption, so I hope it's not a problem I removed it.

Don't worry it's not, I would have done it sooner or later, it's just that it wasn't a priority since it's not part of the library itself.

I moved the iOS app inside the /testapp.

Thanks for that!

If it's all good for you, I'll merge this PR tomorrow, change some things and publish the new version.

@tclement0922 tclement0922 merged commit 2d52b93 into tclement0922:main Aug 10, 2024
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.

2 participants