-
-
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
Changes from 1 commit
409a0f1
db41602
6849560
d024c24
fb6c529
c49288b
46dff9c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,7 +14,7 @@ import AppKit | |
|
||
// MARK: - DotLottieImageProvider | ||
|
||
/// Provides an image for a lottie animation from a provided Bundle. | ||
/// An image provide that loads the images from a DotLottieFile into memory | ||
calda marked this conversation as resolved.
Show resolved
Hide resolved
|
||
class DotLottieImageProvider: AnimationImageProvider { | ||
|
||
// MARK: Lifecycle | ||
|
@@ -38,12 +38,12 @@ class DotLottieImageProvider: AnimationImageProvider { | |
let filepath: URL | ||
|
||
func imageForAsset(asset: ImageAsset) -> CGImage? { | ||
imageCache.object(forKey: asset.name as NSString) | ||
images[asset.name] | ||
} | ||
|
||
// 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. If we store the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When loading a dot lottie file, we currently:
I think deleting the temporary unzipped files after loading them is a good idea because:
That means that we don't have a way to gracefully reload the images if they get removed from the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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? |
||
|
||
private func loadImages() { | ||
filepath.urls.forEach { | ||
|
@@ -52,14 +52,14 @@ class DotLottieImageProvider: AnimationImageProvider { | |
let data = try? Data(contentsOf: $0), | ||
let image = UIImage(data: data)?.cgImage | ||
{ | ||
imageCache.setObject(image, forKey: $0.lastPathComponent as NSString) | ||
images[$0.lastPathComponent] = image | ||
} | ||
#elseif os(macOS) | ||
if | ||
let data = try? Data(contentsOf: $0), | ||
let image = NSImage(data: data)?.lottie_CGImage | ||
{ | ||
imageCache.setObject(image, forKey: $0.lastPathComponent as NSString) | ||
images[$0.lastPathComponent] = image | ||
} | ||
#endif | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. Removed this property in favor of a |
||
} |
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