onevcat / Kingfisher

A lightweight, pure-Swift library for downloading and caching images from the web.
MIT License
23.41k stars 2.66k forks source link

Crash with SwiftUI when image's width is nil #1977

Closed prgorasiya closed 2 years ago

prgorasiya commented 2 years ago

Check List

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

Issue Description

What

I am setting an image on my SwiftUI component using this code

KFImage(imageUrl)
                .resizable()
                .frame(width: model.width, height: model.height)
                .aspectRatio(contentMode: .fit)

I am facing a crashing when image width is nil. The reason for width to be nil is, I am using Server Driven UI(SDUI) so backend could sometime just send height of an image and let it show based on aspect ratio, like SwiftUI's own Image component, I just dont want to care about its width and let it expand based on aspect ratio.

Therefore, my app is crashing when, for an image, width is nil(not Zero).

Reproduce

Download the image with width set as nil. App will crash.

Other Comment

I would like have the functionality where either one or both width and height of an image is nil and the ImageView just takes care of them by itself, based on aspect ratio. If I am using the code the wrong way, please advise.

tzxdtc commented 2 years ago

I set width to nil and add .aspectRatio(contentMode: .fit), but cannot reproduce your issue. Could please provide more details? Such as screenshot, iOS version and crash log. Of course, you can test the code by yourself.👇 https://github.com/onevcat/Kingfisher/blob/master/Demo/Demo/Kingfisher-Demo/SwiftUIViews/SingleViewDemo.swift#L59

prgorasiya commented 2 years ago

@tzxdtc Thanks for your response. Just a note, I am using V5.15.8 of Kingfisher/SwiftUI. However, I cant reproduce the issue with the demo of the same version. Additionally, Xcode doesnt log any crash or error when I face the crash, and crashes at a random place in my code where I am adding a custom SwiftUI View which consists a ScrollView.

public var body: some View {
        GeometryReader { outsideProxy in
            ScrollView(axes, showsIndicators: showIndicators) {
                ZStack(alignment: self.axes == .vertical ? .top : .leading) {
                    GeometryReader { insideProxy in
                        Color.clear
                            .preference(key: ScrollOffsetPreferenceKey.self, value: [self.calculateContentOffset(fromOutsideProxy: outsideProxy, insideProxy: insideProxy)])
                    }
                    self.content
                }
            }
            .onPreferenceChange(ScrollOffsetPreferenceKey.self) { value in
                self.contentOffset = value[0]
            }
        }
    }

One weird thing is if I write my code like below, where I assign aspectRatio first and then frame, i dont face a crash. This same approach, when applied to Kingfisher's demo code, results in aspect ratio not getting set properly, although it still doesnt crash the demo app. As you can see from my questions, I am originally setting frame first and then aspectRatio, which results in crash on my app. I cant seem to wrap my head around why it is happening!

KFImage(imageUrl)
                .resizable()
                .aspectRatio(contentMode: .fit)
                .frame(width: model.width, height: model.height)
onevcat commented 2 years ago

@prgorasiya Thanks for the feedback.

First, I think making aspectRatio before frame is the correct way for your purpose. It gives the result like below:

截屏2022-08-16 22 58 27

when applied to Kingfisher's demo code, results in aspect ratio not getting set properly

I am not sure why you are getting this result. Maybe it is because the using of a quite old version of Kingfisher (5.15.8)? Can you try the same thing with the latest Kingfisher v7.3.2 (and/or with a more recent iOS version)? So we can narrow down the problem scope.

And as in the screenshot above, it does not trigger a crash standalone. So maybe it is due to a combination with other layout code. I am now not sure how this can happen, is it possible for you to try to work out a minimal reproducible sample? So we can track and dig into it better.

Thank you again for your report and contribution!

prgorasiya commented 2 years ago

@onevcat Thanks for the detailed answer. I tried with V6(Because our app supports iOS 13) of of the SDK but still unable to reproduce the issue as a standalone demo. However, since setting aspectRatio fixes the issue for me, although not sure why, I think we can close this issue.