Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Custom Cache Key for images #324

Closed
wants to merge 3 commits into from
Closed

Conversation

graphiclife
Copy link
Contributor

Added parameter customCacheKey in af_setImage in UIImageView+AlamofireImage, UIButton+AlamofireImage.
Added parameter customCacheKey in ImageDownloader.download
Added test testThatImageCanBeCachedWithACustomCacheKey

Goals ⚽

Ability to cache images based on other factors than the url, which could be necessary if the backend hosting the image has a misconfigured cache management or provides images through REST-endpoints.

Implementation Details 🚧

Added an optional cache key parameter to af_setImage-functions.

Testing Details 🔍

Added testThatImageCanBeCachedWithACustomCacheKey in UIImageViewTests .

Copy link
Member

@cnoon cnoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for putting this together @graphiclife! 🍻

Just a few minor changes before we push this through.

@@ -263,6 +263,7 @@ open class ImageDownloader {
_ urlRequest: URLRequestConvertible,
receiptID: String = UUID().uuidString,
filter: ImageFilter? = nil,
customCacheKey: String? = nil,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a docstring for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -284,7 +285,15 @@ open class ImageDownloader {
if let request = urlRequest.urlRequest {
switch request.cachePolicy {
case .useProtocolCachePolicy, .returnCacheDataElseLoad, .returnCacheDataDontLoad:
if let image = self.imageCache?.image(for: request, withIdentifier: filter?.identifier) {
var cachedImage: Image?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be a let?

strongSelf.imageCache?.add(filteredImage, withIdentifier: customCacheKey)
} else {
strongSelf.imageCache?.add(filteredImage, for: request, withIdentifier: filter?.identifier)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👌🏻

@@ -129,6 +129,7 @@ extension UIButton {
url: URL,
placeholderImage: UIImage? = nil,
filter: ImageFilter? = nil,
customCacheKey: String? = nil,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docstring?

@@ -208,6 +208,7 @@ extension UIImageView {
progressQueue: DispatchQueue = DispatchQueue.main,
imageTransition: ImageTransition = .noTransition,
runImageTransitionIfCached: Bool = false,
customCacheKey: String? = nil,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Docstring?

@cnoon
Copy link
Member

cnoon commented Sep 17, 2018

Apologies for not getting back to you sooner about this PR @graphiclife. I like this approach better than #328. I'm going to close that one out in favor of this approach.

Added parameter customCacheKey in af_setImage in UIImageView+AlamofireImage, UIButton+AlamofireImage.
Added parameter customCacheKey in ImageDownloader.download
Added test testThatImageCanBeCachedWithACustomCacheKey

* Added docstring for customCacheKey.
* 'var' -> 'let' for cachedImage
@graphiclife
Copy link
Contributor Author

@cnoon Any updates on this?

@cnoon
Copy link
Member

cnoon commented Apr 1, 2019

Not yet @graphiclife, right now we're very focused on the AF5 release and keeping AFI in sync with that. I'm not sure right now if this change will make it into AFI 4 or not.

@ricardolpd
Copy link

Hi guys, just wondering, do you have a timeline to get into master? I am having the same issue, and this approach would suffice. Is there anything I or someone could do to bring this to master?

@cnoon
Copy link
Member

cnoon commented Feb 22, 2020

Hi @graphiclife, I have squashed these changes with a few edits and added tests in deff207 and pushed the change into master. This will be released shortly as part of the 4.0.0 release. Thank you for all your hard work here and apologies for the poor turnaround time here.

Cheers. 🍻

@cnoon cnoon closed this Feb 22, 2020
@cnoon cnoon self-assigned this Feb 22, 2020
@cnoon cnoon added this to the 4.0.0 milestone Feb 22, 2020
@graphiclife
Copy link
Contributor Author

@cnoon Excellent news, thanks!

ryndinol pushed a commit to ryndinol/AlamofireImage that referenced this pull request Apr 26, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants