Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Add the LottieConfig.Builder.setEnableNetworkCache() method to completely disable internal network cache if needed #2158

Merged
merged 1 commit into from
Nov 14, 2022

Conversation

viakunin
Copy link
Contributor

@viakunin viakunin commented Nov 8, 2022

Rationale
We'd like to implement more sophisticated caching strategy for animations fetched from network. This could be implemented on the network fetcher level e.g. using the OkHttp's cache or the "stale-while-revalidate" strategy.
For this to work the internal Lottie network cache has to be completely disabled.
The option to do so is implemented in this PR.

Implementation
By default the internal cache is enabled to maintain backward compatibility.
We've added the LottieConfig.Builder.setEnableNetworkCache() method to override the default behavior.

…tely disable internal network cache if needed
@LottieSnapshotBot
Copy link

Snapshot Tests
28: Report Diff

Copy link
Collaborator

@gpeal gpeal left a comment

Choose a reason for hiding this comment

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

LGTM, just add a doc and I'll land this 😄

@@ -47,6 +49,10 @@ public static void setTraceEnabled(boolean enabled) {
}
}

public static void setNetworkCacheEnabled(boolean enabled) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add a javadoc for this? Also indicate that it will evict existing cache entries.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But L class is restricted to the library scope, and there are no other javadocs there. I've added javadocs to the publicly accessible LottieConfig.Builder.setEnableNetworkCache(), should I just copy-paste them here as well?
Also disabling the cache should not evict existing cache entries I believe, they will still be there until the explicit LottieCompositionFactory.clearCache() call with the cache enabled.

@viakunin
Copy link
Contributor Author

@gpeal also added a small docs PR here - airbnb/lottie#211

Copy link
Collaborator

@gpeal gpeal left a comment

Choose a reason for hiding this comment

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

LGTM!

gpeal pushed a commit to airbnb/lottie that referenced this pull request Nov 14, 2022
Add docs to support disabling Lottie's internal network cache, see airbnb/lottie-android#2158
@gpeal gpeal merged commit 84893f8 into airbnb:master Nov 14, 2022
@viakunin
Copy link
Contributor Author

btw @gpeal are there any plans to release a new Lottie version with all those recent changes?

@gpeal
Copy link
Collaborator

gpeal commented Nov 16, 2022

Yes, soon! You can use the SNAPSHOT release if you'd like, too.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants