Skip to content
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

[Feature Request] Add cacheType to ImageResponse #361

Closed
Banck opened this issue Apr 26, 2020 · 8 comments
Closed

[Feature Request] Add cacheType to ImageResponse #361

Banck opened this issue Apr 26, 2020 · 8 comments
Labels

Comments

@Banck
Copy link

Banck commented Apr 26, 2020

Hello!
It will be very helpful if ImageResponse will have cacheType property, which contains .memory/.disk/nil. It will help to recognize was this image got from cache or not in complete block of Nuke.loadImage

@kean kean added the feature label Apr 27, 2020
@kean
Copy link
Owner

kean commented Apr 27, 2020

Hi, I was considering adding a feature like that, I think I have something similar in backlog. I was also thinking adding a set of sources to fetch the image from in the request. Both of these features could use the same enum.

The reason why I haven't implemented it yet is that there were no demand for this feature. Could you please give an example where you would need to know whether the image was returns from cache or not?

@Banck
Copy link
Author

Banck commented Apr 28, 2020

Here is an example:
imagine you have base64 image for preview, which you will blur while the high quality image is not downloaded. Then, when high quality image will be downloaded, you will fade out the blur view above your image.
So, if there is an image in your cache, you don't need to fade out the blur view with animation, just hide it. Otherwise if you scroll the cells with these images you will see that blur always fade out with animation, although the picture is already there (or not in scroll view, just when screen appear with image, for example).

So the code looks something like that:

Nuke.loadImage(with: coverImage.imageURL, options: .init(placeholder: coverImage.placeholderImage), into: coverImageView) { [weak self] (result) in
                switch result {
                case .success(let response):
                    let duration = response.urlResponse == nil ? 0 : 0.3
                    self?.coverImageBlurView.animate(.fadeOut, .duration(duration))
                case .failure:
                    break
                }
            }

In this code I'm checking the response.urlResponse for nil, which is not correctly. If there will be the cacheType we can check it

@kean
Copy link
Owner

kean commented Apr 28, 2020

I use a slightly different approach in Nuke to implement this sort of thing. If you look into ImageViewExtensions.swift, you can see that before loading the image, I perform a quick memory check:

// Quick synchronous memory cache lookup
if let image = pipeline.cachedImage(for: request) {
    let response = ImageResponse(container: image)
    // Display image without animation.

This makes sure no animations are run, and no frames are skipped.

Would that work for you?

@Banck
Copy link
Author

Banck commented Apr 29, 2020

As I know it doesn't work if there is no cache in memory, but there is in the disk.

@kean
Copy link
Owner

kean commented Apr 29, 2020

I'm wondering, how do you retrieve images from disk cache?

@Banck
Copy link
Author

Banck commented Apr 29, 2020

First of all I've inited like that:


Nuke.ImagePipeline.shared = ImagePipeline {
           $0.dataCache = try! DataCache(name: Bundle.main.bundleIdentifier!)
}

Then I download image like that:

Nuke.loadImage(with: coverImage.imageURL, options: .init(placeholder: coverImage.placeholderImage), into: coverImageView) { [weak self] (result) in
...
}

So if there is no cache in memory, but is on disk pipeline.cachedImage(for: request) will return nil, but if there is cache on the disk, Nuke.loadImage returns image from cache.
So that's why completion block in Nuke.loadImage should return cacheType , like SDWebImage do

@kean
Copy link
Owner

kean commented Apr 30, 2020

Oh, so the goal is not run animations when the response arrives quickly (e.g. from disk cache)? I think the code that you currently have is fine.

I'm not against adding this new API, but it is probably not something to prioritize as there is already an existing way to do that, and it's quite niche requirement.

Many people rely on URLCache. For URLCache there is no (clear) way to tell whether the request came form the network or from cache. A better check would probably be: did the response come within 300ms, if yes, don't run animations.

This was referenced May 9, 2021
@kean
Copy link
Owner

kean commented May 17, 2021

cacheType, and many other caching improvements, are available in Nuke 10 Beta.

@kean kean closed this as completed May 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants