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

Fixes flickering when the same URL is set to the LottieAnimationView #2328

Closed
wants to merge 1 commit into from

Conversation

chggr
Copy link

@chggr chggr commented Jul 3, 2023

When the same URL is set in a LottieAnimationView multiple times, the view continuously clears and resets the composition from the cache resulting in flickering. This PR fixes the flickering by adding the currently loaded URL as part of the view state. If the same URL is set multiple times, the LottieAnimationView now knows not to reload the composition again and again and thus avoid flickering.

Fixes #2326

When the same URL is set in a LottieAnimationView multiple times, the view
continuously clears and resets the composition from the cache resulting in
flickering. This change fixes the flickering by adding the currently loaded
URL as part of the view state. If the same URL is set multiple times, the
LottieAnimationView now knows not to reload the composition again and again
and thus avoid flickering.
@chggr
Copy link
Author

chggr commented Jul 9, 2023

Hi, I hope you are well. Could I please get some feedback on this PR? Many thanks!

@gpeal
Copy link
Collaborator

gpeal commented Jul 10, 2023

I think the same issue would happen with the same asset or raw res, right? I wonder if there is a more generic way that would handle all of these cases. Maybe something like getting the LottieComposition or null from LottieCompositionFactory and if it exists and is the same instance as the one set on the field in LottieAnimationView then noop. What do you think?

@chggr
Copy link
Author

chggr commented Jul 10, 2023

Thanks so much Gabriel for taking the time to review!

You are absolutely right, looking at the code the same issue could actually happen with any type of resource (url, json, raw res, etc). It seems that the underlying LottieDrawable already short-circuits setting the same composition twice (see here). We also have code within the LottieAnimationView to make sure the view is not invalidated / redrawn if the same composition is set twice (see here).

So I believe the real issue is that we are eagerly clearing the LottieAnimationView on setCompositionTask(), which results in that flickering. I have created another PR 2333 that avoids eagerly clearing the view but instead clears only after there is a load failure. Let me know your thoughts. I believe the only drawback of this approach is that the old composition will be visible until the composition task completes. But that is preferable from a view flickering standpoint.

@chggr
Copy link
Author

chggr commented Jul 11, 2023

Hi Gabriel, thanks for all your help so far! Would it make sense to proceed with this change to fix the issue for URL resources and revisit in the future to fix the issue more broadly for other resource types as well? If yes, then we can merge this PR.

I see there is a validation check failure, but I don't believe it is due to the changes in this PR. It seems more like an environment set up issue (i.e. EW_API_TOKEN is missing). What would be the best way to resolve this issue?

@github-actions
Copy link

Snapshot Tests
API 23: Report Diff
API 31: Report Diff

@gpeal
Copy link
Collaborator

gpeal commented Jul 11, 2023

Hi Gabriel, thanks for all your help so far! Would it make sense to proceed with this change to fix the issue for URL resources and revisit in the future to fix the issue more broadly for other resource types as well? If yes, then we can merge this PR.

I see there is a validation check failure, but I don't believe it is due to the changes in this PR. It seems more like an environment set up issue (i.e. EW_API_TOKEN is missing). What would be the best way to resolve this issue?

CI should be working again

@gpeal
Copy link
Collaborator

gpeal commented Jul 11, 2023

Hi Gabriel, thanks for all your help so far! Would it make sense to proceed with this change to fix the issue for URL resources and revisit in the future to fix the issue more broadly for other resource types as well? If yes, then we can merge this PR.

I think the final fix may look fairly different than this. I'm going to explore a solution that is more robust and based on the cache keys for LottieCompositionTask so I'd rather not land this in the meantime.

@gpeal
Copy link
Collaborator

gpeal commented Dec 30, 2023

Closing in favor of #2441

@gpeal gpeal closed this Dec 30, 2023
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.

Setting the same URL in LottieAnimationView multiple times results in flickering
3 participants