diff --git a/Sources/SwiftUI/ImageBinder.swift b/Sources/SwiftUI/ImageBinder.swift index c0900e824..6acb0cd49 100644 --- a/Sources/SwiftUI/ImageBinder.swift +++ b/Sources/SwiftUI/ImageBinder.swift @@ -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 } @@ -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) @@ -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 @@ -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 { @@ -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 { @@ -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) @@ -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) } diff --git a/Sources/SwiftUI/ImageContext.swift b/Sources/SwiftUI/ImageContext.swift index 2757293ab..b9f4364c6 100644 --- a/Sources/SwiftUI/ImageContext.swift +++ b/Sources/SwiftUI/ImageContext.swift @@ -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 } } } diff --git a/Sources/SwiftUI/KFImageProtocol.swift b/Sources/SwiftUI/KFImageProtocol.swift index 954b6f53f..2a4503c14 100644 --- a/Sources/SwiftUI/KFImageProtocol.swift +++ b/Sources/SwiftUI/KFImageProtocol.swift @@ -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(_ block: @escaping (HoldingView) -> V) -> Self { + public func contentConfigure(@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(@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 } } diff --git a/Sources/SwiftUI/KFImageRenderer.swift b/Sources/SwiftUI/KFImageRenderer.swift index e8c77c0a9..cb03cf744 100644 --- a/Sources/SwiftUI/KFImageRenderer.swift +++ b/Sources/SwiftUI/KFImageRenderer.swift @@ -52,8 +52,6 @@ struct KFImageRenderer : 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 @@ -120,6 +118,20 @@ struct KFImageRenderer : 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 { @@ -139,7 +151,7 @@ struct KFImageRenderer : 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 } diff --git a/Tests/KingfisherTests/ImageBinderTests.swift b/Tests/KingfisherTests/ImageBinderTests.swift index e6ec20e37..f3f38832c 100644 --- a/Tests/KingfisherTests/ImageBinderTests.swift +++ b/Tests/KingfisherTests/ImageBinderTests.swift @@ -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(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(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 diff --git a/Tests/KingfisherTests/KFImageRendererTests.swift b/Tests/KingfisherTests/KFImageRendererTests.swift index a31d1726d..e4a0efea6 100644 --- a/Tests/KingfisherTests/KFImageRendererTests.swift +++ b/Tests/KingfisherTests/KFImageRendererTests.swift @@ -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 — @@ -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(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)) } @@ -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(source: nil) context.swiftUITransition = .opacity @@ -287,7 +361,7 @@ class KFImageRendererTests: XCTestCase { let binder = KFImage.ImageBinder() let context = KFImage.Context(source: nil) // Keep the image branch in the hierarchy before a cache hit resolves. - context.contentConfiguration = { _ in + context.contentConfiguration = { _, _ in AnyView( Color.clear .onAppear { @@ -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"