Closed
Bug 863828
Opened 12 years ago
Closed 12 years ago
New tab increment animation
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 23
People
(Reporter: ibarlow, Assigned: wesj)
References
Details
(Whiteboard: ui-hackathon)
Attachments
(2 files, 3 obsolete files)
31.90 KB,
patch
|
lucasr
:
review+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Updated•12 years ago
|
Whiteboard: ui-hackathon
Assignee | ||
Comment 1•12 years ago
|
||
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.
Comment 2•12 years ago
|
||
(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.
Comment 4•12 years ago
|
||
Also: a test build would probably be useful here for Ian's feedback.
Reporter | ||
Comment 5•12 years ago
|
||
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
Assignee | ||
Comment 6•12 years ago
|
||
Cleaning up the patch, but here's a build for you to look at ian:
http://people.mozilla.com/~wjohnston/anim.apk
Assignee | ||
Comment 7•12 years ago
|
||
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)
Assignee | ||
Comment 8•12 years ago
|
||
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)
Assignee | ||
Comment 9•12 years ago
|
||
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)
Assignee | ||
Comment 10•12 years ago
|
||
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)
Assignee | ||
Comment 11•12 years ago
|
||
Talked with ian on irc and updated the build (and local patch) to use a 500ms animation.
Reporter | ||
Comment 12•12 years ago
|
||
\o/
ship it!
Comment 13•12 years ago
|
||
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+
Comment 14•12 years ago
|
||
When I open my 120 bookmarks, this breaks http://cl.ly/image/3e3w2K3h2O14
Assignee | ||
Comment 15•12 years ago
|
||
That has nothing to do with this bug. Please file a new one for better handling > 100 tabs open.
Assignee | ||
Comment 16•12 years ago
|
||
Assignee | ||
Comment 17•12 years ago
|
||
Whoops. Forgot to update the transition duration. Fixed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ad22d42b4f5
Comment 18•12 years ago
|
||
Backed out for mass bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1421ddeb0348
Comment 19•12 years ago
|
||
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)
Assignee | ||
Comment 20•12 years ago
|
||
Thanks. The toolbar chnages bitrotted me. I fixed it up and verified everything built and worked, but didn't recheck tablet ui apparently.
Assignee | ||
Comment 21•12 years ago
|
||
Retested and landed again:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b68c721a1462
Comment 22•12 years ago
|
||
Backed out (again) for robocop failures. Try kthxbai.
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d8977cbbfc6
https://tbpl.mozilla.org/php/getParsedLog.php?id=22326921&tree=Mozilla-Inbound
Comment 23•12 years ago
|
||
From the log:
findElement: Element 'tabs_count' does not exist in the list
Assignee | ||
Comment 24•12 years ago
|
||
Assignee | ||
Comment 25•12 years ago
|
||
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)
Updated•12 years ago
|
Attachment #742782 -
Flags: review?(mark.finkle) → review+
Assignee | ||
Comment 26•12 years ago
|
||
Folded and pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2fbb0094f6f4
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
Comment 28•11 years ago
|
||
(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 )
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•