Closed Bug 1261044 Opened 4 years ago Closed 4 years ago

Simplify/Replace AnimatorProxy class

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: sebastian, Assigned: Aallam.Mouaad, Mentored)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
Hello Sebastian! :)
Where the new class should be created (i suppose same package) ?
Flags: needinfo?(s.kaspari)
Yeah, the same package (animation) sounds reasonable. Would you like to work on that?
Flags: needinfo?(s.kaspari)
(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.
I think we can go with something more generic like AnimatorUtils.
The class should keep the interface AnimatorProxyImpl ?
Flags: needinfo?(s.kaspari)
Attached patch Simple first intuition changes (obsolete) — Splinter Review
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).
Attachment #8738323 - Flags: review?(s.kaspari)
Attachment #8738323 - Attachment is patch: true
Attachment #8738323 - Attachment mime type: text/x-patch → text/plain
Flags: needinfo?(s.kaspari)
Assignee: nobody → Aallam.Mouaad
Status: NEW → ASSIGNED
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).
Attachment #8738323 - Flags: review?(s.kaspari) → feedback+
(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)
Attached patch bug1261044.diff (obsolete) — Splinter Review
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
Attachment #8738323 - Attachment is obsolete: true
Attachment #8739624 - Flags: review?(s.kaspari)
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.
Attachment #8739624 - Flags: review?(s.kaspari) → feedback+
Attached patch Previous patch update (obsolete) — Splinter Review
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf4c2cafebc4

Added: Braces for 'if' statements
Attachment #8739624 - Attachment is obsolete: true
Attachment #8740595 - Flags: review?(s.kaspari)
Attached patch Fix updatedSplinter Review
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!).
Attachment #8740595 - Attachment is obsolete: true
Attachment #8740595 - Flags: review?(s.kaspari)
Attachment #8740710 - Flags: review?(s.kaspari)
Comment on attachment 8740710 [details] [diff] [review]
Fix updated

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

LGTM. :)

I'll land the patch.
Attachment #8740710 - Flags: review?(s.kaspari) → review+
https://hg.mozilla.org/mozilla-central/rev/d5ee38a7a2f7
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
You need to log in before you can comment on or make changes to this bug.