SDWebImage / SDWebImageSwiftUI

SwiftUI Image loading and Animation framework powered by SDWebImage
https://sdwebimage.github.io/SDWebImageSwiftUI
MIT License
2.16k stars 223 forks source link

Fix rendering mode in toolbar button by setting it on the UIImage itself #175

Closed hartbit closed 3 years ago

hartbit commented 3 years ago

When displaying a WebImage inside a Button, inside a ToolbarItem, the rendering mode modifier doesn't work. It looks like a SwiftUI bug, although I wasn't able to reproduce it with plain SwiftUI, with images coming from the asset catalog. Instead, I've found a fix which is to set the rendering mode on the UIImage itself.

Here's the code to reproduce the issue:

struct ContentView: View {
    let url = URL(string: "https://png-2.findicons.com/files/icons/2118/nuvola/48/help.png")!

    var body: some View {
        NavigationView {
            WebImage(url: url)
                .renderingMode(.original)
                .toolbar {
                    ToolbarItem(placement: .navigationBarLeading) {
                        WebImage(url: url)
                            .renderingMode(.original)
                    }
                    ToolbarItem(placement: .navigationBarTrailing) {
                        Button(action: {}, label: {
                            WebImage(url: url)
                                .renderingMode(.original)
                        })
                    }
                }
        }
    }
}

Note: If you try to reproduce this, you might encounter another SwiftUI bug which seems unrelated: images render at their native size, even when made resizable and set to a fixed frame.

codecov[bot] commented 3 years ago

Codecov Report

Merging #175 (11c3829) into master (88f2d67) will increase coverage by 0.34%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #175      +/-   ##
==========================================
+ Coverage   73.78%   74.13%   +0.34%     
==========================================
  Files          11       11              
  Lines         965      978      +13     
==========================================
+ Hits          712      725      +13     
  Misses        253      253              
Flag Coverage Δ
ios 69.55% <85.71%> (+0.21%) :arrow_up:
macos 75.00% <100.00%> (+0.37%) :arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
SDWebImageSwiftUI/Classes/WebImage.swift 90.30% <100.00%> (+0.68%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 88f2d67...11c3829. Read the comment docs.

dreampiggy commented 3 years ago

This simple fix breaks the Vector image (SVG/PDF) and Animated image (GIF/APNG/WebP) for WebImage struct. So I can not merge.

This is because of [UIImage imageWithRenderingMode:] API recreate a new UIImage instance, which loss the information our framework setup, and does not allows the subclassing SDAnimatedImage.

But I'll try to investigate your demo's issue and provide some solution.

dreampiggy commented 3 years ago

Seems this only works on SwiftUI.Image is because of SwiftUI team do hack type checking to that Image, our WebImage is not considered an actual Image in SwiftUI internal logic.

image

I'll find some trick way to workaround this.

dreampiggy commented 3 years ago

They override the renderingMode Only if your image is inside an TabbarItem, using UIKit's UIToolBarItem.

How can I get this context as well (That WebImage is now using in Tabbar) ? If I can not get this context, actually, as an framework user, you have to provide .renderingMode modifier when use on Tabbar.

However, SwiftUI.Image does not need you to do this, they provide the renderingMode for you automatically.

hartbit commented 3 years ago

Seems this only works on SwiftUI.Image is because of SwiftUI team do hack type checking to that Image, our WebImage is not considered an actual Image in SwiftUI internal logic.

But WebImage contains a SwiftUI.Image. Why does SwiftUI not notice that? Is it a SwiftUI bug? If yes, is there a simple example we can create a bug report with?

dreampiggy commented 3 years ago

But WebImage contains a SwiftUI.Image. Why does SwiftUI not notice that? Is it a SwiftUI bug? If yes, is there a simple example we can create a bug report with?

You're right. It's Bug. See result:

Demo:

// ContentView.swift
    var body: some View {
        NavigationView {
            WebImage(url: url)
                .renderingMode(.original)
                .toolbar {
                    ToolbarItem(placement: .navigationBarLeading) {
                        Button(action: {}, label: {
                            WebImage()
                        })
                    }
                }
        }
    }
// WebImage.swift
// Helper method, just for testing
   public static func createUIImage() -> UIImage {
        let data = try! Data(contentsOf: URL(string: "https://png-2.findicons.com/files/icons/2118/nuvola/48/help.png")!)
        let source = CGImageSourceCreateWithData(data as CFData, nil)!
        let cgImage = CGImageSourceCreateImageAtIndex(source, 0, nil)!

        let image = UIImage(cgImage: cgImage, scale: 1, orientation: .up)
        return image
    }

Code 1:

    public var body: some View {
        Image(uiImage: WebImage.createUIImage())
    }

Code 2:

    public var body: some View {
        Image(decorative: WebImage.createUIImage().cgImage!, scale: 1)
    }

Screenshot:

image

dreampiggy commented 3 years ago

@hartbit Effected by #159

Seems Apple does not fix all of the bad case for Image(uiImage:) initializer. I'll decide to revert that MR.

dreampiggy commented 3 years ago

Fix works well. I'll submit PR and release 2.0.2

hartbit commented 3 years ago

Thanks! Do you plan to open a bug report with Apple or do you want me to do it?

dreampiggy commented 3 years ago

@hartbit See #177

dreampiggy commented 3 years ago

Should be fixed in v2.0.2

hartbit commented 3 years ago

@dreampiggy Thanks again for your the support 👍

dreampiggy commented 3 years ago

Thanks! Do you plan to open a bug report with Apple or do you want me to do it?

@hartbit Hi. I think you can submit the bug report to Apple. It's easy to reproduce. https://bugreport.apple.com/web/

Just using this API Image(uiImage:) to create an new Image struct, put it into the Tabbar via Button and see the result.

The demo code is above. The demo is not related to whether use WebImage or your own View type at all.


But actually, Apple's team may think this is not a bug ? Because UIImage itself does have a renderingMode property. But Image sturct has another renderingMode modifier. In SwiftUI, the Image is both View and Model. However, in UIKit, UIImage is only the Model but not how it rendered.