Merge lp:~vanvugt/mir/resize-events into lp:mir

[Daniel van Vugt]

Add mir_event_type_resize / MirResizeEvent, allowing clients to react
immediately to surface resizing. (LP: #1227744)

You will see FIXME comments in the example code. I spent several days prototyping solutions to bug 1194384 to avoid the FIXMEs, before realizing it's a much bigger can of worms and beyond this scope of this work. Most importantly, the design of resize events is not affected by the presence of a fix for bug 1194384. So any sufficiently powerful toolkit (one with its own event queue and/or locking primitives) will be able to utilize MirResizeEvent immediately, without needing a fix for bug 1194384.

I admit, this was not my first choice of design. I started with "size" being a MirSurfaceAttrib with value:
    int value = (width << 16) | height;
However as nice as that was reusing the existing logic and maintaining "size" as an "attribute", it was a bit kludgy and I feared reviewers would disapprove of the shifting. Not to mention the 4-gigapixel limitation ;)

I also prototyped a solution where size was a multi-value MirSurfaceAttrib. However modifying all the code to support multi-value MirSurfaceAttrib's resulted in a mess that I did not like at all.

And then there were several other possible designs I disliked so much I did not even prototype. So this is a compromise, which I think more people will approve of.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1260
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/448/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/299
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/296
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/177
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/177/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/177
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/177/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

157 +TEST(TestEventSender, sends_all_but_input_events)

I don't think this test does what you think. E.g. you can comment out the following lines and it still passes:

168 + EXPECT_CALL(*msg_sender, send(_))
169 + .Times(0);

It would be better written as a series of individual (or parameterized) tests

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

Alan,

I don't think that's a valid argument. You can comment out asserts/expectations from any tests and they will of course pass.

The point is to verify send is never called at all for those event types. And that it is called for other event types. I just re-tested changing "Times(0)" to "Times(1)" and the test then fails. So the test is correct.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1262
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/456/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/310
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/307
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/185
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/185/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/185
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/185/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

> Alan,
>
> I don't think that's a valid argument. You can comment out
> asserts/expectations from any tests and they will of course pass.
>
> The point is to verify send is never called at all for those event types. And
> that it is called for other event types. I just re-tested changing "Times(0)"
> to "Times(1)" and the test then fails. So the test is correct.

Sorry, I wasn't clear.

The canonical way to write a test is /1/ setup, /2/ expectations, /3/ execution and /4/ validation.

You've mixed expectations and execution - but that doesn't change the way the fact that validation only occurs at the end. Essentially you've written:

TEST(TestEventSender, sends_all_but_input_events)
{
    using namespace testing;

    // Setup
    auto msg_sender = std::make_shared<MockMsgSender>();
    mfd::EventSender event_sender(msg_sender);
    MirEvent event;
    memset(&event, 0, sizeof event);

    // Expectations
    InSequence seq;
    EXPECT_CALL(*msg_sender, send(_))
        .Times(2);

    // Execution
    event.type = mir_event_type_surface;
    event_sender.handle_event(event);
    event.type = mir_event_type_resize;
    event_sender.handle_event(event);
    event.type = mir_event_type_key;
    event_sender.handle_event(event);
    event.type = mir_event_type_motion;
    event_sender.handle_event(event);
}

Which doesn't give any information on which events generate which calls - just that there are two of them.

[Alexandros Frantzis (afrantzis)]

Looks good.

review: Approve

[Alan Griffiths (alan-griffiths)]

ack

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:1263
http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-ci/459/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-trusty-i386-build/315
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-trusty-amd64-build/312
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/188
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-amd64-ci/188/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/188
        deb: http://jenkins.qa.ubuntu.com/job/mir-team-mir-development-branch-trusty-armhf-ci/188/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

=== modified file 'examples/eglapp.c'
--- examples/eglapp.c	2013-11-21 03:16:21 +0000
+++ examples/eglapp.c	2013-11-27 10:05:11 +0000
@@ -93,8 +93,12 @@
         lastcount = count;
     }
 
-    /* This is one way to handle window resizing. But in future it would be
-       better to have resize events coming from the server */
+    /*
+     * Querying the surface (actually the current buffer) dimensions here is
+     * the only truly safe way to be sure that the dimensions we think we
+     * have are those of the buffer being rendered to. But this should be
+     * improved in future; https://bugs.launchpad.net/mir/+bug/1194384
+     */
     if (eglQuerySurface(egldisplay, eglsurface, EGL_WIDTH, &width) &&
         eglQuerySurface(egldisplay, eglsurface, EGL_HEIGHT, &height))
     {
@@ -102,7 +106,7 @@
     }
 }
 
-static void mir_eglapp_handle_input(MirSurface* surface, MirEvent const* ev, void* context)
+static void mir_eglapp_handle_event(MirSurface* surface, MirEvent const* ev, void* context)
 {
     (void) surface;
     (void) context;
@@ -112,6 +116,17 @@
     {
         running = 0;
     }
+    else if (ev->type == mir_event_type_resize)
+    {
+        /*
+         * FIXME: https://bugs.launchpad.net/mir/+bug/1194384
+         * It is unsafe to set the width and height here because we're in a
+         * different thread to that doing the rendering. So we either need
+         * support for event queuing (directing them to another thread) or
+         * full single-threaded callbacks. (LP: #1194384).
+         */
+        printf("Resized to %dx%d\n", ev->resize.width, ev->resize.height);
+    }
 }
 
 static unsigned int get_bpp(MirPixelFormat pf)
@@ -172,7 +187,7 @@
     };
     MirEventDelegate delegate = 
     {
-        mir_eglapp_handle_input,
+        mir_eglapp_handle_event,
         NULL
     };
     EGLConfig eglconfig;
 
=== modified file 'examples/fingerpaint.c'
--- examples/fingerpaint.c	2013-09-19 13:24:22 +0000
+++ examples/fingerpaint.c	2013-11-27 10:05:11 +0000
@@ -247,6 +247,18 @@
             redraw(surface, canvas);
         }
     }
+    else if (event->type == mir_event_type_resize)
+    {
+        /* FIXME: https://bugs.launchpad.net/mir/+bug/1194384
+         * mir_event_type_resize will arrive in a different thread to that of
+         * mir_event_type_motion, so we cannot safely redraw from this thread.
+         * Either the callbacks will need to become thread-safe, or we'd have
+         * to employ some non-trivial event queuing and inter-thread signals,
+         * which I think is beyond the scope of this example code.
+         *
+         *    redraw(surface, canvas);
+         */
+    }
 }
 
 static const MirDisplayOutput *find_active_output(
 
=== modified file 'include/shared/mir_toolkit/event.h'
--- include/shared/mir_toolkit/event.h	2013-11-22 13:53:36 +0000
+++ include/shared/mir_toolkit/event.h	2013-11-27 10:05:11 +0000
@@ -39,7 +39,8 @@
 {
     mir_event_type_key,
     mir_event_type_motion,
-    mir_event_type_surface
+    mir_event_type_surface,
+    mir_event_type_resize
 } MirEventType;
 
 typedef enum {
@@ -194,12 +195,22 @@
     int value;
 } MirSurfaceEvent;
 
+typedef struct
+{
+    MirEventType type;
+
+    int surface_id;
+    int width;
+    int height;
+} MirResizeEvent;
+
 typedef union
 {
     MirEventType    type;
     MirKeyEvent     key;
     MirMotionEvent  motion;
     MirSurfaceEvent surface;
+    MirResizeEvent  resize;
 } MirEvent;
 
 #ifdef __cplusplus
 
=== modified file 'src/server/frontend/event_sender.cpp'
--- src/server/frontend/event_sender.cpp	2013-10-15 08:53:10 +0000
+++ src/server/frontend/event_sender.cpp	2013-11-27 10:05:11 +0000
@@ -37,7 +37,7 @@
 void mfd::EventSender::handle_event(MirEvent const& e)
 {
     // Limit the types of events we wish to send over protobuf, for now.
-    if (e.type == mir_event_type_surface)
+    if (e.type != mir_event_type_key && e.type != mir_event_type_motion)
     {
         // In future we might send multiple events, or insert them into messages
         // containing other responses, but for now we send them individually.
 
=== modified file 'src/server/scene/surface_impl.cpp'
--- src/server/scene/surface_impl.cpp	2013-11-25 11:21:54 +0000
+++ src/server/scene/surface_impl.cpp	2013-11-27 10:05:11 +0000
@@ -247,6 +247,15 @@
 void ms::SurfaceImpl::resize(geom::Size const& size)
 {
     surface->resize(size);
+
+    MirEvent e;
+    memset(&e, 0, sizeof e);
+    e.type = mir_event_type_resize;
+    e.resize.surface_id = id.as_value();
+    e.resize.width = size.width.as_int();
+    e.resize.height = size.height.as_int();
+
+    event_sink->handle_event(e);
 }
 
 void ms::SurfaceImpl::set_rotation(float degrees, glm::vec3 const& axis)
 
=== modified file 'tests/unit-tests/frontend/test_event_sender.cpp'
--- tests/unit-tests/frontend/test_event_sender.cpp	2013-11-22 13:53:36 +0000
+++ tests/unit-tests/frontend/test_event_sender.cpp	2013-11-27 10:05:11 +0000
@@ -67,3 +67,39 @@
 
     sender.handle_display_config_change(config);
 }
+
+TEST(TestEventSender, sends_noninput_events)
+{
+    using namespace testing;
+
+    auto msg_sender = std::make_shared<MockMsgSender>();
+    mfd::EventSender event_sender(msg_sender);
+
+    MirEvent event;
+    memset(&event, 0, sizeof event);
+
+    EXPECT_CALL(*msg_sender, send(_))
+        .Times(2);
+    event.type = mir_event_type_surface;
+    event_sender.handle_event(event);
+    event.type = mir_event_type_resize;
+    event_sender.handle_event(event);
+}
+
+TEST(TestEventSender, never_sends_input_events)
+{
+    using namespace testing;
+
+    auto msg_sender = std::make_shared<MockMsgSender>();
+    mfd::EventSender event_sender(msg_sender);
+
+    MirEvent event;
+    memset(&event, 0, sizeof event);
+
+    EXPECT_CALL(*msg_sender, send(_))
+        .Times(0);
+    event.type = mir_event_type_key;
+    event_sender.handle_event(event);
+    event.type = mir_event_type_motion;
+    event_sender.handle_event(event);
+}
 
=== modified file 'tests/unit-tests/scene/test_surface_impl.cpp'
--- tests/unit-tests/scene/test_surface_impl.cpp	2013-11-25 14:35:33 +0000
+++ tests/unit-tests/scene/test_surface_impl.cpp	2013-11-27 10:05:11 +0000
@@ -40,6 +40,7 @@
 #include "mir_test/fake_shared.h"
 #include "mir_test/event_matchers.h"
 
+#include <cstring>
 #include <stdexcept>
 #include "gmock_set_arg.h"
 #include <gmock/gmock.h>
@@ -70,6 +71,7 @@
 {
 public:
     StubSurfaceBuilder() :
+        mock_surface_state(std::make_shared<mtd::MockSurfaceState>()),
         stub_buffer_stream_(std::make_shared<mtd::StubBufferStream>()),
         dummy_surface()
     {
@@ -77,9 +79,8 @@
 
     std::weak_ptr<ms::BasicSurface> create_surface(msh::Session*, msh::SurfaceCreationParameters const& )
     {
-        auto state = std::make_shared<mtd::MockSurfaceState>();
         dummy_surface = std::make_shared<ms::Surface>(
-            state,
+            mock_surface_state, 
             stub_buffer_stream_,
             std::shared_ptr<mi::InputChannel>(),
             report);
@@ -101,6 +102,8 @@
         return stub_buffer_stream_;
     }
 
+    std::shared_ptr<mtd::MockSurfaceState> mock_surface_state;
+
 private:
     std::shared_ptr<mtd::StubBufferStream> const stub_buffer_stream_;
     std::shared_ptr<ms::BasicSurface> dummy_surface;
@@ -296,6 +299,43 @@
     EXPECT_EQ(mir_surface_state_fullscreen, surf.state());
 }
 
+bool operator==(MirEvent const& a, MirEvent const& b)
+{
+    // We will always fill unused bytes with zero, so memcmp is accurate...
+    return !memcmp(&a, &b, sizeof(MirEvent));
+}
+
+TEST_F(SurfaceImpl, emits_resize_events)
+{
+    using namespace testing;
+
+    auto sink = std::make_shared<MockEventSink>();
+    ms::SurfaceImpl surf(
+        nullptr,
+        mt::fake_shared(surface_builder),
+        std::make_shared<mtd::NullSurfaceConfigurator>(),
+        msh::a_surface(),
+        stub_id,
+        sink);
+
+    geom::Size const new_size{123, 456};
+
+    MirEvent e;
+    memset(&e, 0, sizeof e);
+    e.type = mir_event_type_resize;
+    e.resize.surface_id = stub_id.as_value();
+    e.resize.width = new_size.width.as_int();
+    e.resize.height = new_size.height.as_int();
+
+    EXPECT_CALL(*sink, handle_event(e))
+        .Times(1);
+
+    EXPECT_CALL(*surface_builder.mock_surface_state, resize(Ref(new_size)))
+        .Times(1);
+
+    surf.resize(new_size);
+}
+
 TEST_F(SurfaceImpl, sends_focus_notifications_when_focus_gained_and_lost)
 {
     using namespace testing;