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

Don't cache dpScale in Utils #1915

Merged
merged 1 commit into from
Dec 28, 2021
Merged

Don't cache dpScale in Utils #1915

merged 1 commit into from
Dec 28, 2021

Conversation

eartle
Copy link
Contributor

@eartle eartle commented Oct 4, 2021

It's possible for the screen density to change when moving between windows or screens. In this case lottie renders animations the wrong size. Android already does a pretty good job of caching and updating the system context so just let it do it's job.

I am currently relying on reflection to clear this field on a configuration change and I'd of course like to remove that.

I haven't had time to come up with a generic repro, but I thought I'd open this anyway. Sorry.

It's possible for the screen density to change when moving between windows or screens. In this case lottie renders animations the wrong size. Android already does a pretty good job of caching and updating the system context so just let it do it's job.
@gpeal
Copy link
Collaborator

gpeal commented Oct 4, 2021

@eartle why does screen density change in between screens?

@eartle
Copy link
Contributor Author

eartle commented Oct 4, 2021

@gpeal because different screens may have different densities. You can set this up in an emulator here

Screenshot 2021-10-04 at 15 29 37

@eartle
Copy link
Contributor Author

eartle commented Oct 4, 2021

And here is the bug. Open keyboard on first screen looks good.

Screenshot 2021-10-04 at 15 39 28

Open keyboard on second screen and you'll notice the two lottie animation views on the keyboard toolbar (the i and the three dots) are drawn the wrong size.

Screenshot 2021-10-04 at 15 41 09

@gpeal
Copy link
Collaborator

gpeal commented Oct 4, 2021

@eartle Thanks for that. I'll have to benchmark this function on real devices because it might get called hundreds or thousands of times. It might be necessary to do something slightly smarter here.

This also won't solve all cases because a lot of the dp conversions are done at parse time.

@eartle
Copy link
Contributor Author

eartle commented Oct 4, 2021

@gpeal We're also calling LottieCompositionFactory.clearCache on configuration change which should hopefully deal with everything else. From code inspection is looks like the Context and DisplayMetrics are already cached so I'm hopefully about the benchmarking, but you never know. Thanks!

Copy link

@Mbrittny Mbrittny left a comment

Choose a reason for hiding this comment

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

@gpeal gpeal merged commit a51606c into airbnb:master Dec 28, 2021
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.

3 participants