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

Using null fallback also call onException listener #456

Closed
TomasValenta opened this issue May 5, 2015 · 4 comments
Closed

Using null fallback also call onException listener #456

TomasValenta opened this issue May 5, 2015 · 4 comments

Comments

@TomasValenta
Copy link

Hi Sam,

again, I am not sure if it's a bug. When I use this:

Glide.with(context)
        .load(source) //source = null
        .fallback(R.drawable.empty)
        .listener(new RequestListener<GlideUrl, GlideDrawable>() {
            @Override
            public boolean onException(Exception e, GlideUrl model,
                                       Target<GlideDrawable> target, boolean isFirstResource) {
                return false;
            }

            @Override
            public boolean onResourceReady(GlideDrawable resource, GlideUrl model, Target<GlideDrawable> target,
                                           boolean isFromMemoryCache, boolean isFirstResource) {
                return false;
            }
        })
        .into(ImageView);

then fallback working well, but onException is also called. Is that right?

@sjudd
Copy link
Collaborator

sjudd commented May 6, 2015

I'm not totally sure what the expected behavior here is. I didn't change that in Glide 3.6 to keep the behavior the same.

I think it's ok. If fallbacks actually accepted full requests, you'd probably expect to be notified that your full load failed if you added a listener, even if one of the fallbacks eventually succeeds?

@TomasValenta
Copy link
Author

If I understand correctly the idea of .fallback(int or Drawable), it's for handling null models?
Then it's not exception or it's?

Like in this table #268 .

@TWiStErRob
Copy link
Collaborator

I think onException should be called in correspondence with Target.onLoadFailed and showing .error().
I also think that null is not an exceptional case and load didn't "fail" because "null is a valid non-image" so it is expected behaviour, hence no exception. But since one of the listener methods should be called, the current behaviour may be OK. The above case can be distinguished by checking if there's an actual exception handed to onException.
However one may expect animated fallback in which case you would need onSuccess.

Sam's point is a weird case, I think the question is: Is listener for each target or for each load? and Do we consider fallback a separate load?
I could expect success to be called with fallback drawable on the main listener, but then onException was called before because we went to fallback. That is both methods were called :)

@sjudd
Copy link
Collaborator

sjudd commented Jun 13, 2015

This is hard one but I think I want to stick with the behavior we have. You can infer from the null model that onException was called because of a null model. I don't see a hugely compelling reason to change the behavior, particularly since users may currently be relying on it.

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

No branches or pull requests

3 participants