Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
29 changes: 22 additions & 7 deletions Sources/SwiftUI/ImageBinder.swift
Original file line number Diff line number Diff line change
Expand Up @@ -51,10 +51,25 @@ extension KFImage {

private(set) var animating = false

var loadedImage: KFCrossPlatformImage? = nil { willSet { objectWillChange.send() } }
private(set) var loadedImage: KFCrossPlatformImage? = nil { willSet { objectWillChange.send() } }
var failureView: (() -> AnyView)? = nil { willSet { objectWillChange.send() } }
var progress: Progress = .init()

/// Whether the current `loadedImage` is the fallback supplied by the deprecated `onFailureImage`, instead of
/// an image retrieved from the cache or the network.
private(set) var usesFailureImage = false

/// Sets `loadedImage` together with where that image came from.
///
/// A cancelled request can still deliver its failure after a restarted load has begun, so the two values have
/// to change as a pair. Otherwise the provenance outlives the image it described, and a retrieved image ends
/// up reported as a fallback. Going through here is the only way to set the image, so no assignment site can
/// leave the two out of step. It also covers the change event, since `loadedImage` sends it.
func setLoadedImage(_ image: KFCrossPlatformImage?, isFailureImage: Bool = false) {
usesFailureImage = isFailureImage
loadedImage = image
}

func markLoading() {
loading = true
}
Expand All @@ -73,7 +88,7 @@ extension KFImage {
if let view = context.failureView {
self.failureView = view
} else if let image = context.options.onFailureImage {
self.loadedImage = image
self.setLoadedImage(image, isFailureImage: true)
}
self.loading = false
self.markLoaded(sendChangeEvent: false)
Expand All @@ -97,7 +112,7 @@ extension KFImage {
CallbackQueueMain.currentOrAsync { [weak self] in
guard let self else { return }
self.markLoaded(sendChangeEvent: true)
self.loadedImage = image
self.setLoadedImage(image)
}
},
completionHandler: { [weak self] result in
Expand Down Expand Up @@ -125,7 +140,7 @@ extension KFImage {
context.shouldApplyFade(cacheType: value.cacheType) {
// Apply SwiftUI loadTransition with custom animation (higher priority than fade)
self.animating = true
self.loadedImage = value.image
self.setLoadedImage(value.image)

let animation = context.swiftUIAnimation ?? .default
CallbackQueueMain.async {
Expand All @@ -137,7 +152,7 @@ extension KFImage {
}
} else if let fadeDuration = context.fadeTransitionDuration(cacheType: value.cacheType) {
self.animating = true
self.loadedImage = value.image
self.setLoadedImage(value.image)

let animation = Animation.linear(duration: fadeDuration)
CallbackQueueMain.async {
Expand All @@ -150,7 +165,7 @@ extension KFImage {
}
} else {
self.markLoaded(sendChangeEvent: false)
self.loadedImage = value.image
self.setLoadedImage(value.image)

CallbackQueueMain.async {
context.onSuccessDelegate.call(value)
Expand All @@ -162,7 +177,7 @@ extension KFImage {
if let view = context.failureView {
self.failureView = view
} else if let image = context.options.onFailureImage {
self.loadedImage = image
self.setLoadedImage(image, isFailureImage: true)
}
self.markLoaded(sendChangeEvent: false)
}
Expand Down
5 changes: 3 additions & 2 deletions Sources/SwiftUI/ImageContext.swift
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,9 @@ extension KFImage {
set { propertyQueue.sync { _renderConfigurations = newValue } }
}

var _contentConfiguration: ((HoldingView) -> AnyView)? = nil
var contentConfiguration: ((HoldingView) -> AnyView)? {
// The `Bool` parameter tells whether the passed-in view holds an image retrieved from the cache or the network.
var _contentConfiguration: ((HoldingView, Bool) -> AnyView)? = nil
var contentConfiguration: ((HoldingView, Bool) -> AnyView)? {
get { propertyQueue.sync { _contentConfiguration } }
set { propertyQueue.sync { _contentConfiguration = newValue } }
}
Expand Down
42 changes: 40 additions & 2 deletions Sources/SwiftUI/KFImageProtocol.swift
Original file line number Diff line number Diff line change
Expand Up @@ -98,9 +98,47 @@ extension KFImageProtocol {
/// - Parameter block: The block applies to the loaded image. The block should return a `View` that is configured.
/// - Returns: A ``KFImage`` or ``KFAnimatedImage`` view that configures the internal `Image` with the provided
/// `block`.
public func contentConfigure<V: View>(_ block: @escaping (HoldingView) -> V) -> Self {
public func contentConfigure<V: View>(@ViewBuilder _ block: @escaping (HoldingView) -> V) -> Self {
contentConfigure { view, _ in block(view) }
}

/// Configures the current image with a `block` and returns a `View` to use as the final content, with a flag
/// telling whether the image is loaded as input.
///
/// This block will be lazily applied when creating the final `Image`. It does not run only for images the caller
/// retrieved, so the `isLoaded` parameter lets you gate configurations that only make sense for a real image:
///
/// ```swift
/// KFImage(url)
/// .contentConfigure { image, isLoaded in
/// if isLoaded {
/// image.resizable().scaledToFit().overlay(Badge())
/// } else {
/// image
/// }
/// }
/// ```
///
/// The `isLoaded` parameter is `true` only when the image comes from the cache or the network, including a partial
/// image delivered by progressive loading. It is `false` in two situations, which differ in whether the view the
/// block returns reaches the screen:
///
/// - **Before any image exists.** The default rendering path still evaluates the block, but keeps the image branch
/// hidden, so what the block returns is not displayed. Setting a load transition with
/// `loadTransition(_:animation:)` skips this evaluation instead. Use `placeholder(_:)` to fill the loading state.
/// - **While the fallback supplied by the deprecated `onFailureImage` is shown.** That fallback is a real image, so
/// the block is evaluated *and* its result displayed with `isLoaded` as `false` — on the default path and with a
/// load transition alike. Use ``onFailureView(_:)`` to render a failure state that is not an image.
///
/// If multiple `contentConfigure` modifiers are added to the image, only the last one will be stored and used.
///
/// - Parameter block: The block applies to the loaded image and a flag telling whether the image is loaded. The
/// block should return a `View` that is configured.
/// - Returns: A ``KFImage`` or ``KFAnimatedImage`` view that configures the internal `Image` with the provided
/// `block`.
public func contentConfigure<V: View>(@ViewBuilder _ block: @escaping (HoldingView, Bool) -> V) -> Self {
let result = copyForMutation()
result.context.contentConfiguration = { AnyView(block($0)) }
result.context.contentConfiguration = { AnyView(block($0, $1)) }
return result
}
}
Expand Down
18 changes: 15 additions & 3 deletions Sources/SwiftUI/KFImageRenderer.swift
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,6 @@ struct KFImageRenderer<HoldingView> : View where HoldingView: KFImageHoldingView
}

return ZStack {
let isImageRenderable = binder.loadedImage != nil && binder.loaded

if context.swiftUITransition == nil {
// Fade transition or no transition: use opacity control
// Keep the image branch for external transitions without affecting layout while no
Expand Down Expand Up @@ -120,6 +118,20 @@ struct KFImageRenderer<HoldingView> : View where HoldingView: KFImageHoldingView
.onAppear()
}

/// Whether the image branch takes part in rendering and layout.
///
/// The fallback set by the deprecated `onFailureImage` also has to be rendered, so this stays independent of
/// `isImageLoaded`.
private var isImageRenderable: Bool {
binder.loadedImage != nil && binder.loaded
}

/// Whether the rendered image was retrieved from the cache or the network, as opposed to the fallback set by the
/// deprecated `onFailureImage`.
private var isImageLoaded: Bool {
isImageRenderable && !binder.usesFailureImage
}

@ViewBuilder
private func renderedImage() -> some View {
if let swiftUITransition = context.swiftUITransition {
Expand All @@ -139,7 +151,7 @@ struct KFImageRenderer<HoldingView> : View where HoldingView: KFImageHoldingView

// Apply contentConfiguration first, then loadTransition as the final step
if let contentConfiguration = context.contentConfiguration {
contentConfiguration(configuredImage)
contentConfiguration(configuredImage, isImageLoaded)
} else {
configuredImage
}
Expand Down
35 changes: 35 additions & 0 deletions Tests/KingfisherTests/ImageBinderTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,41 @@ class ImageBinderTests: XCTestCase {

await fulfillment(of: [resultReceived], timeout: 1)
}

// A cancelled request keeps `loadedImage` empty, so the binder can be restarted before that request has delivered
// its failure. When the failure lands afterwards it installs the fallback image and records it, and the restarted
// load must still be able to report its own image as retrieved.
@MainActor
@available(*, deprecated) // Silences the deprecation warning for `onFailureImage` under test.
func testRetrievedImageClearsProvenanceLeftByAStaleFailureCallback() async {
let binder = KFImage.ImageBinder()
let success = expectation(description: "The restarted loading succeeds")

// The restarted request. A fresh cache key keeps it out of the cache, so it stays in flight below.
let provider = RawImageDataProvider(
data: testImagePNGData,
cacheKey: "com.onevcat.KingfisherTests.ImageBinder.\(UUID().uuidString)"
)
let restartedContext = KFImage.Context<Image>(source: .provider(provider))
restartedContext.onSuccessDelegate.delegate(on: self) { _, _ in
success.fulfill()
}
binder.start(context: restartedContext)

// Stands in for the cancelled request delivering its failure while the restarted one is still loading.
let staleFailureContext = KFImage.Context<Image>(source: nil)
staleFailureContext.options.onFailureImage = .some(testImage)
binder.start(context: staleFailureContext)
XCTAssertTrue(binder.usesFailureImage, "The stale failure callback should install the fallback image.")

await fulfillment(of: [success], timeout: 1)

XCTAssertNotNil(binder.loadedImage)
XCTAssertFalse(
binder.usesFailureImage,
"A retrieved image must carry its own provenance instead of inheriting the stale one."
)
}
}

#endif
100 changes: 96 additions & 4 deletions Tests/KingfisherTests/KFImageRendererTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,80 @@ class KFImageRendererTests: XCTestCase {
return measuredSize
}

// MARK: - contentConfigure isLoaded
// The `isLoaded` flag lets the caller gate configurations that only make sense for a real image. It must
// distinguish an image retrieved from the cache or the network from the fallback set by `onFailureImage`, which
// is rendered through the very same branch.
@MainActor
func testContentConfigureReceivesIsLoadedAfterSuccess() async {
let successExpectation = expectation(description: "Image loading succeeds")

let view = KFImage
.data(testImageData, cacheKey: "com.onevcat.KingfisherTests.contentConfigureIsLoadedOnSuccess")
.measuringIsLoaded()
.onSuccess { _ in
successExpectation.fulfill()
}

let measuredSize = await measureLayout(view, after: successExpectation)

XCTAssertEqual(
measuredSize.height,
isLoadedTrueHeight,
accuracy: 0.5,
"`isLoaded` should be `true` once the image is retrieved from the cache or the network."
)
}

@MainActor
@available(*, deprecated) // Silences the deprecation warning for `onFailureImage` under test.
func testContentConfigureReceivesIsLoadedFalseForFailureImage() async {
let failureExpectation = expectation(description: "Image loading fails")

let view = KFImage.dataProvider(FailingImageDataProvider())
.onFailureImage(testImage)
.measuringIsLoaded()
.onFailure { _ in
failureExpectation.fulfill()
}

let measuredSize = await measureLayout(view, after: failureExpectation)

// A non-zero height proves the image branch is rendered, so the `false` flag comes from the fallback image
// rather than from an empty image branch.
XCTAssertEqual(
measuredSize.height,
isLoadedFalseHeight,
accuracy: 0.5,
"`isLoaded` should be `false` for the fallback set by `onFailureImage`, even while it is rendered."
)
}

// A load transition drops the render pass that evaluates the block before an image exists, but it does not change
// what a displayed fallback reports: the fallback is still an image the caller did not retrieve.
@MainActor
@available(*, deprecated) // Silences the deprecation warning for `onFailureImage` under test.
func testContentConfigureReceivesIsLoadedFalseForFailureImageWithLoadTransition() async {
let failureExpectation = expectation(description: "Image loading fails")

let view = KFImage.dataProvider(FailingImageDataProvider())
.onFailureImage(testImage)
.measuringIsLoaded()
.loadTransition(.opacity)
.onFailure { _ in
failureExpectation.fulfill()
}

let measuredSize = await measureLayout(view, after: failureExpectation)

XCTAssertEqual(
measuredSize.height,
isLoadedFalseHeight,
accuracy: 0.5,
"A displayed `onFailureImage` fallback should report `isLoaded` as `false` with a load transition too."
)
}

// MARK: - Renderer intermediate states
// Regression test for the fade scaling artifact. The image branch keeps a zero frame while
// `loadedImage` is nil, and the zero frame must be released as soon as `loadedImage` is set —
Expand All @@ -168,13 +242,13 @@ class KFImageRendererTests: XCTestCase {
func testFadeRestoresImageLayoutBeforeAnimationBegins() async {
let binder = KFImage.ImageBinder()
// Simulates the state ImageBinder creates before starting a fade animation.
binder.loadedImage = testImage
binder.setLoadedImage(testImage)

let context = KFImage.Context<Image>(source: nil)
context.placeholder = { _ in
AnyView(Color.gray.frame(height: 200))
}
context.contentConfiguration = { image in
context.contentConfiguration = { image, _ in
AnyView(image.resizable().aspectRatio(contentMode: .fit))
}

Expand All @@ -197,7 +271,7 @@ class KFImageRendererTests: XCTestCase {

let binder = KFImage.ImageBinder()
// Simulates the render pass after `loadedImage` changes but before `loaded` does.
binder.loadedImage = testImage
binder.setLoadedImage(testImage)

let context = KFImage.Context<Image>(source: nil)
context.swiftUITransition = .opacity
Expand Down Expand Up @@ -287,7 +361,7 @@ class KFImageRendererTests: XCTestCase {
let binder = KFImage.ImageBinder()
let context = KFImage.Context<Image>(source: nil)
// Keep the image branch in the hierarchy before a cache hit resolves.
context.contentConfiguration = { _ in
context.contentConfiguration = { _, _ in
AnyView(
Color.clear
.onAppear {
Expand Down Expand Up @@ -408,6 +482,24 @@ private struct ExternalTransitionHost: View {
}
}

private let isLoadedTrueHeight: CGFloat = 123
private let isLoadedFalseHeight: CGFloat = 45

@available(iOS 14.0, tvOS 14.0, *)
private extension KFImageProtocol where HoldingView == Image {
/// Gives the image a different height per `isLoaded` value, so the measured layout reveals which value the
/// `contentConfigure` block received. The `if` / `else` body also exercises the overload's `@ViewBuilder`.
func measuringIsLoaded() -> Self {
contentConfigure { image, isLoaded in
if isLoaded {
image.resizable().frame(height: isLoadedTrueHeight)
} else {
image.resizable().frame(height: isLoadedFalseHeight)
}
}
}
}

private struct FailingImageDataProvider: ImageDataProvider {
let cacheKey = "com.onevcat.KingfisherTests.FailingImageDataProvider"

Expand Down