Update ButtonToast alpha animation to use NineOldAndroids

RESOLVED WONTFIX

Status

()

defect
RESOLVED WONTFIX
5 years ago
4 years ago

People

(Reporter: Margaret, Assigned: dreamerkiwi, Mentored)

Tracking

Trunk
All
Android
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mozweekend])

Attachments

(3 attachments)

(Reporter)

Description

5 years ago
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.
I think Claas started working on this during the mozweekend.de, Claas, do you want to take this bug?
Whiteboard: [mozweekend]
Sorry for the delay, but yes: I would like to take this bug.
Nice, over to you then.
Assignee: nobody → mozilla
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.
Flags: needinfo?(margaret.leibovic)
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.
Attachment #8635611 - Attachment description: 1044275.diff → ButtonToast.java - use NineOldAndroids
Attachment #8635615 - Flags: review?(margaret.leibovic)
Attachment #8635611 - Flags: review?(margaret.leibovic)
(Reporter)

Comment 6

4 years ago
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.
Attachment #8635611 - Flags: review?(margaret.leibovic) → review?(liuche)
(Reporter)

Comment 7

4 years ago
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.
Attachment #8635615 - Flags: review?(margaret.leibovic) → feedback+
(Reporter)

Comment 8

4 years ago
(Also, thanks for your patches! And thanks for helping to update the code comments!)
Flags: needinfo?(margaret.leibovic)
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
Attachment #8635611 - Flags: review?(liuche) → review+
Assignee: mozilla → dreamerkiwi
(Assignee)

Comment 10

4 years ago
I merged Claas' patches and refactored them according to your reviews.
I tested the animations locally with "Open in New Tab".
Attachment #8690603 - Flags: feedback?(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.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → WONTFIX
Attachment #8690603 - Flags: feedback?(liuche)
You need to log in before you can comment on or make changes to this bug.