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

Allow separate handling of requests to load a thumbnail and a full-sized image using ImageManager #11308

Conversation

ashiagr
Copy link
Contributor

@ashiagr ashiagr commented Feb 14, 2020

Fixes #11304

This PR allows separate handling of thumbnail and a full-sized image requests on using ImageManager's loadWithResultListener.

To test

  • RequestListener's methods
    onLoadFailed
    onResourceReady
    are refactored to include the new param isFirstResource in the method signature.

  • ImageManager's attachRequestListener passes Glide's isFirstResource param to corresponding RequestListener methods.

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

To allow separate handling of requests to load a thumbnail and a full-sized image using ImageManager
@ashiagr ashiagr added the Media label Feb 14, 2020
@ashiagr ashiagr added this to the 14.3 milestone Feb 14, 2020
@ashiagr ashiagr self-assigned this Feb 14, 2020
@ashiagr ashiagr requested a review from malinajirka February 14, 2020 12:15
@peril-wordpress-mobile
Copy link

You can test the changes on this Pull Request by downloading the APK here.

Copy link
Contributor

@malinajirka malinajirka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM 🚢

@malinajirka malinajirka merged commit de9b364 into develop Feb 14, 2020
@malinajirka malinajirka deleted the issue/11304-allow-separate-handling-of-thumbnail-fullsized-url branch February 14, 2020 12:35
@ashiagr
Copy link
Contributor Author

ashiagr commented Feb 17, 2020

@malinajirka seems like isFirstResource is not the best option to differentiate between thumb and full-sized image. When full-sized image is in the cache, Glide straightaway picks it up next time, ignoring the thumbnail and returning isFirstResource as true for the cached full-sized image.

Can we revert this PR and replace isFirstResource with Glide's model param, it carries the url of the image being loaded?

@malinajirka
Copy link
Contributor

I mentioned it as a joke during the call, but I forgot to check it. I didn't expect Glide's API would have such a weird behavior.

Can we revert this PR and replace isFirstResource with Glide's model param, it carries the url of the image being loaded?

Yeah, SGTM. This PR is closed so we'll need to create a new PR, but it doesn't matter. Thanks!

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

Successfully merging this pull request may close these issues.

Allow separate handling of requests to load a thumbnail and a full-sized image using ImageManager
2 participants