Revert "Cleanup Gesture selection."
This reverts commit 809b6acbcbef583825e6f80af0e877fa946b9233.
Reason for revert: Caused regression.
https://b.corp.google.com/issues/78615740
Change-Id: Ic23a08352d3b183518ef90460129099cb65668d8
Bug: 78615740
diff --git a/recyclerview/selection/src/androidTest/java/androidx/recyclerview/selection/GestureSelectionHelperTest.java b/recyclerview/selection/src/androidTest/java/androidx/recyclerview/selection/GestureSelectionHelperTest.java
index d84cb83..de50953 100644
--- a/recyclerview/selection/src/androidTest/java/androidx/recyclerview/selection/GestureSelectionHelperTest.java
+++ b/recyclerview/selection/src/androidTest/java/androidx/recyclerview/selection/GestureSelectionHelperTest.java
@@ -18,7 +18,6 @@
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
-import static org.junit.Assert.fail;
import android.view.MotionEvent;
@@ -26,7 +25,7 @@
import androidx.recyclerview.selection.testing.SelectionTrackers;
import androidx.recyclerview.selection.testing.TestAutoScroller;
import androidx.recyclerview.selection.testing.TestEvents;
-import androidx.recyclerview.selection.testing.TestSelectionPredicate;
+import androidx.recyclerview.selection.testing.TestItemDetailsLookup;
import androidx.recyclerview.widget.RecyclerView;
import androidx.test.ext.junit.runners.AndroidJUnit4;
import androidx.test.filters.SmallTest;
@@ -56,7 +55,7 @@
private GestureSelectionHelper mHelper;
private SelectionTracker<String> mSelectionTracker;
- private TestSelectionPredicate<String> mSelectionPredicate;
+ private TestItemDetailsLookup mDetailsLookup;
private SelectionProbe mSelection;
private OperationMonitor mMonitor;
private TestViewDelegate mView;
@@ -64,14 +63,13 @@
@Before
public void setUp() {
mSelectionTracker = SelectionTrackers.createStringTracker("gesture-selection-test", 100);
- mSelectionPredicate = new TestSelectionPredicate<>();
+ mDetailsLookup = new TestItemDetailsLookup();
+ mDetailsLookup.initAt(3);
mSelection = new SelectionProbe(mSelectionTracker);
mMonitor = new OperationMonitor();
mView = new TestViewDelegate();
mHelper = new GestureSelectionHelper(
- mSelectionTracker, mSelectionPredicate, mView, new TestAutoScroller(), mMonitor);
-
- mSelectionPredicate.setReturnValue(true);
+ mSelectionTracker, mDetailsLookup, mView, new TestAutoScroller(), mMonitor);
}
@Test
@@ -82,26 +80,16 @@
@Test
public void testIgnoresDown_NoItemDetails() {
+ mDetailsLookup.reset();
assertFalse(mHelper.onInterceptTouchEvent(null, DOWN));
}
@Test
- public void testThrowsWhenStartingWithoutRange() {
- mView.mNextPosition = 0;
-
- // Normally, this is controller by the TouchSelectionHelper via a a long press gesture.
- mSelectionTracker.select("1");
- // If mSelectionTracker.anchorRange(1) didn't initialize the range, then start
- // should throw an exception.
- try {
- mHelper.start();
- fail("Should have thrown IllegalStateException.");
- } catch (IllegalStateException expected) { }
- }
-
- @Test
- public void testIgnoresEventsWhenNotStarted() {
- assertFalse(mHelper.onInterceptTouchEvent(null, MOVE));
+ public void testNoStartOnIllegalPosition() {
+ mView.mNextPosition = RecyclerView.NO_POSITION;
+ mHelper.onInterceptTouchEvent(null, DOWN);
+ mHelper.start();
+ assertFalse(mMonitor.isStarted());
}
@Test
@@ -113,12 +101,14 @@
@Test
public void testClaimsMoveIfStarted() {
mView.mNextPosition = 0;
+ // TODO(b/109808552): This should be removed with that bug is fixed because it will be a
+ // no-op at that time.
+ mHelper.onInterceptTouchEvent(null, DOWN);
// Normally, this is controller by the TouchSelectionHelper via a a long press gesture.
mSelectionTracker.select("1");
mSelectionTracker.anchorRange(1);
mHelper.start();
-
assertTrue(mHelper.onInterceptTouchEvent(null, MOVE));
}
@@ -132,7 +122,6 @@
mSelectionTracker.select("1");
mSelectionTracker.anchorRange(1);
-
mHelper.start();
mHelper.onTouchEvent(null, MOVE);
@@ -141,7 +130,7 @@
mHelper.onTouchEvent(null, MOVE);
mHelper.onTouchEvent(null, UP);
- mSelection.assertRangeSelected(1, 9);
+ mSelection.assertRangeSelected(1, 9);
}
private static final class TestViewDelegate extends GestureSelectionHelper.ViewDelegate {
diff --git a/recyclerview/selection/src/androidTest/java/androidx/recyclerview/selection/TouchInputHandlerTest.java b/recyclerview/selection/src/androidTest/java/androidx/recyclerview/selection/TouchInputHandlerTest.java
index 15fe7e3..b4cbf4a 100644
--- a/recyclerview/selection/src/androidTest/java/androidx/recyclerview/selection/TouchInputHandlerTest.java
+++ b/recyclerview/selection/src/androidTest/java/androidx/recyclerview/selection/TouchInputHandlerTest.java
@@ -20,12 +20,13 @@
import static org.junit.Assert.assertFalse;
+import android.view.MotionEvent;
+
import androidx.recyclerview.selection.ItemDetailsLookup.ItemDetails;
import androidx.recyclerview.selection.testing.SelectionProbe;
import androidx.recyclerview.selection.testing.SelectionTrackers;
import androidx.recyclerview.selection.testing.TestAdapter;
import androidx.recyclerview.selection.testing.TestData;
-import androidx.recyclerview.selection.testing.TestDragListener;
import androidx.recyclerview.selection.testing.TestFocusDelegate;
import androidx.recyclerview.selection.testing.TestItemDetailsLookup;
import androidx.recyclerview.selection.testing.TestItemKeyProvider;
@@ -53,10 +54,11 @@
private TestSelectionPredicate mSelectionPredicate;
private TestRunnable mGestureStarted;
private TestRunnable mHapticPerformer;
+ private TestDragListener mOnItemDragListener;
private TestOnItemActivatedListener mActivationCallbacks;
+ private TestFocusDelegate mFocusCallbacks;
private TestItemDetailsLookup mDetailsLookup;
private SelectionProbe mSelection;
- private TestDragListener mDragInitiatedListener;
@Before
public void setUp() {
@@ -66,8 +68,9 @@
mSelection = new SelectionProbe(mSelectionMgr);
mGestureStarted = new TestRunnable();
mHapticPerformer = new TestRunnable();
+ mOnItemDragListener = new TestDragListener();
mActivationCallbacks = new TestOnItemActivatedListener();
- mDragInitiatedListener = new TestDragListener();
+ mFocusCallbacks = new TestFocusDelegate();
mInputDelegate = new TouchInputHandler(
mSelectionMgr,
@@ -77,16 +80,15 @@
mDetailsLookup,
mSelectionPredicate,
mGestureStarted,
- mDragInitiatedListener,
+ mOnItemDragListener,
mActivationCallbacks,
- new TestFocusDelegate(),
+ mFocusCallbacks,
mHapticPerformer);
}
@Test
public void testTap_ActivatesWhenNoExistingSelection() {
ItemDetails doc = mDetailsLookup.initAt(11);
-
mInputDelegate.onSingleTapUp(TAP);
mActivationCallbacks.assertActivated(doc);
@@ -98,69 +100,26 @@
}
@Test
- public void testLongPress_UnselectedItem_Selects() {
+ public void testLongPress_SelectsItem() {
mSelectionPredicate.setReturnValue(true);
- mDetailsLookup.initAt(7);
+ mDetailsLookup.initAt(7);
mInputDelegate.onLongPress(TAP);
mSelection.assertSelection(7);
}
@Test
- public void testLongPress_UnselectedItem_PerformsHapticFeedback() {
+ public void testLongPress_StartsGestureSelection() {
mSelectionPredicate.setReturnValue(true);
+
mDetailsLookup.initAt(7);
-
mInputDelegate.onLongPress(TAP);
-
- mHapticPerformer.assertRan();
- }
-
- @Test
- public void testLongPress_UnselectedItem_StartsGestureSelection() {
- mSelectionPredicate.setReturnValue(true);
- mDetailsLookup.initAt(7);
-
- mInputDelegate.onLongPress(TAP);
-
mGestureStarted.assertRan();
}
@Test
- public void testLongPress_UnselectedItem_DoesNotInitiateDrag() {
- mSelectionPredicate.setReturnValue(true);
- mDetailsLookup.initAt(7);
-
- mInputDelegate.onLongPress(TAP);
-
- mDragInitiatedListener.assertDragInitiated(false);
- }
-
- @Test
- public void testLongPress_SelectedItem_InitiatesDrag() {
- mSelectionPredicate.setReturnValue(true);
- mSelectionMgr.select("7");
- mDetailsLookup.initAt(7);
-
- mInputDelegate.onLongPress(TAP);
-
- mDragInitiatedListener.assertDragInitiated(true);
- }
-
- @Test
- public void testLongPress_SelectedItem_PerformsHapticFeedback() {
- mSelectionPredicate.setReturnValue(true);
- mSelectionMgr.select("7");
- mDetailsLookup.initAt(7);
-
- mInputDelegate.onLongPress(TAP);
-
- mHapticPerformer.assertRan();
- }
-
- @Test
- public void testTapSelectHotspot_UnselectedItem_Selections() {
+ public void testSelectHotspot_StartsSelectionMode() {
mSelectionPredicate.setReturnValue(true);
mDetailsLookup.initAt(7).setInItemSelectRegion(true);
@@ -170,7 +129,7 @@
}
@Test
- public void testTapSelectionHotspot_SelectedItem_Unselected() {
+ public void testSelectionHotspot_UnselectsSelectedItem() {
mSelectionMgr.select("11");
mDetailsLookup.initAt(11).setInItemSelectRegion(true);
@@ -180,6 +139,16 @@
}
@Test
+ public void testStartsSelection_PerformsHapticFeedback() {
+ mSelectionPredicate.setReturnValue(true);
+
+ mDetailsLookup.initAt(7);
+ mInputDelegate.onLongPress(TAP);
+
+ mHapticPerformer.assertRan();
+ }
+
+ @Test
public void testLongPress_AddsToSelection() {
mSelectionPredicate.setReturnValue(true);
@@ -221,4 +190,11 @@
mSelection.assertNoSelection();
}
+
+ private static final class TestDragListener implements OnDragInitiatedListener {
+ @Override
+ public boolean onDragInitiated(MotionEvent e) {
+ return false;
+ }
+ }
}
diff --git a/recyclerview/selection/src/androidTest/java/androidx/recyclerview/selection/testing/TestDragListener.java b/recyclerview/selection/src/androidTest/java/androidx/recyclerview/selection/testing/TestDragListener.java
deleted file mode 100644
index 643c779..0000000
--- a/recyclerview/selection/src/androidTest/java/androidx/recyclerview/selection/testing/TestDragListener.java
+++ /dev/null
@@ -1,43 +0,0 @@
-/*
- * Copyright 2019 The Android Open Source Project
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-package androidx.recyclerview.selection.testing;
-
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertTrue;
-
-import android.view.MotionEvent;
-
-import androidx.recyclerview.selection.OnDragInitiatedListener;
-
-public final class TestDragListener implements OnDragInitiatedListener {
-
- private boolean mDragInitiated;
-
- @Override
- public boolean onDragInitiated(MotionEvent e) {
- mDragInitiated = true;
- return false;
- }
-
- public void assertDragInitiated(boolean expected) {
- if (expected) {
- assertTrue("onDragInitiated never called.", mDragInitiated);
- } else {
- assertFalse("onDragInitiated called, but should not have.", mDragInitiated);
- }
- }
-}
diff --git a/recyclerview/selection/src/main/java/androidx/recyclerview/selection/GestureSelectionHelper.java b/recyclerview/selection/src/main/java/androidx/recyclerview/selection/GestureSelectionHelper.java
index 7f2a5bb..f127576 100644
--- a/recyclerview/selection/src/main/java/androidx/recyclerview/selection/GestureSelectionHelper.java
+++ b/recyclerview/selection/src/main/java/androidx/recyclerview/selection/GestureSelectionHelper.java
@@ -19,6 +19,7 @@
import static androidx.core.util.Preconditions.checkArgument;
import static androidx.core.util.Preconditions.checkState;
+import android.graphics.Point;
import android.util.Log;
import android.view.MotionEvent;
import android.view.View;
@@ -26,7 +27,6 @@
import androidx.annotation.NonNull;
import androidx.annotation.VisibleForTesting;
import androidx.core.view.ViewCompat;
-import androidx.recyclerview.selection.SelectionTracker.SelectionPredicate;
import androidx.recyclerview.widget.RecyclerView;
import androidx.recyclerview.widget.RecyclerView.OnItemTouchListener;
@@ -41,11 +41,12 @@
private static final String TAG = "GestureSelectionHelper";
private final SelectionTracker<?> mSelectionMgr;
- private final SelectionTracker.SelectionPredicate<?> mSelectionPredicate;
+ private final ItemDetailsLookup<?> mDetailsLookup;
private final AutoScroller mScroller;
private final ViewDelegate mView;
private final OperationMonitor mLock;
+ private int mLastStartedItemPos = RecyclerView.NO_POSITION;
private boolean mStarted = false;
/**
@@ -54,19 +55,19 @@
*/
GestureSelectionHelper(
@NonNull SelectionTracker<?> selectionTracker,
- @NonNull SelectionPredicate<?> selectionPredicate,
+ @NonNull ItemDetailsLookup<?> detailsLookup,
@NonNull ViewDelegate view,
@NonNull AutoScroller scroller,
@NonNull OperationMonitor lock) {
checkArgument(selectionTracker != null);
- checkArgument(selectionPredicate != null);
+ checkArgument(detailsLookup != null);
checkArgument(view != null);
checkArgument(scroller != null);
checkArgument(lock != null);
mSelectionMgr = selectionTracker;
- mSelectionPredicate = selectionPredicate;
+ mDetailsLookup = detailsLookup;
mView = view;
mScroller = scroller;
mLock = lock;
@@ -77,9 +78,16 @@
*/
void start() {
checkState(!mStarted);
+ // See: b/70518185. It appears start() is being called via onLongPress
+ // even though we never received an intial handleInterceptedDownEvent
+ // where we would usually initialize mLastStartedItemPos.
+ if (mLastStartedItemPos == RecyclerView.NO_POSITION) {
+ Log.w(TAG, "Illegal state. Can't start without valid mLastStartedItemPos.");
+ return;
+ }
// Partner code in MotionInputHandler ensures items
- // are selected and range anchor initialized prior to
+ // are selected and range established prior to
// start being called.
// Verify the truth of that statement here
// to make the implicit coupling less of a time bomb.
@@ -94,14 +102,20 @@
@Override
/** @hide */
public boolean onInterceptTouchEvent(@NonNull RecyclerView unused, @NonNull MotionEvent e) {
- switch (e.getActionMasked()) {
- case MotionEvent.ACTION_MOVE:
- case MotionEvent.ACTION_UP:
- case MotionEvent.ACTION_CANCEL:
- return mStarted;
- default:
- return false;
+ if (MotionEvents.isMouseEvent(e)) {
+ if (Shared.DEBUG) Log.w(TAG, "Unexpected Mouse event. Check configuration.");
}
+
+ // TODO(b/109808552): It seems that mLastStartedItemPos should likely be set as a method
+ // parameter in start().
+ if (e.getActionMasked() == MotionEvent.ACTION_DOWN) {
+ if (mDetailsLookup.getItemDetails(e) != null) {
+ mLastStartedItemPos = mView.getItemUnder(e);
+ }
+ }
+
+ // See handleTouch(MotionEvent) javadoc for explanation as to why this is correct.
+ return handleTouch(e);
}
@Override
@@ -132,14 +146,6 @@
return false;
}
- if (!mSelectionMgr.isRangeActive()) {
- Log.e(TAG,
- "Internal state of GestureSelectionHelper out of sync w/ SelectionTracker "
- + "(isRangeActive is false). Ignoring event and resetting state.");
- endSelection();
- return false;
- }
-
switch (e.getActionMasked()) {
case MotionEvent.ACTION_MOVE:
handleMoveEvent(e);
@@ -166,6 +172,9 @@
private void handleUpEvent() {
mSelectionMgr.mergeProvisionalSelection();
endSelection();
+ if (mLastStartedItemPos != RecyclerView.NO_POSITION) {
+ mSelectionMgr.startRange(mLastStartedItemPos);
+ }
}
// Called when ACTION_CANCEL event is to be handled.
@@ -179,6 +188,7 @@
private void endSelection() {
checkState(mStarted);
+ mLastStartedItemPos = RecyclerView.NO_POSITION;
mStarted = false;
mScroller.reset();
mLock.stop();
@@ -187,12 +197,14 @@
// Call when an intercepted ACTION_MOVE event is passed down.
// At this point, we are sure user wants to gesture multi-select.
private void handleMoveEvent(@NonNull MotionEvent e) {
+ Point lastInterceptedPoint = MotionEvents.getOrigin(e);
+
int lastGlidedItemPos = mView.getLastGlidedItemPosition(e);
- if (mSelectionPredicate.canSetStateAtPosition(lastGlidedItemPos, true)) {
+ if (lastGlidedItemPos != RecyclerView.NO_POSITION) {
extendSelection(lastGlidedItemPos);
}
- mScroller.scroll(MotionEvents.getOrigin(e));
+ mScroller.scroll(lastInterceptedPoint);
}
// It's possible for events to go over the top/bottom of the RecyclerView.
@@ -220,14 +232,14 @@
*/
static GestureSelectionHelper create(
@NonNull SelectionTracker<?> selectionMgr,
- @NonNull SelectionPredicate<?> selectionPredicate,
+ @NonNull ItemDetailsLookup<?> detailsLookup,
@NonNull RecyclerView recyclerView,
@NonNull AutoScroller scroller,
@NonNull OperationMonitor lock) {
return new GestureSelectionHelper(
selectionMgr,
- selectionPredicate,
+ detailsLookup,
new RecyclerViewDelegate(recyclerView),
scroller,
lock);
diff --git a/recyclerview/selection/src/main/java/androidx/recyclerview/selection/MotionInputHandler.java b/recyclerview/selection/src/main/java/androidx/recyclerview/selection/MotionInputHandler.java
index 0a3d324..a7eb208 100644
--- a/recyclerview/selection/src/main/java/androidx/recyclerview/selection/MotionInputHandler.java
+++ b/recyclerview/selection/src/main/java/androidx/recyclerview/selection/MotionInputHandler.java
@@ -88,7 +88,7 @@
mFocusDelegate.focusItem(details);
}
- final boolean shouldExtendRange(@NonNull MotionEvent e) {
+ final boolean isRangeExtension(@NonNull MotionEvent e) {
return MotionEvents.isShiftKeyPressed(e)
&& mSelectionTracker.isRangeActive()
// Without full corpus access we can't reliably implement range
diff --git a/recyclerview/selection/src/main/java/androidx/recyclerview/selection/MouseInputHandler.java b/recyclerview/selection/src/main/java/androidx/recyclerview/selection/MouseInputHandler.java
index e4f4548..ca45137 100644
--- a/recyclerview/selection/src/main/java/androidx/recyclerview/selection/MouseInputHandler.java
+++ b/recyclerview/selection/src/main/java/androidx/recyclerview/selection/MouseInputHandler.java
@@ -126,7 +126,7 @@
checkState(mSelectionTracker.hasSelection());
checkArgument(item != null);
- if (shouldExtendRange(e)) {
+ if (isRangeExtension(e)) {
extendSelectionRange(item);
} else {
if (shouldClearSelection(e, item)) {
diff --git a/recyclerview/selection/src/main/java/androidx/recyclerview/selection/SelectionTracker.java b/recyclerview/selection/src/main/java/androidx/recyclerview/selection/SelectionTracker.java
index 4e3a932..bd63fd6 100644
--- a/recyclerview/selection/src/main/java/androidx/recyclerview/selection/SelectionTracker.java
+++ b/recyclerview/selection/src/main/java/androidx/recyclerview/selection/SelectionTracker.java
@@ -711,7 +711,7 @@
// of motions and gestures in order to provide gesture driven selection support
// when used in conjunction with RecyclerView.
final GestureSelectionHelper gestureHelper = GestureSelectionHelper.create(
- tracker, mSelectionPredicate, mRecyclerView, scroller, mMonitor);
+ tracker, mDetailsLookup, mRecyclerView, scroller, mMonitor);
// Finally hook the framework up to listening to recycle view events.
mRecyclerView.addOnItemTouchListener(eventRouter);
diff --git a/recyclerview/selection/src/main/java/androidx/recyclerview/selection/TouchInputHandler.java b/recyclerview/selection/src/main/java/androidx/recyclerview/selection/TouchInputHandler.java
index d82812c..271ff3c 100644
--- a/recyclerview/selection/src/main/java/androidx/recyclerview/selection/TouchInputHandler.java
+++ b/recyclerview/selection/src/main/java/androidx/recyclerview/selection/TouchInputHandler.java
@@ -88,7 +88,7 @@
}
if (mSelectionTracker.hasSelection()) {
- if (shouldExtendRange(e)) {
+ if (isRangeExtension(e)) {
extendSelectionRange(item);
} else if (mSelectionTracker.isSelected(item.getSelectionKey())) {
mSelectionTracker.deselect(item.getSelectionKey());
@@ -121,26 +121,32 @@
boolean handled = false;
- if (shouldExtendRange(e)) {
+ if (isRangeExtension(e)) {
extendSelectionRange(item);
- mHapticPerformer.run();
+ handled = true;
} else {
- if (mSelectionTracker.isSelected(item.getSelectionKey())) {
- // Long press on existing selected item initiates drag/drop.
- mOnDragInitiatedListener.onDragInitiated(e);
- mHapticPerformer.run();
- } else if (mSelectionPredicate.canSetStateForKey(item.getSelectionKey(), true)
- && selectItem(item)) {
- // And finally if the item was selected && we can select multiple
- // we kick off gesture selection.
- // NOTE: isRangeActive should ALWAYS be true at this point, but there have
- // been reports indicating that assumption isn't correct. So we explicitly
- // check isRangeActive.
- if (mSelectionPredicate.canSelectMultiple() && mSelectionTracker.isRangeActive()) {
- mGestureStarter.run();
+ if (!mSelectionTracker.isSelected(item.getSelectionKey())
+ && mSelectionPredicate.canSetStateForKey(item.getSelectionKey(), true)) {
+ // If we cannot select it, we didn't apply anchoring - therefore should not
+ // start gesture selection
+ if (selectItem(item)) {
+ // And finally if the item was selected && we can select multiple
+ // we kick off gesture selection.
+ if (mSelectionPredicate.canSelectMultiple()) {
+ mGestureStarter.run();
+ }
+ handled = true;
}
- mHapticPerformer.run();
+ } else {
+ // We only initiate drag and drop on long press for touch to allow regular
+ // touch-based scrolling
+ mOnDragInitiatedListener.onDragInitiated(e);
+ handled = true;
}
}
+
+ if (handled) {
+ mHapticPerformer.run();
+ }
}
}
