onevcat / Kingfisher

A lightweight, pure-Swift library for downloading and caching images from the web.
MIT License
23.06k stars 2.64k forks source link

Resource Identifier and URL #93

Closed RuiAAPeres closed 9 years ago

RuiAAPeres commented 9 years ago

With the current API we are forced to use the URL as the resource identifier. This won't work in non-trivial scenarios, because we might want the identifier to be different from the URL. For example: creating a thumbnail out of an image.

The image itself as a specific URL (e.g. http://abc.jpg). But I want to cache both http://abc.jpg and http://abc.jpg_thumbnail (this _thumbnail is generated from the original one). The problem is when I want to fetch the thumbnail from the cache. Because if I pass http://abc.jpg_thumbnail and it's not in the cache, it will try to fetch http://abc.jpg_thumbnail which will fail. If I pass http://abc.jpg then it will never touch the thumbnail and get the full size one.

The fundamental problem is: the cache identifier and the resource's URL are coupled. A quick and easy solution would be to use a struct like this:

struct Resource {
let key : String
let resourceURL : NSURL
}

We would then pass this struct, instead of the URL. For example:

public func retrieveImageWithURL(URL: NSURL, optionsInfo: Kingfisher.KingfisherOptionsInfo?, progressBlock: Kingfisher.DownloadProgressBlock?, completionHandler: Kingfisher.CompletionHandler?) -> Kingfisher.RetrieveImageTask

to:

public func retrieveImageWithResource(resource: Resource, optionsInfo: Kingfisher.KingfisherOptionsInfo?, progressBlock: Kingfisher.DownloadProgressBlock?, completionHandler: Kingfisher.CompletionHandler?) -> Kingfisher.RetrieveImageTask

Still, for this kind of cases, we can just leave as is. There is no point on passing a Resource.

public func storeImage(image: UIImage, forKey key: String)

Likewise, when the interaction is with the ImageDownloader it's fine to use a NSURL, since the ImageDownload should be responsible only for interactions with the network.

Without seeing all the code and assuming the code respects the SOLID principles, only the KingfisherManager would have to be updated.

dmcrodrigues commented 9 years ago

Yeah I think this is really important, for example URL can contain dynamic parameters which can break the cache. I think the key can be the URL's absolute string by default but we should be able to customise it somehow to handle our specific use-case.

The approach suggested by @RuiAAPeres seems great to handle this. :+1:

RuiAAPeres commented 9 years ago

I think the key can be the URL's absolute string by default

struct Resource {
    let key : String
    let url : NSURL

    init(key: String, url: NSURL) {
        self.key = key
        self.url = url
    }

    init(url: NSURL) {
        self.init(key: url.absoluteString, url: url)
    }
}
onevcat commented 9 years ago

Hi, @RuiAAPeres @dmcrodrigues

Great suggestion for decoupling the key and URL. It is very easy to adjust current code for it. I will handle it soon.