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

Allow exclusive use of .cacheOriginalImage file instead of final file #810

Closed
3 tasks done
ethansinjin opened this issue Nov 20, 2017 · 4 comments
Closed
3 tasks done

Comments

@ethansinjin
Copy link
Contributor

ethansinjin commented Nov 20, 2017

Check List

Thanks for considering to open an issue. Before you submit your issue, please confirm these boxes are checked.

Issue Description

UIButtons of type .system use a property called imageRenderingMode of their image to determine if the image should be used in template or original mode.
I have created a custom ImageProcessor, RenderingModeImageProcessor, to set the rendering mode of an image.
I am trying use Kingfisher with a UIButton as follows:

let myProcessor = RoundCornerImageProcessor(cornerRadius: 18, targetSize: CGSize(width: 36, height: 36), roundingCorners: .all, backgroundColor: nil) >> RenderingModeImageProcessor(renderingMode: .alwaysOriginal)

button.kf.setImage(with: imageURL, for: .normal, placeholder: defaultIcon, options: [.processor(myProcessor), .scaleFactor(3.0), .cacheOriginalImage])

What

The first time the image is downloaded, everything works perfectly. My image is rounded and the rendering mode is set to .alwaysOriginal by my custom image processor.
When the image is taken from the cache, the processors are not rerun, even though the original image is cached, because the "final image" is also cached.

Reproduce

Create a RenderingModeImageProcessor, such as the one in the code block below (my code, MIT License):

 public struct RenderingModeImageProcessor: ImageProcessor {

    /// Identifier of the processor.
    /// - Note: See documentation of `ImageProcessor` protocol for more.
    public let identifier: String

    /// The rendering mode to use.
    /// Defaults to .alwaysOriginal
    public let renderingMode: UIImageRenderingMode

    /// Initialize a `RenderingModeImageProcessor`.
    ///
    /// - Parameters:
    ///   - renderingModel: The rendering mode to use.
    public init(renderingMode: UIImageRenderingMode = .alwaysOriginal) {
        self.renderingMode = renderingMode

        self.identifier = "com.identifier.here.RenderingModeImageProcessor(\(renderingMode))"
    }

    /// Process an input `ImageProcessItem` item to an image for this processor.
    ///
    /// - parameter item:    Input item which will be processed by `self`
    /// - parameter options: Options when processing the item.
    ///
    /// - returns: The processed image.
    ///
    /// - Note: See documentation of `ImageProcessor` protocol for more.
    public func process(item: ImageProcessItem, options: KingfisherOptionsInfo) -> Image? {
        switch item {
        case .image(let image):
            return image.withRenderingMode(renderingMode)
        case .data(_):
            return (DefaultImageProcessor.default >> self).process(item: item, options: options)
        }
    }
}

Use the ImageProcessor above with an image on a UIButton.

Other Comment

When using the option .forceRefresh, the processor gets a chance to run every time. This is a bad workaround though because it completely avoids caching.

Suggested Fix

Add an option to avoid using the final image, or to just not cache the final image
OR
Add an option to run processors even when the image is cached.
OR
A combination of both of the above.

If this problem is recognized, I can work on a PR if necessary.

@onevcat
Copy link
Owner

onevcat commented Nov 21, 2017

Hi, @ethansinjin

Thanks for reporting this!

Yes, this is a problem. For now, as a quick workaround, you need to also implement your own CacheSerializer to also apply the render mode in the final image. When getting an image from disk, it is the serializer/deserializer which takes response. (And I also found a regression in version 4.3.0 which caused the cache not working well with a customized processor, it should be already fixed in 4.3.1).

Like this:

public struct MyCacheSerializer: CacheSerializer {
    public func data(with image: Image, original: Data?) -> Data? {
        return DefaultCacheSerializer.default.data(with: image, original: original)
    }

    public func image(with data: Data, options: KingfisherOptionsInfo?) -> Image? {
        let image = DefaultCacheSerializer.default.image(with: data, options: options)
        return image?.withRenderingMode(.alwaysOriginal)
    }
}

And also set it as an option.

Re-running of the processor on a processed image might cause some confusion, so compared to re-running, I personally prefer to adding an option to not cache the final image in your suggestion.

However, what I am concerning is that, is this a good use case for an image processor/serializer or not? An ImageProcessor is designed to "decode" and "transform" the images from Internet, or say, it manipulates the downloaded data. After that, the final processed data could be sent to cache and we could use it later without any other modification (just applying a correct CacheSerializer). So neither of them should do this kind of work IMO.

In your case, instead of manipulating the data, you are in fact setting some properties for the image. It should happen just before you set the image to the view. So maybe a better way is injecting the setting image by adding something like "imageModifier" to give the user a final chance to modify the images already in memory, just before send it to an image view or a button.

How do you think about it?

@ethansinjin
Copy link
Contributor Author

ethansinjin commented Nov 21, 2017

Thanks for the quick response and the workaround!

I like the idea of an imageModifier, especially since there are other properties on UIImage that can only be modified after being fetched from the cache. A few examples would be flipsForRightToLeftLayoutDirection and alignmentRectInsets.

Please let me know if you plan to start a PR; I'd be happy to start one if needed!

@onevcat
Copy link
Owner

onevcat commented Nov 21, 2017

Great. Sure, I'd like to receive one from you, if you wish to add it. : ]

@onevcat
Copy link
Owner

onevcat commented Nov 21, 2017

And you could check the existing KingfisherOptionsInfoItem.requestModifier and ImageDownloadRequestModifier, which contains similar concept, but for the request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants