SDWebImage / SDWebImageSwiftUI

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

Fix iOS 16 undefined behavior warnings #225

Closed garrettrayj closed 1 year ago

garrettrayj commented 1 year ago

Fixes warnings triggered by bad state updates mentioned in https://github.com/SDWebImage/SDWebImageSwiftUI/issues/222

Not tested with AppKit and could use the input of someone familiar with SwiftUICompatibility.swift (this removes usage)

dreampiggy commented 1 year ago

Thanks !

Can you run the SDWebImageSwiftUI Demo here to check the result ?

You can see the README.md about that Demo, it's not hard to run the demo, just using the pod install on Example directory (because which need some thirdparty image codecs, which use CocoaPods...)

Or let me check your PR's branch on this week ?

garrettrayj commented 1 year ago

Oh neat, I had overlooked the demo. Good news is that the WebImage changes seem fine, even on MacOS Ventura. Bad news is that AnimatedImage also triggers the state warnings and I'm at a loss on how to fix the issue there.

dreampiggy commented 1 year ago

I'll try to fix the AnimatedImage, for WebImage seems OK here...

And, I strongly want to drop the iOS 13 support (SwiftUI 1.0), or I have to use another compatibility support for @StateObject, maybe the https://github.com/shaps80/SwiftUIBackports can help ?

Using @ObservedObject for ImageManager is a wrong design, because it's acutally not shared and behave like a @State

pjacobs commented 1 year ago

I would say at this point, no reason to continue supporting iOS 13, given the current version is 15.6.1, with iOS 16 to be released on Sept

  1. Most iOS developers update relatively rapidly.

PJ

On 2022-09-08 15:38, DreamPiggy wrote:

I'll try to fix the AnimatedImage, for WebImage seems OK here...

And, I strongly want to drop the iOS 13 support (SwiftUI 1.0), or I have to use another compatibility support for @StateObject, maybe the https://github.com/shaps80/SwiftUIBackports can help ?

Using @ObservedObject for ImageManager is a wrong design, because it's acutally not shared and behave like a @State

-- Reply to this email directly, view it on GitHub [1], or unsubscribe [2]. You are receiving this because you are subscribed to this thread.Message ID: @.***>

-- PJ

Links:

[1] https://github.com/SDWebImage/SDWebImageSwiftUI/pull/225#issuecomment-1240288199 [2] https://github.com/notifications/unsubscribe-auth/AACRN3CNDMF3LOSFLSXY6P3V5GCXDANCNFSM6AAAAAAQE6MOU4

dreampiggy commented 1 year ago

Hi.

Status

Today I tried the SwiftUIBackports, change the code to use that and refactor the logic into onAppear and onDisappear. Xcode 14 now happy with it and I think it's okay.

However, it seems that the SwiftUIBackports provided @StateObject does not match the behavior in some aspects with iOS 14's behavior.

See comparation in video. The only changes is that Backport.StateObject -> SwiftUI.StateObject

SDWebImageSwfitUIDemo.mov.zip

Solution

I found no way to support iOS 13's correct behavior without hacking and hacking. I think drop iOS 13 and bump min deployment target to iOS 14 is the correct way to go.

dreampiggy commented 1 year ago

Status

I use the ugly hack to support both iOS 13 && 14

1

And I try to fix the AnimatedImage using the UIViewRepresentableContext.AnimatedImageCoordinator, it seems OK.

So this PR can be replaced with my new PR #227

garrettrayj commented 1 year ago

Closing in favor of #227

dreampiggy commented 1 year ago

Please have a try with v2.1.0