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

Support for different types of indicators, including gif images #425

Merged
merged 4 commits into from
Sep 12, 2016
Merged

Support for different types of indicators, including gif images #425

merged 4 commits into from
Sep 12, 2016

Conversation

jdmoreira
Copy link
Contributor

This pull request is my proposed solution for issue #315.

I basically changed 'kf_indicator' from being a view to being of type 'Indicator', which is a protocol. Having a protocol allows us to add different kinds of Indicators.

I've already added a struct that conforms to 'Indicator' for Images (including GIF support) and another for NSProgressIndicator/UIActivivityIndicator.

Since the setter for 'kf_indicator' is private, the user chooses what indicator to use through the 'kf_indicatorType' enum. There are default values, plus values that receive parameters as associated values.
There's a .custom value that allows the user to pass any type that conforms to 'Indicator', allowing the user to have his own custom indicator view.

Also, since I needed to save any swift type that conforms to a protocol in the associated objected 'kf_indicator', I had to created a wrapped/lifted version of objc_set/objc_getAssociatedObject. That's my first commit, I added this to file WrappedAssociatedObjects.swift and replaced all objc_set/get_AssociatedObject calls with calls to the wrapped version.

All suggestions and changes are welcome.

@codecov-io
Copy link

Current coverage is 73.04% (diff: 71.08%)

Merging #425 into master will decrease coverage by 1.22%

@@             master       #425   diff @@
==========================================
  Files            14         16     +2   
  Lines          1691       1755    +64   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           1256       1282    +26   
- Misses          435        473    +38   
  Partials          0          0          

Powered by Codecov. Last update b1fa34e...126f42a

*/
extension ImageView {
public enum IndicatorType {
case none // no indicator
Copy link
Owner

Choose a reason for hiding this comment

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

To follow Swift 2 API guideline, please use upper case for enum member here.

@onevcat
Copy link
Owner

onevcat commented Sep 8, 2016

Hi, @jdmoreira

Just had a brief look at it. Great implementation!

But I have some concern before merging it. You have removed kf_showIndicatorWhenLoading and IndicatorView typealias, which were marked as public before. I understand that by doing so could make code cleaner and expressive. However, as a framework, it is also important to keep back compatible as mush as possible. As semantic versioning requiring, we may need a major update for such API breaking. IMO, it is not a good idea to do so for this kind of change.

So before it could be merged, may I ask to add back the IndicatorView typealias as well as the kf_showIndicatorWhenLoading property, and make it behave as .activity is set when kf_indicatorType is set to .none. (and maybe mark this API as deprecated with warning).

I also commented in the code for a trivial style/convention. And it would be even greater if you can also make a p-r to swift3 branch (there you could do the breaking change by removing the kf_showIndicatorWhenLoading), since the main development of Kingfisher will continue there instead of Swift 2.x version.

Thanks again for it. It's impressive to me!

@jdmoreira
Copy link
Contributor Author

Absolutely. I'll do it :)

@onevcat onevcat merged commit 126f42a into onevcat:master Sep 12, 2016
@onevcat
Copy link
Owner

onevcat commented Sep 12, 2016

Fix and merged in #430

@hifall
Copy link

hifall commented Sep 13, 2016

Looks like an awesome feature! I can't wait to add this in ASAP.

Will the document be updated soon?

@onevcat
Copy link
Owner

onevcat commented Sep 13, 2016

@hifall It's already in version 2.6.0. And I will continue work on it for Swift 3 later.

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.

4 participants