Skip to content

Fix iOS 16 undefined behavior warnings #225

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

Fix iOS 16 undefined behavior warnings #225

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
91afe3b
Fix iOS 16 publishishing changes is not allowed warnings.
garrettrayj Sep 4, 2022
ae55272
Fix iOS 16 undefined behavior warnings related to setupPlayer().
garrettrayj Sep 5, 2022
d9feae5
Restore isFirstLoad check to WebImage init.
garrettrayj Sep 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 38 additions & 36 deletions SDWebImageSwiftUI/Classes/WebImage.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,29 +52,30 @@ public struct WebImage : View {
}
self.imageManager = ImageManager(url: url, options: options, context: context)
self.imagePlayer = ImagePlayer()
}

public var body: some View {

// 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 •
edited
Loading

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.

}

public var body: some View {
return Group {
if let image = imageManager.image {
if isAnimating && !imageManager.isIncremental {
setupPlayer()
.onPlatformAppear(appear: {
self.imagePlayer.startPlaying()
}, disappear: {
if self.pausable {
self.imagePlayer.pausePlaying()
} else {
self.imagePlayer.stopPlaying()
.onAppear {
Copy link

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)

self.imagePlayer.startPlaying()
}.onDisappear {
if self.pausable {
self.imagePlayer.pausePlaying()
} else {
self.imagePlayer.stopPlaying()
}
if self.purgeable {
self.imagePlayer.clearFrameBuffer()
}
}
if self.purgeable {
self.imagePlayer.clearFrameBuffer()
}
})
} else {
if let currentFrame = imagePlayer.currentFrame {
configure(image: currentFrame)
Expand All @@ -84,24 +85,24 @@ public struct WebImage : View {
}
} else {
setupPlaceholder()
.onPlatformAppear(appear: {
// Load remote image when first appear
if self.imageManager.isFirstLoad {
self.imageManager.load()
return
}
guard self.retryOnAppear else { return }
// When using prorgessive loading, the new partial image will cause onAppear. Filter this case
if self.imageManager.image == nil && !self.imageManager.isIncremental {
self.imageManager.load()
}
}, disappear: {
guard self.cancelOnDisappear else { return }
// When using prorgessive loading, the previous partial image will cause onDisappear. Filter this case
if self.imageManager.image == nil && !self.imageManager.isIncremental {
self.imageManager.cancel()
.onAppear {
// Load remote image when first appear
if self.imageManager.isFirstLoad {
self.imageManager.load()
return
}
guard self.retryOnAppear else { return }
// When using prorgessive loading, the new partial image will cause onAppear. Filter this case
if self.imageManager.image == nil && !self.imageManager.isIncremental {
self.imageManager.load()
}
}.onDisappear {
guard self.cancelOnDisappear else { return }
// When using prorgessive loading, the previous partial image will cause onDisappear. Filter this case
if self.imageManager.image == nil && !self.imageManager.isIncremental {
self.imageManager.cancel()
}
}
})
}
}
}
Expand Down Expand Up @@ -158,13 +159,14 @@ public struct WebImage : View {
/// Animated Image Support
func setupPlayer() -> some View {
if let currentFrame = imagePlayer.currentFrame {
return configure(image: currentFrame)
return configure(image: currentFrame).onAppear()
} else {
if let animatedImage = imageManager.image as? SDAnimatedImageProvider {
self.imagePlayer.setupPlayer(animatedImage: animatedImage)
self.imagePlayer.startPlaying()
return configure(image: imageManager.image!).onAppear {
if let animatedImage = imageManager.image as? SDAnimatedImageProvider {
self.imagePlayer.setupPlayer(animatedImage: animatedImage)
self.imagePlayer.startPlaying()
}
}
return configure(image: imageManager.image!)
}
}

Expand Down