Merge lp:~dandrader/mir/pressure_lp1238417 into lp:mir

[Daniel d'Andrada]

Don't assume pressure value is zero if not yet known

Instead, ignore pressure axis until its value is known.
Otherwise a touch point coulbe be wrongly considered as hovering.

Fixes https://bugs.launchpad.net/mir/+bug/1238417

[Kevin DuBois (kdub)]

lgtm

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1336
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/677/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/658
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/654
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/261
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/407
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/407/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/411
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/411/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/261
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/261/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/278
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/3156

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-team-mir-development-branch-ci/677/rebuild

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

Looks good.

I have previously encountered problems on N7 (and N10?) where touch events weren't registered unless I pushed very hard. Would that be related?... Can't reproduce the issue today though.

review: Approve

[Daniel d'Andrada (dandrader)]

> I have previously encountered problems on N7 (and N10?) where touch events
> weren't registered unless I pushed very hard. Would that be related?... Can't
> reproduce the issue today though.

Not to this specific bug at least. You experience the bug fixed here only when there were input events before mir was runnning and the pressure value is constant, which is the case for a fake touch device. On a real touch device, if it does have a pressure axis, it will surely change its value and therefore mir will be aware of its state.

If you got responses only when you pushed very hard, one explanation would be that the pressure values were not going beyond zero (i.e., they were all zero or negative), which is the boundary android-input considers between a pressed touch point and a hovering one. But I don't believe that would be happening as android works fine in all those devices, of course.

[Alexandros Frantzis (afrantzis)]

Looks good (AFAICT).

Nit:
// which is de default value when the accumulator is cleared

s/de /the /

review: Approve

=== modified file '3rd_party/android-input/android/frameworks/base/services/input/InputReader.cpp'
--- 3rd_party/android-input/android/frameworks/base/services/input/InputReader.cpp	2014-01-10 03:59:43 +0000
+++ 3rd_party/android-input/android/frameworks/base/services/input/InputReader.cpp	2014-01-16 20:44:57 +0000
@@ -1676,6 +1676,7 @@
             case ABS_MT_PRESSURE:
                 slot->mInUse = true;
                 slot->mAbsMTPressure = rawEvent->value;
+                slot->mHaveAbsMTPressure = true;
                 break;
             case ABS_MT_DISTANCE:
                 slot->mInUse = true;
@@ -1715,6 +1716,7 @@
     mInUse = false;
     mHaveAbsMTTouchMinor = false;
     mHaveAbsMTWidthMinor = false;
+    mHaveAbsMTPressure = false;
     mHaveAbsMTToolType = false;
     mAbsMTPositionX = 0;
     mAbsMTPositionY = 0;
@@ -5909,7 +5911,8 @@
 
         bool isHovering = mTouchButtonAccumulator.getToolType() != AMOTION_EVENT_TOOL_TYPE_MOUSE
                 && (mTouchButtonAccumulator.isHovering()
-                        || (mRawPointerAxes.pressure.valid && inSlot->getPressure() <= 0));
+                        || (mRawPointerAxes.pressure.valid
+                            && inSlot->havePressure() && inSlot->getPressure() <= 0));
         outPointer.isHovering = isHovering;
 
         // Assign pointer id using tracking id if available.

=== modified file '3rd_party/android-input/android/frameworks/base/services/input/InputReader.h'
--- 3rd_party/android-input/android/frameworks/base/services/input/InputReader.h	2014-01-10 03:59:43 +0000
+++ 3rd_party/android-input/android/frameworks/base/services/input/InputReader.h	2014-01-16 20:44:57 +0000
@@ -812,6 +812,7 @@
         inline int32_t getOrientation() const { return mAbsMTOrientation; }
         inline int32_t getTrackingId() const { return mAbsMTTrackingId; }
         inline int32_t getPressure() const { return mAbsMTPressure; }
+        inline bool havePressure() const { return mHaveAbsMTPressure; }
         inline int32_t getDistance() const { return mAbsMTDistance; }
         inline int32_t getToolType() const;
 
@@ -821,6 +822,7 @@
         bool mInUse;
         bool mHaveAbsMTTouchMinor;
         bool mHaveAbsMTWidthMinor;
+        bool mHaveAbsMTPressure;
         bool mHaveAbsMTToolType;
 
         int32_t mAbsMTPositionX;

=== modified file 'tests/unit-tests/android_input/input_reader.cpp'
--- tests/unit-tests/android_input/input_reader.cpp	2014-01-10 03:59:43 +0000
+++ tests/unit-tests/android_input/input_reader.cpp	2014-01-16 20:44:57 +0000
@@ -4785,4 +4785,35 @@
     process(mapper, ARBITRARY_TIME, DEVICE_ID, EV_SYN, SYN_REPORT, 0x00000000);
 }
 
+// Regression test for LP#1238417:
+// https://bugs.launchpad.net/mir/+bug/1238417
+//
+// Don't assume that ABS_MT_PRESSURE is 0 if you haven't heard from it yet. Instead,
+// the pressure axis should be considered as absent.
+// If a touch point has always the same positive, non-zero, pressure, that value
+// will be informed only once, on its first event. But if mir wasn't running at
+// that time, it would consider the pressure to be zero and therefore the point to be
+// hovering, which is wrong.
+TEST_F(MultiTouchInputMapperTest, Process_IgnorePressureAxisIfValueUnknown) {
+    MultiTouchInputMapper* mapper = new MultiTouchInputMapper(mDevice);
+    addConfigurationProperty("touch.deviceType", "touchScreen");
+    addConfigurationProperty("device.internal", "1");
+    setDisplayInfoAndReconfigure(DISPLAY_ID, 1024, 768, DISPLAY_ORIENTATION_0);
+    prepareAxes(POSITION | PRESSURE | ID);
+    addMapperAndConfigure(mapper);
+
+    // Note that there's no ABS_MT_PRESSURE
+    process(mapper, ARBITRARY_TIME, DEVICE_ID, EV_ABS, ABS_MT_TRACKING_ID, 0x00000018);
+    process(mapper, ARBITRARY_TIME, DEVICE_ID, EV_ABS, ABS_MT_POSITION_X, 500);
+    process(mapper, ARBITRARY_TIME, DEVICE_ID, EV_ABS, ABS_MT_POSITION_Y, 300);
+    process(mapper, ARBITRARY_TIME, DEVICE_ID, EV_SYN, SYN_REPORT, 0x00000000);
+
+    NotifyMotionArgs args;
+    ASSERT_NO_FATAL_FAILURE(mFakeListener->assertNotifyMotionWasCalled(&args));
+
+    // Would be AMOTION_EVENT_ACTION_HOVER_ENTER if pressure value was assumed to be 0 (zero),
+    // which is de default value when the accumulator is cleared.
+    ASSERT_EQ(AMOTION_EVENT_ACTION_DOWN, args.action);
+}
+
 } // namespace android