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

Use placeholderRes/errorRes if url is null #61

Closed
mare011 opened this issue Aug 22, 2019 · 5 comments · Fixed by #151
Closed

Use placeholderRes/errorRes if url is null #61

mare011 opened this issue Aug 22, 2019 · 5 comments · Fixed by #151
Labels
enhancement New feature or request

Comments

@mare011
Copy link

mare011 commented Aug 22, 2019

It would be cool if passing null url would use placeholder drawable or error drawable if set. Right now it defeats the purpose of fluent api when you have to do something like:

if (imageUrl != null) { it.load(imageUrl) { placeholder(placeholderRes) } } else { it.setImageResource(placeholderRes) }

@mare011 mare011 added the enhancement New feature or request label Aug 22, 2019
@colinrtwhite
Copy link
Member

To help design this, if you run:

imageView.load(null) {
    placeholder(R.drawable.placeholder)
    error(R.drawable.error)
}

Would you expect it to show the placeholder drawable or the error drawable?

@mare011
Copy link
Author

mare011 commented Aug 28, 2019

To me it would make more sense to use error when defined, and placehold as a fallback. Same goes for the case where you do have an url and there's an error.

@mlilienberg
Copy link

mlilienberg commented Oct 10, 2019

I saw multiple points of view about this topic. Some people think that null is a valid case and should show nothing. Maybe it makes more sense to define your own null strategy.

imageView.load(itemModel.imageUrl) {
   placeholder(R.drawable.ic_placeholder)
   error(R.drawable.ic_error)
   isNull(R.drawable.ic_null)
}

or

imageView.load(itemModel.imageUrl) {
   placeholder(R.drawable.ic_placeholder)
   error(R.drawable.ic_error)
   isNull(null)
}

Related topic in glide a few years ago:
bumptech/glide#268

@colinrtwhite
Copy link
Member

@mlilienberg Great point. I think it makes sense to use the same API as Glide. i.e.:

imageView.load(itemModel.imageUrl) {
   placeholder(R.drawable.ic_placeholder)
   error(R.drawable.ic_error)
   fallback(R.drawable.ic_error)
}

Similar to placeholder and error, you should be able to set the default fallback on your ImageLoader.

@colinrtwhite
Copy link
Member

This is now supported in master. To get the fix asap you can depend on 0.9.0-SNAPSHOT.

NOTE: There might be minor behaviour changes to the feature before 0.9.0 stable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants