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

ImageView.setTag cannot be used for ViewHolder when root of layout #370

Closed
TWiStErRob opened this issue Mar 13, 2015 · 13 comments
Closed

ImageView.setTag cannot be used for ViewHolder when root of layout #370

TWiStErRob opened this issue Mar 13, 2015 · 13 comments
Labels
Milestone

Comments

@TWiStErRob
Copy link
Collaborator

@TWiStErRob TWiStErRob commented Mar 13, 2015

Glide Version/Integration library (if any): 3.5.2
Device/Android Version: S4/4.4
Issue details/Repro steps:
Consider a ListView containing a dead simple layout:

<?xml version="1.0" encoding="utf-8"?>
<ImageView
    xmlns:android="http://schemas.android.com/apk/res/android"
    android:id="@+id/image"
    android:layout_width="wrap_content"
    android:layout_height="wrap_content"
    />

and an Adapter which is using the usual ViewHolder pattern, i.e. inflating the view from resource (above) and setTag(vh);, now Glide wants to do the same when loading the image. There could easily be a way to prevent conflicts, by allowing the ViewTarget users to set an ID to use with setTag(int, Object), even if it's just a static method on ViewTarget (default beingView.NO_ID -> setTag(Object)).

Glide load line:

Glide.with(context).load(...).into(vh.image);

Stack trace:

    java.lang.IllegalArgumentException: You must not call setTag() on a view Glide is targeting
            at com.bumptech.glide.request.target.ViewTarget.getRequest(ViewTarget.java:105)
            at com.bumptech.glide.GenericRequestBuilder.into(GenericRequestBuilder.java:605)
            at net.twisterrob.app.android.view.Adapter.bindView()
            ...
@sjudd

This comment has been minimized.

Copy link
Member

@sjudd sjudd commented Mar 13, 2015

This is a good idea, we should definitely let people optionally set a tag. A static is kind of a hack, but it would work for v3. In v4 we will let people register their own target factories, so we can add the tag optionally to the constructor of the Target and provide it from the factory.

We could also release as an aar and use a resource id ourselves, but last time I looked aars weren't supported as well as jars by gradle/maven.

@TWiStErRob

This comment has been minimized.

Copy link
Collaborator Author

@TWiStErRob TWiStErRob commented Mar 13, 2015

All that sounds reasonable.

I though about the static, because it's not really a Glide configuration, only the ViewTarget and subclasses use this method to store/retrieve the Request.

You don't and shouldn't have anything in Glide which justifies using resources and aar.
Though it works in Gradle, you must have checked it long ago, because almost everything in android-sdk\extras\android\m2repository\com\android\support is an .aar.

@sjudd

This comment has been minimized.

Copy link
Member

@sjudd sjudd commented Mar 15, 2015

It was more about transitive dependencies. I think gradle and/or maven don't (or didn't) support transitive dependencies for aars the way they do for jars.

Also worth noting, applications can always define their own resource ids use setTag with a resource id to get around the problem. In fact it might almost be preferable to just say that's the correct solution. It's just about the same amount of hassle to define a resource id and use it in your own tags as it is to define one and tell Glide to use it.

@TWiStErRob

This comment has been minimized.

Copy link
Collaborator Author

@TWiStErRob TWiStErRob commented Mar 15, 2015

Yes I was thinking about the amount of hassle, but at the same time I like if libraries have common methods, like in almost every Java program I write I have to write a copyStream method instead of using the framework's non-existent method.

I guess this is a rare use case (ImageView being root) and you don't really need a ViewHolder in that case, just (ImageView)convertView, so we can accept that as a workaround.

There's also the case of using some base Adapter where you can't change the use of setTag yourself (in this case there's no hassle involved, it's plain impossible to do it without copying some existing class). I was trying to find an example, but SimpleAdapter, CursorAdapter are not using view holders nor the problem arises with RecyclerView.

However another argument for the library supporting this is: I would have N adapters to modify instead of "a line" in MyApp.java setting up which ID Glide should use for tags and everything can work without modification. Note that there's a View.generateViewId() since 16, so no need for xml.

I guess based on the factory in v4 we can close this one. There's a record of multiple ways how to work around it above in case someone ever finds this issue in v3. But still it may worth adding the support to ViewTarget even if there's no central way of providing it.

@sjudd sjudd added the enhancement label Mar 30, 2015
@ultimaters

This comment has been minimized.

Copy link

@ultimaters ultimaters commented Apr 6, 2015

I got the same problem. I used Picasso before, Picasso works good. I hope Glide can surpport it too.

@TWiStErRob

This comment has been minimized.

Copy link
Collaborator Author

@TWiStErRob TWiStErRob commented Apr 6, 2015

@ultimaters, I wouldn't wait for it, this is a nice to have, and there are several workarounds mentioned in the above comments, feel free to pick one of those instead. Extending ImageViewTarget and overriding get/setRequest seems the easiest one (you'll need .into(new YourImageViewTarget(iv))).

Also this should be obsolete once v4 comes.

sjudd added a commit to sjudd/glide that referenced this issue May 1, 2015
sjudd added a commit to sjudd/glide that referenced this issue May 1, 2015
@sjudd sjudd added this to the 3.6.0 milestone May 1, 2015
@sjudd sjudd closed this in b9a2d3b May 1, 2015
@TWiStErRob

This comment has been minimized.

Copy link
Collaborator Author

@TWiStErRob TWiStErRob commented Aug 11, 2015

Another use case for ViewTarget.setTagId(int): storing model data in setTag

@w446108264

This comment has been minimized.

Copy link

@w446108264 w446108264 commented Nov 6, 2015

iv_image.setTag(iv_image.getId(),object);

or

implements GlideModule

or

Glide...into(new ImageViewTarget(iv_image));

@deviant-studio

This comment has been minimized.

Copy link

@deviant-studio deviant-studio commented Mar 2, 2016

I have same exception when using Glide + Google DataBinding framework (http://developer.android.com/intl/ru/tools/data-binding/guide.html)
Internally databinding heavily uses setTag() method.
How to reproduce:
Create simple layout with ImageView wrapped into <layout> tag
Use Glide programmatically (not via binding adapters):

Glide.with(ctx).load(imageUrl).into(image);
@slelyuk

This comment has been minimized.

Copy link

@slelyuk slelyuk commented Mar 2, 2016

While using bindings I'm experiencing same issue as described above. Are there any workaround or so?

@TWiStErRob

This comment has been minimized.

Copy link
Collaborator Author

@TWiStErRob TWiStErRob commented Mar 2, 2016

You can see the commit that closed this issue above.

If you search for the exception in Google this should rank pretty high:
http://stackoverflow.com/a/35096552/253468

RE: data binding, there should be no conflict above API 14:
https://code.google.com/p/android/issues/detail?id=195391

@deviant-studio

This comment has been minimized.

Copy link

@deviant-studio deviant-studio commented Mar 2, 2016

@TWiStErRob, thanks for quick response. I'll check this solution.

@patelhari

This comment has been minimized.

Copy link

@patelhari patelhari commented Jun 29, 2018

@TWiStErRob Thank you working for me in http://stackoverflow.com/a/35096552/253468 this solution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants
You can’t perform that action at this time.