1044275 - Update ButtonToast alpha animation to use NineOldAndroids

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: General, defect)

Description

[:Margaret Leibovic]

When bug 1044133 lands, we can switch from using our own gecko animation library to NineOldAndroids.

We should update the alpha animation in ButtonToast.java.

Comment 1

[Axel Hecht [:Pike]]

I think Claas started working on this during the mozweekend.de, Claas, do you want to take this bug?

Comment 2

[Claas Augner [:claas]]

Sorry for the delay, but yes: I would like to take this bug.

Comment 3

[Axel Hecht [:Pike]]

Nice, over to you then.

Comment 4

[Claas Augner [:claas]]

Attachment: ButtonToast.java - use NineOldAndroids

I changed ButtonToast.java to use NineOldAndroids rather than the gecko animation library. I have tested it and got the same animations as before.

If this (minimal) patch is okay, I would have an additional one which enriches the class with explanatory Javadoc.

Comment 5

[Claas Augner [:claas]]

Attachment: ButtonToast.java - better documentation

This would be my proposal for better documentation (plus extraction of the animation parts into two methods fadeIn/fadeOut).

It has patch 1044275.diff as a prerequisite.

Comment 6

[:Margaret Leibovic]

Comment on attachment 8635611 [details] [diff] [review]
ButtonToast.java - use NineOldAndroids

Review of attachment 8635611 [details] [diff] [review]:
-----------------------------------------------------------------

I tested this out locally, and when I panned the page to make the toast disappear, it looks like it immediately hides, as opposed to fading out like it did before. Do you see that when testing locally?

I'm going to redirect this review to liuche, since she has more recent experience than me with working with these animations.

Comment 7

[:Margaret Leibovic]

Comment on attachment 8635615 [details] [diff] [review]
ButtonToast.java - better documentation

Review of attachment 8635615 [details] [diff] [review]:
-----------------------------------------------------------------

This is generally looking fine, but I don't think we need the new helper methods.

::: mobile/android/base/widget/ButtonToast.java
@@ +191,5 @@
> +    /**
> +     * Fades in our toast.
> +     * @param immediate true if toast should be shown immediately.
> +     */
> +    private void fadeIn(boolean immediate) {

This method name is a bit misleading, since the toast won't actually "fade in" if the immediate parameter is true. This helper method only has one consumer, so I don't think we should split this out. Same goes with the `fadeOut` helper method below.

@@ +201,5 @@
>          animator.start();
>      }
>  
> +    /**
> +     * Hides our toast and stores the reason.

The reason isn't stored anywhere, so this isn't quite correct. The reason is just passed along to the onToastHidden listener.

@@ +256,5 @@
> +     * Returns true if the toast is not visible.
> +     */
> +    public boolean isGone() {
> +        return (mView.getVisibility() == View.GONE);
> +    }

I don't think this helper method does enough to be worth the extra layer of complexity. Let's just leave the visibility check inline above as it was before.

@@ +265,2 @@
>      public boolean isVisible() {
>          return (mView.getVisibility() == View.VISIBLE);

I do see that we have this corresponding `isVisible` method, but this is used by external consumers, which is why I think it makes sense to keep this.

Comment 8

[:Margaret Leibovic]

(Also, thanks for your patches! And thanks for helping to update the code comments!)

Comment 9

[Chenxia Liu [:liuche]]

Comment on attachment 8635611 [details] [diff] [review]
ButtonToast.java - use NineOldAndroids

Review of attachment 8635611 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/widget/ButtonToast.java
@@ +127,5 @@
>  
>          mView.setVisibility(View.VISIBLE);
>          int duration = immediate ? 0 : mView.getResources().getInteger(android.R.integer.config_longAnimTime);
>  
> +        ValueAnimator animator = ObjectAnimator.ofFloat(mView, "alpha", 0.0f, 1.0f);

Nit: make this final

@@ +150,5 @@
>          mView.clearAnimation();
>          if (immediate) {
>              mView.setVisibility(View.GONE);
>          } else {
> +            ObjectAnimator animator = ObjectAnimator.ofFloat(mView, "alpha", 1.0f, 0.0f);

Nit: final

Comment 10

[Kim Kern]

Attachment: ButtonToast.java - implement suggestions from reviews

I merged Claas' patches and refactored them according to your reviews.
I tested the animations locally with "Open in New Tab".

Comment 11

[Chenxia Liu [:liuche]]

Thanks for the patch Kim, however we're actually deprecating ButtonToast, and switching to Android snackbars. I'm sorry that this bug slipped through the cracks for being closed WONTFIX!

If you're looking for something else, you can check http://www.joshmatthews.net/bugsahoy/?mobileandroid=1&unowned=1&simple=1 , or if you want a good first JS bug, you can take a look at bug 1225563!

Let me know if you have any other questions.