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.

Sign up for GitHub

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

Jump to bottom

.load(null) can be a valid .load request #268

Closed
TWiStErRob opened this issue Nov 16, 2014 · 7 comments
Closed

.load(null) can be a valid .load request #268

TWiStErRob opened this issue Nov 16, 2014 · 7 comments
Labels
enhancement
Milestone
3.6.0

Comments

@TWiStErRob
Copy link
Collaborator

In my app the user can create an entity and then take an OPTIONAL image for that entity.
If the image is not taken the entity's category is shown as the image. Category is mandatory, though it may be called unknown category which is visualized with a styled questionmark :)

So when loading with:

Entity e = ...;
Glide.with(this)
     .error(R.drawable.error) // red cross (Android res)
     .placeholder(e.categoryDrawable) // category's image (SVG)
     .load(e.imagePath) // user taken image (JPG)
     .into(imageView);

it may be expected to not take any action after setting the .fallback drawable:

.load While loading Load finished
null fallback fallback (currently error)
valid path fallback load
Exception fallback error

The current implementation treats null as an error, but in my case it's a valid non-image. This means that if a user didn't give take a picture it would display a red cross with Glide 3.4.
If I place the category image in .error() then I lose error indication, which may be appealing to the user (they never get a red cross). But while developing that's not useful and since it's an unexpected case I'm fine with users seeing it. So my current implementation is:

if (e.imagePath == null) {
    // note that categoryDrawable is never null, since it is used as a placeholder as well
    imageView.setImageDrawable(e.categoryDrawable);
} else {
    Glide.with(this).error(R.drawable.error).placeholder(e.categoryDrawable).load(e.imagePath).into(imageView);
}
@alex2069
Copy link

Just to justify this a little more - we also do this in a number of projects.
For us null is perfectly valid, in which case a default value is used - this is frequently the placeholder, but not always.
Perhaps a .default() option which is used when the request is null? If default is not set and null is used, then it could fallback like normal, preserving compatibility with old code/uses.

@Jogan
Copy link

Jogan commented Apr 8, 2015

+1

@TWiStErRob TWiStErRob mentioned this issue Apr 14, 2015
Closed
@sjudd sjudd added this to the 3.6.0 milestone May 1, 2015
sjudd added a commit to sjudd/glide that referenced this issue May 1, 2015
@sjudd
Add a fallback Drawable to display for null models
c8b907c 
Fixes bumptech#268
sjudd added a commit to sjudd/glide that referenced this issue May 1, 2015
@sjudd
Add a fallback Drawable to display for null models
8adaf7a 
Fixes bumptech#268
@sjudd sjudd closed this as completed in 9548211 May 1, 2015
@TWiStErRob
Copy link
Collaborator Author

Nice :) Thanks @sjudd! So now, instead of the above if, I can use:

Glide.with(this)
     .placeholder(e.categoryDrawable) // while loading
     .fallback(e.categoryDrawable) // if load was null
     .error(R.drawable.error) // e.g. if path is not valid image
     .load(e.imagePath) // null or non-null image
     .into(imageView);

and it's backward compatible!

Too bad that I actually diverted from using this method because categoryDrawable is actually an SVG drawable that needs async loading. When I wrote this issue, I had my own cache and decoded SVGs on the UI thread. Would it be possible to extend the fallback interface to accept a full request builder?

@TWiStErRob
Copy link
Collaborator Author

Doing so would also help this scenario, however that 404 may be a little stretch, because that would validly display the error drawable.

@eicm
Copy link

eicm commented May 1, 2015

it would have been more useful if fallback() was able to take any param that load() can take.

@sjudd sjudd reopened this May 1, 2015
@sjudd
Copy link
Collaborator

sjudd commented May 1, 2015

We can re-open and target taking a second request later. It's vastly more complicated to do so, and in many ways similar to functionality you can already get out of thumbnail.

@sjudd
Copy link
Collaborator

sjudd commented May 1, 2015

Actually, lets close this an open a new issue: #451

@sjudd sjudd closed this as completed May 1, 2015
@TomasValenta TomasValenta mentioned this issue May 6, 2015
Closed
@TWiStErRob TWiStErRob mentioned this issue May 29, 2015
Closed
@TWiStErRob TWiStErRob mentioned this issue Nov 23, 2015
Closed
@TWiStErRob TWiStErRob mentioned this issue Mar 21, 2016
Closed
@mlilienberg mlilienberg mentioned this issue Oct 10, 2019
Closed
@Ph0tonic Ph0tonic mentioned this issue Mar 26, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement
Projects
None yet
Milestone
3.6.0
Development

No branches or pull requests
5 participants
@Jogan @alex2069 @sjudd @TWiStErRob @eicm