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

Keep a reference to the image URL #560

Merged
merged 6 commits into from
Jun 13, 2014

Conversation

klaaspieter
Copy link
Contributor

Currently it's impossible to verify that the image view is showing the correct image when it's loaded from a URL. This makes it hard to test code that interacts with this category because it's impossible to verify that the correct URL was loaded.

This PR uses associated objects to keep a reference to the set imageURL. Before I add this to the other categories I'd like to know if people agree with this approach.

@rs
Copy link
Member

rs commented Nov 25, 2013

Good idea. Could you please extend it to all supported category please?

Additionally, the direct change of the image property should reset this property.

@klaaspieter
Copy link
Contributor Author

Yes. I will extend it to the other categories somewhere this week.

How would you suggest I detect the change of the image property? Since this is a category I don't think it's a good idea to implement the image property since that will override the default implementation.

The only option I think is to use method swizzling which a lot of developers still consider dangerous. I personally don't have a big issue with it because, as the author points out, it can be useful when used appropriately.

Before I go down this road let me know if this is an acceptable solution.

@rs
Copy link
Member

rs commented Nov 26, 2013

I'm not very confortable with method swizzling in this kind of project. KVO could work but maybe it's too much of a burden for such edge case. What do you think?

@klaaspieter
Copy link
Contributor Author

I think KVO has the same problem as simple overriding the -image method on UIImageView. If the default implementation of UIImageView has implemented observeValueForKeyPath:ofObject:change:context: then implementing it in the category will replace that implementation. This has the possibility of creating obscure really hard to find bugs.

The only solution that I can think of that doesn't involve method swizzling is subclassing UIImageView.

@rs
Copy link
Member

rs commented Nov 27, 2013

Ok, that's not really an option in term of ease of use. I guess we'll have to live with the fact that you can get imageURL property out of sync with the actual image content. A note in the property description should be enough.

@klaaspieter
Copy link
Contributor Author

@rs Done. Let me know what you think.

@rs
Copy link
Member

rs commented Nov 29, 2013

Sounds good! Last thing before the merge, could you please add returns before all open brackets in order to comply with the project's coding style?

@myell0w
Copy link

myell0w commented Dec 6, 2013

regarding KVO:

you can create a separate class that acts as the observer and let the imageView hold a reference to that object (associated object). In this case it doesn't matter, whether the default implementation of UIImageView implements observeValueForKeyPath:ofObject:change:context: or not.

@bpoplauschi
Copy link
Member

@rs What do you think we should do over here? Update this pull request? It still relies on dynamically adding properties to the classes (a practice I don't think we should encourage).

@rs
Copy link
Member

rs commented Jun 11, 2014

Why not?

@bpoplauschi
Copy link
Member

@klaaspieter could you rebase this on top of master, so it can be merged?

@klaaspieter
Copy link
Contributor Author

Done.

@bpoplauschi
Copy link
Member

Thanks @klaaspieter. Merging

bpoplauschi added a commit that referenced this pull request Jun 13, 2014
@bpoplauschi bpoplauschi merged commit 7abff88 into SDWebImage:master Jun 13, 2014
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