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.

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

Images never load if GeometryReader is used to set its frame width #1687

Closed
3 tasks done
ruippeixotog opened this issue Apr 18, 2021 · 8 comments
Closed
3 tasks done

Comments

@ruippeixotog
Copy link

ruippeixotog commented Apr 18, 2021

Check List

Thanks for considering to open an issue. Before you submit your issue, please confirm these boxes are checked.

Issue Description

I have a SwiftUI view with a very simple body:

    var body: some View {
        GeometryReader { geo in
            KFImage(
                URL(string: "https://www.apple.com/ac/structured-data/images/knowledge_graph_logo.png")!
            )
                .placeholder { ProgressView() }
                .resizable()
                .scaledToFit()
                .frame(width: geo.size.width)
        }
    }

Unfortunately, this image never loads for me the first time (and I can add .forceRefresh() to reproduce this issue consistently). I added .onProgress and .onSuccess hooks and checked that while the image is downloaded successfully, KF never replaces the placeholder view with the actual image. This doesn't happen if the image is loaded from cache.

This seems to be somehow related to how GeometryReader plays out with KFImage. If I remove GeometryReader, or even just assign a fixed width to the image, the image loads correctly.

Is this a known issue?

@onevcat
Copy link
Owner

onevcat commented Apr 18, 2021

Ummm,

I cannot reproduce this issue in the demo app in this repo.

What is your Kingfisher version or is it an issue only happens on some certain device or iOS version?

I also append a GIF of this, by using forceRefresh as well as manually cleaning the cache to make sure it loads from network.

2021-04-18 10 56 43

Is it possible for you to prepare a reproducible sample so we can have a detail look? Thanks!

@ruippeixotog
Copy link
Author

ruippeixotog commented Apr 18, 2021

@onevcat thanks for checking. I created a minimal iOS project for that and I wasn't able to reproduce it either, so I dug a little bit and found out that the problem only happens when wrapped inside a NavigationView:

            NavigationView {
                GeometryReader { geo in
                    KFImage(
                        URL(string: "https://www.apple.com/ac/structured-data/images/knowledge_graph_logo.png")!
                    )
                        .placeholder { ProgressView() }
                        .forceRefresh()
                        .resizable()
                        .scaledToFit()
                        .frame(width: geo.size.width)
                }
            }

I overlooked this in my initial post because NavigationView was applied on the previous list view, sorry for the trouble. Can you reproduce it now?

@ruippeixotog
Copy link
Author

Hi @onevcat, did you have a chance to take a second look at it?

@onevcat
Copy link
Owner

onevcat commented Apr 20, 2021

@ruippeixotog Sure! Please give me some more time...Crazy busy recently. T-T

@onevcat
Copy link
Owner

onevcat commented Apr 20, 2021

Hi, @ruippeixotog

I checked this in detail and it is yet another side effect as #1660

I suggest add a .loadImmediately() to make it work for now, until we can drop support of iOS 13 and switch to the @StateObject to have an actual solution.

@onevcat
Copy link
Owner

onevcat commented Apr 20, 2021

Or if you do not like the loadImmediately way, you can extract the image related part of the code and make it "context-free" (which means it does not depend on the geo in its context, so it won't be evaluated again).

NavigationView {
  GeometryReader { geo in
    MyView()
      .frame(width: geo.size.width)
  }
}

struct MyView: View {
  var body: some View {
    KFImage(
      URL(string: "https://www.apple.com/ac/structured-data/images/knowledge_graph_logo.png")!
    )
    .placeholder { ProgressView() }
    .forceRefresh()
    .resizable()
    .scaledToFit()
  }
}

Frankly speaking I don't know why the original code works without a NavigationView, but it seems that the NavigationView triggers a re-layout and it is just leading to some internal magic of SwiftUI View's lifetime 😂

@ruippeixotog
Copy link
Author

Thanks @onevcat, .loadImmediately() works well for me! Incidentally, it also fixed a similar problem I had when I used .animation(.spring()) on Kingfisher images.

@onevcat
Copy link
Owner

onevcat commented Apr 20, 2021

Nice to hear that. That was too bad the @ObservedObject failed in several places. I really wish we could get rid of that soon.

Thanks for the feedback. Please let me close this for now.

@onevcat onevcat closed this as completed Apr 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants