Merge "Fixed RecyclerView scrap matching bug" into lmp-preview-dev
diff --git a/v7/recyclerview/src/android/support/v7/widget/RecyclerView.java b/v7/recyclerview/src/android/support/v7/widget/RecyclerView.java
index 1026331..95a1d0e 100644
--- a/v7/recyclerview/src/android/support/v7/widget/RecyclerView.java
+++ b/v7/recyclerview/src/android/support/v7/widget/RecyclerView.java
@@ -418,7 +418,8 @@
for (int i = mAnimatingViewIndex; i < getChildCount(); ++i) {
final View view = getChildAt(i);
ViewHolder holder = getChildViewHolder(view);
- if (holder.getPosition() == position && holder.getItemViewType() == type) {
+ if (holder.getPosition() == position &&
+ ( type == INVALID_TYPE || holder.getItemViewType() == type)) {
return view;
}
}
@@ -2399,6 +2400,38 @@
}
/**
+ * Helper method for getViewForPosition.
+ * <p>
+ * Checks whether a given view holder can be used for the provided position.
+ *
+ * @param holder ViewHolder
+ * @param offsetPosition The position which is updated by UPDATE_OP changes on the adapter
+ * @return true if ViewHolder matches the provided position, false otherwise
+ */
+ boolean validateViewHolderForOffsetPosition(ViewHolder holder, int offsetPosition) {
+ // if it is a removed holder, nothing to verify since we cannot ask adapter anymore
+ // if it is not removed, verify the type and id.
+ if (holder.isRemoved()) {
+ return true;
+ }
+ if (offsetPosition < 0 || offsetPosition >= mAdapter.getItemCount()) {
+ if (DEBUG) {
+ Log.d(TAG, "validateViewHolderForOffsetPosition: invalid position, returning "
+ + "false");
+ }
+ return false;
+ }
+ final int type = mAdapter.getItemViewType(offsetPosition);
+ if (type != holder.getItemViewType()) {
+ return false;
+ }
+ if (mAdapter.hasStableIds()) {
+ return holder.getItemId() == mAdapter.getItemId(offsetPosition);
+ }
+ return true;
+ }
+
+ /**
* Obtain a view initialized for the given position.
*
* <p>This method should be used by {@link LayoutManager} implementations to obtain
@@ -2414,29 +2447,48 @@
*/
public View getViewForPosition(int position) {
ViewHolder holder;
+ holder = getScrapViewForPosition(position, INVALID_TYPE);
+ final int offsetPosition = findPositionOffset(position);
+ if (holder != null) {
+ if (!validateViewHolderForOffsetPosition(holder, offsetPosition)) {
+ // recycle this scrap
+ removeDetachedView(holder.itemView, false);
+ quickRecycleScrapView(holder.itemView);
- final int type = mAdapter.getItemViewType(position);
- if (mAdapter.hasStableIds()) {
- final long id = mAdapter.getItemId(position);
- holder = getScrapViewForId(id, type);
+ // if validate fails, we can query scrap again w/ type. that may return a
+ // different view holder from cache.
+ final int type = mAdapter.getItemViewType(offsetPosition);
+ if (mAdapter.hasStableIds()) {
+ final long id = mAdapter.getItemId(offsetPosition);
+ holder = getScrapViewForId(id, type);
+ } else {
+ holder = getScrapViewForPosition(offsetPosition, type);
+ }
+ }
} else {
- holder = getScrapViewForPosition(position, type);
+ // try recycler.
+ holder = getRecycledViewPool()
+ .getRecycledView(mAdapter.getItemViewType(offsetPosition));
}
if (holder == null) {
- holder = mAdapter.createViewHolder(RecyclerView.this, type);
- if (DEBUG) Log.d(TAG, "getViewForPosition created new ViewHolder");
+ if (offsetPosition < 0 || offsetPosition >= mAdapter.getItemCount()) {
+ throw new IndexOutOfBoundsException("Invalid item position " + position
+ + "(" + offsetPosition + ")");
+ } else {
+ holder = mAdapter.createViewHolder(RecyclerView.this,
+ mAdapter.getItemViewType(offsetPosition));
+ if (DEBUG) {
+ Log.d(TAG, "getViewForPosition created new ViewHolder");
+ }
+ }
}
- if (!holder.isBound() || holder.needsUpdate()) {
+ if (!holder.isRemoved() && (!holder.isBound() || holder.needsUpdate())) {
if (DEBUG) {
Log.d(TAG, "getViewForPosition unbound holder or needs update; updating...");
}
- int offsetPosition = findPositionOffset(position);
- if (offsetPosition < 0 || offsetPosition >= mAdapter.getItemCount()) {
- if (DEBUG) Log.d(TAG, "getViewForPosition: invalid position, returning null");
- return null;
- }
+
// TODO: think through when getOffsetPosition() is called. I use it here because
// existing views have already been offset appropriately through the mOldOffset
// mechanism, but new views do not have this mechanism.
@@ -2564,6 +2616,14 @@
mAttachedScrap.clear();
}
+ /**
+ * Returns a scrap view for the position. If type is not INVALID_TYPE, it also checks if
+ * ViewHolder's type matches the provided type.
+ *
+ * @param position Item position
+ * @param type View type
+ * @return a ViewHolder that can be re-used for this position.
+ */
ViewHolder getScrapViewForPosition(int position, int type) {
final int scrapCount = mAttachedScrap.size();
@@ -2572,7 +2632,7 @@
final ViewHolder holder = mAttachedScrap.get(i);
if (holder.getPosition() == position && !holder.isInvalid() &&
(mInPreLayout || !holder.isRemoved())) {
- if (holder.getItemViewType() != type) {
+ if (type != INVALID_TYPE && holder.getItemViewType() != type) {
Log.e(TAG, "Scrap view for position " + position + " isn't dirty but has" +
" wrong view type! (found " + holder.getItemViewType() +
" but expected " + type + ")");
@@ -2602,7 +2662,8 @@
final ViewHolder holder = mCachedViews.get(i);
if (holder.getPosition() == position) {
mCachedViews.remove(i);
- if (holder.isInvalid() && holder.getItemViewType() != type) {
+ if (holder.isInvalid() &&
+ (type != INVALID_TYPE && holder.getItemViewType() != type)) {
// Can't use it. We don't know where it's been.
if (DEBUG) {
Log.d(TAG, "getScrapViewForPosition(" + position + ", " + type +
@@ -2634,7 +2695,7 @@
Log.d(TAG, "getScrapViewForPosition(" + position + ", " + type +
") fetching from shared pool");
}
- return getRecycledViewPool().getRecycledView(type);
+ return type == INVALID_TYPE ? null : getRecycledViewPool().getRecycledView(type);
}
ViewHolder getScrapViewForId(long id, int type) {
diff --git a/v7/recyclerview/tests/src/android/support/v7/widget/BaseRecyclerViewInstrumentationTest.java b/v7/recyclerview/tests/src/android/support/v7/widget/BaseRecyclerViewInstrumentationTest.java
index 02be171..3c66d71 100644
--- a/v7/recyclerview/tests/src/android/support/v7/widget/BaseRecyclerViewInstrumentationTest.java
+++ b/v7/recyclerview/tests/src/android/support/v7/widget/BaseRecyclerViewInstrumentationTest.java
@@ -26,6 +26,7 @@
import java.util.List;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicInteger;
abstract public class BaseRecyclerViewInstrumentationTest extends
ActivityInstrumentationTestCase2<TestActivity> {
@@ -202,6 +203,8 @@
}
static class Item {
+ final static AtomicInteger idCounter = new AtomicInteger(0);
+ final public int mId = idCounter.incrementAndGet();
int originalIndex;
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 215ab23..d734a57 100644
--- a/v7/recyclerview/tests/src/android/support/v7/widget/RecyclerViewAnimationsTest.java
+++ b/v7/recyclerview/tests/src/android/support/v7/widget/RecyclerViewAnimationsTest.java
@@ -22,6 +22,11 @@
import android.util.Log;
import android.view.View;
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.TimeUnit;
@@ -58,9 +63,19 @@
RecyclerView setupBasic(int itemCount, int firstLayoutStartIndex, int firstLayoutItemCount)
throws Throwable {
+ return setupBasic(itemCount, firstLayoutStartIndex, firstLayoutItemCount, null);
+ }
+
+ RecyclerView setupBasic(int itemCount, int firstLayoutStartIndex, int firstLayoutItemCount,
+ TestAdapter testAdapter)
+ throws Throwable {
final TestRecyclerView recyclerView = new TestRecyclerView(getActivity());
recyclerView.setHasFixedSize(true);
- mTestAdapter = new TestAdapter(itemCount);
+ if (testAdapter == null) {
+ mTestAdapter = new TestAdapter(itemCount);
+ } else {
+ mTestAdapter = testAdapter;
+ }
recyclerView.setAdapter(mTestAdapter);
mLayoutManager = new AnimationLayoutManager();
recyclerView.setLayoutManager(mLayoutManager);
@@ -76,6 +91,65 @@
return recyclerView;
}
+ public void getItemForDeletedViewTest() throws Throwable {
+ testGetItemForDeletedView(false);
+ testGetItemForDeletedView(true);
+ }
+
+ public void testGetItemForDeletedView(boolean stableIds) throws Throwable {
+ final Set<Integer> itemViewTypeQueries = new HashSet<Integer>();
+ final Set<Integer> itemIdQueries = new HashSet<Integer>();
+ TestAdapter adapter = new TestAdapter(10) {
+ @Override
+ public int getItemViewType(int position) {
+ itemViewTypeQueries.add(position);
+ return super.getItemViewType(position);
+ }
+
+ @Override
+ public long getItemId(int position) {
+ itemIdQueries.add(position);
+ return mItems.get(position).mId;
+ }
+ };
+ adapter.setHasStableIds(stableIds);
+ setupBasic(10, 0, 10, adapter);
+ assertEquals("getItemViewType for all items should be called", 10,
+ itemViewTypeQueries.size());
+ if (adapter.hasStableIds()) {
+ assertEquals("getItemId should be called when adapter has stable ids", 10,
+ itemIdQueries.size());
+ } else {
+ assertEquals("getItemId should not be called when adapter does not have stable ids", 0,
+ itemIdQueries.size());
+ }
+ itemViewTypeQueries.clear();
+ itemIdQueries.clear();
+ mLayoutManager.expectLayouts(2);
+ // delete last two
+ final int deleteStart = 8;
+ final int deleteCount = adapter.getItemCount() - deleteStart;
+ adapter.deleteAndNotify(deleteStart, deleteCount);
+ mLayoutManager.waitForLayout(2);
+ for (int i = 0; i < deleteStart; i++) {
+ assertTrue("getItemViewType for existing item " + i + " should be called",
+ itemViewTypeQueries.contains(i));
+ if (adapter.hasStableIds()) {
+ assertTrue("getItemId for existing item " + i
+ + " should be called when adapter has stable ids",
+ itemIdQueries.contains(i));
+ }
+ }
+ for (int i = deleteStart; i < deleteStart + deleteCount; i++) {
+ assertFalse("getItemViewType for deleted item " + i + " SHOULD NOT be called",
+ itemViewTypeQueries.contains(i));
+ if (adapter.hasStableIds()) {
+ assertTrue("getItemId for deleted item " + i + " SHOULD NOT be called",
+ itemIdQueries.contains(i));
+ }
+ }
+ }
+
public void testAddInvisibleAndVisible() throws Throwable {
setupBasic(10, 1, 7);
mLayoutManager.expectLayouts(2);
@@ -323,6 +397,7 @@
}
class TestRecyclerView extends RecyclerView {
+
CountDownLatch drawLatch;
public TestRecyclerView(Context context) {