From 3f330429a38fd1c35a0412e07f7e2090db31822a Mon Sep 17 00:00:00 2001 From: Tony Wickham Date: Thu, 9 Jan 2020 15:19:02 -0800 Subject: [PATCH] Fix BaseSwipeDetector#setState() called inside another setState() Clients of BaseSwipeDetector are required to call finishedScrolling(), which calls setState(IDLE). An obvious place to call this is in onDragEnd(), which itself is called from a setState(SETTLING). If the client does this, then the SETTLING state actually clobbers the IDLE state, leading to undefined behavior. The reason we don't see this in practice is because we usually call finishedScrolling() after an animation from onDragEnd() instead of calling it immediately. To fix this, we add a simple queue such that any calls to setState() while one is in progress have to wait and are executed in turn. This ensures we get all the proper state callbacks and end in the correct one. Also fix an incorrect call in AbstractStateChangeTouchController which was masked by this bug. We were calling setState(IDLE) in onDragStart(), which only worked because the original setState(DRAGGING) incorrectly clobbered this. Now we only setState(IDLE) (via finishedScrolling()) when we fully clear the state, i.e. when the interaction is finished. Test: added testInterleavedSetState Bug: 141939911 Change-Id: Iae630ee7101921b57a85d40646468cf19f59b674 --- .../AbstractStateChangeTouchController.java | 6 +++--- .../launcher3/touch/BaseSwipeDetector.java | 18 +++++++++++++++++- .../touch/SingleAxisSwipeDetectorTest.java | 19 +++++++++++++++++++ 3 files changed, 39 insertions(+), 4 deletions(-) diff --git a/src/com/android/launcher3/touch/AbstractStateChangeTouchController.java b/src/com/android/launcher3/touch/AbstractStateChangeTouchController.java index f40f9762d0..662838be9c 100644 --- a/src/com/android/launcher3/touch/AbstractStateChangeTouchController.java +++ b/src/com/android/launcher3/touch/AbstractStateChangeTouchController.java @@ -508,7 +508,7 @@ public abstract class AbstractStateChangeTouchController mAtomicComponentsController.getAnimationPlayer().end(); mAtomicComponentsController = null; } - cancelAnimationControllers(); + clearState(); boolean shouldGoToTargetState = true; if (mPendingAnimation != null) { boolean reachedTarget = mToState == targetState; @@ -546,13 +546,13 @@ public abstract class AbstractStateChangeTouchController mAtomicAnim = null; } mScheduleResumeAtomicComponent = false; + mDetector.finishedScrolling(); + mDetector.setDetectableScrollConditions(0, false); } private void cancelAnimationControllers() { mCurrentAnimation = null; cancelAtomicComponentsController(); - mDetector.finishedScrolling(); - mDetector.setDetectableScrollConditions(0, false); } private void cancelAtomicComponentsController() { diff --git a/src/com/android/launcher3/touch/BaseSwipeDetector.java b/src/com/android/launcher3/touch/BaseSwipeDetector.java index 12ca5ee7b1..30283dab8d 100644 --- a/src/com/android/launcher3/touch/BaseSwipeDetector.java +++ b/src/com/android/launcher3/touch/BaseSwipeDetector.java @@ -24,6 +24,10 @@ import android.view.VelocityTracker; import android.view.ViewConfiguration; import androidx.annotation.NonNull; +import androidx.annotation.VisibleForTesting; + +import java.util.LinkedList; +import java.util.Queue; /** * Scroll/drag/swipe gesture detector. @@ -49,13 +53,15 @@ public abstract class BaseSwipeDetector { protected final boolean mIsRtl; protected final float mTouchSlop; protected final float mMaxVelocity; + private final Queue mSetStateQueue = new LinkedList<>(); private int mActivePointerId = INVALID_POINTER_ID; private VelocityTracker mVelocityTracker; private PointF mLastDisplacement = new PointF(); private PointF mDisplacement = new PointF(); protected PointF mSubtractDisplacement = new PointF(); - private ScrollState mState = ScrollState.IDLE; + @VisibleForTesting ScrollState mState = ScrollState.IDLE; + private boolean mIsSettingState; protected boolean mIgnoreSlopWhenSettling; @@ -195,6 +201,12 @@ public abstract class BaseSwipeDetector { // SETTLING -> (View settled) -> IDLE private void setState(ScrollState newState) { + if (mIsSettingState) { + mSetStateQueue.add(() -> setState(newState)); + return; + } + mIsSettingState = true; + if (DBG) { Log.d(TAG, "setState:" + mState + "->" + newState); } @@ -212,6 +224,10 @@ public abstract class BaseSwipeDetector { } mState = newState; + mIsSettingState = false; + if (!mSetStateQueue.isEmpty()) { + mSetStateQueue.remove().run(); + } } private void initializeDragging() { diff --git a/tests/src/com/android/launcher3/touch/SingleAxisSwipeDetectorTest.java b/tests/src/com/android/launcher3/touch/SingleAxisSwipeDetectorTest.java index 5174e4dab8..6d463b5684 100644 --- a/tests/src/com/android/launcher3/touch/SingleAxisSwipeDetectorTest.java +++ b/tests/src/com/android/launcher3/touch/SingleAxisSwipeDetectorTest.java @@ -21,9 +21,11 @@ import static com.android.launcher3.touch.SingleAxisSwipeDetector.DIRECTION_POSI import static com.android.launcher3.touch.SingleAxisSwipeDetector.HORIZONTAL; import static com.android.launcher3.touch.SingleAxisSwipeDetector.VERTICAL; +import static org.junit.Assert.assertTrue; import static org.mockito.Matchers.anyBoolean; import static org.mockito.Matchers.anyFloat; import static org.mockito.Matchers.anyObject; +import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; @@ -168,4 +170,21 @@ public class SingleAxisSwipeDetectorTest { // TODO: actually calculate the following parameters and do exact value checks. verify(mMockListener).onDragEnd(anyFloat()); } + + @Test + public void testInterleavedSetState() { + doAnswer(invocationOnMock -> { + // Sets state to IDLE. (Normally onDragEnd() will have state SETTLING.) + mDetector.finishedScrolling(); + return null; + }).when(mMockListener).onDragEnd(anyFloat()); + + mGenerator.put(0, 100, 100); + mGenerator.move(0, 100, 100 + mTouchSlop); + mGenerator.move(0, 100, 100 + mTouchSlop * 2); + mGenerator.lift(0); + verify(mMockListener).onDragEnd(anyFloat()); + assertTrue("SwipeDetector should be IDLE but was " + mDetector.mState, + mDetector.isIdleState()); + } }