-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Add video support to AsyncImageKit #25000
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
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
File renamed without changes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| import UIKit | ||
| import Foundation | ||
| import Collections | ||
|
|
||
| @ImageDownloaderActor | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 suggest to store these images in the existing
URLCache(seeurlSessionWithCache). I recently usedURLCachestore storing thumbnails for Gutenberg patterns, and it worked pretty well – it doesn't have to be a network response. You can always re-create a preview fromAVURLAssetif needed. If these are stored on disk with no cleanup mechanism, it will leave lingering files on disk increasing the space the app takes on disk.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.
The cleanup mechanism is that the system will purge anything in the
Cachesdirectory if it needs to, and #24972 adds a cache cleaning mechanism that can do it on user request. There's no such great way to do this withURLCachebecause under the hood it's a sqlite DB in~/Library/Caches/com.automattic.jetpack/org.automattic.ImageDownloaderfor the in-memory DB and stores its files in the same directory. Because that's all managed byurlsessiondI'm not sure we can just erase it?I'm not sure what we lose by storing the files in
~/Library/Caches/image-cacheaside from the automatic cache eviction based on size? But again, if the system needs the space it'll just take it back?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.
Just realized we could call
ImageDownloader.clearURLSessionCache()which is presumably thread-safe so never mind that part. 🤦It'd be weird/kinda annoying to work it into the cache clearing stuff but it's certainly doable.
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.
The longer I look at this the less I feel like
URLCacheshould be used as a general-purpose disk cache?If I want to store something in it, I need to use a
URLResponsealongside my data, but do I also need to manually add fake HTTP headers so thatURLCachewill store it indefinitely? If I just use an emptyURLResponsewill it store the results at all? Storing bytes on-disk seems much more straightforward in comparison tbhThere 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'd be happy to have a custom disk cache with LRU cleanup as an alternative. If you'd like to address it separately, that's OK.
It's only the last resort. It's best to be a good citizen and not waste space by setting a limit and cleanup up automatically.