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 dotLottie error handling #1969

Merged
merged 9 commits into from
Feb 21, 2023
Merged

Add dotLottie error handling #1969

merged 9 commits into from
Feb 21, 2023

Conversation

mkj-is
Copy link
Contributor

@mkj-is mkj-is commented Feb 21, 2023

Previously discussed in #1957.

  • Adds proper error propagation to dotLottie loading.
    • Handles manifest loading errors.
    • Handles animation loading errors.
  • Adds LottieLogger warnings to methods loading from dotLottie.
  • Removes unnecessary animationUrl property from DotLottieAnimation.
  • Unifies asset loading mechanism for dotLottie and standard .json files.

Propagating errors has actually made the error handling shorter and easier to debug.

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.

Thanks! Great improvements, appreciate it.

@@ -52,7 +52,10 @@ extension FileManager {
// MARK: - DotLottieError

public enum DotLottieError: Error {
case invalidFileFormat
Copy link
Member

Choose a reason for hiding this comment

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

Since this is public I think we need to preserve these enum cases, otherwise this would be a breaking change. Could we deprecate the existing cases instead of removing them?

Copy link
Member

Choose a reason for hiding this comment

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

Haha oops accidentally merged the PR after I finished reviewing it. I posted a follow-up PR to add back these three public enum cases: #1970

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, it makes sense to keep them until the next minor/major version is released.

return NSDataAsset(name: assetName, bundle: bundle)?.data
if let asset = NSDataAsset(name: assetName, bundle: bundle) {
self = asset.data
return
Copy link
Member

Choose a reason for hiding this comment

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

Is this necessary? It seems like it may be redundant

Suggested change
return

@calda calda merged commit e1a05ad into airbnb:master Feb 21, 2023
calda added a commit that referenced this pull request Feb 21, 2023
@mkj-is mkj-is deleted the feature/lottie-logger-for-dot-lottie branch February 21, 2023 21:01
iago849 pushed a commit to atteamapps/lottie-ios that referenced this pull request Feb 8, 2024
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
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.

None yet

2 participants