Closed
Bug 1055606
Opened 10 years ago
Closed 10 years ago
Animate tab strip when tabs added added or removed
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 36
People
(Reporter: lucasr, Assigned: lucasr)
References
Details
Attachments
(1 file)
(deleted),
patch
|
mcomella
:
review+
|
Details | Diff | Splinter Review |
Animating TwoWayView has to account for view recycling. Some investigation is needed here.
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Comment 2•10 years ago
|
||
Comment on attachment 8513620 [details] [diff] [review]
Animate tab strip when tabs added added or removed (r=mcomella)
Feels a lot better. We might want to tweak the animation after it lands but this is a pretty good start.
Attachment #8513620 -
Flags: review?(michael.l.comella)
Assignee | ||
Comment 3•10 years ago
|
||
FYI: I noticed the animations are not as smooth as they should be. This is likely related to the expensive drawing operations done in each tab item in the strip. Filed bug 1091519.
Comment 4•10 years ago
|
||
Comment on attachment 8513620 [details] [diff] [review]
Animate tab strip when tabs added added or removed (r=mcomella)
Review of attachment 8513620 [details] [diff] [review]:
-----------------------------------------------------------------
r+ w/ nits
::: mobile/android/base/tabs/TabStripView.java
@@ +91,4 @@
> }
> }
>
> + private void animateTabRemoval(Tab removedTab) {
nit: I'd like this and animateNewTab to have the same naming convention, e.g. animateRemoveTab.
@@ +93,5 @@
>
> + private void animateTabRemoval(Tab removedTab) {
> + final int removedPosition = adapter.getPositionForTab(removedTab);
> +
> + final View removedView = getViewForTab(removedTab);
Add a comment for when this might be null.
@@ +110,5 @@
> + final List<Animator> childAnimators = new ArrayList<Animator>();
> +
> + final int childCount = getChildCount();
> + for (int i = removedPosition - firstPosition; i < childCount; i++) {
> + final View child = getChildAt(i);
You should make a comment here, or at the top of the method, that mentions that the removed view is not actually animated, i.e. just disappears.
@@ +149,5 @@
> + if (newChild == null) {
> + return true;
> + }
> +
> + final List<Animator> childAnimators = new ArrayList<Animator>();
nit: Move this above where it is first used.
@@ +152,5 @@
> +
> + final List<Animator> childAnimators = new ArrayList<Animator>();
> +
> + final int tabSize = newChild.getWidth();
> + final int newIndex = newPosition - firstPosition;
nit: Move above the loop where it is first used.
@@ +159,5 @@
> + ObjectAnimator.ofFloat(newChild, "translationY", newChild.getHeight(), 0));
> +
> + final int childCount = getChildCount();
> + for (int i = newIndex + 1; i < childCount; i++) {
> + final View child = getChildAt(i);
I'd add a comment here, or at the top of the method, mentioning that a view that gets pushed off-screen will not get animated back here due to a limitation in android.
@@ +161,5 @@
> + final int childCount = getChildCount();
> + for (int i = newIndex + 1; i < childCount; i++) {
> + final View child = getChildAt(i);
> +
> + child.setLayerType(View.LAYER_TYPE_HARDWARE, null);
Why don't we setLayoutType on newChild as well?
Attachment #8513620 -
Flags: review?(michael.l.comella) → review+
Assignee | ||
Comment 5•10 years ago
|
||
(In reply to Michael Comella (:mcomella) from comment #4)
> Comment on attachment 8513620 [details] [diff] [review]
> Animate tab strip when tabs added added or removed (r=mcomella)
>
> Review of attachment 8513620 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> r+ w/ nits
>
> ::: mobile/android/base/tabs/TabStripView.java
> @@ +91,4 @@
> > }
> > }
> >
> > + private void animateTabRemoval(Tab removedTab) {
>
> nit: I'd like this and animateNewTab to have the same naming convention,
> e.g. animateRemoveTab.
Done.
> @@ +93,5 @@
> >
> > + private void animateTabRemoval(Tab removedTab) {
> > + final int removedPosition = adapter.getPositionForTab(removedTab);
> > +
> > + final View removedView = getViewForTab(removedTab);
>
> Add a comment for when this might be null.
Done.
> @@ +110,5 @@
> > + final List<Animator> childAnimators = new ArrayList<Animator>();
> > +
> > + final int childCount = getChildCount();
> > + for (int i = removedPosition - firstPosition; i < childCount; i++) {
> > + final View child = getChildAt(i);
>
> You should make a comment here, or at the top of the method, that mentions
> that the removed view is not actually animated, i.e. just disappears.
Good point, done.
> @@ +149,5 @@
> > + if (newChild == null) {
> > + return true;
> > + }
> > +
> > + final List<Animator> childAnimators = new ArrayList<Animator>();
>
> nit: Move this above where it is first used.
Ok, done.
> @@ +152,5 @@
> > +
> > + final List<Animator> childAnimators = new ArrayList<Animator>();
> > +
> > + final int tabSize = newChild.getWidth();
> > + final int newIndex = newPosition - firstPosition;
>
> nit: Move above the loop where it is first used.
Done.
> @@ +159,5 @@
> > + ObjectAnimator.ofFloat(newChild, "translationY", newChild.getHeight(), 0));
> > +
> > + final int childCount = getChildCount();
> > + for (int i = newIndex + 1; i < childCount; i++) {
> > + final View child = getChildAt(i);
>
> I'd add a comment here, or at the top of the method, mentioning that a view
> that gets pushed off-screen will not get animated back here due to a
> limitation in android.
Ok, done.
> @@ +161,5 @@
> > + final int childCount = getChildCount();
> > + for (int i = newIndex + 1; i < childCount; i++) {
> > + final View child = getChildAt(i);
> > +
> > + child.setLayerType(View.LAYER_TYPE_HARDWARE, null);
>
> Why don't we setLayoutType on newChild as well?
Good catch. I should have removed this line from the patch. I'll probably add it back in a different form as part of bug 1091519.
Assignee | ||
Comment 6•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
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
•