Merge lp:~vanvugt/mir/fix-1481570 into lp:mir

[Daniel van Vugt]

Fix incorrect detection of end-of-gesture (AMOTION_EVENT_ACTION_UP).
Since this regression was introduced in r2591 Mir would often not
report the end of a multi-finger gesture, ending with just
mir_touch_action_change instead of mir_touch_action_up.
(LP: #1481570)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2818
http://jenkins.qa.ubuntu.com/job/mir-ci/4481/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3414
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/mir-clang-ts-wily-amd64-build/270
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/942
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3362
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/630
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/630/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3362
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3362/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/6151
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/22333

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/mir-ci/4481/rebuild

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

I don't know this input stack well enough to confirm the logic, but looks OK

review: Approve

[Andreas Pokorny (andreas-pokorny)]

That should not happen with the old android event structures, since those allowed only a single finger up or down per event... But I guess we no longer have that restriction. So the change of the logic looks good.

review: Approve

[Kevin DuBois (kdub)]

lgtm

review: Approve

=== modified file 'src/server/input/android/input_translator.cpp'
--- src/server/input/android/input_translator.cpp	2015-07-23 02:39:20 +0000
+++ src/server/input/android/input_translator.cpp	2015-08-05 07:55:44 +0000
@@ -139,8 +139,16 @@
         auto masked_action = action & AMOTION_EVENT_ACTION_MASK;
         for (unsigned i = 0; i < args->pointerCount; i++)
         {
-            auto action = (i == index_with_action) ? mia::mir_touch_action_from_masked_android(masked_action) :
-                mir_touch_action_change;
+            MirTouchAction action;
+            if (masked_action == AMOTION_EVENT_ACTION_UP)
+                action = mir_touch_action_up;   // irrespective of index
+            else if (masked_action == AMOTION_EVENT_ACTION_DOWN)
+                action = mir_touch_action_down; // irrespective of index
+            else if (i == index_with_action)
+                action = mir_touch_action_from_masked_android(masked_action);
+            else
+                action = mir_touch_action_change;
+
             mev::add_touch(*mir_event, args->pointerProperties[i].id, action,
                            mia::mir_tool_type_from_android(args->pointerProperties[i].toolType),
                            args->pointerCoords[i].getX(),

=== modified file 'tests/unit-tests/input/android/test_input_translator.cpp'
--- tests/unit-tests/input/android/test_input_translator.cpp	2015-07-23 02:39:20 +0000
+++ tests/unit-tests/input/android/test_input_translator.cpp	2015-08-05 07:55:44 +0000
@@ -125,6 +125,55 @@
     translator.notifyMotion(&motion);
 }
 
+MATCHER(EndOfTouchGesture, "")
+{
+    MirEvent const& ev = arg;
+    if (mir_event_get_type(&ev) != mir_event_type_input)
+        return false;
+
+    auto iev = mir_event_get_input_event(&ev);
+    if (mir_input_event_get_type(iev) != mir_input_event_type_touch)
+        return false;
+
+    auto tev = mir_input_event_get_touch_event(iev);
+    if (mir_touch_event_point_count(tev) < 1)
+        return false;
+
+    return mir_touch_event_action(tev, 0) == mir_touch_action_up;
+}
+
+TEST_F(InputTranslator, translates_multifinger_release_correctly)
+{   // Regression test for LP: #1481570
+    using namespace ::testing;
+
+    EXPECT_CALL(dispatcher, dispatch(EndOfTouchGesture()))
+        .Times(1);
+
+    /*
+     * Android often provides old indices on AMOTION_EVENT_ACTION_UP.
+     * But that doesn't matter at all because AMOTION_EVENT_ACTION_UP means
+     * that all fingers were released. (LP: #1481570).
+     */
+    int32_t const invalid_id = 7;
+    int32_t const end_of_gesture = AMOTION_EVENT_ACTION_UP
+                    | (invalid_id << AMOTION_EVENT_ACTION_POINTER_INDEX_SHIFT);
+
+    /*
+     * A multifinger test with only one finger? Yep, that's right. That's
+     * what we see in production. A dummy single finger event used to indicate
+     * all fingers (regardless of number) were released.
+     */
+    properties[0].id = 1;
+    properties[0].toolType = AMOTION_EVENT_TOOL_TYPE_FINGER;
+
+    droidinput::NotifyMotionArgs motion(
+        some_time, device_id, source_id, 0, end_of_gesture,
+        no_flags, meta_state, button_state, edge_flags, 1, properties,
+        coords, x_precision, y_precision, later_time);
+
+    translator.notifyMotion(&motion);
+}
+
 TEST_F(InputTranslator, ignores_motion_with_duplicated_pointerids)
 {
     using namespace ::testing;