Fix selection crash in GestureSelectionHelper MotionEvent handling. (ac4bf9d9) · Commits · e / os / android_packages_apps_DocumentsUI

src/com/android/documentsui/selection/GestureSelectionHelper.java

+55 −41

Original line number	Diff line number	Diff line
		@@ -21,9 +21,12 @@ import static androidx.core.util.Preconditions.checkState;

		import android.graphics.Point;
		import android.os.Build;

		import androidx.annotation.NonNull;
		import androidx.annotation.VisibleForTesting;
		import androidx.recyclerview.widget.RecyclerView;
		import androidx.recyclerview.widget.RecyclerView.OnItemTouchListener;

		import android.util.Log;
		import android.view.MotionEvent;
		import android.view.View;
		@@ -47,7 +50,7 @@ public final class GestureSelectionHelper extends ScrollHost implements OnItemTo
		private final ContentLock mLock;
		private final ItemDetailsLookup mItemLookup;

		private int mLastTouchedItemPosition = -1;
		private int mLastTouchedItemPosition = RecyclerView.NO_POSITION;
		private boolean mStarted = false;
		private Point mLastInterceptedPoint;

		@@ -85,7 +88,7 @@ public final class GestureSelectionHelper extends ScrollHost implements OnItemTo
		// 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 (mLastTouchedItemPosition < 0){
		if (mLastTouchedItemPosition == RecyclerView.NO_POSITION) {
		Log.w(TAG, "Illegal state. Can't start without valid mLastStartedItemPos.");
		return;
		}
		@@ -109,63 +112,72 @@ public final class GestureSelectionHelper extends ScrollHost implements OnItemTo
		if (Shared.DEBUG) Log.w(TAG, "Unexpected Mouse event. Check configuration.");
		}

		switch (e.getActionMasked()) {
		case MotionEvent.ACTION_DOWN:
		// NOTE: Unlike events with other actions, RecyclerView eats
		// "DOWN" events. So even if we return true here we'll
		// never see an event w/ ACTION_DOWN passed to onTouchEvent.
		return handleInterceptedDownEvent(e);
		case MotionEvent.ACTION_MOVE:
		return mStarted;
		// TODO(b/109808552): It seems that mLastStartedItemPos should likely be set as a method
		// parameter in start().
		if (e.getActionMasked() == MotionEvent.ACTION_DOWN) {
		if (mItemLookup.getItemDetails(e) != null) {
		mLastTouchedItemPosition = mView.getItemUnder(e);
		}
		}

		return false;
		// See handleTouch(MotionEvent) javadoc for explanation as to why this is correct.
		return handleTouch(e);
		}

		@Override
		public void onTouchEvent(RecyclerView unused, MotionEvent e) {
		// Note: There were a couple times I as this check firing
		// after combinations of mouse + touch + rotation.
		// But after further investigation I couldn't repro.
		// For that reason we guard this check (for now) w/ IS_DEBUGGABLE.
		if (Build.IS_DEBUGGABLE) checkState(mStarted);
		/** @hide */
		public void onTouchEvent(@NonNull RecyclerView unused, @NonNull MotionEvent e) {
		// See handleTouch(MotionEvent) javadoc for explanation as to why this is correct.
		handleTouch(e);
		}

		/**
		* If selection has started, will handle all appropriate types of MotionEvents and will return
		* true if this OnItemTouchListener should start intercepting the rest of the MotionEvents.
		*
		* <p>This code, and the fact that this method is used by both OnInterceptTouchEvent and
		* OnTouchEvent, is correct and valid because:
		* <ol>
		* <li>MotionEvents that aren't ACTION_DOWN are only ever passed to either onInterceptTouchEvent
		* or onTouchEvent; never to both. The MotionEvents we are handling in this method are not
		* ACTION_DOWN, and therefore, its appropriate that both the onInterceptTouchEvent and
		* onTouchEvent code paths cross this method.
		* <li>This method returns true when we want to intercept MotionEvents. OnInterceptTouchEvent
		* uses that information to determine its own return, and OnMotionEvent doesn't have a return
		* so this methods return value is irrelevant to it.
		* </ol>
		*/
		private boolean handleTouch(MotionEvent e) {
		if (!mStarted) {
		return false;
		}

		switch (e.getActionMasked()) {
		case MotionEvent.ACTION_MOVE:
		handleMoveEvent(e);
		break;
		return true;
		case MotionEvent.ACTION_UP:
		handleUpEvent(e);
		break;
		handleUpEvent();
		return true;
		case MotionEvent.ACTION_CANCEL:
		handleCancelEvent(e);
		break;
		}
		handleCancelEvent();
		return true;
		}

		@Override
		public void onRequestDisallowInterceptTouchEvent(boolean disallowIntercept) {}

		// Called when an ACTION_DOWN event is intercepted. See onInterceptTouchEvent
		// for additional notes.
		// If down event happens on an item, we mark that item's position as last started.
		private boolean handleInterceptedDownEvent(MotionEvent e) {
		// Ignore events where details provider doesn't return details.
		// These objects don't participate in selection.
		if (mItemLookup.getItemDetails(e) == null) {
		return false;
		}
		mLastTouchedItemPosition = mView.getItemUnder(e);
		return mLastTouchedItemPosition != RecyclerView.NO_POSITION;

		@Override
		public void onRequestDisallowInterceptTouchEvent(boolean disallowIntercept) {
		}

		// Called when ACTION_UP event is to be handled.
		// Essentially, since this means all gesture movement is over, reset everything and apply
		// provisional selection.
		private void handleUpEvent(MotionEvent e) {
		private void handleUpEvent() {
		mSelectionMgr.mergeProvisionalSelection();
		endSelection();
		if (mLastTouchedItemPosition > -1) {
		if (mLastTouchedItemPosition != RecyclerView.NO_POSITION) {
		mSelectionMgr.startRange(mLastTouchedItemPosition);
		}
		}
		@@ -173,7 +185,7 @@ public final class GestureSelectionHelper extends ScrollHost implements OnItemTo
		// Called when ACTION_CANCEL event is to be handled.
		// This means this gesture selection is aborted, so reset everything and abandon provisional
		// selection.
		private void handleCancelEvent(MotionEvent unused) {
		private void handleCancelEvent() {
		mSelectionMgr.clearProvisionalSelection();
		endSelection();
		}
		@@ -181,7 +193,7 @@ public final class GestureSelectionHelper extends ScrollHost implements OnItemTo
		private void endSelection() {
		checkState(mStarted);

		mLastTouchedItemPosition = -1;
		mLastTouchedItemPosition = RecyclerView.NO_POSITION;
		mStarted = false;
		mLock.unblock();
		}
		@@ -253,7 +265,9 @@ public final class GestureSelectionHelper extends ScrollHost implements OnItemTo
		@VisibleForTesting
		static abstract class ViewDelegate extends ScrollerCallbacks {
		abstract int getHeight();

		abstract int getItemUnder(MotionEvent e);

		abstract int getLastGlidedItemPosition(MotionEvent e);
		}

tests/unit/com/android/documentsui/selection/GestureSelectionHelperTest.java

+8 −4

Original line number	Diff line number	Diff line
		@@ -22,7 +22,9 @@ import static org.junit.Assert.fail;

		import android.support.test.filters.SmallTest;
		import android.support.test.runner.AndroidJUnit4;

		import androidx.recyclerview.widget.RecyclerView;

		import android.view.MotionEvent;

		import com.android.documentsui.selection.testing.SelectionProbe;
		@@ -95,15 +97,17 @@ public class GestureSelectionHelperTest {
		}

		@Test
		public void testClaimsDownOnItem() {
		public void testDoesNotClaimDownOnItem() {
		mView.mNextPosition = 0;
		assertTrue(mHelper.onInterceptTouchEvent(null, DOWN));
		assertFalse(mHelper.onInterceptTouchEvent(null, DOWN));
		}

		@Test
		public void testClaimsMoveIfStarted() {
		mView.mNextPosition = 0;
		assertTrue(mHelper.onInterceptTouchEvent(null, DOWN));
		// 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.
		mSelectionHelper.select("1");