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

add support for loading DotLottieFile by name and filename synchronously #1968

Merged
merged 6 commits into from
Feb 21, 2023
Merged

add support for loading DotLottieFile by name and filename synchronously #1968

merged 6 commits into from
Feb 21, 2023

Conversation

ilendemli
Copy link
Contributor

@ilendemli ilendemli commented Feb 20, 2023

We need to be able to load dotLottie files synchronously in out project. This PR enables us to create DotLottieFile directly.

Reference: #1967

@ilendemli ilendemli changed the title make init public make DotLottieFile init public Feb 20, 2023
@ilendemli ilendemli changed the title make DotLottieFile init public add support for loading DotLottieFile by name and filename synchronously Feb 20, 2023
/// Decode the lottie.
let url = URL(fileURLWithPath: filepath)
let data = try Data(contentsOf: url)
let lottie = try DotLottieFile(data: data, filename: url.deletingPathExtension().lastPathComponent)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should provide a synchronous method for loading DotLottie files. This initializer performs file system operations, like decompressing data to a temporary directly and reading various files. This would block the current thread, so we need to make sure we aren't making it too easy for folks to accidentally call this from the main thread.

One reason I think it's especially important to not provide synchronous loading methods for DotLottieFiles is because we do provide synchronous loading methods for LottieAnimations (which are less problematic to load on the main thread). If we provide synchronous methods for both I worry the difference would be too subtle.

@calda
Copy link
Member

calda commented Feb 21, 2023

@ilendemli, can you share more details about your use case here? Why do you want to load these files synchronously? Are you loading them on a background thread or on the main thread? Why can't you use the existing async methods?

@ilendemli
Copy link
Contributor Author

@calda In our case, when the app starts we show a static image of the first frame of the lottie animation as the launch screen. When the app is loaded into memory we show a splash screen which is basically the same as the launch screen but with the dotlottie loaded and animated. If we load the animation asynchronously then there is a white flash/screen until the dotlottie is loaded and displayed. I know this is blocking the main thread during the operation. But as it is during the startup process of the app, it's less noticeable. I would suggest to inform the user to use the asynchronous methods whenever possible, but in this (and a few other cases where we have to use dotlottie files) we have to use synchronous loading.

@calda
Copy link
Member

calda commented Feb 21, 2023

Intentionally doing this on the main thread seems relatively uncommon and something I think we probably don't want to "officially" support for now. I'm open to reconsidering if we get more requests for this in the future, though. Have you explored using a something like a DispatchGroup to manually block the main thread while waiting for the async operation to complete? That seems like it would work for your use case without having to change the API of Lottie itself.

/// Loads an DotLottie from a specific filepath synchronously.
/// - Parameter filepath: The absolute filepath of the lottie to load. EG "/User/Me/starAnimation.lottie"
/// - Parameter dotLottieCache: A cache for holding loaded lotties. Defaults to `LRUDotLottieCache.sharedCache`. Optional.
public static func loadedFrom(
Copy link
Member

Choose a reason for hiding this comment

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

I could also imagine being ok with merging this change if we give this method a more scary sounding name like:

Suggested change
public static func loadedFrom(
public static func loadSynchronouslyBlockingCurrentThread(

with documentation explaining that this performs file system operations like decompressing a zip archive.

What do you think?

Copy link
Contributor Author

@ilendemli ilendemli Feb 21, 2023

Choose a reason for hiding this comment

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

I would be ok with this but how about using a SynchronouslyBlockingCurrentThread namespace instead?
Edit: see my latest commit.

Copy link
Member

@calda calda left a comment

Choose a reason for hiding this comment

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

This feels reasonable, thanks.

@calda calda enabled auto-merge (squash) February 21, 2023 17:31
@calda calda linked an issue Feb 21, 2023 that may be closed by this pull request
auto-merge was automatically disabled February 21, 2023 17:52

Head branch was pushed to by a user without write access

@calda calda merged commit acd56fa into airbnb:master Feb 21, 2023
iago849 pushed a commit to atteamapps/lottie-ios that referenced this pull request Feb 8, 2024
MoroziOS pushed a commit to MoroziOS/tmg-lottie-ios that referenced this pull request May 22, 2024
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.

possibility to load dotLottie files synchronously
2 participants