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

Add image download support to CPListItem #1820

Merged
merged 5 commits into from
Oct 3, 2021

Conversation

waynehartman
Copy link
Contributor

This adds image download support for CPListItem. CPGridButton does not get this same support because its image cannot be updated asynchronously without using private API.

@onevcat
Copy link
Owner

onevcat commented Sep 30, 2021

This looks nice and promising. It extends the boundary of Kingfisher. Thank you @waynehartman !

I am not a CarPlay developer and have never tried it before. It seems that when building with iOS, the CarPlay related code is also compiled (with an error on CI).

I will try to read the CarPlay programming documentation and make sure I can know what is going on. After some brief look, it seems that I have to start from preparing a CarPlay simulator before I can try it.

It is also welcome if you can give some suggestion or fix it, so we can continue this.

@waynehartman
Copy link
Contributor Author

This looks nice and promising. It extends the boundary of Kingfisher. Thank you @waynehartman !

I am not a CarPlay developer and have never tried it before. It seems that when building with iOS, the CarPlay related code is also compiled (with an error on CI).

I will try to read the CarPlay programming documentation and make sure I can know what is going on. After some brief look, it seems that I have to start from preparing a CarPlay simulator before I can try it.

It is also welcome if you can give some suggestion or fix it, so we can continue this.

Taking a quick look at the build run, it looks like it has having a problem I do not experience with the latest iOS SDK. I’ll check it out later this afternoon on Xcode 12; my changes were done in Xcode 13.

@onevcat
Copy link
Owner

onevcat commented Sep 30, 2021

Oh!

That is a good hint! I will also try it tomorrow (too late in my time zone now).

@waynehartman
Copy link
Contributor Author

Oh!

That is a good hint! I will also try it tomorrow (too late in my time zone now).

I tried it on Xcode 12.5, but it built just fine and all tests passed. :|

@waynehartman
Copy link
Contributor Author

waynehartman commented Oct 1, 2021

Oh!

That is a good hint! I will also try it tomorrow (too late in my time zone now).

I fired up Xcode 12.4 (what you’re using for the build process) and it indeed breaks. ‘CPListItem’ had a change made. 12.5 made the setImage parameter optional, whereas previously it was non-nil.

@waynehartman
Copy link
Contributor Author

Oh!

That is a good hint! I will also try it tomorrow (too late in my time zone now).

Because the SDK made a change that is not backward compatible with SDK <14.5, I put in a compiler(>=5.4) check that was also introduced in Xcode 12.5 so that older users of the SDK can still use this capability, albeit in a slightly crippled way.

@onevcat
Copy link
Owner

onevcat commented Oct 3, 2021

@waynehartman Thanks for it. It seems that I need a CarPlay Entitlements to build a demo app and test it even on the CarPlay simulator. It requires special apply process and seems to beyond what I can do now.

Since all is addition to current library so it should not break anything. I will merge and tag it without further validation as long as the CI passes.

Thank you again for it, this is an area I have never thought about Kingfisher can help.

@onevcat onevcat merged commit eab0df6 into onevcat:master Oct 3, 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