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

Refactor the cancel logic #771

Merged
merged 2 commits into from
Jun 25, 2014
Merged

Conversation

Whirlwind
Copy link
Contributor

I found there are many cancel function in each categories. But they are some error:

  1. some cancel function have same name.
  2. lost some cancel function (e.g. -[UIButton cancelBackgroundImageLoad])
  3. UIButton's cancel function should have UIControlState argument.

So I refactor the cancel logic to easy.

Conflicts:
	SDWebImage/MKAnnotationView+WebCache.m
	SDWebImage/UIButton+WebCache.m
	SDWebImage/UIImageView+HighlightedWebCache.h
	SDWebImage/UIImageView+HighlightedWebCache.m
	SDWebImage/UIImageView+WebCache.m
@Whirlwind
Copy link
Contributor Author

anybody agree with me?

@bpoplauschi
Copy link
Member

@Whirlwind You worked a bit on this :) It's a big change to me and I must say I don't know if we really need this. What issues are you trying to fix with this?

@bpoplauschi
Copy link
Member

@rs @matej Please take a look at this and comment.

@bpoplauschi bpoplauschi added this to the Future milestone Jun 24, 2014
@Whirlwind
Copy link
Contributor Author

I had said the reason. And if we will not need it, you should check and fix all cancel methods!!

@rs
Copy link
Member

rs commented Jun 25, 2014

I personally like the change. It will certainly easier to maintain and less error prone.

@bpoplauschi
Copy link
Member

Ok. Merging based on @rs's comment.
@Whirlwind I couldn't understand from your comments what are the issues, but after looking again at the code, I did. Thanks

@bpoplauschi bpoplauschi modified the milestones: Future, 3.7.0 Jun 25, 2014
bpoplauschi added a commit that referenced this pull request Jun 25, 2014
@bpoplauschi bpoplauschi merged commit 9b18145 into SDWebImage:master Jun 25, 2014
@matej
Copy link
Contributor

matej commented Jun 25, 2014

Consolidating some of the category functionality into a single utility class and enhancing the cancelation behavior seems like a good thing to do, so I approve as well. There's actually even more stuff that could be shared here. The main thing I'd personally do differently is to define a new class for the shared functionality instead of cramping the shared code into a UIView category. Adding base classes like UIView should in my opinion be done only if it really makes sense. Something like +[SDWebImageCache setImageLoadOperation:onObject:forKey:] could do the same thing. There were also some public API changes here, that were made without first marking the methods as deprecated or something like that. Probably not a big deal, but in general it's better if API changes (e.g., renaming methods) are only made with major releases.

@bpoplauschi
Copy link
Member

@matej you are right, we should deprecate the old methods.

bpoplauschi added a commit that referenced this pull request Jun 25, 2014
@bpoplauschi
Copy link
Member

Update: I did deprecate them, just 2 methods [UIButton cancelCurrentImageLoad] and [UIImageView cancelCurrentArrayLoad]

@matej
Copy link
Contributor

matej commented Jun 25, 2014

Great. Thanks.

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