Closed Bug 863828 Opened 11 years ago Closed 11 years ago

New tab increment animation

Categories

(Firefox for Android Graveyard :: General, defect)

Product:
Firefox for Android Graveyard
Component:
General
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Firefox 23

People

(Reporter: ibarlow, Assigned: wesj)

References

Details

(Whiteboard: ui-hackathon)

Attachments

(2 files, 3 obsolete files)

Patch v2.1
31.90 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
Test fixes
2.03 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review 
bug 863379 gave us a nice new tab icon. This bug should give us a lovely and subtle tab increment animation.

Please refer to the first example in this video (click video to advance) http://cl.ly/1J361E1p3q1N
No longer blocks: 863379
Blocks: 863379
Whiteboard: ui-hackathon
Attached patch WIP patch (obsolete) — — Splinter Review 
Putting a stake in the ground. This works and looks ok, but I think I need to write a 3d rotation animation to get the effect from the video.
(In reply to Wesley Johnston (:wesj) from comment #1)
> Created attachment 741608 [details] [diff] [review]
> WIP patch
> 
> Putting a stake in the ground. This works and looks ok, but I think I need
> to write a 3d rotation animation to get the effect from the video.

Quick feedback: given that the tab count is getting fancier now, you should probably encapsulate the animation bits into a separate custom view with a simpler API to update the number in it.
Wes, you touched it, you own it :-)
Assignee: nobody → wjohnston
Also: a test build would probably be useful here for Ian's feedback.
It occured to me that I gave Sriram the tab counter icon as two parts, to make it possible for the animation look like the video. Not sure if those graphics made it into code before he left, so here they are again. 

http://cl.ly/0a3a3a0q1E27
Cleaning up the patch, but here's a build for you to look at ian:

http://people.mozilla.com/~wjohnston/anim.apk
Attached patch Patch v1 (obsolete) — — Splinter Review 
This is ready (except for ian's tweaks). I think this is actually SIMPLER than what we had before, and I really don't see a huge need to put it in its own class. In fact, we wouldn't need much of this code at all if we weren't trying to be 3d-ish.
Attachment #741988 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 741988 [details] [diff] [review]
Patch v1

Clearing reivew. need to put back in the backwards playing stuff!
Attachment #741988 - Flags: review?(lucasr.at.mozilla)
Attached patch Patch v2 (obsolete) — — Splinter Review 
Updated the build. This move everything into a class like you wanted, which nicely means we can dig a bunch out of stuff from BrowserToolbar that doesn't need to be there anymore.
Attachment #741608 - Attachment is obsolete: true
Attachment #741988 - Attachment is obsolete: true
Attachment #742050 - Flags: review?(lucasr.at.mozilla)
Attached patch Patch v2.1 — — Splinter Review 
Apologies for the churn. Updated to work on things that aren't my phone (i.e. with the menu button).
Attachment #742050 - Attachment is obsolete: true
Attachment #742050 - Flags: review?(lucasr.at.mozilla)
Attachment #742055 - Flags: review?(lucasr.at.mozilla)
Talked with ian on irc and updated the build (and local patch) to use a 500ms animation.
\o/

ship it!
Comment on attachment 742055 [details] [diff] [review]
Patch v2.1

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

Looks great with nits fixed.

::: mobile/android/base/BrowserToolbar.java
@@ +72,5 @@
>      public ImageButton mStop;
>      public ImageButton mSiteSecurity;
>      public ImageButton mReader;
>      private AnimationDrawable mProgressSpinner;
> +    private TabCounter mTabsCount;

mTabsCounter for consistency?

::: mobile/android/base/Rotate3DAnimation.java
@@ +1,2 @@
> +/*
> + * Copyright (C) 2007 The Android Open Source Project

Fetched from Android's API demos?

@@ +25,5 @@
> +/**
> + * An animation that rotates the view on the Y axis between two specified angles.
> + * This animation also adds a translation on the Z axis (depth) to improve the effect.
> + */
> +public class Rotate3DAnimation extends Animation {

I wonder if we should create an "animation" directory containing:
Rotate3DAnimation.java
AnimatorProxy.java
PropertyAnimator.java
HeightChangeAnimation.java

No need to do this in this patch. File a follow-up bug?

@@ +31,5 @@
> +    private final float mToDegrees;
> +
> +    private final float mCenterX;
> +    private final float mCenterY;
> +    

nit: remove trailing spaces here.

::: mobile/android/base/TabCounter.java
@@ +1,1 @@
> +package org.mozilla.gecko;

Missing license header?

@@ +16,5 @@
> +
> +    private final float CENTER_X = 0.5f;
> +    private final float CENTER_Y = 1.25f;
> +    private final int DURATION = 300;
> +    private final float Z_DISTANCE = 200;

All these should be 'private static final'

@@ +21,5 @@
> +
> +    private final AnimationSet mFlipInForward;
> +    private final AnimationSet mFlipInBackward;
> +    private final AnimationSet mFlipOutForward;
> +    private final AnimationSet mFlipOutBackward;

nit: empty line here.

@@ +22,5 @@
> +    private final AnimationSet mFlipInForward;
> +    private final AnimationSet mFlipInBackward;
> +    private final AnimationSet mFlipOutForward;
> +    private final AnimationSet mFlipOutBackward;
> +    private final LayoutInflater mInflater;

nit: empty line here.

@@ +39,5 @@
> +        mFlipInBackward = createAnimation(90, 0, FadeMode.FADE_IN, Z_DISTANCE, false);
> +        mFlipOutForward = createAnimation(0, -90, FadeMode.FADE_OUT, -1 * Z_DISTANCE, true);
> +        mFlipOutBackward = createAnimation(0, 90, FadeMode.FADE_OUT, Z_DISTANCE, true);
> +
> +        removeAllViews();

Why is this necessary?

@@ +74,5 @@
> +    }
> +
> +    private AnimationSet createAnimation(float startAngle, float endAngle,
> +                                         FadeMode fadeMode,
> +                                         float zEnd, boolean reverse) {

Maybe micro-optimizing it a bit, add:

  final Context context = getContext();

and use it below.

@@ +80,5 @@
> +        set.addAnimation(new Rotate3DAnimation(startAngle, endAngle, CENTER_X, CENTER_Y, zEnd, reverse));
> +        set.addAnimation(fadeMode == FadeMode.FADE_IN ? new AlphaAnimation(0.0f, 1.0f) :
> +                                                        new AlphaAnimation(1.0f, 0.0f));
> +        set.setDuration(DURATION);
> +        set.setInterpolator(getContext(), android.R.anim.accelerate_interpolator);

nit: empty line.

::: mobile/android/base/resources/layout-large-v11/browser_toolbar_menu.xml
@@ +34,5 @@
>               16dp on all sides. However this image has a perception of 
>               2 layers. Hence to center this, an additional 4dp is added to the right.
>               The margins will be 12dp on left, 48dp on right, instead of ideal 16dp
>               and 44dp. -->
> +        <org.mozilla.gecko.TabCounter android:id="@+id/tabs_count"

tabs_counter for consistency?

@@ +35,5 @@
>               2 layers. Hence to center this, an additional 4dp is added to the right.
>               The margins will be 12dp on left, 48dp on right, instead of ideal 16dp
>               and 44dp. -->
> +        <org.mozilla.gecko.TabCounter android:id="@+id/tabs_count"
> +                            style="@style/AddressBar.ImageButton.TabCount"

TabCounter for consistency?

::: mobile/android/base/resources/layout/browser_toolbar.xml
@@ +9,5 @@
>                style="@style/BrowserToolbar">
>  
>      <RelativeLayout android:id="@+id/address_bar"
> +                    android:clipChildren="false"
> +                    android:clipToPadding="false"

Why is this necessary?
Attachment #742055 - Flags: review?(lucasr.at.mozilla) → review+
When I open my 120 bookmarks, this breaks http://cl.ly/image/3e3w2K3h2O14
That has nothing to do with this bug. Please file a new one for better handling > 100 tabs open.
Whoops. Forgot to update the transition duration. Fixed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ad22d42b4f5
04-26 17:59:06.757 E/GeckoAppShell( 2083): >>> REPORTING UNCAUGHT EXCEPTION FROM THREAD 1 ("main")
04-26 17:59:06.757 E/GeckoAppShell( 2083): java.lang.ClassCastException: org.mozilla.gecko.GeckoTextSwitcher cannot be cast to org.mozilla.gecko.TabCounter
04-26 17:59:06.757 E/GeckoAppShell( 2083): 	at org.mozilla.gecko.BrowserToolbar.from(BrowserToolbar.java:243)
04-26 17:59:06.757 E/GeckoAppShell( 2083): 	at org.mozilla.gecko.BrowserApp.onCreate(BrowserApp.java:379)
04-26 17:59:06.757 E/GeckoAppShell( 2083): 	at android.app.Activity.performCreate(Activity.java:4465)
04-26 17:59:06.757 E/GeckoAppShell( 2083): 	at android.app.Instrumentation.callActivityOnCreate(Instrumentation.java:1049)
04-26 17:59:06.757 E/GeckoAppShell( 2083): 	at android.app.ActivityThread.performLaunchActivity(ActivityThread.java:1920)
04-26 17:59:06.757 E/GeckoAppShell( 2083): 	at android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:1981)
04-26 17:59:06.757 E/GeckoAppShell( 2083): 	at android.app.ActivityThread.access$600(ActivityThread.java:123)
04-26 17:59:06.757 E/GeckoAppShell( 2083): 	at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1147)
04-26 17:59:06.757 E/GeckoAppShell( 2083): 	at android.os.Handler.dispatchMessage(Handler.java:99)
04-26 17:59:06.757 E/GeckoAppShell( 2083): 	at android.os.Looper.loop(Looper.java:137)
04-26 17:59:06.757 E/GeckoAppShell( 2083): 	at android.app.ActivityThread.main(ActivityThread.java:4424)
04-26 17:59:06.757 E/GeckoAppShell( 2083): 	at java.lang.reflect.Method.invokeNative(Native Method)
04-26 17:59:06.757 E/GeckoAppShell( 2083): 	at java.lang.reflect.Method.invoke(Method.java:511)
04-26 17:59:06.757 E/GeckoAppShell( 2083): 	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:784)
04-26 17:59:06.757 E/GeckoAppShell( 2083): 	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:551)
04-26 17:59:06.757 E/GeckoAppShell( 2083): 	at dalvik.system.NativeStart.main(Native Method)
Thanks. The toolbar chnages bitrotted me. I fixed it up and verified everything built and worked, but didn't recheck tablet ui apparently.
From the log:
findElement: Element 'tabs_count' does not exist in the list
Attached patch Test fixes — — Splinter Review 
Yay for UI tests that depend on us never changing ids! This looks fine on try:

https://tbpl.mozilla.org/?tree=Try&rev=95b7707d0c80
Attachment #742782 - Flags: review?(mark.finkle)
Attachment #742782 - Flags: review?(mark.finkle) → review+
https://hg.mozilla.org/mozilla-central/rev/2fbb0094f6f4
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
Blocks: 868222
No longer blocks: 868222
(In reply to Lucas Rocha (:lucasr) from comment #13)
> Comment on attachment 742055 [details] [diff] [review]
> Patch v2.1
> 
> Review of attachment 742055 [details] [diff] [review]:
> ::: mobile/android/base/resources/layout/browser_toolbar.xml
> @@ +9,5 @@
> >                style="@style/BrowserToolbar">
> >  
> >      <RelativeLayout android:id="@+id/address_bar"
> > +                    android:clipChildren="false"
> > +                    android:clipToPadding="false"
> 
> Why is this necessary?

Exactly! Why is this necessary???

(Oh lucasr, for once you didn't r- when having a doubt on patch and it regressed as in Bug 890006 ;) Just kidding :D )
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: