-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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 snapshot tests for dot lottie animations, and other cleanup #1794
Conversation
@@ -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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bumped the test target to 15.0 so we can use async/await in the tests
} | ||
|
||
// MARK: Private | ||
|
||
private var imageCache: NSCache<NSString, CGImage> = .init() | ||
private var images = [String: CGImage]() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we store the filepath.url
couldn't we recover from a memory pressure eviction? It seems risky to have a bunch of CGImage
s in memory and not be able to release them on memory pressure warning as it may lead to apps being jetsam'd
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha, sounds good. Perhaps we could add a note about adding that behavior if needed?
5a8bda5
to
409a0f1
Compare
/// - 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, *) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added async
variants for all of these methods
fyi @eharrison |
@@ -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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed this property in favor of a DotLottieFile.Animation
type that includes a LottieAnimation
and DotLottieConfiguration
/// - 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( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added async
methods for loading DotLottieFile
s so figured we may as well also add this one for remote LottieAnimation
s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@calda Good stuff! LGTM with a few suggestions/question
} | ||
|
||
// MARK: Private | ||
|
||
private var imageCache: NSCache<NSString, CGImage> = .init() | ||
private var images = [String: CGImage]() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we store the filepath.url
couldn't we recover from a memory pressure eviction? It seems risky to have a bunch of CGImage
s in memory and not be able to release them on memory pressure warning as it may lead to apps being jetsam'd
/// - 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( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
closure: @escaping DotLottieLoadClosure, | ||
dotLottieCache: DotLottieCacheProvider? = DotLottieCache.sharedCache) | ||
dotLottieCache: DotLottieCacheProvider? = DotLottieCache.sharedCache, | ||
handleResult: @escaping (Result<DotLottieFile, Error>) -> Void) | ||
{ | ||
DispatchQueue.global().async { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
#1785 added support for dot lottie animations. This PR adds snapshot tests for these animations, plus other cleanup for the changes in that PR.