Add an isLoaded flag to the contentConfigure block - #2565
Conversation
The block is also evaluated before an image is available, and it is applied to the fallback set by the deprecated `onFailureImage` as well. The caller had no way to tell those states apart from a real image. Add a `contentConfigure(_:)` overload that passes an `isLoaded` flag along with the image. It is `true` only when the rendered image comes from the cache or the network, including a partial image delivered by progressive loading, so callers can gate configurations that only make sense for a real image. The existing single-argument overload keeps its signature and forwards to the new one. Both take a `@ViewBuilder` block now, so a block with an `if` / `else` works the same way in either of them.
onevpaw
left a comment
There was a problem hiding this comment.
The API direction looks sound, but there is a blocking provenance issue when a cancelled request overlaps a restarted load.
A reproducible sequence at head 389df8869793911d60e30b01d7281c4c1111c173 is:
- Request A starts and is cancelled by
cancelOnDisappear. - Request B starts and clears
usesFailureImage. - A's delayed failure callback installs the fallback and sets
usesFailureImage = true. - B succeeds and installs the retrieved image, but does not reset that flag.
The final image is therefore real, while isImageLoaded remains false. A focused test forcing “A failure → B success” fails on XCTAssertFalse(binder.usesFailureImage); it passes after resetting usesFailureImage before each real-image assignment.
Please update the progressive and all success assignment paths so the image and its provenance change together, ideally through a shared helper, and add a regression test covering this stale-callback ordering.
onevpaw - an assistant to @onevcat
A cancelled request leaves `loadedImage` empty, so the binder can be restarted before that request delivers its failure. When the failure landed afterwards it installed the fallback image and recorded it, but the restarted load then replaced only the image. A retrieved image was still reported as a fallback, and `isLoaded` stayed `false` for it. Route every `loadedImage` assignment through a single setter that records where the image came from, so the two can no longer drift apart. The reset at the start of `start()` goes away with it: it was the one remaining place that changed the provenance without changing the image, and the `onFailureImage(nil)` case it covered is handled by the setter now.
|
Fixed in 2ef0424.
I also dropped the Regression test: Full suite on the iOS simulator: 400 tests, 0 failures. |
onevclaw
left a comment
There was a problem hiding this comment.
The implementation correctly keeps the rendered image and its provenance in sync, including the stale failure → later success sequence. However, the public DocC note for contentConfigure currently contradicts the actual fallback behavior.
In KFImageProtocol.swift, the note says that a view returned for isLoaded == false is not displayed and that configuring loadTransition means the block only receives true. With onFailureImage, the fallback is rendered while isLoaded is false; this remains true when loadTransition is configured.
Please update the documentation to distinguish these cases:
- Before any image exists, the default rendering path may evaluate the block while keeping the branch hidden.
- An
onFailureImagefallback is displayed and invokes the block withisLoaded == false. loadTransitionavoids the pre-load evaluation, but a displayed failure fallback still invokes the block withfalse.
A regression test covering loadTransition together with onFailureImage would also be valuable, though the documentation correction is the blocking issue here.
onevclaw - an assistant to @onevcat
Make `loadedImage` private(set) so the provenance-aware setter is the only way to install an image, instead of relying on every future assignment site remembering to go through it. Move the stale-callback regression test to `ImageBinderTests`, where the other binder-level tests live, and follow that file's provider and cache key conventions. Fold the duplicated `contentConfigure` probe in the two renderer tests into a shared helper.
The note claimed that a view returned while `isLoaded` is `false` never reaches the screen, and that setting a load transition makes the block run with `true` only. Neither holds once `onFailureImage` is set: the fallback fills `loadedImage`, so the image branch renders and the block runs with `false` on the default path and with a load transition alike. The existing fallback test already asserted that non-zero layout, so the note contradicted the suite it shipped with. Split the note into the two cases, which differ in whether the returned view is displayed, and pin the load transition case with a test.
|
Fixed in b7264cc. The note did contradict the behaviour. Once The note is now split into the two cases, which differ in whether the returned view reaches the screen:
I reworded the lead-in as well. It said the block "is also evaluated before the image is loaded", which stops being true once a load transition is set; it now says the block does not run only for images the caller retrieved, which covers both cases. For the test you suggested, Full suite on the iOS simulator: 401 tests, 0 failures. |
Motivation
contentConfigurehands the loaded image to the caller, but the block is also evaluated in two states where there is no retrieved image:swiftUITransition == nil) the image branch is always rendered and only hidden withopacity(0)and a zero frame, so the block runs with an emptyImage.onFailureImagefillsbinder.loadedImagewith a caller-supplied fallback.The caller has no way to tell either of those apart from a real image.
What this adds
A
contentConfigure(_:)overload that passes anisLoadedflag along with the image:loadedImageisLoadednilfalsetruetrueonFailureImagefalseonFailureView, or neithernilfalseProgressive partial images are
trueon purpose. They come from a successful load in flight, and treating them as not loaded would drop the caller's sizing configuration and make the layout jump once the load completes.Notes on the implementation
placeholderalready uses for its(Progress) -> P/() -> Ppair.@ViewBuilderblock now, so anif/elsebody behaves the same in either one. This is source compatible: it only widens the accepted block shapes, and a block with an explicitreturnis left untransformed.isImageRenderablekeeps its meaning and all of its uses (opacity, frame, branching). The fallback image still has to be rendered, so the newisImageLoadedis derived separately rather than changing it.isLoadedisfalsedoes not appear on screen, since the image branch stays hidden until an image is available.isLoadedis meant for gating the caller's own configuration, not for drawing loading or failure UI —placeholderandonFailureViewown those states. This is spelled out in the DocC comment.ImageBindergains ausesFailureImageflag, reset at the start ofstart().onFailureImageaccepts anilimage, which leavesloadedImageempty while the flag is set; that state is not terminal, so without the reset a later successful load would still look like a fallback image.Tests
Three tests in
KFImageRendererTests, reusing the existingmeasureLayoutharness. They apply a differentframe(height:)perisLoadedvalue and assert the measured layout, so no extra recorder object is needed.testContentConfigureReceivesIsLoadedAfterSuccesstestContentConfigureReceivesIsLoadedFalseForFailureImage— the non-zero measured height proves the image branch is rendered whileisLoadedstaysfalsetestFailureImageFlagIsResetWhenLoadingRestarts— covers theonFailureImage(nil)case described aboveTwo existing tests assign
context.contentConfigurationdirectly and were updated for the new closure arity.Verified on the iOS simulator: 400 tests, 0 failures. I also checked that the new tests fail in both directions when
isImageLoadedis forced to a constant. Other platforms are left to CI.CHANGELOG.mdis untouched, since it looks like it is updated in the release commits.