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

Fix iOS 16 undefined behavior warnings #225

Closed

Conversation

garrettrayj
Copy link

Fixes warnings triggered by bad state updates mentioned in #222

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

@dreampiggy
Copy link
Collaborator

dreampiggy commented Sep 5, 2022

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 ?

@dreampiggy dreampiggy added the fix label Sep 5, 2022
@garrettrayj
Copy link
Author

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
Copy link
Collaborator

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
Copy link

pjacobs commented Sep 8, 2022 via email

// This solve the case when WebImage created with new URL, but `onAppear` not been called, for example, some transaction indeterminate state, SwiftUI :)
if imageManager.isFirstLoad {
imageManager.load()
}
Copy link

@mbruegmann mbruegmann Sep 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simply removing this leads to cases where the images are not loaded in our app.
It seems imageManager.load() can be added to the init() so this is still called initially, but outside of view updates.
Not quite sure if the isFirstLoad property would still be needed then.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I eventually ran into this too and the fix, restoring the lines to init(), is working for me. PR updated.

}
if self.purgeable {
self.imagePlayer.clearFrameBuffer()
.onAppear {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure we should not stick to onPlatformAppear() ? I guess there were cases when the pure SwiftUI onAppear() was not sufficient.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Am I sure, no, but it seems to be working ok and I'm even less sure of how to keep it and fix the errors. 😄

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is buggy on SwiftUI 1.0 (iOS 13) in some area so I write the UIView wrapper to get the similiar behavior. But this should be fixed in SwiftUI 2.0 (iOS 14)

@dreampiggy
Copy link
Collaborator

dreampiggy commented Sep 14, 2022

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
Copy link
Collaborator

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
Copy link
Author

Closing in favor of #227

@dreampiggy
Copy link
Collaborator

Please have a try with v2.1.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants