1261044 - Simplify/Replace AnimatorProxy class

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

AnimatorProxy was a helper class to use the different animation frameworks through a unified interface. 
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/animation/AnimatorProxy.java

Nowadays there's only one implementation left: AnimatorProxyPostHC. So we should be able to move this code into a helper class that doesn't need to support multiple implementations.

Comment 1

[Mouaad Aallam]

Hello Sebastian! :)
Where the new class should be created (i suppose same package) ?

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

Yeah, the same package (animation) sounds reasonable. Would you like to work on that?

Comment 3

[Mouaad Aallam]

(In reply to Sebastian Kaspari (:sebastian) from comment #2)
> Yeah, the same package (animation) sounds reasonable. Would you like to work
> on that?

Yes, i will try do it; i will name the new class AnimatorProxyPostHC then.

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

I think we can go with something more generic like AnimatorUtils.

Comment 5

[Mouaad Aallam]

The class should keep the interface AnimatorProxyImpl ?

Comment 6

[Mouaad Aallam]

Attachment: Simple first intuition changes

To not stay and starting line i did a simple first intuition changes.
I looked into the current implementation of the class, and i tried to adapt the class to them, i have tested this on my device and it's working, maybe the code need more adjustments, please tell me what you think and what i need to add/remove :)

Note: i kept the same name of the class for the moment just to avoid doing a full build (the test i did was with artifact mode).

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8738323 [details] [diff] [review]
Simple first intuition changes

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

I just saw that we have a class called ViewHelper that uses AnimatorProxy:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/animation/ViewHelper.java

Let's get rid of AnimatorProxy completely and move the functionality to ViewHelper. All calls to AnimatorProxy should be redirected to ViewHelper. Instead of using a refrence to a view let's make the method static and add the view to the parameter list (like the other methods in ViewHelper).

Comment 8

[Mouaad Aallam]

(In reply to Sebastian Kaspari (:sebastian) from comment #7)

> I just saw that we have a class called ViewHelper that uses AnimatorProxy:
> https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/
> mozilla/gecko/animation/ViewHelper.java

it's used here too: https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/animation/PropertyAnimator.java#79 i think we will need to modify this class too (to get rid of the 'create' method)

Comment 9

[Mouaad Aallam]

Attachment: bug1261044.diff

Here you go :D ! in this patch :
- Deleting the class AnimatorProxy
- Updating the class ViewHelper (making the old methods static, and adding new methods from AnimatorProxy)
- Updating the class PropertyAnimator (from using AnimatorProxy to use ViewHelper)
- Updating the file moz.build (removing the class AnimatorProxy from it)

I built this with artifact (with both Android Studio and mach) and it worked for me like a charm ! :D

Comment 10

[Mouaad Aallam]

> attachment 8739624 [details] [diff] [review]
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e1d552b249b&selectedJob=19219353

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8739624 [details] [diff] [review]
bug1261044.diff

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

This looks already quite good. Can you look at the failed lint task? Running "mach gradle lint" should create a report. It seems like your change introduced and error.

::: mobile/android/base/java/org/mozilla/gecko/animation/ViewHelper.java
@@ +11,5 @@
>      private ViewHelper() {
>      }
>  
>      public static float getTranslationX(View view) {
> +        if (view != null)

NIT: Please add braces here, even to single line if statements.

Comment 12

[Mouaad Aallam]

Attachment: Previous patch update

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf4c2cafebc4

Added: Braces for 'if' statements

Comment 13

[Mouaad Aallam]

Attachment: Fix updated

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0cacd6cb0235

For the previous patch try showed a "checkstyle" error, i found out later that the error is due to one missing 'tab' missing at the closing bracets (due to the fact that i edited the java file with |nano|, the missing tab wasn't visible on |nano| somehow!).

Comment 14

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8740710 [details] [diff] [review]
Fix updated

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

LGTM. :)

I'll land the patch.

Comment 15

[Sebastian Kaspari (:sebastian; :pocmo)]

https://hg.mozilla.org/integration/fx-team/rev/d5ee38a7a2f7d2237e994a556462444c99590fcf
Bug 1261044 - Move AnimatorProxy functionality into ViewHelper. r=sebastian

Comment 16

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

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