End animations on scrap views, clear updated views from cache
This CL fixes a bug where we would not remove updated views
from cache which was bad utilization of cache since they'll
need to be rebound anyways and they could be rebound back in
preLayout with their bad state.
I also changed how we handle remaining scrap views at the end
of a layout. We used to recycle them wihtout ending their
animations which would prevent them from being recycled.
Instead, we first end their animations inside a recycle
barrier and then recycle them.
I also did some cleanup in layout tests to make them easier
to read :).
Bug: 21615412
Change-Id: Ic4fecfe9b78ee66e46b13bff83cf903be2cc2e1a
diff --git a/v7/recyclerview/src/android/support/v7/widget/RecyclerView.java b/v7/recyclerview/src/android/support/v7/widget/RecyclerView.java
index c479033..0bb1dae 100644
--- a/v7/recyclerview/src/android/support/v7/widget/RecyclerView.java
+++ b/v7/recyclerview/src/android/support/v7/widget/RecyclerView.java
@@ -4040,6 +4040,9 @@
if (mMaxScrap.get(viewType) <= scrapHeap.size()) {
return;
}
+ if (DEBUG && scrapHeap.contains(scrap)) {
+ throw new IllegalArgumentException("this scrap item already exists");
+ }
scrap.resetInternal();
scrapHeap.add(scrap);
}
@@ -4545,8 +4548,13 @@
&& mAdapter.onFailedToRecycleView(holder);
boolean cached = false;
boolean recycled = false;
+ if (DEBUG && mCachedViews.contains(holder)) {
+ throw new IllegalArgumentException("cached view received recycle internal? " +
+ holder);
+ }
if (forceRecycle || holder.isRecyclable()) {
- if (!holder.isInvalid() && !holder.isRemoved() && !holder.isChanged()) {
+ if (!holder.hasAnyOfTheFlags(ViewHolder.FLAG_INVALID | ViewHolder.FLAG_REMOVED |
+ ViewHolder.FLAG_CHANGED | ViewHolder.FLAG_UPDATE)) {
// Retire oldest cached view
final int cachedViewSize = mCachedViews.size();
if (cachedViewSize == mViewCacheMax && cachedViewSize > 0) {
@@ -4898,7 +4906,7 @@
void viewRangeUpdate(int positionStart, int itemCount) {
final int positionEnd = positionStart + itemCount;
final int cachedCount = mCachedViews.size();
- for (int i = 0; i < cachedCount; i++) {
+ for (int i = cachedCount - 1; i >= 0; i--) {
final ViewHolder holder = mCachedViews.get(i);
if (holder == null) {
continue;
@@ -4907,6 +4915,7 @@
final int pos = holder.getLayoutPosition();
if (pos >= positionStart && pos < positionEnd) {
holder.addFlags(ViewHolder.FLAG_UPDATE);
+ recycleCachedViewAt(i);
// cached views should not be flagged as changed because this will cause them
// to animate when they are returned from cache.
}
@@ -6569,9 +6578,19 @@
if (vh.shouldIgnore()) {
continue;
}
+ // If the scrap view is animating, we need to cancel them first. If we cancel it
+ // here, ItemAnimator callback may recycle it which will cause double recycling.
+ // To avoid this, we mark it as not recycleable before calling the item animator.
+ // Since removeDetachedView calls a user API, a common mistake (ending animations on
+ // the view) may recycle it too, so we guard it before we call user APIs.
+ vh.setIsRecyclable(false);
if (vh.isTmpDetached()) {
mRecyclerView.removeDetachedView(scrap, false);
}
+ if (mRecyclerView.mItemAnimator != null) {
+ mRecyclerView.mItemAnimator.endAnimation(vh);
+ }
+ vh.setIsRecyclable(true);
recycler.quickRecycleScrapView(scrap);
}
recycler.clearScrap();
diff --git a/v7/recyclerview/tests/src/android/support/v7/widget/LinearLayoutManagerTest.java b/v7/recyclerview/tests/src/android/support/v7/widget/LinearLayoutManagerTest.java
index 83ce068..2622efd 100644
--- a/v7/recyclerview/tests/src/android/support/v7/widget/LinearLayoutManagerTest.java
+++ b/v7/recyclerview/tests/src/android/support/v7/widget/LinearLayoutManagerTest.java
@@ -1454,12 +1454,17 @@
@Override
public void run() {
final int childCount = getChildCount();
+ Rect layoutBounds = new Rect(0, 0,
+ mLayoutManager.getWidth(), mLayoutManager.getHeight());
for (int i = 0; i < childCount; i++) {
View child = getChildAt(i);
RecyclerView.LayoutParams lp = (RecyclerView.LayoutParams) child
.getLayoutParams();
TestViewHolder vh = (TestViewHolder) lp.mViewHolder;
- items.put(vh.mBoundItem, getViewBounds(child));
+ Rect childBounds = getViewBounds(child);
+ if (new Rect(childBounds).intersect(layoutBounds)) {
+ items.put(vh.mBoundItem, childBounds);
+ }
}
}
});
diff --git a/v7/recyclerview/tests/src/android/support/v7/widget/RecyclerViewAnimationsTest.java b/v7/recyclerview/tests/src/android/support/v7/widget/RecyclerViewAnimationsTest.java
index f9e3d0c..9fd5427 100644
--- a/v7/recyclerview/tests/src/android/support/v7/widget/RecyclerViewAnimationsTest.java
+++ b/v7/recyclerview/tests/src/android/support/v7/widget/RecyclerViewAnimationsTest.java
@@ -668,7 +668,7 @@
};
// original 1 item
setupBasic(1, 0, 1, adapter);
- mRecyclerView.getItemAnimator().setAddDuration(3000);
+ mRecyclerView.getItemAnimator().setAddDuration(10000);
mLayoutManager.expectLayouts(2);
// add 2 items
setExpectedItemCounts(1, 3);
diff --git a/v7/recyclerview/tests/src/android/support/v7/widget/RecyclerViewLayoutTest.java b/v7/recyclerview/tests/src/android/support/v7/widget/RecyclerViewLayoutTest.java
index e55cacb..630c12d 100644
--- a/v7/recyclerview/tests/src/android/support/v7/widget/RecyclerViewLayoutTest.java
+++ b/v7/recyclerview/tests/src/android/support/v7/widget/RecyclerViewLayoutTest.java
@@ -56,6 +56,9 @@
@RunWith(AndroidJUnit4.class)
public class RecyclerViewLayoutTest extends BaseRecyclerViewInstrumentationTest {
+ private static final int FLAG_HORIZONTAL = 1;
+ private static final int FLAG_VERTICAL = 1 << 1;
+ private static final int FLAG_FLING = 1 << 2;
private static final boolean DEBUG = true;
@@ -252,22 +255,22 @@
@Test
public void testDragHorizontal() throws Throwable {
- scrollInOtherOrientationTest(true, false, false);
+ scrollInOtherOrientationTest(FLAG_HORIZONTAL);
}
@Test
public void testDragVertical() throws Throwable {
- scrollInOtherOrientationTest(false, true, false);
+ scrollInOtherOrientationTest(FLAG_VERTICAL);
}
@Test
public void testFlingHorizontal() throws Throwable {
- scrollInOtherOrientationTest(true, false, true);
+ scrollInOtherOrientationTest(FLAG_HORIZONTAL | FLAG_FLING);
}
@Test
public void testFlingVertical() throws Throwable {
- scrollInOtherOrientationTest(false, true, true);
+ scrollInOtherOrientationTest(FLAG_VERTICAL | FLAG_FLING);
}
@Test
@@ -275,7 +278,7 @@
TestedFrameLayout tfl = getActivity().mContainer;
tfl.setNestedScrollMode(TestedFrameLayout.TEST_NESTED_SCROLL_MODE_CONSUME);
tfl.setNestedFlingMode(TestedFrameLayout.TEST_NESTED_SCROLL_MODE_CONSUME);
- scrollInOtherOrientationTest(false, true, false, false, false, false);
+ scrollInOtherOrientationTest(FLAG_VERTICAL, 0);
}
@Test
@@ -283,19 +286,15 @@
TestedFrameLayout tfl = getActivity().mContainer;
tfl.setNestedScrollMode(TestedFrameLayout.TEST_NESTED_SCROLL_MODE_CONSUME);
tfl.setNestedFlingMode(TestedFrameLayout.TEST_NESTED_SCROLL_MODE_CONSUME);
- scrollInOtherOrientationTest(true, false, false, false, false, false);
+ scrollInOtherOrientationTest(FLAG_HORIZONTAL, 0);
}
- private void scrollInOtherOrientationTest(boolean horizontal, boolean vertical, boolean fling)
+ private void scrollInOtherOrientationTest(int flags)
throws Throwable {
- scrollInOtherOrientationTest(horizontal, vertical, horizontal, vertical, fling, fling);
+ scrollInOtherOrientationTest(flags);
}
- private void scrollInOtherOrientationTest(
- final boolean horizontal, final boolean vertical,
- final boolean expectedHorizontal, final boolean expectedVertical,
- final boolean fling, final boolean expectedFling)
- throws Throwable {
+ private void scrollInOtherOrientationTest(final int flags, int expectedFlags) throws Throwable {
RecyclerView recyclerView = new RecyclerView(getActivity());
final AtomicBoolean scrolledHorizontal = new AtomicBoolean(false);
final AtomicBoolean scrolledVertical = new AtomicBoolean(false);
@@ -303,12 +302,12 @@
final TestLayoutManager tlm = new TestLayoutManager() {
@Override
public boolean canScrollHorizontally() {
- return horizontal;
+ return (flags & FLAG_HORIZONTAL) != 0;
}
@Override
public boolean canScrollVertically() {
- return vertical;
+ return (flags & FLAG_VERTICAL) != 0;
}
@Override
@@ -337,15 +336,19 @@
tlm.expectLayouts(1);
setRecyclerView(recyclerView);
tlm.waitForLayout(2);
- if (fling) {
- assertEquals("fling started", expectedFling, fling(600, 600));
+ if ( (flags & FLAG_FLING) != 0 ) {
+ int flingVelocity = (mRecyclerView.getMaxFlingVelocity() +
+ mRecyclerView.getMinFlingVelocity()) / 2;
+ assertEquals("fling started", (expectedFlags & FLAG_FLING) != 0,
+ fling(flingVelocity, flingVelocity));
} else { // drag
- TouchUtils.dragViewTo(this, recyclerView, Gravity.LEFT | Gravity.TOP, 200, 200);
+ TouchUtils.dragViewTo(this, recyclerView, Gravity.LEFT | Gravity.TOP,
+ mRecyclerView.getWidth() / 2, mRecyclerView.getHeight() / 2);
}
assertEquals("horizontally scrolled: " + tlm.mScrollHorizontallyAmount,
- expectedHorizontal, scrolledHorizontal.get());
+ (expectedFlags & FLAG_HORIZONTAL) != 0, scrolledHorizontal.get());
assertEquals("vertically scrolled: " + tlm.mScrollVerticallyAmount,
- expectedVertical, scrolledVertical.get());
+ (expectedFlags & FLAG_VERTICAL) != 0, scrolledVertical.get());
}
private boolean fling(final int velocityX, final int velocityY) throws Throwable {
@@ -1056,10 +1059,13 @@
}
@Test
+ public void testSmoothScrollWithRemovedItemsAndRemoveItem() throws Throwable {
+ smoothScrollTest(true);
+ }
+
+ @Test
public void testSmoothScrollWithRemovedItems() throws Throwable {
smoothScrollTest(false);
- removeRecyclerView();
- smoothScrollTest(true);
}
public void smoothScrollTest(final boolean removeItem) throws Throwable {