-
Notifications
You must be signed in to change notification settings - Fork 6.1k
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
Exposing image view on ImageViewTarget #38
Conversation
@@ -8,8 +10,6 @@ | |||
import android.widget.ImageView; | |||
import com.bumptech.glide.presenter.ImagePresenter; | |||
|
|||
import static android.view.ViewGroup.LayoutParams; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a big deal, but probably best to try to avoid committing formatting changes to the imports so we don't go back and forth each time our editors disagree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh man, I thought I had fixed those. Let me recommit the change. My IntelliJ is configured like this cause our team is using this ordering.
My only hesitation with this is that it implies that the current target interface isn't sufficient. If it's possible to fix or expand that interface so that you can do what you need to do without having direct access to the ImageView that would probably be nicer. As an alternative you could also subclass ImageViewTarget and override onImageReady in the subclass. You'd then call Glide.load(yourModel).into(new ImageViewTargetSubclass(yourImageView)). It would be nice to not have to cast your target to a specific subclass in the listener to get the functionality you need. I'm not particularly happy with the target interface and I haven't used it a ton, so if you have any ideas on how it might be improved I'd love to hear them. |
You are totally right there. My first attempt was to try and generify the RequestListener interface with another generic type that extends Target. That would work apart from the builder-like pattern used in the Request object. You don't know the type of your target until the final call. This was my naive, quick solution. I do like the idea of using my own custom sublass of imageviewtarget, so if you don't want to pollute the API, I can do that in our codebase and drop this pull request. Let me know |
That was my first thought when I saw this too, but I haven't had time to actually try it, the generics in the Request object are rather convoluted. Maybe try using a subclass for now and we can try to think of an alternative way to structure the target interface so it's easier to add additional functionality. |
Will do. I am closing this then |
This was helpful for us since we need access to the image view on our listener callbacks. Cannot see a reason why the view should not be accessible unless I am missing something subtle.