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

Only apply orientation fix for iOS 13 #1629

Merged
merged 1 commit into from
Feb 15, 2021
Merged

Conversation

onevcat
Copy link
Owner

@onevcat onevcat commented Feb 13, 2021

No description provided.

@onevcat onevcat merged commit ec75001 into master Feb 15, 2021
@onevcat onevcat deleted the fix/improve-exif-image branch February 15, 2021 08:29
@dreampiggy
Copy link

@onevcat This may cause some, emm, bad effect :)

SwiftUI's Image(uiImage:) is not a good solution. Suggest to use Image(decorative:scale:orientation:) as much as possible.

See: SDWebImage/SDWebImageSwiftUI#177

@onevcat
Copy link
Owner Author

onevcat commented Mar 10, 2021

@dreampiggy Thanks for letting me know it.

Sigh. Why am I not surprised at all.. 😂

@onevcat
Copy link
Owner Author

onevcat commented Mar 14, 2021

@dreampiggy Tried the identical code in Kingfisher but not reproducible with the UIImage based Image view.

截屏2021-03-15 00 28 19

Not sure what am I missing here. But I am going to wait and see if there are any reports on this and then give it a fix if necessary then.

@dreampiggy
Copy link

Trying not set renderingMode. Because SwiftUI.Image does not need this modifier, but they can magically use the always original mode.

@onevcat
Copy link
Owner Author

onevcat commented Mar 15, 2021

Some more results:

截屏2021-03-15 09 40 01
截屏2021-03-15 09 40 19

@dreampiggy

These behaviors seems to be a little different from the original issue in SDWebImage. (In SDWebImage/SDWebImageSwiftUI#177 , the image is rendered as template even when .original rendering mode is set. Is it?)

Without any rendering mode changing, the image is also rendered originally. And both .original and .template work fine as expected. Can it be a system version related behavior? (say Apple has fixed it in some certain iOS version?)

I am going to dig it deeper to see if I can find anything.

@dreampiggy
Copy link

Seems KF works fine on this case.

Did you override the original UIImage with withRenderingMode ?

The disassemble result shows, if you create the Image with platform UIImage, the platform UIImage will be used to create UIBarButtonItem, then set it into UINavigationViewController. All of this is UIKit-based.

If you create Image with CGImage, the Graphics and HostingView will be used to create custom UIBarButton with customView API, not that with image API.

So differences result the strange behavior.

@onevcat
Copy link
Owner Author

onevcat commented Mar 15, 2021

No. There is no withRenderingMode overriding in this case here.

create the Image with platform UIImage...UIImage will be used to create UIBarButtonItem

create Image with CGImage...be used to create custom UIBarButton with customView API

Is it opposite? According to your disassembly result:

// ....
	if let uiImage = platformItem.platformItemImage {
	    let imageView = UIImageView(image: uiImage)
	    item = UIBarButtonItem(customView:imageView)
	} else if platformItem.decorative {
	    let image = UIImage(cgImge:platformItem.contents)
	    let renderingMode = context.flags.renderingMode ?? .alwaysOriginal
	    let image2 = image.renderingMode(renderingMode)
	    item = UIBarButtonItem(image: image2, .plain, target, selector))
	} else {
	   let hostingView = context.hostingView
	   item = UIBarButtonItem(customView:hostView)
	}

It seems that the platformItemImage (UIImage) goes to customView way, while the decorative (CGImage) goes to the image based API. (And as it is from an CGImage, a renderingMode in context has to be considered here).

Not sure how the platformItem is assembled in this pseudo code, but won't both KFImage and WebImage fall into the third (hostingView) statement? Both are wrapping the SwiftUI.Image into a Group I guess?

@onevcat
Copy link
Owner Author

onevcat commented Mar 15, 2021

I didn't yet have a chance to go deep into the assembly code yet. So maybe my understanding on it is totally wrong.

I'll check it later when I have more time.

skoduricg pushed a commit to rentpath/Kingfisher that referenced this pull request Sep 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants