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.

Sign up for GitHub

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

Fix image modifier cache #1612

Merged
merged 4 commits into from
Jan 31, 2021
Merged

Fix image modifier cache #1612

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
09e28b0
Do not store the modified version to memory
onevcat Jan 30, 2021
60c9dc6
Move image modifying to Kingfisher manager class
onevcat Jan 31, 2021
f7471c1
Fix tests for new behavior
onevcat Jan 31, 2021
6ad9673
Update documentation to reflect the fact
onevcat Jan 31, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions Sources/Cache/ImageCache.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)) }
Expand All @@ -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)) }
Expand Down
52 changes: 17 additions & 35 deletions Sources/General/KingfisherManager.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
}
}
Expand Down Expand Up @@ -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(
Expand All @@ -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))
}
}
Expand All @@ -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))
}

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand All @@ -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)) }
}
}
},
Expand Down
8 changes: 1 addition & 7 deletions Sources/Networking/ImageDataProcessor.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
8 changes: 4 additions & 4 deletions Sources/Networking/ImageModifier.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
///
Expand Down
12 changes: 6 additions & 6 deletions Tests/KingfisherTests/ImageCacheTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]

Expand All @@ -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]

Expand All @@ -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()
}
}
Expand Down
6 changes: 3 additions & 3 deletions Tests/KingfisherTests/ImageDownloaderTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand All @@ -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()
}

Expand Down
30 changes: 30 additions & 0 deletions Tests/KingfisherTests/KingfisherManagerTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down