Merge lp:~afrantzis/mir/always-consume-input-events-in-clients into lp:mir

[Alexandros Frantzis]

client: Process input events even in clients that don't handle input

client: Process input events even in clients that don't handle input

This is one alternative solution for bug 1213804. See comments in bug 1213804 for more information and pointers to other options.

Currently, focused client surfaces that don't handle input events don't process them at all. Such clients never acknowledge receiving input events, causing the input system to retry sending the events.

If in the interval between two send attempts another app gains the focus, the input system sends the event to the newly focused app/surface, leading to bug 1213804 (i.e. the target of events is always the currently focused surfaced, not a particular surface).

Ideally, the input system should be able to handle this situation better, e.g., by resending the event only if the focused surface the event was originally targeted at hasn't changed. However, the input subsystem is full of intricacies and I am not familiar enough with it to tell if this behavior is a bug or has a particular reason for being there.

If we decide that the proposed solution/workaround is not acceptable, I can dive more deeply into the abyss known as the input subsystem.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1445
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/980/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/1115
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/1113
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-trusty-touch/699
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/712
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/712/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/717
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/717/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/700
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-trusty-armhf/700/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/666
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/4463

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

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

It definitely sounds like a workaround more than a fix. It also leads to clients being woken up by events they should never receive at all. But we possibly already had that problem...

[Andreas Pokorny (andreas-pokorny)]

> It definitely sounds like a workaround more than a fix. It also leads to
> clients being woken up by events they should never receive at all. But we
> possibly already had that problem...

I guess toolkits will always have event handles set?

[Alan Griffiths (alan-griffiths)]

It sounds to me as though the real problem is in the server - that if a client fails to consume an event then "bad stuff happens". I'd rather fix the "bad stuff happens" - but if that is a lot of work then this attempt to mitigate the problem is OK (at least for now).

review: Needs Information

[Andreas Pokorny (andreas-pokorny)]

InputDispatcher.cpp has its own app switching logic which does a lot of interesting things, as there is something like a dedicated app switch key... Maybe we have to drop all key events when focused surface changes?

[Alexandros Frantzis (afrantzis)]

> I'd rather fix the "bad stuff happens" - but if that is a lot of work then this attempt to mitigate the problem is OK (at least for now).

> Maybe we have to drop all key events when focused surface changes?

I am looking more into the input subsystem to see if there is a clean and safe way we can handle the issue there.

Unmerged revisions

1445. By Alexandros Frantzis on 2014-03-04

client: Process input events even in clients that don't handle input

=== modified file 'src/client/mir_surface.cpp'
--- src/client/mir_surface.cpp	2014-02-10 09:07:48 +0000
+++ src/client/mir_surface.cpp	2014-03-04 17:39:59 +0000
@@ -213,6 +213,8 @@
         accelerated_window = platform->create_egl_native_window(this);
     }
 
+    set_event_handler(nullptr);
+
     connection->on_surface_created(id(), this);
     callback(this, context);
     create_wait_handle.result_received();
@@ -366,13 +368,18 @@
         handle_event_callback = std::bind(delegate->callback, this,
                                           std::placeholders::_1,
                                           delegate->context);
+    }
+    else
+    {
+        handle_event_callback = [](MirEvent const*) {};
+    }
 
-        if (surface.fd_size() > 0 && handle_event_callback)
-        {
-            input_thread = input_platform->create_input_thread(surface.fd(0),
-                                                        handle_event_callback);
-            input_thread->start();
-        }
+    if (surface.fd_size() > 0)
+    {
+        input_thread =
+            input_platform->create_input_thread(surface.fd(0),
+                                                handle_event_callback);
+        input_thread->start();
     }
 }
 

=== modified file 'tests/unit-tests/client/test_client_mir_surface.cpp'
--- tests/unit-tests/client/test_client_mir_surface.cpp	2014-02-11 03:04:50 +0000
+++ tests/unit-tests/client/test_client_mir_surface.cpp	2014-03-04 17:39:59 +0000
@@ -248,9 +248,16 @@
 
 struct StubClientInputPlatform : public mircv::InputPlatform
 {
-    std::shared_ptr<mircv::InputReceiverThread> create_input_thread(int /* fd */, std::function<void(MirEvent*)> const& /* callback */)
+    std::shared_ptr<mircv::InputReceiverThread> create_input_thread(
+        int /* fd */, std::function<void(MirEvent*)> const& /* callback */)
     {
-        return std::shared_ptr<mircv::InputReceiverThread>();
+        struct NullInputReceiverThread : public mircv::InputReceiverThread
+        {
+            void start() override {}
+            void stop() override {}
+            void join() override {}
+        };
+        return std::make_shared<NullInputReceiverThread>();
     }
 };
 
@@ -493,38 +500,106 @@
 static void null_event_callback(MirSurface*, MirEvent const*, void*)
 {
 }
+
+class StubBuffer : public mcl::ClientBuffer
+{
+public:
+    StubBuffer(geom::Size size, geom::Stride stride, MirPixelFormat pf)
+        : size_{size}, stride_{stride}, pf_{pf}
+    {
+    }
+
+    std::shared_ptr<mcl::MemoryRegion> secure_for_cpu_write() override
+    {
+        auto raw = new mcl::MemoryRegion{size_.width,
+                                         size_.height,
+                                         stride_,
+                                         pf_,
+                                         nullptr};
+
+        return std::shared_ptr<mcl::MemoryRegion>(raw);
+    }
+
+    geom::Size size() const override { return size_; }
+    geom::Stride stride() const override { return stride_; }
+    MirPixelFormat pixel_format() const override { return pf_; }
+    uint32_t age() const override { return 0; }
+    void increment_age() override {}
+    void mark_as_submitted() override {}
+
+    std::shared_ptr<mg::NativeBuffer> native_buffer_handle() const override
+    {
+        return std::shared_ptr<mg::NativeBuffer>();
+    }
+
+private:
+    geom::Size const size_;
+    geom::Stride const stride_;
+    MirPixelFormat const pf_;
+};
+
+struct StubClientBufferFactory : public mcl::ClientBufferFactory
+{
+    std::shared_ptr<mcl::ClientBuffer> create_buffer(
+        std::shared_ptr<MirBufferPackage> const& package,
+        geom::Size size, MirPixelFormat pf)
+    {
+        return std::make_shared<StubBuffer>(size,
+                                            geom::Stride{package->stride},
+                                            pf);
+    }
+};
 }
 
-TEST_F(MirClientSurfaceTest, input_fd_used_to_create_input_thread_when_delegate_specified)
+TEST_F(MirClientSurfaceTest, creates_input_thread_at_construction)
 {
     using namespace ::testing;
 
-    auto mock_input_platform = std::make_shared<mt::MockClientInputPlatform>();
-    auto mock_input_thread = std::make_shared<NiceMock<mt::MockInputReceiverThread>>();
-    MirEventDelegate delegate = {null_event_callback, nullptr};
-
-    EXPECT_CALL(*mock_buffer_factory, create_buffer(_,_,_)).Times(2);
-
-    EXPECT_CALL(*mock_input_platform, create_input_thread(_, _)).Times(1)
+    auto const mock_input_platform = std::make_shared<mt::MockClientInputPlatform>();
+    auto const mock_input_thread = std::make_shared<NiceMock<mt::MockInputReceiverThread>>();
+
+    EXPECT_CALL(*mock_input_platform, create_input_thread(_, _))
         .WillOnce(Return(mock_input_thread));
     EXPECT_CALL(*mock_input_thread, start()).Times(1);
     EXPECT_CALL(*mock_input_thread, stop()).Times(1);
 
-    {
-        auto surface = std::make_shared<MirSurface> (connection.get(), *client_comm_channel,
-            mock_buffer_factory, mock_input_platform, params, &empty_callback, nullptr);
-        auto wait_handle = surface->get_create_wait_handle();
-        wait_handle->wait_for_all();
-        surface->set_event_handler(&delegate);
-    }
-
-    {
-        // This surface should not trigger a call to the input platform as no input delegate is specified.
-        auto surface = std::make_shared<MirSurface> (connection.get(), *client_comm_channel,
-            mock_buffer_factory, mock_input_platform, params, &empty_callback, nullptr);
-        auto wait_handle = surface->get_create_wait_handle();
-        wait_handle->wait_for_all();
-    }
+    MirSurface mir_surface(connection.get(), *client_comm_channel,
+                           std::make_shared<StubClientBufferFactory>(),
+                           mock_input_platform, params, &empty_callback, nullptr);
+
+    mir_surface.get_create_wait_handle()->wait_for_all();
+}
+
+TEST_F(MirClientSurfaceTest, recreates_input_thread_when_setting_event_handler)
+{
+    using namespace ::testing;
+
+    auto const mock_input_platform = std::make_shared<mt::MockClientInputPlatform>();
+    auto const mock_input_thread1 = std::make_shared<NiceMock<mt::MockInputReceiverThread>>();
+
+    EXPECT_CALL(*mock_input_platform, create_input_thread(_, _))
+        .WillOnce(Return(mock_input_thread1));
+    EXPECT_CALL(*mock_input_thread1, start()).Times(1);
+
+    MirSurface mir_surface(connection.get(), *client_comm_channel,
+                           std::make_shared<StubClientBufferFactory>(),
+                           mock_input_platform, params, &empty_callback, nullptr);
+
+    mir_surface.get_create_wait_handle()->wait_for_all();
+
+    Mock::VerifyAndClearExpectations(mock_input_platform.get());
+    Mock::VerifyAndClearExpectations(mock_input_thread1.get());
+
+    auto const mock_input_thread2 = std::make_shared<NiceMock<mt::MockInputReceiverThread>>();
+
+    EXPECT_CALL(*mock_input_thread1, stop()).Times(1);
+    EXPECT_CALL(*mock_input_platform, create_input_thread(_, _))
+        .WillOnce(Return(mock_input_thread2));
+    EXPECT_CALL(*mock_input_thread2, start()).Times(1);
+    EXPECT_CALL(*mock_input_thread2, stop()).Times(1);
+
+    MirEventDelegate const delegate = {null_event_callback, nullptr};
+    mir_surface.set_event_handler(&delegate);
 }
 
 TEST_F(MirClientSurfaceTest, get_buffer_returns_last_received_buffer_package)
@@ -633,57 +708,6 @@
               surface->attrib(mir_surface_attrib_state));
 }
 
-namespace
-{
-
-struct StubBuffer : public mcl::ClientBuffer
-{
-    StubBuffer(geom::Size size, geom::Stride stride, MirPixelFormat pf)
-        : size_{size}, stride_{stride}, pf_{pf}
-    {
-    }
-
-    std::shared_ptr<mcl::MemoryRegion> secure_for_cpu_write()
-    {
-        auto raw = new mcl::MemoryRegion{size_.width,
-                                         size_.height,
-                                         stride_,
-                                         pf_,
-                                         nullptr};
-
-        return std::shared_ptr<mcl::MemoryRegion>(raw);
-    }
-
-    geom::Size size() const { return size_; }
-    geom::Stride stride() const { return stride_; }
-    MirPixelFormat pixel_format() const { return pf_; }
-    uint32_t age() const { return 0; }
-    void increment_age() {}
-    void mark_as_submitted() {}
-
-    std::shared_ptr<mg::NativeBuffer> native_buffer_handle() const
-    {
-        return std::shared_ptr<mg::NativeBuffer>();
-    }
-
-    geom::Size size_;
-    geom::Stride stride_;
-    MirPixelFormat pf_;
-};
-
-struct StubClientBufferFactory : public mcl::ClientBufferFactory
-{
-    std::shared_ptr<mcl::ClientBuffer> create_buffer(
-        std::shared_ptr<MirBufferPackage> const& package,
-        geom::Size size, MirPixelFormat pf)
-    {
-        return std::make_shared<StubBuffer>(size,
-                                            geom::Stride{package->stride},
-                                            pf);
-    }
-};
-
-}
 TEST_F(MirClientSurfaceTest, get_cpu_region_returns_correct_data)
 {
     using namespace testing;