Closed Bug 1127139 Opened 9 years ago Closed 9 years ago

BounceAnimator incorrectly overrides start

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

(firefox40 fixed)

RESOLVED FIXED
Firefox 40
Tracking Status
firefox40 --- fixed

People

(Reporter: liuche, Assigned: tomas.flek, Mentored)

Details

(Whiteboard: [good first bug][lang=java])

Attachments

(2 files, 2 obsolete files)

While implementing first run work, I found that BounceAnimator (for making fine-tuned bounce animations) doesn't correctly override start() for the animation, so it interferes with other animations - for example, it can't be added to an AnimatorSet.

I took a look at some of the Android animation code for ValueAnimator and ObjectAnimator, and since the handler code for queuing and starting animations is hidden, we should just turn BounceAnimator into a BounceAnimatorBuilder that returns the AnimatorSet represented by the Builder that the caller can then treat as an Animator.
Assignee: liuche → nobody
Mentor: liuche
Whiteboard: [good first bug][lang=java]
Hello!
I would really like to work on this issue. It is my firt attempt to contribute in open source project so it will be fine if someone could guide me through. 
I'll start by following steps here:  https://developer.mozilla.org/en-US/docs/Introduction

Could you please provide me more information about the issue and where I can find files realated to it? I would also appreaciate "steps-to-reproduce" if the bug is somehow reflected in UI.
Thanks!
Hi Tomas, welcome!

You'll need to get a local build of Firefox for Android, which you can do by following the setup instructions here: https://wiki.mozilla.org/Mobile/Fennec/Android
That way, you can make changes locally so that you can upload a patch to be reviewed.

If you run into any problems, drop into #mobile and ask for help - there are almost always people around if you have any questions, or ask in-bug.

BounceAnimator is used in the bouncing titles on the home pager screen, where you can see the "History" and "Bookmarks" title bounce in from each side. Currently, the BounceAnimator isn't a proper Animator that can be chained with an AnimatorSet. This should be fixed by turning the BounceAnimator into a BounceAnimatorBuilder, that returns an Animator instance.

To repro the bug, try chaining all three animators into an AnimatorSet at http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/HomePagerTabStrip.java#126 - the AnimatorSet fails to run correctly.
Hi again,

I had finally time to dig in. I've turned BounceAnimator into BounceAnimatorBuilder with build() method returning new AnimatorSet created from inner animatorChain List. Then updated HomePagerTabStrip to test my change.

Animation is now created as follows:

final AnimatorSet bounceAnimatorSet = new AnimatorSet();
bounceAnimatorSet.playTogether(prevBounceAnimatorBuilder.build(), nextBounceAnimatorBuilder.build());
bounceAnimatorSet.setStartDelay(ANIMATION_DELAY_MS);

bounceAnimatorSet.start();

I've tested it on real device Samsung Galaxy S4 (5.0.1) and the bounce animation looks good to me. Can you please give me some feedback and give me an advice, how to create patch? 
Thanks!
Great! Thanks, Tomas, sounds like you've already gotten this patch worked out!

To make a patch, you'll need to enable mercurial queues, and then just us the command
 hg qnew
to put those changes into a patch.

See:
https://developer.mozilla.org/en-US/docs/Mercurial_Queues
(The warning you see is because we're starting to move to using hg bookmarks, but right now, the review software needs extra permissions so we can just use patch files.)

Since we use commit messages to associate with bugs, make sure the commit message is of the format:
Bug <number> - <title>. r=<reviewer>
like:
Bug 1127139 - BounceAnimator incorrectly overrides start. r=liuche
(You can use qref -e to rename a patch.)

Then just upload the patch - select r? in the review dropdown, and type liuche to flag me for review.
Assignee: nobody → tomas.flek
Status: NEW → ASSIGNED
Flags: needinfo?(liuche)
Attached patch Bug-1127139-fix.patch (obsolete) — Splinter Review
Patch attached for review. 

I forgot to mention that in previous post but while I was testing my changes I had to leave the name of the "BounceAnimator" unchanged so I could compile it. Gecko engine .jar file expected that name during compilation. The patch contains already refactored name of the class to "BounceAnimatorBuilder" but it is not tested. I think you'd know where else it should be configured so building script knows that it should use renamed file :)

Thanks!
Attachment #8597715 - Flags: review?(liuche)
Comment on attachment 8597715 [details] [diff] [review]
Bug-1127139-fix.patch

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

Nice, take a look at mobile/android/base/moz.build to see where the filenames are included. I'm glad to see that you used 'hg mv' to rename the filename - that's good, because that means we can see the history of the file.

In the future, you can use mxr.mozilla.org to search for strings to find where they are used - it's a really useful tool if you're touching multiple files.

This looks good! Send it back to me again with these changes, and I'll r+ it :)

::: mobile/android/base/animation/BounceAnimatorBuilder.java
@@ +46,5 @@
>          animatorChain.add(animator);
>      }
>  
>      @Override
>      public void start() {

Since you're just calling the super here and not making any other calls, you should just remove this method from this subclass, because we're not actually overriding anything.
Attachment #8597715 - Flags: review?(liuche) → feedback+
Hey Tomas, I looked over your patch, so if you could address the comments and upload a new version of the patch, I can review it again and (hopefully) land it! :)
Flags: needinfo?(tomas.flek)
Hello, I hope I can upload fixed patch today. I didn't have machine which I work on available until today.
Flags: needinfo?(tomas.flek)
Attached patch Bug-1127139-fix2.patch (obsolete) — Splinter Review
Hi again, I have finally created new patch. It contains also changed dependency in moz.build file so I it is also tested with refactored name. Hope it's OK now :)
Attachment #8597715 - Attachment is obsolete: true
Attachment #8599835 - Flags: review?(liuche)
Comment on attachment 8599835 [details] [diff] [review]
Bug-1127139-fix2.patch

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

Thanks Tomas! This looks great :) Just a few small changes. (Sorry for the back and forth, now that you got it building, I saw a few more things this time when actually running the patch.)

::: mobile/android/base/home/HomePagerTabStrip.java
@@ +116,5 @@
>  
> +        final BounceAnimatorBuilder nextBounceAnimatorBuilder = new BounceAnimatorBuilder(nextTextView, "translationX");
> +        nextBounceAnimatorBuilder.queue(new Attributes(-bounceDistance, BOUNCE1_MS));
> +        nextBounceAnimatorBuilder.queue(new Attributes(bounceDistance/4, BOUNCE2_MS));
> +        nextBounceAnimatorBuilder.queue(new Attributes(0, BOUNCE4_MS));

I noticed that BOUNCE3_MS isn't used at all - if you want, could you clean that up in this patch as well? Just remove BOUNCE4 and use BOUNCE3 everywhere (since they are the same value).

@@ +121,4 @@
>  
> +        final AnimatorSet bounceAnimatorSet = new AnimatorSet();
> +        bounceAnimatorSet.playTogether(prevBounceAnimatorBuilder.build(), nextBounceAnimatorBuilder.build());
> +        bounceAnimatorSet.setStartDelay(ANIMATION_DELAY_MS);

It also looks like the ANIMATION_DELAY_MS is now too long now that we combined these two bounces into one - would you mind changing the animation timing so that it looks the way it was before? This may also be related to race conditions with the alphaAnimator...so see the next comment!

@@ +127,3 @@
>  
>          // Start animations.
>          alphaAnimatorSet.start();

You should also add the alphaAnimator to the same set as the bounceAnimator. Something like titlesAnimator.playTogether(bounceAnimatorSet, alphaAnimatorSet), since bounce and alpha should be linked, and hadn't been because the bounceAnimator wasn't working properly.
Attachment #8599835 - Flags: review?(liuche) → feedback+
Hi again. I've removed unused BOUNCE4_MS variable and replaced it's occurrence by BOUNCE3_MS. I've also created new titlesAnimatorSet which now chains all animations. Animation delay is set for all animations in titlesAnimatorSet. Please check if the animation delay is OK now :)
Attachment #8599835 - Attachment is obsolete: true
Attachment #8601354 - Flags: review?(liuche)
Sorry I didn't get to this today, but I will definitely review it for you tomorrow!
Comment on attachment 8601354 [details] [diff] [review]
Bug-1127139-fix3.patch

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

Thanks Tomas! This looks great.

I'm going to tweak some of these delay values (which would be hard for you to do, because it's somewhat subjective), and land this patch for you.

Thanks for contributing! :)
Attachment #8601354 - Flags: review?(liuche) → review+
https://hg.mozilla.org/mozilla-central/rev/441bcf641aa7
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.