998700 - Animate sync icon to show activity

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, enhancement)

Description

[Paul [pwd]]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux i686; rv:31.0) Gecko/20100101 Firefox/31.0 (Beta/Release)
Build ID: 20140410061737

Steps to reproduce:

If the drawer is open, animate the sync icon when there's activity

Comment 1

[Nick Alexander :nalexander [he/him]]

sabret00the: thanks for filing. 

ibarlow: this is a really fun idea!  Do you have a strong feeling for or against?

Implementation wise, now that Bug 850600 has landed (almost?), the hooks are already in place.  This is definitely a good mentor bug, but not necessarily a good first bug -- a working build environment and device to test on is needed.

The relevant places to put this in are |onSyncStarted| and |onSyncFinished|, at or around https://hg.mozilla.org/integration/fx-team/rev/ce5ba3c8eb47#l1.116

Comment 2

[Vivek Balakrishnan[:vivek]]

I would like to work on this.

Comment 3

[Nick Alexander :nalexander [he/him]]

(In reply to vivek from comment #2)
> I would like to work on this.

Hi vivek, I see you're new to Bugzilla, but it looks like you've got a working Fennec build and have already contributed some things over in Bug 977167.  Excellent!

We'd love to have your help on this ticket, but Bug 850600 has some issues so needs to be re-landed.  Can you CC yourself on that ticket so you can track its progress?  In the meantime, I'm hoping ibarlow will let us know if this is even wanted.

vivek, you could get started by trying to make the Sync icon rotate in the tabs tray all the time, since I don't know how to do that yet :)

Comment 4

[Vivek Balakrishnan[:vivek]]

Attachment: Sync icon rotate patch

With this patch, sync icon in tabs tray rotate. However, when it is selected the selector bar underneath the icon rotate with it :(

Comment 5

[Ian Barlow (:ibarlow)]

(In reply to Nick Alexander :nalexander from comment #1)
> sabret00the: thanks for filing. 
> 
> ibarlow: this is a really fun idea!  Do you have a strong feeling for or
> against?
> 

Makes sense to me, I like it. Just so we're all on the same page, here's how I understand this working. 

When a user opens the tab tray, they will see a spinning Sync icon if 

a. they happen to open the tray while a sync is taking place
b. they pull to refresh their synced tabs panel

--

We should probably set a minimum duration for the animation, as well. I am not sure how long a sync usually takes, but regardless of that we should let the icon rotate for long enough that a user sees it and gets what is going on. Maybe at least a second or so?

Comment 6

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8409495 [details] [diff] [review]
Sync icon rotate patch

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

Vivek: great start.  Thanks for figuring out how to do this rotation, at least part of it.  It looks like we need to add methods to IconTabWidget to start and stop animating, and we need to figure out how to rotate just the icon.

Bug 850600 is about to land, so in a day or two we should be able to hook this up to the refresh.

Comment 7

[Vivek Balakrishnan[:vivek]]

Attachment: imagebutton drawable rotation patch

This patch makes use of ValueAnimator to rotate the imagebutton drawable instead of the whole view. However, ValueAnimator is available only from API level 11.
nalexander : is this condition acceptable ?

Yet To Do :  
1) IconTabWidget defines layout_width as wrap_content, which adds a padding on the left and right by default to center the drawable. Since, we scale the matrix this padding information is lost. This padding_value can be calculated at runtime after the view is rendered. Then calling postTranslate(padding_value, 0)  on the matrix would center the drawable. I have verified this behavior by calculating the padding value manually from Dump View Hierarchy

2) Refactor start and Stop animation to separate functions

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Pulling Lucas in because Android and Animations have caused him some gray hair. Maybe he has some feedback on the approach.

Comment 9

[Lucas Rocha (:lucasr)]

Comment on attachment 8410706 [details] [diff] [review]
imagebutton drawable rotation patch

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

Drive-by: we can't use post-Honeycomb API unconditionally here as we need to support Froyo and Gingerbread devices. Here are a few options:
1. Add rotation APIs to our own PropertyAnimator (for both pre and post Honeycomb versions), use that here.
2. Use a RotateDrawable for when the animation is active. The animation will not be super smooth but might be good enough for this use case.
3. Use RotateAnimation for when the animation is active.
4. Conditionally animate on post-Honeycomb, no animation on pre-Honeycomb. Use Android's animation API.

Option 1 is more work but it's more future-proof and will give us smooth animations. Options 2 and 3 are simpler and might give us good enough results. I'd like to avoid option 4 if possible as we're not using Android's animation API anywhere else in our code.

Comment 10

[Nick Alexander :nalexander [he/him]]

Bug 850600 landed!  Looping in jdover, since he pushed that one over the line.

We can add the animation start/stop calls now, at, respectively:

https://hg.mozilla.org/mozilla-central/rev/6fa987a53e37#l1.126
https://hg.mozilla.org/mozilla-central/rev/6fa987a53e37#l1.117

Comment 11

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8410706 [details] [diff] [review]
imagebutton drawable rotation patch

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

Let's try one of lucasr's suggestions (suggestion #3): let's use RotateAnimation.   Suggestion #1 is a good deal of work; RotateDrawable looks like as much work.  I found http://stackoverflow.com/questions/12882967/rotate-animation-android looking for information about using RotateAnimation.

We don't want to switch on the Android resource ID; instead, let's try adding start/stop animating methods to IconTabWidget.  Then we can call the helper methods from the points I referenced in an earlier comment.

Comment 12

[Aaron Train [:aaronmt]]

Laser show? Ship it.

https://www.youtube.com/watch?v=9V91PMiSVU0

Comment 13

[Vivek Balakrishnan[:vivek]]

Attachment: 998700.patch

Helper functions plugged-in to start and stop the animation.

Comment 14

[Nick Alexander :nalexander [he/him]]

This is pretty ugly, but: smooth animations based on rotation are unlikely to work here, because such animations are view based, and ImageButton's view includes its state indicators.  vivek's patches have shown that the state indicator rotates with the view.

What does work is defining an animation-list in resources/drawable/tabs_synced_syncing.xml, and using code like:

        // Get the sync icon view, set the animation and start the animation.
        final ImageButton button = (ImageButton) mTabWidget.getChildAt(2);
        button.setImageResource(R.drawable.remote_tabs_animation);
        final AnimationDrawable animationDrawable = (AnimationDrawable) button.getDrawable();
        animationDrawable.start();

to start the drawable animating.

lucasr, this is not one of your suggestions, *and* animation-list is not used elsewhere in Fennec.  Presumably there's a reason for that, care to comment?

Thinking more about this, it seems like we could in fact implement 1. with a custom PropertyAnimator that updated the RotateDrawable.  That's a good deal of work.

On the other hand, this would require a finite set of image resources, say 6-12 different rotated states.  Ugh.  That's work for ibarlow.

Comment 15

[Lucas Rocha (:lucasr)]

(In reply to Nick Alexander :nalexander from comment #14)
> This is pretty ugly, but: smooth animations based on rotation are unlikely
> to work here, because such animations are view based, and ImageButton's view
> includes its state indicators.  vivek's patches have shown that the state
> indicator rotates with the view.
> 
> What does work is defining an animation-list in
> resources/drawable/tabs_synced_syncing.xml, and using code like:
> 
>         // Get the sync icon view, set the animation and start the animation.
>         final ImageButton button = (ImageButton) mTabWidget.getChildAt(2);
>         button.setImageResource(R.drawable.remote_tabs_animation);
>         final AnimationDrawable animationDrawable = (AnimationDrawable)
> button.getDrawable();
>         animationDrawable.start();
> 
> to start the drawable animating.
> 
> lucasr, this is not one of your suggestions, *and* animation-list is not
> used elsewhere in Fennec.  Presumably there's a reason for that, care to
> comment?

I think we've used animation list in the past (in an old version of the toolbar throbber I think?). The problem is that the framerate kinda sucks i.e. the rotation is not very smooth. However, it might look good enough in the context of this bug as this is not a very prominent part of the UI.

> Thinking more about this, it seems like we could in fact implement 1. with a
> custom PropertyAnimator that updated the RotateDrawable.  That's a good deal
> of work.

Yeah, that wouldn't be a trivial refactoring given that PropertyAnimator is very view-oriented at the moment.

> On the other hand, this would require a finite set of image resources, say
> 6-12 different rotated states.  Ugh.  That's work for ibarlow.

Yeah, that's an annoying drawback of using AnimationDrawables.

Comment 16

[Nick Alexander :nalexander [he/him]]

Attachment: Animate sync icon during refresh of Remote Tabs panel. r=lucasr

It turns out that animation-list can be combined with rotate to avoid
shipping extra bitmaps, and just six frames is relatively smooth
enough.  This patch takes that approach.

The trickiest part was exposing the ImageButton view in a reasonable way
to the inner PanelView.

Comment 17

[Nick Alexander :nalexander [he/him]]

vivek: I'm sorry to post a patch over-top of yours.  I know you spent some time in this code, so if you have any comments or suggestions, please let me know.

Comment 18

[Lucas Rocha (:lucasr)]

Comment on attachment 8420434 [details] [diff] [review]
Animate sync icon during refresh of Remote Tabs panel. r=lucasr

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

Looking good overall. Needs some questions answered and some final tweaks to give r+.

::: mobile/android/base/resources/drawable/tabs_synced_animation.xml
@@ +5,5 @@
> +         frame-by-frame rotation animation without adding lots of small
> +         resources. The inner drawables rotate the master drawable. -->
> +    <item
> +        android:drawable="@drawable/tabs_synced"
> +        android:duration="150"/>

I wish we could inline all the rotate drawables here. Have you tried doing that to see if it works? I think it doesn't but it's worth a try :-)

::: mobile/android/base/tabspanel/RemoteTabsContainer.java
@@ +34,5 @@
> +     * Refresh indicators (the swipe-to-refresh "laser show" and the spinning
> +     * icon) will never be shown for less than the following duration, in
> +     * milliseconds.
> +     */
> +    private static final long MINIMUM_REFRESH_INDICATOR_DURATION_IN_MILLISECONDS = 1000;

This is a rather long constant name :-) ...IN_MS maybe?

@@ +127,5 @@
>          public Account getAccount() {
>              return FirefoxAccounts.getFirefoxAccount(getContext());
>          }
>  
> +        public void onSyncStarted() {

Does this run off of main thread? Just wondering why lastSyncStarted needs to be volatile.

@@ +132,4 @@
>              ThreadUtils.postToUiThread(new Runnable() {
>                  @Override
>                  public void run() {
> +                    lastSyncStarted = System.currentTimeMillis();

Use SystemClock.uptimeMillis() instead as it's a more reliable way of tracking relative time.

@@ +158,5 @@
> +
> +            // ... but we want the refresh indicators to persist for long enough
> +            // to be visible.
> +            final long last = lastSyncStarted;
> +            final long now = System.currentTimeMillis();

Ditto for SystemClock.uptimeMillis()

@@ +168,5 @@
>                      setRefreshing(false);
> +
> +                    // Get the sync icon view and stop the drawable's animation.
> +                    final ImageButton icon = panel.getIcon(Panel.REMOTE_TABS);
> +                    if (icon == null) {

In what situations would icon be null? Is it an issue you'd actually want to ignore?

::: mobile/android/base/widget/IconTabWidget.java
@@ +72,5 @@
>                  mListener.onTabChanged(mIndex);
>          }
>      }
> +
> +    public ImageButton getIcon(int index) {

Icon doesn't seem like a good name to use here because the child views in IconTabWidget can have both an icon and a label. Maybe getTabView or something?

Comment 19

[Nick Alexander :nalexander [he/him]]

(In reply to Lucas Rocha (:lucasr) from comment #18)
> Comment on attachment 8420434 [details] [diff] [review]
> Animate sync icon during refresh of Remote Tabs panel. r=lucasr
> 
> Review of attachment 8420434 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looking good overall. Needs some questions answered and some final tweaks to
> give r+.
> 
> ::: mobile/android/base/resources/drawable/tabs_synced_animation.xml
> @@ +5,5 @@
> > +         frame-by-frame rotation animation without adding lots of small
> > +         resources. The inner drawables rotate the master drawable. -->
> > +    <item
> > +        android:drawable="@drawable/tabs_synced"
> > +        android:duration="150"/>
> 
> I wish we could inline all the rotate drawables here. Have you tried doing
> that to see if it works? I think it doesn't but it's worth a try :-)

Agree, but I tried and it's not possible.

> ::: mobile/android/base/tabspanel/RemoteTabsContainer.java
> @@ +34,5 @@
> > +     * Refresh indicators (the swipe-to-refresh "laser show" and the spinning
> > +     * icon) will never be shown for less than the following duration, in
> > +     * milliseconds.
> > +     */
> > +    private static final long MINIMUM_REFRESH_INDICATOR_DURATION_IN_MILLISECONDS = 1000;
> 
> This is a rather long constant name :-) ...IN_MS maybe?

Sure, whatever.  Autocomplete, man :)

> @@ +127,5 @@
> >          public Account getAccount() {
> >              return FirefoxAccounts.getFirefoxAccount(getContext());
> >          }
> >  
> > +        public void onSyncStarted() {
> 
> Does this run off of main thread? Just wondering why lastSyncStarted needs
> to be volatile.

I suppose it doesn't need to be.

> @@ +132,4 @@
> >              ThreadUtils.postToUiThread(new Runnable() {
> >                  @Override
> >                  public void run() {
> > +                    lastSyncStarted = System.currentTimeMillis();
> 
> Use SystemClock.uptimeMillis() instead as it's a more reliable way of
> tracking relative time.
> 
> @@ +158,5 @@
> > +
> > +            // ... but we want the refresh indicators to persist for long enough
> > +            // to be visible.
> > +            final long last = lastSyncStarted;
> > +            final long now = System.currentTimeMillis();
> 
> Ditto for SystemClock.uptimeMillis()
> 
> @@ +168,5 @@
> >                      setRefreshing(false);
> > +
> > +                    // Get the sync icon view and stop the drawable's animation.
> > +                    final ImageButton icon = panel.getIcon(Panel.REMOTE_TABS);
> > +                    if (icon == null) {
> 
> In what situations would icon be null? Is it an issue you'd actually want to
> ignore?

It could be null in error situations, so you still need to do something (or crash, which is something) when you get null.  But I really added the check because IconTabWidget's "button" is a textview sometimes.  I return null in that case.

> ::: mobile/android/base/widget/IconTabWidget.java
> @@ +72,5 @@
> >                  mListener.onTabChanged(mIndex);
> >          }
> >      }
> > +
> > +    public ImageButton getIcon(int index) {
> 
> Icon doesn't seem like a good name to use here because the child views in
> IconTabWidget can have both an icon and a label. Maybe getTabView or
> something?

I elected to return null when you don't have an icon.  The point being, if we make it getInnerView or whatever, then consumers have to interrogate the returned type, or interrogate IconTabWidget's (currently private) state to figure out what is exactly present.  That's awful, so I just ask for the icon, and accept null if there isn't one.  What say you?

Comment 20

[Lucas Rocha (:lucasr)]

(In reply to Nick Alexander :nalexander from comment #19)
> (In reply to Lucas Rocha (:lucasr) from comment #18)
> > Comment on attachment 8420434 [details] [diff] [review]
> > Animate sync icon during refresh of Remote Tabs panel. r=lucasr
> > 
> > Review of attachment 8420434 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Looking good overall. Needs some questions answered and some final tweaks to
> > give r+.
> > 
> > ::: mobile/android/base/resources/drawable/tabs_synced_animation.xml
> > @@ +5,5 @@
> > > +         frame-by-frame rotation animation without adding lots of small
> > > +         resources. The inner drawables rotate the master drawable. -->
> > > +    <item
> > > +        android:drawable="@drawable/tabs_synced"
> > > +        android:duration="150"/>
> > 
> > I wish we could inline all the rotate drawables here. Have you tried doing
> > that to see if it works? I think it doesn't but it's worth a try :-)
> 
> Agree, but I tried and it's not possible.

:-(
 
> > ::: mobile/android/base/tabspanel/RemoteTabsContainer.java
> > @@ +34,5 @@
> > > +     * Refresh indicators (the swipe-to-refresh "laser show" and the spinning
> > > +     * icon) will never be shown for less than the following duration, in
> > > +     * milliseconds.
> > > +     */
> > > +    private static final long MINIMUM_REFRESH_INDICATOR_DURATION_IN_MILLISECONDS = 1000;
> > 
> > This is a rather long constant name :-) ...IN_MS maybe?
> 
> Sure, whatever.  Autocomplete, man :)

It's the reading part that bugs me, not the writing :-)

> > @@ +127,5 @@
> > >          public Account getAccount() {
> > >              return FirefoxAccounts.getFirefoxAccount(getContext());
> > >          }
> > >  
> > > +        public void onSyncStarted() {
> > 
> > Does this run off of main thread? Just wondering why lastSyncStarted needs
> > to be volatile.
> 
> I suppose it doesn't need to be.

Ok.

> > @@ +132,4 @@
> > >              ThreadUtils.postToUiThread(new Runnable() {
> > >                  @Override
> > >                  public void run() {
> > > +                    lastSyncStarted = System.currentTimeMillis();
> > 
> > Use SystemClock.uptimeMillis() instead as it's a more reliable way of
> > tracking relative time.
> > 
> > @@ +158,5 @@
> > > +
> > > +            // ... but we want the refresh indicators to persist for long enough
> > > +            // to be visible.
> > > +            final long last = lastSyncStarted;
> > > +            final long now = System.currentTimeMillis();
> > 
> > Ditto for SystemClock.uptimeMillis()
> > 
> > @@ +168,5 @@
> > >                      setRefreshing(false);
> > > +
> > > +                    // Get the sync icon view and stop the drawable's animation.
> > > +                    final ImageButton icon = panel.getIcon(Panel.REMOTE_TABS);
> > > +                    if (icon == null) {
> > 
> > In what situations would icon be null? Is it an issue you'd actually want to
> > ignore?
> 
> It could be null in error situations, so you still need to do something (or
> crash, which is something) when you get null.  But I really added the check
> because IconTabWidget's "button" is a textview sometimes.  I return null in
> that case.

Ok, fair enough.

> > ::: mobile/android/base/widget/IconTabWidget.java
> > @@ +72,5 @@
> > >                  mListener.onTabChanged(mIndex);
> > >          }
> > >      }
> > > +
> > > +    public ImageButton getIcon(int index) {
> > 
> > Icon doesn't seem like a good name to use here because the child views in
> > IconTabWidget can have both an icon and a label. Maybe getTabView or
> > something?
> 
> I elected to return null when you don't have an icon.  The point being, if
> we make it getInnerView or whatever, then consumers have to interrogate the
> returned type, or interrogate IconTabWidget's (currently private) state to
> figure out what is exactly present.  That's awful, so I just ask for the
> icon, and accept null if there isn't one.  What say you?

If you only need the icon drawable in all cases, maybe this method should actually be 'Drawable getIcon()', in which case a null return would make more sense i.e. meaning 'no icon' instead of 'this is not an ImageButton'.

Comment 21

[Nick Alexander :nalexander [he/him]]

Attachment: Animate sync icon during refresh of Remote Tabs panel. r=lucasr

I bumped the animation list to twelve frames: antlam thought it was
jumpy, and I didn't want to redefine the existing frames.

I do see that the lastSyncStarted variable is, in fact, read and
written on different threads, so I kept it volatile.  If this is
inappropriate, I'll drop that.  I *really* don't think it'll be a perf
concern :)

I uncovered a bug testing on older devices; so the Drawable fetching
code has been made more sophisticated.

Comment 22

[Lucas Rocha (:lucasr)]

Comment on attachment 8426699 [details] [diff] [review]
Animate sync icon during refresh of Remote Tabs panel. r=lucasr

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

Great.

Comment 23

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/fx-team/rev/d188c5fbf21e

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d188c5fbf21e