bumptech / glide

An image loading and caching library for Android focused on smooth scrolling
https://bumptech.github.io/glide/
Other
34.59k stars 6.12k forks source link

Problems with placeholder and notifyDataSetChanged #73

Closed mwyszomierski closed 10 years ago

mwyszomierski commented 10 years ago

Hi,

I'm using glide 2.0.3. I have a ListAdapter, and I'm having trouble with photos loaded per row. Here's my sample adapter:

public class MyListAdapter {
    @Override
    public View getView(int position, View convertView, ViewGroup parent) {
            ...

            // Set a placeholder image before loading.
            holder.thumbnail.setImageResource(R.drawable.placeholder);

            // Request the image.
            String imageUrl = ...;
            Request<String> req = Glide.load(imageUrl);
            req.into(holder.thumbnail);
        }
    }

This works, but when the parent activity/fragment calls notifyDataSetChanged() on the adapter, all the imageview instances get reverted back to their placeholder resource.

I tried using glide's placeholder facility instead of explicitly resetting the image each time getView() is called:

// Don't do this:
// holder.thumbnail.setImageResource(R.drawable.placeholder);

// Use this instead:
req.placeholder(R.drawable.placeholder);

which fixes the problem of the photos disappearing when notifyDataSetChanged() is called - but it causes a separate problem with row recycling. It seems that when you're scrolling down the listview, rows that haven't finished loading their image don't get the placeholder resource set - instead they just show the last bitmap they happened to display.

Any ideas what I'm doing wrong?

Thanks

mwyszomierski commented 10 years ago

It looks like doing this:

holder.thumbnail.setImageResource(R.drawable.placeholder);

without notifying glide is wrong - I think glide is using view.tag for storage, so things will get out of sync. Doing this:

holder.thumbnail.setImageResource(R.drawable.placeholder);
holder.thumbnail.setTag(null);

seems to work, but not sure of other side effects.

sjudd commented 10 years ago

Thanks for reporting this, sorry for the confusion. The right way to do this is:

Glide.load(imageUrl).placeholder(R.drawable.placeholder).into(ImageVIew).

You're correct that calling setImageResource is not safe and is the reason why images fail to load. The 2.x version of Glide keeps track of the last url set on a view and simply ignores new requests for the same url. If you set some placeholder manually, Glide doesn't know about it and therefore assumes that the last url is still set and ignores the new request leading to images being replaced by placeholders.

You're also totally correct that Glide uses view tags to store request metadata, it's what allows Glide to reuse resources like Bitmaps. Unfortunately clearing out the tag is not safe either, if an old request is still in progress it may finish after you've cleared the tag and replace the placeholder. Clearing the tag also prevents Glide from reusing Bitmaps which will introduce jank.

It's entirely possible that there is a bug with how I'm handling placeholders that might cause them not to be set. Can you paste more of the code you used to test Glide's request.placeholder method? If you create the request in one line as I demonstrated earlier in this reply do you still see placeholders not being set?

mwyszomierski commented 10 years ago

Hi, thanks for the reply!

Yes it looks like the one-liner you posted works ok. The reason I'm trying to set the bitmaps + tag to null myself is because some of my adapter rows don't have an image url available. For example, a list of users on a social network - some users don't have a profile photo url. The scenario is like this:

public View getView(int position, View convertView, ViewGroup parent) {
    User user = getItem(position);
    if (user.getPhotoUrl() == null) {
        // Can't give glide a null I think, so was trying to do this myself:
        holder.thumbnail.setImageResource(...);
        holder.thumbnail.setTag(null);
    } else {
        // normal glide load
        Glide.load(imageUrl).into(holder.thumbnail);
    }

Giving the load method a null url will throw an exception - I'm not sure what passing an empty string will do, looks like glide is treating it as a valid url and tries to find a resource at that location. Ideally glide would be ok with a null url and clear the tag data and set the place holder for us.

Sorry if there's already a way to do this, otherwise any recommendations would be great.

Thanks

sjudd commented 10 years ago

Got it, thanks for the code, it's super helpful. Again you're completely correct. The 2.x version of Glide does throw on null urls, which in retrospect was not a good design decision. I've fixed this in the 3.0 branch so that, exactly as you suggest, Glide just cancels the previous load and sets the placeholder.

For the 2.x branch, the only the library really supports to handle this is to do almost what you have, with one slight difference:

if (user.getPhotoUrl() == null) {
    Glide.cancel(holder.thumbnail);
    holder.thumbnail.setImageResource(...);
} else {
    Glide.load(imageUrl).into(holder.thumbnail);
}

The call to Glide#cancel will make sure that no image from a previous load will be set on the view, which in turn means it's safe to set your own placeholder on the view.

You could also use empty urls. Doing so will just cause the load to fail, which in turn will cause the error drawable to be set.

Again this should all be a lot easier in the 3.x versions, which I'm hoping to have out and stable in the next couple of weeks.

Hope that answers your question!

mwyszomierski commented 10 years ago

Great thanks this works perfectly - really a fantastic library!