-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
[Compose] Breaking Change: Major Compose API Refactor #1793
Conversation
7c2967e
to
1387e8a
Compare
1387e8a
to
d671309
Compare
48fb2aa
to
8f7429a
Compare
*/ | ||
suspend fun animateLottieComposition( | ||
composition: LottieComposition?, | ||
progress: MutableState<Float>, |
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.
@lelandrichardson is it okay for this to take MutableState<Float>
or should it be a MutableStateFlow
? The semantics of this API works nicely with MutableState
but it is rare to see a third party library use MutableState
in a public API.
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.
If Leland hasn't gotten back to you yet maybe we can ping him somewhere else. I think using a MutableState
here works well and is more Compose-agnostic than a StateFlow
and also a little easier to understand if you're just starting out with Compose; there are more pitfalls around StateFlow
.
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 MutableState
seems to have two sources of truth. I'm guessing the intention here is to support seeking while playing. Since when progress
is mutated outside of this suspend fun, it effectively resets the initialProgress of the animation, I wonder if it would more clear to have separate concepts for initialProgress vs. animatedProgress.
BTW, what's the recommended way seek with gesture?
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.
@doris4lt you are correct. It does, in fact, have 2 sources of truth so you can adjust the progress while it animates. When you do that, it snaps to that progress within the current repeatCount. This is how we achieve playing + dragging the seek bar in the sample app. Does this seem like a reasonable approach to do that?
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.
The multiple sources of truth may be error-prone, as it's hard to track down a bug when something goes wrong. At the same time, it's limiting - using MutableState
here only allows devs to change progress. If they were to imperatively control other behaviors of the animation, it needs to be done at an entirely separate place. I can imagine scenarios where when changing the progress with a gesture, devs may prefer the animation to not auto-play until finger release.
I would create a set of imperative suspend fun
APIs for various controls that would affect animation's lifecycle, such as changing the starting progress of a run, stopping the animation, snapping to a progress without running (i.e. seek only), etc. That way devs could control the animation in one LaunchedEffect.
For the multiple sources of truth, I would recommend writing the current progress of the animation to a different animation state object, along with other animation related states (e.g. whether it's playing, iteration #, etc). They could read only. I assume the reason for writing the animation progress back to the MutableState is so that devs could inspect the current progress of the animation and make decisions accordingly. Though the progress alone may not be sufficient for such a decision. Hence the recommendation for a separate state object altogether. :-)
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.
@doris4lt I think you raise a lot of valid points. However, I can't seem to get an imperative corutines API to come together in a way I'm happy with. Here are 3 branches in which I tried some different approaches:
https://github.com/airbnb/lottie-android/commits/gpeal/compose-min-max-frame-imperative-api
https://github.com/airbnb/lottie-android/commits/gpeal/compose-min-max-frame-imperative-suspend-api-2
https://github.com/airbnb/lottie-android/commits/gpeal/compose-min-max-frame-imperative-api-3
I'd love another set of eyes or ideas for an API that works. In general, the dual-ownership aspect of these animations made most of these APIs really tricky/confusing.
Do you think there are any major issues with these APIs as they are? If not, I'd be open to adding additional animation APIs in the future now that the base LottieAnimation composable just takes a raw progress float.
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.
So I finally had a chance to look through this, sorry for taking so long!
Awesome! It took me a bit to get used to the new API surface, but looking at the samples it makes a lot of sense and is easy to use. I haven't gotten around to trying a snapshot of this in my side project yet but I'm planning to do that this week.
Compose-wise, I didn't see anything major! Amazing PR :)
* Numbers between 0 and 1 will slow it down. Numbers less than 0 will play it backwards. | ||
* @param repeatCount The number of times the animation should repeat before stopping. It must be | ||
* a positive number. [Integer.MAX_VALUE] can be used to repeat forever. | ||
* @param onRepeat An optional callback to be notified every time the animation repeats. Return whether |
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.
Should onRepeat
return a Boolean
to indicate whether the animation should continue to repeat?
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.
Good catch. I original had this functionality but removed it because the suspend API was more useful for its original purpose. Do you think it's worth it to keep it around? It'll have the side effect of awkwardly dangling true
at the end of 99% of the onRepeat lambdas.
*/ | ||
suspend fun animateLottieComposition( | ||
composition: LottieComposition?, | ||
progress: MutableState<Float>, |
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.
If Leland hasn't gotten back to you yet maybe we can ping him somewhere else. I think using a MutableState
here works well and is more Compose-agnostic than a StateFlow
and also a little easier to understand if you're just starting out with Compose; there are more pitfalls around StateFlow
.
lottie-compose/src/main/java/com/airbnb/lottie/compose/animateLottieComposition.kt
Show resolved
Hide resolved
): Float { | ||
val progress = remember { mutableStateOf(0f) } | ||
val states = remember { MutableStateFlow(state) } | ||
states.value = state |
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.
SideEffect
?
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.
@jossiwolf For what, exactly?
* val compositionResult = lottieComposition(LottieCompositionSpec.RawRes(R.raw.your_animation)) | ||
* val progress = lottieTransition(state) { progress -> | ||
* val composition = compositionResult.await() | ||
* when (state) { |
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'm not quite sure I understand this example😅 Where is the state
mutated?
Should the animate
lambda return a LottieComposition
?
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.
Upon second look, I got now that this is using the mutable state APIs. Just from reading this sample code, it wasn't quite clear to me how it worked. What do you think about returning a LottieComposition
from animate
instead?
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.
@jossiwolf Not sure I follow. animateLottieComposition
here actually returns Unit. It suspends during the animation and updates the progress MutableState passed in.
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.
@jossiwolf It is a little bit complicated but there is a transition example further down this PR that might clarify things.
@Composable | ||
fun animateLottieComposition( | ||
composition: LottieComposition?, | ||
isPlaying: Boolean = true, |
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.
Have you considered playMode vs. seekMode here, to avoid having isPlaying = true after animation is finished? :-)
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.
@doris4lt what would that look like, exactly?
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 seem to have misunderstood what isPlaying
is for. Sorry about the confusion... Upon reading the impl below, it is for pausing/resuming the animation.
This really isn't a big deal. But I'd gravitate towards: mode: PlayMode = Play
, where enum class PlayMode { Play, Paused }
over isPlaying = true, since isPlaying implies the animation is ongoing and unfinished. Alternatively, you could consider isPaused
, isPaused = false
doesn't imply the animation is finished or otherwise.
Looking forward to this overhaul, good job guys! |
…iple ways to do the same thing
Closing in favor of #1827 |
This PR is a major update to the Lottie Compose APIs.
Notable changes:
LottieAnimationState
no longer exists.LottieAnimation
composable now just takes a composition, progress as a Float, and base properties such as ImageAssetDelegate, mergePaths, etc.animateLottieComposition()
function.lottieComposition()
andanimateLottieComposition()
.lottieTransition()
function which lets you set up animations for different states. It also makes it trivial to ensure that the current state plays to completion before starting the next one which used to be very challenging.The next set of APIs after this PR will be dynamic properties.
Manual Progress
Animating Progress
Retries
There is now a simple way to retry animations that fail to load.
Transitions
This also includes the ability to create transitions where different states map to different compositions or clip specs within an animation: