Merge lp:~cemil-azizoglu/mir/fix-1556210 into lp:mir

[Cemil Azizoglu]

XNextEvent is a blocking call. Check to make sure there is an event before calling it. (LP: #1556210)

XNextEvent is a blocking call. Check to make sure there is an event before calling it.

[Cemil Azizoglu (cemil-azizoglu)]

Let me know if there is a better way to handle the gmock ugliness.

[Mir CI Bot (mir-ci-bot)]

PASSED: Continuous integration, rev:3386
https://mir-jenkins.ubuntu.com/job/mir-ci/551/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/433
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/463
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/455
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/455
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/442
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/442/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/442
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/442/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/442
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/442/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/442
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/442/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/442
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/442/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/551/rebuild

review: Approve (continuous-integration)

[Andreas Pokorny (andreas-pokorny)]

Yes.

On the mock default action...

 ON_CALL(*this, XPending(_))
    .WillByDefault(InvokeWithoutArgs([this](){ return fake_x11.event_vector.size();}));

 ON_CALL(*this, XNextEvent(_,_)
     .WillByDefault(Invoke([this](Display *, XEvent *dest)
                           {
                              *dest = fake_x11.event_vector.front();
                              fake_x11.event_vector.pop_front();
                           });

... or something like that.

review: Approve

[Daniel van Vugt (vanvugt)]

I was confused for a moment as the diff doesn't show the relevant XNextEvent.

But yeah this looks right...

review: Approve

[Alan Griffiths (alan-griffiths)]

Fixes the linked bug.

But the test code is confusing (as you obviously realize) and seems to relate more to other tests than the one proposed.

It seems that we need some stub logic (to link XPending and XNextEvent) rather than a pure Mock for the global X11 test double.

Perhaps something like:

    ON_CALL(*this, XPending(_))
        .WillByDefault(InvokeWithoutArgs([this](){ return fake_x11.pending_events; }));

int XNextEvent(Display* display, XEvent* event_return)
{
    auto const result = global_mock->XNextEvent(display, event_return);
    if (result) --global_mock->fake_x11.pending_events;
    return result;
}

And XMaskEvent etc.

review: Needs Fixing

[Cemil Azizoglu (cemil-azizoglu)]

@Alan, @Andreas, thanks for your suggestions. Should be fixed now.

[Cemil Azizoglu (cemil-azizoglu)]

@Alan, @Andreas, thanks for your suggestions. Should be fixed now.

[Mir CI Bot (mir-ci-bot)]

PASSED: Continuous integration, rev:3387
https://mir-jenkins.ubuntu.com/job/mir-ci/565/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/457
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/487
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/479
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/479
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/466
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/466/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/466
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/466/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/466
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/466/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/466
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/466/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/466
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/466/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/565/rebuild

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

Fix still looks OK

review: Approve

=== modified file 'src/platforms/mesa/server/x11/input/input_platform.cpp'
--- src/platforms/mesa/server/x11/input/input_platform.cpp	2016-03-02 08:43:21 +0000
+++ src/platforms/mesa/server/x11/input/input_platform.cpp	2016-03-14 20:56:18 +0000
@@ -80,7 +80,7 @@
 
 void mix::XInputPlatform::process_input_event()
 {
-    do
+    while(XPending(x11_connection.get()))
     {
         // This code is based on :
         // https://tronche.com/gui/x/xlib/events/keyboard-pointer/keyboard-pointer.html
@@ -248,5 +248,4 @@
         else
             mir::log_error("input event received with no sink to handle it");
     }
-    while(XPending(x11_connection.get()));
 }

=== modified file 'tests/include/mir/test/doubles/mock_x11.h'
--- tests/include/mir/test/doubles/mock_x11.h	2016-03-02 08:43:21 +0000
+++ tests/include/mir/test/doubles/mock_x11.h	2016-03-14 20:56:18 +0000
@@ -47,6 +47,7 @@
     XEvent focus_out_event_return = { 0 };
     XEvent vscroll_event_return = { 0 };
     XEvent motion_event_return = { 0 };
+    int pending_events = 1;
 };
 
 class MockX11

=== modified file 'tests/mir_test_doubles/mock_x11.cpp'
--- tests/mir_test_doubles/mock_x11.cpp	2016-03-02 08:43:21 +0000
+++ tests/mir_test_doubles/mock_x11.cpp	2016-03-14 20:56:18 +0000
@@ -71,6 +71,12 @@
 
     ON_CALL(*this, XInitThreads())
     .WillByDefault(Return(1));
+
+    ON_CALL(*this, XPending(_))
+    .WillByDefault(InvokeWithoutArgs([this]()
+                                     {
+                                         return fake_x11.pending_events;
+                                     }));
 }
 
 mtd::MockX11::~MockX11()
@@ -135,7 +141,9 @@
 
 int XNextEvent(Display* display, XEvent* event_return)
 {
-    return global_mock->XNextEvent(display, event_return);
+    auto const result = global_mock->XNextEvent(display, event_return);
+    if (result) --global_mock->fake_x11.pending_events;
+    return result;
 }
 
 int XLookupString(XKeyEvent* event_struct, char* buffer_return, int bytes_buffer, KeySym* keysym_return, XComposeStatus* status_in_out)

=== modified file 'tests/unit-tests/input/test_x11_platform.cpp'
--- tests/unit-tests/input/test_x11_platform.cpp	2016-03-02 08:43:21 +0000
+++ tests/unit-tests/input/test_x11_platform.cpp	2016-03-14 20:56:18 +0000
@@ -30,7 +30,6 @@
 #include "mir/test/doubles/mock_input_sink.h"
 #include "mir/test/doubles/mock_input_device_registry.h"
 #include "mir/test/doubles/mock_x11.h"
-#include "mir/test/doubles/mock_x11.h"
 #include "mir/test/fake_shared.h"
 #include "mir/cookie/authority.h"
 #include "mir/test/event_matchers.h"
@@ -176,6 +175,8 @@
 
 TEST_F(X11PlatformTest, motion_event_trigger_pointer_movement)
 {
+    mock_x11.fake_x11.pending_events = 2;
+
     prepare_event_processing();
 
     InSequence seq;
@@ -231,3 +232,16 @@
 
     process_input_event();
 }
+
+TEST_F(X11PlatformTest, does_not_block_on_events)
+{
+    prepare_event_processing();
+
+    ON_CALL(mock_x11, XPending(_))
+    .WillByDefault(Return(0));
+
+    EXPECT_CALL(mock_x11, XNextEvent(_,_))
+    .Times(Exactly(0));
+
+    process_input_event();
+}