Closed Bug 1097121 Opened 10 years ago Closed 9 years ago

Animate items being removed from the tabs panel grid

Categories

(Firefox for Android Graveyard :: General, defect, P1)

All
Android
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 36

People

(Reporter: mhaigh, Assigned: mhaigh)

References

Details

Attachments

(1 file, 6 obsolete files)

      No description provided.
Pretty happy with this overall, but there is one thing which I could do with a second pair onf eyes on:  when the gridview has many tabs (say 30) and is scrolled all the way to the bottom with only one item left remaining on the last row, removing a different tab makes the content jump as suddenly the gridview doesn't need to draw the row which now hasn't got any items on it, so doesn't, and the row above now becomes the last row.  It's almost like we need to add in an empty view to the end of the gridview to maintain that empty space until that view is scrolled out of view.  

Also, I added in a delay for the animations as doing them all at once felt a bit jarring.
Attachment #8523908 - Flags: feedback?(lucasr.at.mozilla)
Comment on attachment 8523908 [details] [diff] [review]
Animate items being removed from the tabs panel grid

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

This looking pretty cool, good job! Maybe show a build to antlam for feedback?

::: mobile/android/base/tabs/TabsGridLayout.java
@@ +311,5 @@
> +                        animator.setStartDelay(x * delayMultiple);
> +                        childAnimators.add(animator);
> +
> +                        animator = ObjectAnimator.ofFloat(child, "translationX", -(removedWidth * numberOfColumns), 0);
> +                        animator.setStartDelay(x * delayMultiple);

Instead of creating one animator per property being animated, you should create PropertyValueHolder for each property and then create a single animator out of them. Have a look how PropertyValueHolder is used here, for example: https://gist.github.com/lucasr/9508844#file-animatedlinearlayout-java-L198

@@ +334,5 @@
> +            }
> +        });
> +    }
> +    private int getNumColumnsCompat() {
> +        if (Build.VERSION.SDK_INT >= 11) {

This code guaranteed to run on 11+, no need to add the compat code path.
Attachment #8523908 - Flags: feedback?(lucasr.at.mozilla) → feedback+
Priority: -- → P1
Yeah I've played with this on my N9 and got exactly what we talked about in the meeting.

If this is a P1, can(In reply to Martyn Haigh (:mhaigh) from comment #1)
> It's almost like we need to add in an empty view to the end of the gridview to maintain that
> empty space until that view is scrolled out of view.  

This might be a good idea. Some sort of easing/ animation would make this seem less weird.
Flags: needinfo?(alam)
PropertyValuesHolder implemented but getting a graphical artifact where the views in the GridView are appearing briefly in their final position before the animation starts.
Attachment #8523908 - Attachment is obsolete: true
Attachment #8526114 - Flags: feedback?(lucasr.at.mozilla)
Attachment #8526114 - Flags: feedback?(lucasr.at.mozilla)
Still getting flash, despite shifting around of code
Attachment #8526114 - Attachment is obsolete: true
Attachment #8526145 - Flags: feedback?(lucasr.at.mozilla)
View flashing as been fixed and we're now animating the bottom padding of the gridview to overcome the sudden heigh shift when the last item in the last row in the grid shifts up a line
Attachment #8526145 - Attachment is obsolete: true
Attachment #8526145 - Flags: feedback?(lucasr.at.mozilla)
Attachment #8527738 - Flags: feedback?(lucasr.at.mozilla)
Comment on attachment 8527738 [details] [diff] [review]
Animate items being removed from the tabs panel

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

This is looking promising but still needs work. The pattern you should follow is this: https://speakerdeck.com/lucasr/layout-traversals?slide=31

::: mobile/android/base/tabs/TabsGridLayout.java
@@ +45,3 @@
>                       implements TabsLayout,
>                                  Tabs.OnTabsChangedListener {
>      private static final String LOGTAG = "Gecko" + TabsGridLayout.class.getSimpleName();

nit: add empty line here.

@@ +45,5 @@
>                       implements TabsLayout,
>                                  Tabs.OnTabsChangedListener {
>      private static final String LOGTAG = "Gecko" + TabsGridLayout.class.getSimpleName();
> +    private static final int ANIM_TIME_MS = 200;
> +    private static final DecelerateInterpolator ANIM_INTERPOLATOR = new DecelerateInterpolator();

nit: add empty line here.

@@ +50,3 @@
>      private final Context mContext;
>      private TabsPanel mTabsPanel;
> +    private final SparseArray<Point> mTabLocations = new SparseArray<>();

You better use 'new SparseArray<Point>()' here.

@@ +100,4 @@
>              mCloseClickListener = new Button.OnClickListener() {
>                  @Override
>                  public void onClick(View v) {
> +                    populateTabLocations(v);

This should be called in animateRemoveTab, just before the OnPreDrawListener.

@@ +145,5 @@
> +
> +        for(int x = 1, i = (removedPosition - firstPosition) + 1; i < childCount; i++, x++) {
> +            final View child = getChildAt(i);
> +            if (child != null) {
> +                mTabLocations.append(x, new Point((int) child.getX(), (int) child.getY()));

Use PointF instead, if you want float coordinates instead of int.

@@ +148,5 @@
> +            if (child != null) {
> +                mTabLocations.append(x, new Point((int) child.getX(), (int) child.getY()));
> +            }
> +        }
> +        

nit: remove trailing space.

@@ +157,5 @@
> +            // We need to increase and animate down the view padding to prevent
> +            // a sudden jump as the last item in the row is being removed
> +
> +            final int removedHeight = getChildAt(0).getMeasuredHeight();
> +            final int verticalSpacing = getVerticalSpacing();

Why is this code necessary?

@@ +166,5 @@
> +            varl.addUpdateListener(new ValueAnimator.AnimatorUpdateListener() {
> +
> +                @Override
> +                public void onAnimationUpdate(ValueAnimator animation) {
> +                    setPadding(getPaddingLeft(), getPaddingTop(), getPaddingRight(), (Integer) animation.getAnimatedValue());

Animating on padding will trigger a layout traversal on each frame. This likely means choppy animations :-(

@@ +374,5 @@
> +                animatorSet.setInterpolator(ANIM_INTERPOLATOR);
> +
> +                animatorSet.start();
> +
> +                // set the starting position of the child views

Is this necessary because you're using a delay?
Attachment #8527738 - Flags: feedback?(lucasr.at.mozilla)
I've left the padding animation in as without it the source and target destinations for the animations get messed up.  We need the gridview to display an empty space at the bottom of the screen so that if the last item on a row is visible

I've also added a call for clipToPadding(true) as the one in bug 1101784, set in the style.xml file, doesn't seem to work and this is needed for the padding animation.
Attachment #8527738 - Attachment is obsolete: true
Attachment #8528363 - Flags: review?(lucasr.at.mozilla)
Oops, left some test code in.  This fixes it and removed some unnecessary imports.
Attachment #8528363 - Attachment is obsolete: true
Attachment #8528363 - Flags: review?(lucasr.at.mozilla)
Attachment #8528365 - Flags: review?(lucasr.at.mozilla)
Fixed some of the previously addressed nits
Attachment #8528365 - Attachment is obsolete: true
Attachment #8528365 - Flags: review?(lucasr.at.mozilla)
Attachment #8529045 - Flags: review?(lucasr.at.mozilla)
Comment on attachment 8529045 [details] [diff] [review]
Animate items being removed from the tabs panel grid

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

Looking great with the suggested changes. Tested this locally and it's looking awesome! Can't wait for it to land :-)

::: mobile/android/base/tabs/TabsGridLayout.java
@@ +145,5 @@
> +        final int numberOfColumns = getNumColumns();
> +        final int childCount = getChildCount();
> +        final int removedPosition = mTabsAdapter.getPositionForTab(removedTab);
> +
> +        for(int x = 1, i = (removedPosition - firstPosition) + 1; i < childCount; i++, x++) {

nit: space after 'for'

@@ +148,5 @@
> +
> +        for(int x = 1, i = (removedPosition - firstPosition) + 1; i < childCount; i++, x++) {
> +            final View child = getChildAt(i);
> +            if (child != null) {
> +                mTabLocations.append(x, new Point((int) child.getX(), (int) child.getY()));

You can avoid the type casting here by using PointF instead.

@@ +311,5 @@
> +        return getChildAt(position - getFirstVisiblePosition());
> +    }
> +
> +    void closeTab(View v) {
> +        //((TabsLayoutItemView) v.getTag()).setVisibility(View.INVISIBLE);

Leftover? Remove.

@@ +341,5 @@
> +                getViewTreeObserver().removeOnPreDrawListener(this);
> +                // We don't animate the removed child view (it just disappears)
> +                // but we still need its size to animate all affected children
> +                // within the visible viewport.
> +                final int removedHeight = removedView.getMeasuredHeight();

Maybe move the removeHeight declaration to be just after the null check on removeView above? This way you don't need to access the view here which, strictly speaking, is not safe as removedView will probably be recycled in the next display frame.

@@ +342,5 @@
> +                // We don't animate the removed child view (it just disappears)
> +                // but we still need its size to animate all affected children
> +                // within the visible viewport.
> +                final int removedHeight = removedView.getMeasuredHeight();
> +                final int delayMultiple = 20; //in ms

Turn this into a ANIM_DELAY_MULTIPLE_MS constant? Declare it together with ANIM_TIME_MS, etc?

@@ +355,5 @@
> +                    final View child = getChildAt(i);
> +                    ObjectAnimator animator;
> +
> +                    if (i % numberOfColumns == numberOfColumns - 1) {
> +                        // animate X & Y

nit: Animate...

@@ +360,5 @@
> +                        translateX = PropertyValuesHolder.ofFloat("translationX", -(mColumnWidth * numberOfColumns), 0);
> +                        translateY = PropertyValuesHolder.ofFloat("translationY", removedHeight, 0);
> +                        animator = ObjectAnimator.ofPropertyValuesHolder(child, translateX, translateY);
> +                    } else {
> +                        // just animate X

nit: Just animate...

@@ +374,5 @@
> +                animatorSet.setDuration(ANIM_TIME_MS);
> +                animatorSet.setInterpolator(ANIM_INTERPOLATOR);
> +                animatorSet.start();
> +
> +                // set the starting position of the child views - because we are delaying the start

nit: Set the...

@@ +378,5 @@
> +                // set the starting position of the child views - because we are delaying the start
> +                // of the animation, we need to prevent the items being drawn in their final position
> +                // prior to the animation starting
> +                for (int x = 1, i = (removedPosition - firstPosition) + 1; i < childCount; i++, x++) {
> +                    final View child = getChildAt(i);

nit: add empty line here.

@@ +382,5 @@
> +                    final View child = getChildAt(i);
> +                    final Point targetLocation = mTabLocations.get(x+1);
> +                    if (targetLocation == null) {
> +                        continue;
> +                    }

nit: add empty line here.
Attachment #8529045 - Flags: review?(lucasr.at.mozilla) → review+
https://hg.mozilla.org/mozilla-central/rev/f2312ecb6fd0
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Depends on: 1108418
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.