Merge pull request onevcat#1612 from onevcat/fix/image-modifier-cache

Fix image modifier cache
rentpath · Jan 31, 2021 · 31afdb8 · 31afdb8
2 parents 50b8c6b + 69f8200
commit 31afdb8
Show file tree

Hide file tree

Showing 7 changed files with 63 additions and 59 deletions.
diff --git a/Sources/Cache/ImageCache.swift b/Sources/Cache/ImageCache.swift
@@ -456,7 +456,6 @@ open class ImageCache {
 
         // Try to check the image from memory cache first.
         if let image = retrieveImageInMemoryCache(forKey: key, options: options) {
-            let image = options.imageModifier?.modify(image) ?? image
             callbackQueue.execute { completionHandler(.success(.memory(image))) }
         } else if options.fromMemoryCacheOrRefresh {
             callbackQueue.execute { completionHandler(.success(.none)) }
@@ -474,20 +473,19 @@ open class ImageCache {
                         return
                     }
 
-                    let finalImage = options.imageModifier?.modify(image) ?? image
                     // Cache the disk image to memory.
                     // We are passing `false` to `toDisk`, the memory cache does not change
                     // callback queue, we can call `completionHandler` without another dispatch.
                     var cacheOptions = options
                     cacheOptions.callbackQueue = .untouch
                     self.store(
-                        finalImage,
+                        image,
                         forKey: key,
                         options: cacheOptions,
                         toDisk: false)
                     {
                         _ in
-                        callbackQueue.execute { completionHandler(.success(.disk(finalImage))) }
+                        callbackQueue.execute { completionHandler(.success(.disk(image))) }
                     }
                 case .failure(let error):
                     callbackQueue.execute { completionHandler(.failure(error)) }

diff --git a/Sources/General/KingfisherManager.swift b/Sources/General/KingfisherManager.swift
@@ -359,9 +359,8 @@ public class KingfisherManager {
                         return
                     }
 
-                    let finalImage = options.imageModifier?.modify(image) ?? image
                     options.callbackQueue.execute {
-                        let result = ImageLoadingResult(image: finalImage, url: nil, originalData: data)
+                        let result = ImageLoadingResult(image: image, url: nil, originalData: data)
                         completionHandler(.success(result))
                     }
                 }
@@ -390,6 +389,12 @@ public class KingfisherManager {
                                            options.processor != DefaultImageProcessor.default
             let coordinator = CacheCallbackCoordinator(
                 shouldWaitForCache: options.waitForCache, shouldCacheOriginal: needToCacheOriginalImage)
+            let result = RetrieveImageResult(
+                image: options.imageModifier?.modify(value.image) ?? value.image,
+                cacheType: .none,
+                source: source,
+                originalSource: context.originalSource
+            )
             // Add image to cache.
             let targetCache = options.targetCache ?? self.cache
             targetCache.store(
@@ -401,12 +406,6 @@ public class KingfisherManager {
             {
                 _ in
                 coordinator.apply(.cachingImage) {
-                    let result = RetrieveImageResult(
-                        image: value.image,
-                        cacheType: .none,
-                        source: source,
-                        originalSource: context.originalSource
-                    )
                     completionHandler?(.success(result))
                 }
             }
@@ -423,24 +422,12 @@ public class KingfisherManager {
                 {
                     _ in
                     coordinator.apply(.cachingOriginalImage) {
-                        let result = RetrieveImageResult(
-                            image: value.image,
-                            cacheType: .none,
-                            source: source,
-                            originalSource: context.originalSource
-                        )
                         completionHandler?(.success(result))
                     }
                 }
             }
 
             coordinator.apply(.cacheInitiated) {
-                let result = RetrieveImageResult(
-                    image: value.image,
-                    cacheType: .none,
-                    source: source,
-                    originalSource: context.originalSource
-                )
                 completionHandler?(.success(result))
             }
 
@@ -537,7 +524,7 @@ public class KingfisherManager {
                             if let image = cacheResult.image {
                                 value = result.map {
                                     RetrieveImageResult(
-                                        image: image,
+                                        image: options.imageModifier?.modify(image) ?? image,
                                         cacheType: $0.cacheType,
                                         source: source,
                                         originalSource: context.originalSource
@@ -603,6 +590,13 @@ public class KingfisherManager {
                             let coordinator = CacheCallbackCoordinator(
                                 shouldWaitForCache: options.waitForCache, shouldCacheOriginal: false)
 
+                            let result = RetrieveImageResult(
+                                image: options.imageModifier?.modify(processedImage) ?? processedImage,
+                                cacheType: .none,
+                                source: source,
+                                originalSource: context.originalSource
+                            )
+
                             targetCache.store(
                                 processedImage,
                                 forKey: key,
@@ -611,24 +605,12 @@ public class KingfisherManager {
                             {
                                 _ in
                                 coordinator.apply(.cachingImage) {
-                                    let value = RetrieveImageResult(
-                                        image: processedImage,
-                                        cacheType: .none,
-                                        source: source,
-                                        originalSource: context.originalSource
-                                    )
-                                    options.callbackQueue.execute { completionHandler?(.success(value)) }
+                                    options.callbackQueue.execute { completionHandler?(.success(result)) }
                                 }
                             }
 
                             coordinator.apply(.cacheInitiated) {
-                                let value = RetrieveImageResult(
-                                    image: processedImage,
-                                    cacheType: .none,
-                                    source: source,
-                                    originalSource: context.originalSource
-                                )
-                                options.callbackQueue.execute { completionHandler?(.success(value)) }
+                                options.callbackQueue.execute { completionHandler?(.success(result)) }
                             }
                         }
                     },

diff --git a/Sources/Networking/ImageDataProcessor.swift b/Sources/Networking/ImageDataProcessor.swift
@@ -61,13 +61,7 @@ class ImageDataProcessor {
 
             let result: Result<KFCrossPlatformImage, KingfisherError>
             if let image = image {
-                var finalImage = image
-                if let imageModifier = callback.options.imageModifier {
-                    finalImage = imageModifier.modify(image)
-                }
-                if callback.options.backgroundDecode {
-                    finalImage = finalImage.kf.decoded
-                }
+                let finalImage = callback.options.backgroundDecode ? image.kf.decoded : image
                 result = .success(finalImage)
             } else {
                 let error = KingfisherError.processorError(

diff --git a/Sources/Networking/ImageModifier.swift b/Sources/Networking/ImageModifier.swift
@@ -26,10 +26,10 @@
 
 import Foundation
 
-/// An `ImageModifier` can be used to change properties on an image in between
-/// cache serialization and use of the image. The modified returned image will be
-/// only used for current rendering purpose, the serialization data will not contain
-/// the changes applied by the `ImageModifier`.
+/// An `ImageModifier` can be used to change properties on an image between cache serialization and the actual use of
+/// the image. The `modify(_:)` method will be called after the image retrieved from its source and before it returned
+/// to the caller. This modified image is expected to be only used for rendering purpose, any changes applied by the
+/// `ImageModifier` will not be serialized or cached.
 public protocol ImageModifier {
     /// Modify an input `Image`.
     ///

diff --git a/Tests/KingfisherTests/ImageCacheTests.swift b/Tests/KingfisherTests/ImageCacheTests.swift
@@ -385,7 +385,7 @@ class ImageCacheTests: XCTestCase {
     }
 
 #if os(iOS) || os(tvOS) || os(watchOS)
-    func testGettingMemoryCachedImageCouldBeModified() {
+    func testModifierShouldOnlyApplyForFinalResultWhenMemoryLoad() {
         let exp = expectation(description: #function)
         let key = testKeys[0]
 
@@ -397,15 +397,15 @@ class ImageCacheTests: XCTestCase {
 
         cache.store(testImage, original: testImageData, forKey: key) { _ in
             self.cache.retrieveImage(forKey: key, options: [.imageModifier(modifier)]) { result in
-                XCTAssertTrue(modifierCalled)
-                XCTAssertEqual(result.value?.image?.renderingMode, .alwaysTemplate)
+                XCTAssertFalse(modifierCalled)
+                XCTAssertEqual(result.value?.image?.renderingMode, .automatic)
                 exp.fulfill()
             }
         }
         waitForExpectations(timeout: 3, handler: nil)
     }
 
-    func testGettingDiskCachedImageCouldBeModified() {
+    func testModifierShouldOnlyApplyForFinalResultWhenDiskLoad() {
         let exp = expectation(description: #function)
         let key = testKeys[0]
 
@@ -418,8 +418,8 @@ class ImageCacheTests: XCTestCase {
         cache.store(testImage, original: testImageData, forKey: key) { _ in
             self.cache.clearMemoryCache()
             self.cache.retrieveImage(forKey: key, options: [.imageModifier(modifier)]) { result in
-                XCTAssertTrue(modifierCalled)
-                XCTAssertEqual(result.value?.image?.renderingMode, .alwaysTemplate)
+                XCTAssertFalse(modifierCalled)
+                XCTAssertEqual(result.value?.image?.renderingMode, .automatic)
                 exp.fulfill()
             }
         }

diff --git a/Tests/KingfisherTests/ImageDownloaderTests.swift b/Tests/KingfisherTests/ImageDownloaderTests.swift
@@ -512,7 +512,7 @@ class ImageDownloaderTests: XCTestCase {
     }
 
 #if os(iOS) || os(tvOS) || os(watchOS)
-    func testDownloadedImageCouldBeModified() {
+    func testModifierShouldOnlyApplyForFinalResultWhenDownload() {
         let exp = expectation(description: #function)
 
         let url = testURLs[0]
@@ -525,8 +525,8 @@ class ImageDownloaderTests: XCTestCase {
         }
 
         downloader.downloadImage(with: url, options: [.imageModifier(modifier)]) { result in
-            XCTAssertTrue(modifierCalled)
-            XCTAssertEqual(result.value?.image.renderingMode, .alwaysTemplate)
+            XCTAssertFalse(modifierCalled)
+            XCTAssertEqual(result.value?.image.renderingMode, .automatic)
             exp.fulfill()
         }
 

diff --git a/Tests/KingfisherTests/KingfisherManagerTests.swift b/Tests/KingfisherTests/KingfisherManagerTests.swift
@@ -661,6 +661,36 @@ class KingfisherManagerTests: XCTestCase {
         }
         waitForExpectations(timeout: 3, handler: nil)
     }
+
+    func testImageModifierResultShouldNotBeCached() {
+        let exp = expectation(description: #function)
+        let url = testURLs[0]
+        stub(url, data: testImageData)
+
+        var modifierCalled = false
+        let modifier = AnyImageModifier { image in
+            modifierCalled = true
+            return image.withRenderingMode(.alwaysTemplate)
+        }
+        manager.retrieveImage(with: url, options: [.imageModifier(modifier)]) { result in
+            XCTAssertTrue(modifierCalled)
+            XCTAssertEqual(result.value?.image.renderingMode, .alwaysTemplate)
+
+            let memoryCached = self.manager.cache.retrieveImageInMemoryCache(forKey: url.absoluteString)
+            XCTAssertNotNil(memoryCached)
+            XCTAssertEqual(memoryCached?.renderingMode, .automatic)
+
+            self.manager.cache.retrieveImageInDiskCache(forKey: url.absoluteString) { result in
+                XCTAssertNotNil(result.value!)
+                XCTAssertEqual(result.value??.renderingMode, .automatic)
+
+                exp.fulfill()
+            }
+        }
+
+        waitForExpectations(timeout: 3, handler: nil)
+    }
+
 #endif
 
     func testRetrieveWithImageProvider() {