Bug 1310081 - THIS COMMIT WILL BE ADDED IN
BUG 1313144 - notifyItemChanged RecyclerView animator issues.
The tabs list issues fixed here are dependent on timing.
If we add a tab A and the currently selected tab B gets moved as a result, and
if the UNSELECT on B arrives after the move animation on B has started, the bind
on B which occurs as a result of the notifyItemChanged triggered by the UNSELECT
was reseting translationY to 0, which effectively cancels the move (since it was
animating translationY to 0). If the UNSELECT arrives before the move is
queued, which is what usually happens, then the animation runs normally.
The other fix is to DefaultItemAnimator's animateChange, which is just generally
not well behaved in the case where setSupportsChangeAnimations(false) is set and
multiple animations on a view can occur at the same time.
For example, after the fix from the first paragraph is applied, if tab A is
added and then the SELECT for tab A arrives, the animateChange call resulting
from the SELECT's notifyItemChanged will be rerouted to an animateMove, which
cancels the existing add animation without taking it over (animateMove only
checks for already running translation* animations, not alpha animations as in
an add). The add animation cancel causes the added tab to appear immediately
instead of waiting to appear after other tabs have moved to make room for it.
If the SELECT arrives before the animateAdd for the add is called, which is what
usually happens, then the animation runs normally.
More generally the provided change to animateChange prevents the passing off of
existing running move animations into new pending animations when the
animateChange call started off with no new move data.
MozReview-Commit-ID: 6B2LNtvg9G8
--- a/mobile/android/base/java/org/mozilla/gecko/tabs/TabsLayoutAdapter.java
+++ b/mobile/android/base/java/org/mozilla/gecko/tabs/TabsLayoutAdapter.java
@@ -103,20 +103,19 @@ public class TabsLayoutAdapter
return tabs.get(position);
}
@Override
public void onBindViewHolder(TabsListViewHolder viewHolder, int position) {
final Tab tab = getItem(position);
final TabsLayoutItemView itemView = (TabsLayoutItemView) viewHolder.itemView;
itemView.assignValues(tab);
- // Make sure we didn't miss any resets after animations and swipes:
- itemView.setAlpha(1);
- itemView.setTranslationX(0);
- itemView.setTranslationY(0);
+ // Be careful (re)setting position values here: bind is called on each notifyItemChanged,
+ // so you could be stomping on values that have been set in support of other animations
+ // that are already underway.
}
@Override
public TabsListViewHolder onCreateViewHolder(ViewGroup parent, int viewType) {
final TabsLayoutItemView viewItem = (TabsLayoutItemView) inflater.inflate(tabLayoutId, parent, false);
viewItem.setPrivateMode(isPrivate);
viewItem.setCloseOnClickListener(closeOnClickListener);
--- a/mobile/android/base/java/org/mozilla/gecko/tabs/TabsListLayout.java
+++ b/mobile/android/base/java/org/mozilla/gecko/tabs/TabsListLayout.java
@@ -37,22 +37,17 @@ public class TabsListLayout extends Tabs
protected float alphaForItemSwipeDx(float dX, int distanceToAlphaMin) {
return Math.max(0.1f,
Math.min(1f, 1f - 2f * Math.abs(dX) / distanceToAlphaMin));
}
};
final ItemTouchHelper touchHelper = new ItemTouchHelper(callback);
touchHelper.attachToRecyclerView(this);
- final TabsListLayoutAnimator animator = new TabsListLayoutAnimator();
- animator.setRemoveDuration(ANIMATION_DURATION);
- // A fade in/out each time the title/thumbnail/etc. gets updated isn't helpful, so disable
- // the change animation.
- animator.setSupportsChangeAnimations(false);
- setItemAnimator(animator);
+ setItemAnimator(new TabsListLayoutAnimator(ANIMATION_DURATION));
}
@Override
public void closeAll() {
final int childCount = getChildCount();
// Just close the panel if there are no tabs to close.
if (childCount == 0) {
--- a/mobile/android/base/java/org/mozilla/gecko/tabs/TabsListLayoutAnimator.java
+++ b/mobile/android/base/java/org/mozilla/gecko/tabs/TabsListLayoutAnimator.java
@@ -1,34 +1,38 @@
/* -*- Mode: Java; c-basic-offset: 4; tab-width: 20; indent-tabs-mode: nil; -*-
/* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
* You can obtain one at http://mozilla.org/MPL/2.0/. */
package org.mozilla.gecko.tabs;
import org.mozilla.gecko.util.ThreadUtils;
+import org.mozilla.gecko.widget.NoChangeItemAnimator;
import android.animation.Animator;
import android.animation.AnimatorListenerAdapter;
import android.support.v4.animation.AnimatorCompatHelper;
-import android.support.v7.widget.DefaultItemAnimator;
import android.support.v7.widget.RecyclerView;
import android.view.View;
import android.view.ViewPropertyAnimator;
import java.util.ArrayList;
import java.util.List;
// This is a light rewrite of DefaultItemAnimator to support a non-default remove animation.
-class TabsListLayoutAnimator extends DefaultItemAnimator {
+class TabsListLayoutAnimator extends NoChangeItemAnimator {
private List<RecyclerView.ViewHolder> pendingRemovals = new ArrayList<>();
// ViewHolders on which remove animations are currently being run.
private List<RecyclerView.ViewHolder> removeAnimations = new ArrayList<>();
+ public TabsListLayoutAnimator(int removeDuration) {
+ setRemoveDuration(removeDuration);
+ }
+
@Override
public void runPendingAnimations() {
if (pendingRemovals.isEmpty()) {
super.runPendingAnimations();
return;
}
for (RecyclerView.ViewHolder holder : pendingRemovals) {
new file mode 100644
--- /dev/null
+++ b/mobile/android/base/java/org/mozilla/gecko/widget/NoChangeItemAnimator.java
@@ -0,0 +1,43 @@
+/* -*- Mode: Java; c-basic-offset: 4; tab-width: 20; indent-tabs-mode: nil; -*-
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+package org.mozilla.gecko.widget;
+
+import android.support.v7.widget.DefaultItemAnimator;
+import android.support.v7.widget.RecyclerView;
+
+// In spite of what the (self-contradictory) Android documentation says, animateChange can be called
+// in response to an adapter.notifyItemChanged call even when setSupportsChangeAnimations(false) has
+// been set. Internally what happens in that case is that the animateChange call is rerouted to an
+// animateMove call. Such calls must be handled when a move is required, but if the original change
+// call data doesn't indicate a move is required, then turning it into a move can have unwanted side
+// effects:
+// * even if the animateMove successfully takes over a move animation already underway, it can
+// still cause a visible animation delay since the new move animation doesn't start running
+// again until after the next runPendingAnimations call.
+// * DefaultItemAnimator.animateMove (as written) calls resetAnimation on every call, no matter
+// what, so an already scheduled animation can be dropped if it, for example, doesn't animate a
+// translation value.
+//
+// This class more closely implements the idea that setSupportsChangeAnimations(false) should mean
+// that calling adapter.notifyItemChanged has no impact on animations.
+public class NoChangeItemAnimator extends DefaultItemAnimator {
+
+ public NoChangeItemAnimator() {
+ setSupportsChangeAnimations(false);
+ }
+
+ @Override
+ public boolean animateChange(RecyclerView.ViewHolder oldHolder, RecyclerView.ViewHolder newHolder,
+ int fromX, int fromY, int toX, int toY) {
+ if (oldHolder == newHolder && fromX == toX && fromY == toY) {
+ // We're not supporting change animations, and this call provides no new move
+ // information, so cancel it to avoid side effects.
+ dispatchChangeFinished(oldHolder, true);
+ return false;
+ }
+ return super.animateChange(oldHolder, newHolder, fromX, fromY, toX, toY);
+ }
+}
--- a/mobile/android/base/moz.build
+++ b/mobile/android/base/moz.build
@@ -747,16 +747,17 @@ gbjar.sources += ['java/org/mozilla/geck
'widget/FilledCardView.java',
'widget/FlowLayout.java',
'widget/GeckoActionProvider.java',
'widget/GeckoPopupMenu.java',
'widget/GridSpacingDecoration.java',
'widget/HistoryDividerItemDecoration.java',
'widget/IconTabWidget.java',
'widget/LoginDoorHanger.java',
+ 'widget/NoChangeItemAnimator.java',
'widget/RecyclerViewClickSupport.java',
'widget/ResizablePathDrawable.java',
'widget/RoundedCornerLayout.java',
'widget/SiteLogins.java',
'widget/SpacingDecoration.java',
'widget/SquaredImageView.java',
'widget/SquaredRelativeLayout.java',
'widget/SwipeDismissListViewTouchListener.java',