Feature ImageManager for fetching image without using an image view #49
Conversation
ImageManager can fetch images in the same way as the extension on `ImageView`. It works pretty much in the exact same way except it does not use any pre or post configuration closures. If the URL already exists in the cache, that resource will be used by default, however you can opt-out of using cache by setting `useCache` to `false` at the call site. `Fetcher` is now equitable, it is used to remove fetchers when they are done doing their job in `ImageManager`. This should not affect the current implementation of Imaginary as `ImageManager` is completely separate from the image view extensions that we had (and still have).
|
@zenangst, thanks for your PR! By analyzing the history of the files in this pull request, we identified @vadymmarkov and @RamonGilabert to be potential reviewers. |
Sources/Shared/ImageManager.swift
Outdated
| } | ||
|
|
||
| // Cancel and remove all fetchers from `ImageManager`. | ||
| public func purge() { |
There was a problem hiding this comment.
purge sounds like clearing all the cache?
|
Look good to me, except the code duplication in |
|
@onmyway133 yeah, I was thinking about refactoring the extensions to use the managers internally but that is a bit more work and I didn't want to tinker too much with what already works. If the manager ends up nicely I think it can replace that implementation. Meaning that it will run through like a shared manager for all the extensions. I think that way we might be able to drop the associated objects that we have now. |
|
@vadymmarkov @onmyway133 what do you think about that idea? |
|
@zenangst yeah, I vote for that too. Then we have 1 place to change. I also vote for injecting a |
|
I think it's fine to use manager internally because it's the same code. |
| @@ -96,4 +96,11 @@ class Fetcher { | |||
| closure() | |||
| } | |||
| } | |||
|
|
|||
| static func == (lhs: Fetcher, rhs: Fetcher) -> Bool { | |||
There was a problem hiding this comment.
To remove the fetcher in the manager when they are done. I thought we need it to be equatable for us to use index(of:). Maybe we don't need that at all :)
There was a problem hiding this comment.
I was right, we need it. If we don't add it then index(of) won't work.
Sources/Shared/ImageManager.swift
Outdated
| /// - url: The URL of the image that should be fetch from the network. | ||
| /// - completion: A completion block that gets called after the network request is done. | ||
| /// - Note: The completion will get `nil` back if the request fails to fetch the image. | ||
| fileprivate func fetchFromNetwork(url: URL, useCache: Bool = true, completion: Completion? = nil) { |
There was a problem hiding this comment.
Could be just private.
ImageManager can fetch images in the same way as the extension on
ImageView. It works pretty much in the exact same way except it doesnot use any pre or post configuration closures. If the URL already
exists in the cache, that resource will be used by default, however you
can opt-out of using cache by setting
useCachetofalseat the callsite.
Fetcheris now equitable, it is used to remove fetchers when they aredone doing their job in
ImageManager.This should not affect the current implementation of Imaginary as
ImageManageris completely separate from the image view extensionsthat we had (and still have).