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

[Daniel van Vugt]

Fix delayed shifting of touch focus when switching apps by gesture
(LP: #1480654).

If your shell (correctly) consumes finger-up events at the end of an
app switch gesture, or if you get none at all (LP: #1481570), then
the old window was still getting touch events even after it's no longer
visible. This branch ensures that the start of the next gesture always
goes to the new window immediately, and never accidentally to the
previous one (a regression introduced in r2652).

[Alan Griffiths (alan-griffiths)]

The code changes look plausible (and certainly address the usecase mentioned).

However, they make the code and comments disagree. (Cue rant on superfluous comments that restate the code.)

review: Needs Fixing

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2823
http://jenkins.qa.ubuntu.com/job/mir-ci/4482/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3416
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/mir-clang-ts-wily-amd64-build/272
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/944
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3364
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/631
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/631/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3364
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3364/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/6153
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/22337

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

review: Approve (continuous-integration)

[Kevin DuBois (kdub)]

lgtm

review: Approve

[Andreas Pokorny (andreas-pokorny)]

hum I am unsure now. The other bugfix gave me the impression that we have to deal with multi-finger up events. Or with gesture-end events that for some reason contain more than one finger. But the logic inside this class still only reacts on pointer_count == 1?

review: Needs Information

[Daniel van Vugt (vanvugt)]

Alan: What comments disagree with what?

Andreas: Good observation. That's a pre-existing problem/feature I was thinking about addressing separately. It appears racarr has designed a lot of the newer input logic with the Android input semantics only in mind. That is he assumes that N-fingers down happens in N separate events (one per finger). That might be true for Android input right now, but it's an assumption we're going to have to polish out later. I could have started on it here, but it's not directly related to the bug fix and other locations need the same fix. So do it separately.

[Daniel van Vugt (vanvugt)]

Before anyone asks, I possibly don't expect this to be the only fix required to fully resolve LP: #1480654. It seems likely other fixes in QtMir/Unity/etc may be required. I will do full stack testing today to confirm. Although the outcome of that testing does not change the need for this Mir fix.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2826
http://jenkins.qa.ubuntu.com/job/mir-ci/4492/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3429
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/mir-clang-ts-wily-amd64-build/285
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/957
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3377/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/641
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/641/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3377
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3377/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/6164/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/22359

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

review: Needs Fixing (continuous-integration)

[Alan Griffiths (alan-griffiths)]

On Thursday, 6 August 2015 04:08:48 BST, Daniel van Vugt wrote:
> Alan: What comments disagree with what?

The comments describing "owner vs non-owner" logic and the code with new
gesture vs owner vs non-owner logic.

--
Sent using Dekko from my Ubuntu device

[Daniel van Vugt (vanvugt)]

I still don't see what comment is wrong or misleading. Can you point it out?

[Alan Griffiths (alan-griffiths)]

> I still don't see what comment is wrong or misleading. Can you point it out?

    "// Either we have a gesture owner or a gesture is just beginning"

This is obsolete: the code now first identifies a new gesture before even considering a gesture owner.

    "// In this case where there is no gesture owner"...

Refers to code that executes with (and without) a gesture owner.

[Daniel van Vugt (vanvugt)]

Oh, you're talking about one comment not shown in the diff, and another comment I have not touched at all. I think only the second one needs updating but am happy to fix both.

[Alan Griffiths (alan-griffiths)]

It isn't about whether the comments appear in the diff: You changed the logic the comments describe.

28 + // We will only deliver events if they signify the start of a new

is still a bit misleading - we also deliver events (a few lines later) if there's a gesture owner.

review: Approve

[Daniel van Vugt (vanvugt)]

That's true, but when read in the full context of the file it's more clear that comment refers to the block of code below it (not shown in the diff). Admittedly, yes, it's not totally correct to say that for the overall function.

[Alan Griffiths (alan-griffiths)]

Yeah, it's mostly the word "only" that misleads.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2828
http://jenkins.qa.ubuntu.com/job/mir-ci/4494/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/3431
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/mir-clang-ts-wily-amd64-build/287
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-wily-amd64-build/959
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/3379
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/643
        deb: http://jenkins.qa.ubuntu.com/job/mir-wily-amd64-ci/643/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3379
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/3379/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/6166
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/22364

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

review: Approve (continuous-integration)

=== modified file 'src/server/input/surface_input_dispatcher.cpp'
--- src/server/input/surface_input_dispatcher.cpp	2015-07-23 02:39:20 +0000
+++ src/server/input/surface_input_dispatcher.cpp	2015-08-06 09:05:17 +0000
@@ -310,26 +310,18 @@
 bool mi::SurfaceInputDispatcher::dispatch_touch(MirInputDeviceId id, MirTouchEvent const* tev)
 {
     std::lock_guard<std::mutex> lg(dispatcher_mutex);
-    // Either we have a gesture owner or a gesture is just beginning
 
     auto point_count = mir_touch_event_point_count(tev);
     
     auto& touch_state = ensure_touch_state(id);
-    if (touch_state.gesture_owner)
-    {
-        deliver(touch_state.gesture_owner, tev);
-        if (point_count == 1 && mir_touch_event_action(tev, 0) == mir_touch_action_up)
-        {
-            // Last touch is coming up. Gesture is over.
-            touch_state.gesture_owner = nullptr;
-        }
-        return true;
-    }
 
-    // In this case where there is no gesture owner we will only deliver events if they signify the start of a new
-    // gesture (as detected by this conditional). This prevents gesture ownership from transfering in the event
-    // a gesture receiver closes mid gesture (e.g. when a surface closes mid swipe we do not want the surface under
-    // to receive events).
+    // We will only deliver events if they signify the start of a new
+    // gesture (as detected by this conditional). This prevents gesture
+    // ownership from transfering in the event a gesture receiver closes mid
+    // gesture (e.g. when a surface closes mid swipe we do not want the
+    // surface under to receive events). This also allows a gesture to continue
+    // outside of the target surface, providing it started in the target
+    // surface.
     if (point_count == 1 && mir_touch_event_action(tev, 0) == mir_touch_action_down)
     {
         geom::Point event_x_y = { mir_touch_event_axis_value(tev, 0, mir_touch_axis_x),
@@ -343,6 +335,17 @@
             return true;
         }
     }
+
+    if (touch_state.gesture_owner)
+    {
+        deliver(touch_state.gesture_owner, tev);
+        if (point_count == 1 && mir_touch_event_action(tev, 0) == mir_touch_action_up)
+        {
+            // Last touch is coming up. Gesture is over.
+            touch_state.gesture_owner = nullptr;
+        }
+        return true;
+    }
         
     return false;
 }

=== modified file 'tests/unit-tests/input/test_surface_input_dispatcher.cpp'
--- tests/unit-tests/input/test_surface_input_dispatcher.cpp	2015-06-25 03:00:08 +0000
+++ tests/unit-tests/input/test_surface_input_dispatcher.cpp	2015-08-06 09:05:17 +0000
@@ -603,6 +603,27 @@
     EXPECT_TRUE(dispatcher.dispatch(*toucher.release_at({2, 2})));
 }
 
+TEST_F(SurfaceInputDispatcher, touch_target_switches_on_finger_down)
+{   // Regression test for LP: #1480654
+    using namespace ::testing;
+    
+    auto left_surface = scene.add_surface({{0, 0}, {1, 1}});
+    auto right_surface = scene.add_surface({{5, 5}, {1, 1}});
+
+    InSequence seq;
+
+    EXPECT_CALL(*left_surface, consume(_)).Times(1);
+    // Note: No TouchUpEvent expected
+    EXPECT_CALL(*right_surface, consume(_)).Times(1);
+
+    dispatcher.start();
+    
+    FakeToucher toucher;
+    EXPECT_TRUE(dispatcher.dispatch(*toucher.touch_at({0, 0})));
+    // Note: No touch release event produced
+    EXPECT_TRUE(dispatcher.dispatch(*toucher.touch_at({5, 5})));
+}
+
 TEST_F(SurfaceInputDispatcher, touch_gestures_terminated_by_device_reset)
 {
     using namespace ::testing;