-
Notifications
You must be signed in to change notification settings - Fork 41
refactor: Preparing core module for Compose support #198
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
Conversation
feat: Compose global snapshot handling in render loop
| [versions] | ||
| agp = "8.12.3" | ||
| compose = "1.9.3" | ||
| compose-mini = "0.0.1" |
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.
This is the dependency I mentioned for some compose helpers, repo here: https://github.com/0ffz/compose-mini. I've tried to split things into modules where it makes sense and support all the current targets, the only thing it's necessary for in the core module is a cross platform nanoTime implementation which I've included in the runtime module, so I'd like to hear your preferences:
- I can split just nanoTime into its own module under this repo or leave it as is (the runtime just contains some helpers for creating a composition)
- I noticed Kool already has a
SystemClockimplementation though this doesn't expose nanoseconds. I could update its implementation, it's not too bad for jvm/web but there are a lot of native implementations if Kool ever adds those targets.
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.
I think adding it to Time should be fine. I think SystemClock actually isn't really needed anymore since Clock.System made it it to stdlib. So Time.kt could now look something like this:
val precisionTime: Double get() = nanoTime / 1_000_000_000.0
val nanoTime: Long get() {
val now = Clock.System.now()
return now.epochSeconds * 1_000_000_000L + now.nanosecondsOfSecond
}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.
Updated the implementation to use this and removed the dependency in core module 👍
kool-core/src/commonMain/kotlin/de/fabmax/kool/modules/ui2/ScrollPane.kt
Show resolved
Hide resolved
kool-core/src/commonMain/kotlin/de/fabmax/kool/modules/ui2/UiModifier.kt
Show resolved
Hide resolved
| * Any blocks waiting for a new frame will run immediately when the frame is emitted, then switch | ||
| * to their parent context to return the result. | ||
| */ | ||
| val frameClock = BroadcastFrameClock() |
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.
Need UI surfaces to be able to access this in the compose module, not sure if keeping it in Time is the best since this may feel like a redundant way to listen to new frames (given we have a flow), I've tried to explain it in the KDoc.
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.
Hmm yeah seems a bit redundant indeed. I think I would prefer it to be in a more UI specific place and / or maybe have it renamed to something like composeFrameClock or so.
Another option might be to somehow merge it with frameFlow? Not sure if that's actually possible though
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.
I think it should be doable with just the flow, but there's some extra work to make sure the collection gets dispatched in the right order (it might just mean calling KoolDispatchers.Frontend.executeDispatchedTasks() twice), as well as making sure the value is emitted in the right place.
I think to avoid changing too much I'll leave it with this and annotate it as internal for now.
This PR adds compose multiplatform dependencies and makes small updates to the core module to enable a future
kool-compose-uimodule to function correctly. I've left comments about some changes below where I'd like some feedback.Main changes are: