Add snapshot tests for dot lottie animations, and other cleanup #1794

calda · 2022-11-01T17:51:21Z

#1785 added support for dot lottie animations. This PR adds snapshot tests for these animations, plus other cleanup for the changes in that PR.

calda · 2022-11-01T17:51:37Z

Lottie.xcodeproj/project.pbxproj

@@ -2658,7 +2658,7 @@
 				CODE_SIGN_STYLE = Automatic;
 				CURRENT_PROJECT_VERSION = 1;
 				GENERATE_INFOPLIST_FILE = YES;
-				IPHONEOS_DEPLOYMENT_TARGET = 11.0;
+				IPHONEOS_DEPLOYMENT_TARGET = 15.0;


Bumped the test target to 15.0 so we can use async/await in the tests

calda · 2022-11-01T17:52:10Z

Sources/Private/Model/DotLottie/DotLottieImageProvider.swift

  }

  // MARK: Private

-  private var imageCache: NSCache<NSString, CGImage> = .init()
+  private var images = [String: CGImage]()


Updated this to use a dictionary instead of a cache, since this data isn't something that can be recomputed once evicted from a cache

If we store the filepath.url couldn't we recover from a memory pressure eviction? It seems risky to have a bunch of CGImages in memory and not be able to release them on memory pressure warning as it may lead to apps being jetsam'd

When loading a dot lottie file, we currently:

decompress the zip archive to a temporary directory

load the file contents into memory

delete the files in the temporary directory

I think deleting the temporary unzipped files after loading them is a good idea because:

I think we don't want to be leaving behind a large number of files in the host app's temporary directory. The OS cleans these up in theory but I'm not sure we should rely on that.

Since the OS can clean up those files at any time in theory, I don't think its safe to assume that they will still exist at any point in the future

That means that we don't have a way to gracefully reload the images if they get removed from the NSCache, since the filepath URLs are no longer valid.

I suppose we could re-decompress the zip archive to reload the images if necessary after they're evicted from the cache, but that functionality doesn't current exist.

Gotcha, sounds good. Perhaps we could add a note about adding that behavior if needed?

calda · 2022-11-01T17:55:44Z

Sources/Public/DotLottie/DotLottieFileHelpers.swift

+  /// - Parameter bundle: The bundle in which the lottie is located. Defaults to `Bundle.main`
+  /// - Parameter subdirectory: A subdirectory in the bundle in which the lottie is located. Optional.
+  /// - Parameter dotLottieCache: A cache for holding loaded lotties. Defaults to `LRUDotLottieCache.sharedCache`. Optional.
+  @available(iOS 13.0, macOS 10.15, tvOS 13.0, *)


Added async variants for all of these methods

calda · 2022-11-01T18:00:47Z

fyi @eharrison

calda · 2022-11-01T18:01:39Z

Sources/Public/Animation/LottieAnimation.swift

@@ -157,7 +157,4 @@ public final class LottieAnimation: Codable, DictionaryInitializable {
  /// Markers
  let markers: [Marker]?
  let markerMap: [String: Marker]?
-
-  /// DotLottie configuration to setup the player
-  var dotLottieConfiguration: DotLottieConfiguration?


Removed this property in favor of a DotLottieFile.Animation type that includes a LottieAnimation and DotLottieConfiguration

…nimations

calda · 2022-11-01T18:19:30Z

Sources/Public/Animation/LottieAnimationHelpers.swift

+  /// - Parameter animationCache: A cache for holding loaded animations. Defaults to `LottieAnimationCache.shared`. Optional.
+  ///
+  @available(iOS 13.0, macOS 10.15, tvOS 13.0, *)
+  public static func loadedFrom(


I added async methods for loading DotLottieFiles so figured we may as well also add this one for remote LottieAnimations

erichoracek

@calda Good stuff! LGTM with a few suggestions/question

Sources/Private/Model/DotLottie/DotLottieImageProvider.swift

Tests/AnimationKeypathTests.swift

erichoracek · 2022-11-02T17:00:46Z

Sources/Private/Model/DotLottie/DotLottieImageProvider.swift

  }

  // MARK: Private

-  private var imageCache: NSCache<NSString, CGImage> = .init()
+  private var images = [String: CGImage]()


If we store the filepath.url couldn't we recover from a memory pressure eviction? It seems risky to have a bunch of CGImages in memory and not be able to release them on memory pressure warning as it may lead to apps being jetsam'd

erichoracek · 2022-11-02T17:10:57Z

Sources/Public/Animation/LottieAnimationHelpers.swift

+  /// - Parameter animationCache: A cache for holding loaded animations. Defaults to `LottieAnimationCache.shared`. Optional.
+  ///
+  @available(iOS 13.0, macOS 10.15, tvOS 13.0, *)
+  public static func loadedFrom(


erichoracek · 2022-11-02T17:15:20Z

Sources/Public/DotLottie/DotLottieFileHelpers.swift

-    closure: @escaping DotLottieLoadClosure,
-    dotLottieCache: DotLottieCacheProvider? = DotLottieCache.sharedCache)
+    dotLottieCache: DotLottieCacheProvider? = DotLottieCache.sharedCache,
+    handleResult: @escaping (Result<DotLottieFile, Error>) -> Void)
  {
    DispatchQueue.global().async {


Seems like if you wanted you could potentially check for task cancellation in these methods via Task.isCancelled to support async cancellation

Co-authored-by: Eric Horacek <eric.horacek@airbnb.com>

…nb#1794)

calda commented Nov 1, 2022

View reviewed changes

Add snapshot tests for dot lottie animations, and other cleanup

409a0f1

calda force-pushed the cal--dot-lottie-tests branch from 5a8bda5 to 409a0f1 Compare November 1, 2022 17:55

calda commented Nov 1, 2022

View reviewed changes

calda added 2 commits November 1, 2022 11:06

make tests pass

db41602

While we're here, also add an async method for loading remote LottieA…

6849560

…nimations

calda commented Nov 1, 2022

View reviewed changes

erichoracek approved these changes Nov 2, 2022

View reviewed changes

calda and others added 2 commits November 2, 2022 11:19

Update AnimationKeypathTests.swift

d024c24

Apply suggestions from code review

fb6c529

Co-authored-by: Eric Horacek <eric.horacek@airbnb.com>

calda enabled auto-merge (squash) November 2, 2022 18:20

lint

c49288b

calda disabled auto-merge November 2, 2022 20:00

Update DotLottieImageProvider.swift

46dff9c

calda enabled auto-merge (squash) November 2, 2022 21:38

calda merged commit 0c43942 into master Nov 2, 2022

calda deleted the cal--dot-lottie-tests branch November 2, 2022 22:04

calda mentioned this pull request Nov 3, 2022

Update to latest version of Airbnb style formatter plugin #1797

Merged

calda added a commit that referenced this pull request Nov 28, 2022

Add snapshot tests for dot lottie animations, and other cleanup (#1794)

0a9d2fc

calda added a commit that referenced this pull request Dec 1, 2022

Add snapshot tests for dot lottie animations, and other cleanup (#1794)

ef95d34

iago849 pushed a commit to atteamapps/lottie-ios that referenced this pull request Feb 8, 2024

Add snapshot tests for dot lottie animations, and other cleanup (airb…

37e787c

…nb#1794)

MoroziOS pushed a commit to MoroziOS/tmg-lottie-ios that referenced this pull request May 22, 2024

Add snapshot tests for dot lottie animations, and other cleanup (airb…

684f236

…nb#1794)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add snapshot tests for dot lottie animations, and other cleanup #1794

Add snapshot tests for dot lottie animations, and other cleanup #1794

calda commented Nov 1, 2022

calda Nov 1, 2022

calda Nov 1, 2022

erichoracek Nov 2, 2022

calda Nov 2, 2022

calda Nov 2, 2022

erichoracek Nov 2, 2022

calda Nov 1, 2022

calda commented Nov 1, 2022

calda Nov 1, 2022

calda Nov 1, 2022

erichoracek Nov 2, 2022

erichoracek left a comment

erichoracek Nov 2, 2022

erichoracek Nov 2, 2022

erichoracek Nov 2, 2022

Add snapshot tests for dot lottie animations, and other cleanup #1794

Add snapshot tests for dot lottie animations, and other cleanup #1794

Conversation

calda commented Nov 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calda commented Nov 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erichoracek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment