Merge "GestureConverter: fix inconsistencies from clicks during gestures" into main

Author: Treehugger Robot

services/inputflinger/reader/mapper/gestures/GestureConverter.cpp

@@ -14,11 +14,15 @@
  * limitations under the License.
  */
 
+#include "../Macros.h"
+
 #include "gestures/GestureConverter.h"
 
+#include <ios>
 #include <optional>
 #include <sstream>
 
+#include <android-base/logging.h>
 #include <android-base/stringprintf.h>
 #include <com_android_input_flags.h>
 #include <ftl/enum.h>
@@ -250,6 +254,18 @@ std::list<NotifyArgs> GestureConverter::handleButtonsChange(nsecs_t when, nsecs_
                                                             const Gesture& gesture) {
     std::list<NotifyArgs> out = {};
 
+    if (mCurrentClassification != MotionClassification::NONE) {
+        // Handling button changes during an ongoing gesture would be tricky, as we'd have to avoid
+        // sending duplicate DOWN events or premature UP events (e.g. if the gesture ended but the
+        // button was still down). It would also make handling touchpad events more difficult for
+        // apps, which would have to handle cases where e.g. a scroll gesture ends (and therefore
+        // the event lose the TWO_FINGER_SWIPE classification) but there isn't an UP because the
+        // button's still down. It's unclear how one should even handle button changes during most
+        // gestures, and they're probably accidental anyway. So, instead, just ignore them.
+        LOG(INFO) << "Ignoring button change because a gesture is ongoing.";
+        return out;
+    }
+
     PointerCoords coords;
     coords.clear();
     coords.setAxisValue(AMOTION_EVENT_AXIS_RELATIVE_X, 0);
@@ -312,6 +328,15 @@ std::list<NotifyArgs> GestureConverter::handleButtonsChange(nsecs_t when, nsecs_
     for (uint32_t button = 1; button <= GESTURES_BUTTON_FORWARD; button <<= 1) {
         if (buttonsReleased & button) {
             uint32_t actionButton = gesturesButtonToMotionEventButton(button);
+            if (!(newButtonState & actionButton)) {
+                // We must have received the ButtonsChange gesture that put this button down during
+                // another gesture, and therefore dropped the BUTTON_PRESS action for it, or
+                // released the button when another gesture began during its press. Drop the
+                // BUTTON_RELEASE too to keep the stream consistent.
+                LOG(INFO) << "Dropping release event for button 0x" << std::hex << actionButton
+                          << " as it wasn't in the button state.";
+                continue;
+            }
             newButtonState &= ~actionButton;
             out.push_back(makeMotionArgs(when, readTime, AMOTION_EVENT_ACTION_BUTTON_RELEASE,
                                          actionButton, newButtonState, /* pointerCount= */ 1,
@@ -362,7 +387,7 @@ std::list<NotifyArgs> GestureConverter::handleScroll(nsecs_t when, nsecs_t readT
     std::list<NotifyArgs> out;
     PointerCoords& coords = mFakeFingerCoords[0];
     if (mCurrentClassification != MotionClassification::TWO_FINGER_SWIPE) {
-        out += exitHover(when, readTime);
+        out += prepareForFakeFingerGesture(when, readTime);
 
         mCurrentClassification = MotionClassification::TWO_FINGER_SWIPE;
         coords.clear();
@@ -421,7 +446,7 @@ std::list<NotifyArgs> GestureConverter::handleFling(nsecs_t when, nsecs_t readTi
                     std::list<NotifyArgs> out;
                     mDownTime = when;
                     mCurrentClassification = MotionClassification::TWO_FINGER_SWIPE;
-                    out += exitHover(when, readTime);
+                    out += prepareForFakeFingerGesture(when, readTime);
                     out.push_back(makeMotionArgs(when, readTime, AMOTION_EVENT_ACTION_DOWN,
                                                  /*actionButton=*/0, /*buttonState=*/0,
                                                  /*pointerCount=*/1, &coords));
@@ -479,7 +504,7 @@ std::list<NotifyArgs> GestureConverter::endScroll(nsecs_t when, nsecs_t readTime
         // separate swipes with an appropriate lift event between them, so we don't have to worry
         // about the finger count changing mid-swipe.
 
-        out += exitHover(when, readTime);
+        out += prepareForFakeFingerGesture(when, readTime);
 
         mCurrentClassification = MotionClassification::MULTI_FINGER_SWIPE;
 
@@ -567,9 +592,7 @@ std::list<NotifyArgs> GestureConverter::endScroll(nsecs_t when, nsecs_t readTime
         LOG_ALWAYS_FATAL_IF(gesture.details.pinch.zoom_state != GESTURES_ZOOM_START,
                             "First pinch gesture does not have the START zoom state (%d instead).",
                             gesture.details.pinch.zoom_state);
-        std::list<NotifyArgs> out;
-
-        out += exitHover(when, readTime);
+        std::list<NotifyArgs> out = prepareForFakeFingerGesture(when, readTime);
 
         mCurrentClassification = MotionClassification::PINCH;
         mPinchFingerSeparation = INITIAL_PINCH_SEPARATION_PX;
@@ -644,6 +667,16 @@ std::list<NotifyArgs> GestureConverter::exitHover(nsecs_t when, nsecs_t readTime
     }
 }
 
+std::list<NotifyArgs> GestureConverter::prepareForFakeFingerGesture(nsecs_t when,
+                                                                    nsecs_t readTime) {
+    std::list<NotifyArgs> out;
+    if (isPointerDown(mButtonState)) {
+        out += releaseAllButtons(when, readTime);
+    }
+    out += exitHover(when, readTime);
+    return out;
+}
+
 NotifyMotionArgs GestureConverter::makeHoverEvent(nsecs_t when, nsecs_t readTime, int32_t action) {
     PointerCoords coords;
     coords.clear();

services/inputflinger/reader/mapper/gestures/GestureConverter.h

@@ -92,6 +92,8 @@ class GestureConverter {
     [[nodiscard]] std::list<NotifyArgs> enterHover(nsecs_t when, nsecs_t readTime);
     [[nodiscard]] std::list<NotifyArgs> exitHover(nsecs_t when, nsecs_t readTime);
 
+    [[nodiscard]] std::list<NotifyArgs> prepareForFakeFingerGesture(nsecs_t when, nsecs_t readTime);
+
     NotifyMotionArgs makeHoverEvent(nsecs_t when, nsecs_t readTime, int32_t action);
 
     NotifyMotionArgs makeMotionArgs(nsecs_t when, nsecs_t readTime, int32_t action,

services/inputflinger/tests/GestureConverter_test.cpp

@@ -15,11 +15,15 @@
  */
 
 #include <memory>
+#include <tuple>
 
+#include <android-base/result-gmock.h>
+#include <android-base/result.h>
 #include <com_android_input_flags.h>
 #include <flag_macros.h>
 #include <gestures/GestureConverter.h>
 #include <gtest/gtest.h>
+#include <input/InputVerifier.h>
 
 #include "FakeEventHub.h"
 #include "FakeInputReaderPolicy.h"
@@ -43,8 +47,12 @@ const auto TOUCHPAD_PALM_REJECTION =
 const auto TOUCHPAD_PALM_REJECTION_V2 =
         ACONFIG_FLAG(input_flags, enable_v2_touchpad_typing_palm_rejection);
 
+constexpr stime_t ARBITRARY_GESTURE_TIME = 1.2;
+constexpr stime_t GESTURE_TIME = ARBITRARY_GESTURE_TIME;
+
 } // namespace
 
+using android::base::testing::Ok;
 using testing::AllOf;
 using testing::Each;
 using testing::ElementsAre;
@@ -55,9 +63,8 @@ class GestureConverterTest : public testing::Test {
 protected:
     static constexpr int32_t DEVICE_ID = END_RESERVED_ID + 1000;
     static constexpr int32_t EVENTHUB_ID = 1;
-    static constexpr stime_t ARBITRARY_GESTURE_TIME = 1.2;
 
-    void SetUp() {
+    GestureConverterTest() {
         mFakeEventHub = std::make_unique<FakeEventHub>();
         mFakePolicy = sp<FakeInputReaderPolicy>::make();
         mFakeListener = std::make_unique<TestInputListener>();
@@ -1698,4 +1705,135 @@ TEST_F_WITH_FLAGS(GestureConverterTest, KeypressCancelsHoverMove,
                                     WithMotionAction(AMOTION_EVENT_ACTION_HOVER_MOVE))));
 }
 
+/**
+ * Tests that the event stream output by the converter remains consistent when converting sequences
+ * of Gestures interleaved with button presses in various ways. Takes tuples of three Gestures: one
+ * that starts the gesture sequence, one that continues it (which may or may not be used in a
+ * particular test case), and one that ends it.
+ */
+class GestureConverterConsistencyTest
+      : public GestureConverterTest,
+        public testing::WithParamInterface<std::tuple<Gesture, Gesture, Gesture>> {
+protected:
+    GestureConverterConsistencyTest()
+          : GestureConverterTest(),
+            mParamStartGesture(std::get<0>(GetParam())),
+            mParamContinueGesture(std::get<1>(GetParam())),
+            mParamEndGesture(std::get<2>(GetParam())),
+            mDeviceContext(*mDevice, EVENTHUB_ID),
+            mConverter(*mReader->getContext(), mDeviceContext, DEVICE_ID),
+            mVerifier("Test verifier") {
+        mConverter.setDisplayId(ui::LogicalDisplayId::DEFAULT);
+    }
+
+    base::Result<void> processMotionArgs(NotifyMotionArgs arg) {
+        return mVerifier.processMovement(arg.deviceId, arg.source, arg.action,
+                                         arg.getPointerCount(), arg.pointerProperties.data(),
+                                         arg.pointerCoords.data(), arg.flags);
+    }
+
+    void verifyArgsFromGesture(const Gesture& gesture, size_t gestureIndex) {
+        std::list<NotifyArgs> args =
+                mConverter.handleGesture(ARBITRARY_TIME, READ_TIME, ARBITRARY_TIME, gesture);
+        for (const NotifyArgs& notifyArg : args) {
+            const NotifyMotionArgs& arg = std::get<NotifyMotionArgs>(notifyArg);
+            ASSERT_THAT(processMotionArgs(arg), Ok())
+                    << "when processing " << arg.dump() << "\nfrom gesture " << gestureIndex << ": "
+                    << gesture.String();
+        }
+    }
+
+    void verifyArgsFromGestures(const std::vector<Gesture>& gestures) {
+        for (size_t i = 0; i < gestures.size(); i++) {
+            ASSERT_NO_FATAL_FAILURE(verifyArgsFromGesture(gestures[i], i));
+        }
+    }
+
+    Gesture mParamStartGesture;
+    Gesture mParamContinueGesture;
+    Gesture mParamEndGesture;
+
+    InputDeviceContext mDeviceContext;
+    GestureConverter mConverter;
+    InputVerifier mVerifier;
+};
+
+TEST_P(GestureConverterConsistencyTest, ButtonChangesDuringGesture) {
+    verifyArgsFromGestures({
+            mParamStartGesture,
+            Gesture(kGestureButtonsChange, GESTURE_TIME, GESTURE_TIME,
+                    /*down=*/GESTURES_BUTTON_LEFT, /*up=*/GESTURES_BUTTON_NONE, /*is_tap=*/false),
+            mParamContinueGesture,
+            Gesture(kGestureButtonsChange, GESTURE_TIME, GESTURE_TIME,
+                    /*down=*/GESTURES_BUTTON_NONE, /*up=*/GESTURES_BUTTON_LEFT, /*is_tap=*/false),
+            mParamEndGesture,
+    });
+}
+
+TEST_P(GestureConverterConsistencyTest, ButtonDownDuringGestureAndUpAfterEnd) {
+    verifyArgsFromGestures({
+            mParamStartGesture,
+            Gesture(kGestureButtonsChange, GESTURE_TIME, GESTURE_TIME,
+                    /*down=*/GESTURES_BUTTON_LEFT, /*up=*/GESTURES_BUTTON_NONE, /*is_tap=*/false),
+            mParamContinueGesture,
+            mParamEndGesture,
+            Gesture(kGestureButtonsChange, GESTURE_TIME, GESTURE_TIME,
+                    /*down=*/GESTURES_BUTTON_NONE, /*up=*/GESTURES_BUTTON_LEFT, /*is_tap=*/false),
+    });
+}
+
+TEST_P(GestureConverterConsistencyTest, GestureStartAndEndDuringButtonDown) {
+    verifyArgsFromGestures({
+            Gesture(kGestureButtonsChange, GESTURE_TIME, GESTURE_TIME,
+                    /*down=*/GESTURES_BUTTON_LEFT, /*up=*/GESTURES_BUTTON_NONE, /*is_tap=*/false),
+            mParamStartGesture,
+            mParamContinueGesture,
+            mParamEndGesture,
+            Gesture(kGestureButtonsChange, GESTURE_TIME, GESTURE_TIME,
+                    /*down=*/GESTURES_BUTTON_NONE, /*up=*/GESTURES_BUTTON_LEFT, /*is_tap=*/false),
+    });
+}
+
+TEST_P(GestureConverterConsistencyTest, GestureStartsWhileButtonDownAndEndsAfterUp) {
+    verifyArgsFromGestures({
+            Gesture(kGestureButtonsChange, GESTURE_TIME, GESTURE_TIME,
+                    /*down=*/GESTURES_BUTTON_LEFT, /*up=*/GESTURES_BUTTON_NONE, /*is_tap=*/false),
+            mParamStartGesture,
+            mParamContinueGesture,
+            Gesture(kGestureButtonsChange, GESTURE_TIME, GESTURE_TIME,
+                    /*down=*/GESTURES_BUTTON_NONE, /*up=*/GESTURES_BUTTON_LEFT, /*is_tap=*/false),
+            mParamEndGesture,
+    });
+}
+
+TEST_P(GestureConverterConsistencyTest, TapToClickDuringGesture) {
+    verifyArgsFromGestures({
+            mParamStartGesture,
+            Gesture(kGestureButtonsChange, GESTURE_TIME, GESTURE_TIME,
+                    /*down=*/GESTURES_BUTTON_LEFT, /*up=*/GESTURES_BUTTON_LEFT, /*is_tap=*/false),
+            mParamEndGesture,
+    });
+}
+
+INSTANTIATE_TEST_SUITE_P(
+        GestureAndButtonInterleavings, GestureConverterConsistencyTest,
+        testing::Values(
+                std::make_tuple(Gesture(kGestureScroll, GESTURE_TIME, GESTURE_TIME, 0, -10),
+                                Gesture(kGestureScroll, GESTURE_TIME, GESTURE_TIME, 0, -5),
+                                Gesture(kGestureFling, GESTURE_TIME, GESTURE_TIME, 1, 1,
+                                        GESTURES_FLING_START)),
+                std::make_tuple(Gesture(kGestureSwipe, GESTURE_TIME, GESTURE_TIME, 0, -10),
+                                Gesture(kGestureSwipe, GESTURE_TIME, GESTURE_TIME, 0, 5),
+                                Gesture(kGestureSwipeLift, GESTURE_TIME, GESTURE_TIME)),
+                std::make_tuple(Gesture(kGestureFourFingerSwipe, GESTURE_TIME, GESTURE_TIME, 0,
+                                        -10),
+                                Gesture(kGestureFourFingerSwipe, GESTURE_TIME, GESTURE_TIME, 0, 5),
+                                Gesture(kGestureFourFingerSwipeLift, GESTURE_TIME, GESTURE_TIME)),
+                std::make_tuple(Gesture(kGesturePinch, GESTURE_TIME, GESTURE_TIME,
+                                        /*dz=*/1, GESTURES_ZOOM_START),
+                                Gesture(kGesturePinch, GESTURE_TIME, GESTURE_TIME,
+                                        /*dz=*/0.8, GESTURES_ZOOM_UPDATE),
+                                Gesture(kGesturePinch, GESTURE_TIME, GESTURE_TIME,
+                                        /*dz=*/1, GESTURES_ZOOM_END))));
+
 } // namespace android