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

Add Gif loop end listener #3438

Merged
merged 1 commit into from
Jan 10, 2019
Merged

Add Gif loop end listener #3438

merged 1 commit into from
Jan 10, 2019

Conversation

AnwarShahriar
Copy link
Contributor

@AnwarShahriar AnwarShahriar commented Dec 2, 2018

Description

This PR adds a listener to the GifDrawable to listen to gif loop end, when it's explicitly specified by the user.

Possibly solves #1872

Motivation and Context

I worked on a feature for showing and stopping a gif after certain loop count. The spec was,

  • Show Gif in a RecyclerView
  • Render it as a static picture (first frame of the Gif), but with an UI indicator that the picture is a gif
  • When the user taps on the gif, hide the UI indicator and start playing it
  • After 3 times of looping, stop the Gif and show the UI indicator again on top of the static first frame again.

To implement the above spec, (showing and hiding some UI stuff based on the Gif start/stop) I need to know when the Gif finishes looping 3 times. As glide doesn't expose decoder from GifDrawable and we cannot use anything other than GifDrawable in Request listener for gifs, I had to add a callback that can notify client when certain amount of looping finishes. This solution solves my issue and I wanted to share it with the community.

Please, let me know if it can be done in better way or any more work I need to do.

@AnwarShahriar AnwarShahriar force-pushed the master branch 3 times, most recently from 3952f09 to 7fa56dc Compare December 2, 2018 09:10
@sjudd
Copy link
Collaborator

sjudd commented Dec 3, 2018

Good idea and thank you for putting this together. However it's probably best to stick to the Android interfaces. Can you change this to use Animatable2Compat? https://developer.android.com/reference/android/support/graphics/drawable/Animatable2Compat

@AnwarShahriar
Copy link
Contributor Author

@sjudd I'm a bit confused, and before putting more code I just want to make sure I'm understanding you correctly. Are you suggesting something like this?

@Override
public boolean onResourceReady(GifDrawable gifDrawable, Object model, Target<GifDrawable> target, DataSource dataSource, boolean isFirstResource) {
  gifDrawable.setLoopCount(3);
  gifDrawable.setGifLoopEndedListener(new Animatable2Compat.AnimationCallback() {
    @Override
    public void onAnimationEnd(Drawable drawable) {
      // Animation is done. update the UI or whatever you want
    }
  });
  return false;
}

If this is our intention, shouldn't we also provide the onAnimationStart implementation? The unused interface may create confusion. What do you think?

@sjudd
Copy link
Collaborator

sjudd commented Dec 7, 2018

@AnwarShahriar That's similar to what I'm suggesting. I'm suggesting making GifDrawable implement Animatable2Compat:

class GifDrawable implements Animatable, Animatable2Compat {
...

Your listener would just be a Animatable2Compat.AnimationCallback. You'd add and remove with the register/unregister methods in Animatable2Compat:

@Override
public boolean onResourceReady(GifDrawable gifDrawable, Object model, Target<GifDrawable> target, DataSource dataSource, boolean isFirstResource) {
  gifDrawable.setLoopCount(3);
  // Note that you'd need to unregister this in onLoadCleared to avoid a potential memory leak.
  gifDrawable.registerAnimationCallback(new Animatable2Compat.AnimationCallback() {
    @Override
    public void onAnimationEnd(Drawable drawable) {
      // Animation is done. update the UI or whatever you want
    }
  });
  return false;
}

Does that help clarify?

@AnwarShahriar AnwarShahriar force-pushed the master branch 3 times, most recently from 6ad9406 to 0cd10e6 Compare December 7, 2018 07:20
@AnwarShahriar
Copy link
Contributor Author

@sjudd updated the PR. Let me know if I need to modify anything.

@stale
Copy link

stale bot commented Dec 14, 2018

This issue has been automatically marked as stale because it has not had activity in the last seven days. It will be closed if no further activity occurs within the next seven days. Thank you for your contributions.

@stale stale bot added the stale label Dec 14, 2018
Copy link
Collaborator

@sjudd sjudd left a comment

Choose a reason for hiding this comment

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

Thanks! Sorry for the delay.

*/
@Override
public void registerAnimationCallback(@NonNull AnimationCallback animationCallback) {
if (animationCallback == null) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd either skip this check or make it a Precondition. I think with the @NonNull annotation you're fine just skipping the check.

If you do want a check, I'd prefer a Precondition because it would immediately show the developer where they went wrong, where the existing check will just silently fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

*/
@Override
public boolean unregisterAnimationCallback(@NonNull AnimationCallback animationCallback) {
if (animationCallbacks == null || animationCallback == null) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto here for animationCallback, I think you can skip the null check with the annotation (or make it a Precondition if you want one).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@AnwarShahriar
Copy link
Contributor Author

AnwarShahriar commented Dec 14, 2018

@sjudd fixed as you suggested.

@AnwarShahriar
Copy link
Contributor Author

@sjudd is there any update?

@stale
Copy link

stale bot commented Dec 29, 2018

This issue has been automatically marked as stale because it has not had activity in the last seven days. It will be closed if no further activity occurs within the next seven days. Thank you for your contributions.

@stale stale bot added the stale label Dec 29, 2018
@stale stale bot closed this Jan 6, 2019
@sjudd sjudd reopened this Jan 7, 2019
@stale stale bot removed the stale label Jan 7, 2019
Copy link
Collaborator

@sjudd sjudd left a comment

Choose a reason for hiding this comment

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

Two minor javadoc nits, otherwise looks good, thanks!

@@ -390,6 +407,47 @@ public void setLoopCount(int loopCount) {
}
}

/**
* Register callback to listen to GifDrawable animation end event after specific loop count
* set by {@link GifDrawable#setLoopCount(int)}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Worth noting that this will only be called if the Gif stops because it reaches the loop count. It doesn't look like you're notifying if someone calls stop() directly.

}

/**
* Unregister animation callback.
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can probably avoid javadoc for these methods that isn't specific to their behavior in this particular class. The javadoc for the interface will be shown by default.

@AnwarShahriar
Copy link
Contributor Author

@sjudd docs are updated

@sjudd
Copy link
Collaborator

sjudd commented Jan 10, 2019

Thanks!

@sjudd sjudd merged commit a150301 into bumptech:master Jan 10, 2019
@pietroconta
Copy link

 gifDrawable.setGifLoopEndedListener(new Animatable2Compat.AnimationCallback() {
    @Override
    public void onAnimationEnd(Drawable drawable) {
      // Animation is done. update the UI or whatever you want
    }
  });

i dont know why i have this error: Cannot resolve method 'setGifLoopEndedListener' in 'GifDrawable

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.

4 participants