-
Notifications
You must be signed in to change notification settings - Fork 7
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
fix(cache): improve caching and deletion of images #513
Conversation
The overall changelog of this pull request, while may look "large", is rather small; there are a bunch of renames, and then updated tests (which definitely affect the total changes). |
case .move: | ||
return | ||
@unknown default: | ||
func handle(images: [Image]?) { |
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.
NIT: can we name it other than "handle"?
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.
@timc-mozilla probably, i couldn't think of a good name. do you have any suggestions?
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.
handleImageCache?
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.
@timc-mozilla ¯_(ツ)_/¯ but the thought of renaming this is surprisingly overwhelming so i'll leave it for now 😆
// Delete all orphan URLs that are not to be skipped | ||
let toDelete = orphanURLs.subtracting(skipDelete) | ||
|
||
toDelete.forEach { delete(url: $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.
@dskuza are these async methods? if not we should make them 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.
@bassrock not really… they "rely" on kingfisher code (behind the scenes and some protocols) which does things async, so there's not really anything synchronous here. the KF functions themselves aren't async, they're just performed async (using dispatch queues, i believe)
Summary
Improve how images are (un)cached when there are Image changes in Source.
References
IN-1222
Implementation Details
The usage of
ImagesController
is now simplified - we will handle Image changes when all changes have been saved, rather than on each insert / delete / etc. There is now some additional logic for when images should be downloaded:.item
)Test Steps
PR Checklist: