Merge lp:~alan-griffiths/mir/some-surface-modification-infrastructure into lp:mir

[Alan Griffiths]

RPC, frontend: "wiring up" to pass generic surface modification requests from wire to the shell.

RPC, frontend: "wiring up" to pass generic surface modification requests from wire to the shell.

This needs to be followed up with specific client APIs to make the requests and window manager code to process them. The tests will be written to drive that work.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2469
http://jenkins.qa.ubuntu.com/job/mir-ci/3482/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/1999
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/1998
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/1949
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1479/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1949
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1949/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/4886
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/19507

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

review: Needs Fixing (continuous-integration)

[Alan Griffiths (alan-griffiths)]

11: [ FAILED ] SimpleDispatchThreadTest.keeps_dispatching_after_signal_interruption (716 ms)

Oh well, I might as well roll in the nest batch of changes to cause a rebuild.

[lp:~alan-griffiths/mir/some-surface-modification-infrastructure]

2470. By Alan Griffiths on 2015-04-09

Client side changes to put surface modification onto the wire

2471. By Alan Griffiths on 2015-04-09

Update a couple of copyright dates

2472. By Alan Griffiths on 2015-04-09

merge lp:mir

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2472
http://jenkins.qa.ubuntu.com/job/mir-ci/3485/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2003
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/2002
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/1953
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1482
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1482/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1953
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1953/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/4889
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/19512

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

review: Approve (continuous-integration)

[Chris Halse Rogers (raof)]

35 + mir::optional_value<geometry::Width> width;
36 + mir::optional_value<geometry::Height> height;
37 + optional_value<MirPixelFormat> pixel_format;
38 + optional_value<graphics::BufferUsage> buffer_usage;
39 optional_value<std::string> name;
40 - // TODO: type/state/size etc (LP: #1422522) (LP: #1420573)
41 - // Once fully populated for surface modification this can probably
42 - // also replace scene::SurfaceCreationParameters in create_surface()
43 + optional_value<graphics::DisplayConfigurationOutputId> output_id;
44 + mir::optional_value<MirSurfaceType> type;

Why is there a mix of mir::optional_value<> and optional_value<> here?

Other than that, looks sensible.

review: Needs Fixing

[lp:~alan-griffiths/mir/some-surface-modification-infrastructure]

2473. By Alan Griffiths on 2015-04-10

Be consistent

2474. By Alan Griffiths on 2015-04-10

merge lp:mir

[Alan Griffiths (alan-griffiths)]

> Why is there a mix of mir::optional_value<> and optional_value<> here?

Fixed

> Other than that, looks sensible.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2474
http://jenkins.qa.ubuntu.com/job/mir-ci/3494/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2016
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/2015
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/1966
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1491
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1491/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1966
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/1966/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/4900
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/19531

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

review: Approve (continuous-integration)

[Chris Halse Rogers (raof)]

Happy with this now.

review: Approve

=== modified file 'include/common/mir/optional_value.h'
--- include/common/mir/optional_value.h	2014-12-19 04:37:16 +0000
+++ include/common/mir/optional_value.h	2015-04-10 08:43:04 +0000
@@ -25,6 +25,9 @@
 class optional_value
 {
 public:
+    optional_value() = default;
+    optional_value(T const& value) : value_{value}, is_set_{true} {}
+
     optional_value& operator=(T const& value)
     {
         value_ = value;

=== modified file 'include/server/mir/shell/surface_specification.h'
--- include/server/mir/shell/surface_specification.h	2015-04-02 13:34:15 +0000
+++ include/server/mir/shell/surface_specification.h	2015-04-10 08:43:04 +0000
@@ -20,6 +20,12 @@
 #define MIR_SHELL_SURFACE_SPECIFICATION_H_
 
 #include "mir/optional_value.h"
+#include "mir_toolkit/common.h"
+#include "mir/frontend/surface_id.h"
+#include "mir/geometry/point.h"
+#include "mir/graphics/buffer_properties.h"
+#include "mir/graphics/display_configuration.h"
+#include "mir/scene/depth_id.h"
 
 #include <string>
 
@@ -30,10 +36,31 @@
 /// Specification of surface properties requested by client
 struct SurfaceSpecification
 {
+    optional_value<geometry::Width> width;
+    optional_value<geometry::Height> height;
+    optional_value<MirPixelFormat> pixel_format;
+    optional_value<graphics::BufferUsage> buffer_usage;
     optional_value<std::string> name;
-    // TODO: type/state/size etc (LP: #1422522) (LP: #1420573)
-    // Once fully populated for surface modification this can probably
-    // also replace scene::SurfaceCreationParameters in create_surface()
+    optional_value<graphics::DisplayConfigurationOutputId> output_id;
+    optional_value<MirSurfaceType> type;
+    optional_value<MirSurfaceState> state;
+    optional_value<MirOrientationMode> preferred_orientation;
+    optional_value<frontend::SurfaceId> parent_id;
+    optional_value<geometry::Rectangle> aux_rect;
+    optional_value<MirEdgeAttachment> edge_attachment;
+    optional_value<geometry::Width> min_width;
+    optional_value<geometry::Height> min_height;
+    optional_value<geometry::Width> max_width;
+    optional_value<geometry::Height> max_height;
+
+    // TODO scene::SurfaceCreationParameters overlaps this content but has additional fields:
+    //    geometry::Point top_left;
+    //    scene::DepthId depth;
+    //    input::InputReceptionMode input_mode;
+    //    std::weak_ptr<Surface> parent;
+    //
+    //    it also has size instead of width + height
+    // Maybe SurfaceCreationParameters /HasA/ SurfaceSpecification?
 };
 }
 }

=== modified file 'src/client/mir_surface.cpp'
--- src/client/mir_surface.cpp	2015-04-09 06:20:31 +0000
+++ src/client/mir_surface.cpp	2015-04-10 08:43:04 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright © 2012 Canonical Ltd.
+ * Copyright © 2012, 2015 Canonical Ltd.
  *
  * This program is free software: you can redistribute it and/or modify it
  * under the terms of the GNU Lesser General Public License version 3,
@@ -54,7 +54,8 @@
     : connection{connection},
       width{width},
       height{height},
-      pixel_format{format}
+      pixel_format{format},
+      buffer_usage{mir_buffer_usage_hardware}
 {
 }
 
@@ -81,18 +82,20 @@
 {
     mir::protobuf::SurfaceParameters message;
 
-    message.set_width(width);
-    message.set_height(height);
-    message.set_pixel_format(pixel_format);
-    message.set_buffer_usage(buffer_usage);
-
+    SERIALIZE_OPTION_IF_SET(width, message);
+    SERIALIZE_OPTION_IF_SET(height, message);
+    SERIALIZE_OPTION_IF_SET(pixel_format, message);
+    SERIALIZE_OPTION_IF_SET(buffer_usage, message);
     SERIALIZE_OPTION_IF_SET(surface_name, message);
     SERIALIZE_OPTION_IF_SET(output_id, message);
     SERIALIZE_OPTION_IF_SET(type, message);
     SERIALIZE_OPTION_IF_SET(state, message);
     SERIALIZE_OPTION_IF_SET(pref_orientation, message);
+    SERIALIZE_OPTION_IF_SET(edge_attachment, message);
+
     if (parent.is_set() && parent.value() != nullptr)
         message.set_parent_id(parent.value()->id());
+
     if (aux_rect.is_set())
     {
         message.mutable_aux_rect()->set_left(aux_rect.value().left);
@@ -100,7 +103,7 @@
         message.mutable_aux_rect()->set_width(aux_rect.value().width);
         message.mutable_aux_rect()->set_height(aux_rect.value().height);
     }
-    SERIALIZE_OPTION_IF_SET(edge_attachment, message);
+
     return message;
 }
 
@@ -534,8 +537,48 @@
         mods.mutable_surface_id()->set_value(surface.id().value());
     }
 
+    auto const surface_specification = mods.mutable_surface_specification();
+
+    #define COPY_IF_SET(field)\
+        if (spec.field.is_set())\
+        surface_specification->set_##field(spec.field.value())
+
+    COPY_IF_SET(width);
+    COPY_IF_SET(height);
+    COPY_IF_SET(pixel_format);
+    COPY_IF_SET(buffer_usage);
+    // name is a special case (below)
+    COPY_IF_SET(output_id);
+    COPY_IF_SET(type);
+    COPY_IF_SET(state);
+    // preferred_orientation is a special case (below)
+    // parent_id is a special case (below)
+    // aux_rect is a special case (below)
+    COPY_IF_SET(edge_attachment);
+    COPY_IF_SET(min_width);
+    COPY_IF_SET(min_height);
+    COPY_IF_SET(max_width);
+    COPY_IF_SET(max_height);
+    #undef COPY_IF_SET
+
     if (spec.surface_name.is_set())
-        mods.set_name(spec.surface_name.value());
+        surface_specification->set_name(spec.surface_name.value());
+
+    if (spec.pref_orientation.is_set())
+        surface_specification->set_preferred_orientation(spec.pref_orientation.value());
+
+    if (spec.parent.is_set() && spec.parent.value())
+        surface_specification->set_parent_id(spec.parent.value()->id());
+
+    if (spec.aux_rect.is_set())
+    {
+        auto const rect = surface_specification->mutable_aux_rect();
+        auto const& value = spec.aux_rect.value();
+        rect->set_left(value.left);
+        rect->set_top(value.top);
+        rect->set_width(value.width);
+        rect->set_height(value.height);
+    }
 
     modify_wait_handle.expect_result();
     server->modify_surface(0, &mods, &modify_result,

=== modified file 'src/client/mir_surface.h'
--- src/client/mir_surface.h	2015-04-09 06:20:31 +0000
+++ src/client/mir_surface.h	2015-04-10 08:43:04 +0000
@@ -1,5 +1,5 @@
 /*
- * Copyright © 2012 Canonical Ltd.
+ * Copyright © 2012, 2015 Canonical Ltd.
  *
  * This program is free software: you can redistribute it and/or modify it
  * under the terms of the GNU Lesser General Public License version 3,
@@ -70,12 +70,13 @@
 
     // Required parameters
     MirConnection* connection{nullptr};
-    int width{-1};
-    int height{-1};
-    MirPixelFormat pixel_format{mir_pixel_format_invalid};
-    MirBufferUsage buffer_usage{mir_buffer_usage_hardware};
 
     // Optional parameters
+    mir::optional_value<int> width;
+    mir::optional_value<int> height;
+    mir::optional_value<MirPixelFormat> pixel_format;
+    mir::optional_value<MirBufferUsage> buffer_usage;
+
     mir::optional_value<std::string> surface_name;
     mir::optional_value<uint32_t> output_id;
 
@@ -87,6 +88,11 @@
     mir::optional_value<MirSurface*> parent;
     mir::optional_value<MirRectangle> aux_rect;
     mir::optional_value<MirEdgeAttachment> edge_attachment;
+
+    mir::optional_value<int> min_width;
+    mir::optional_value<int> min_height;
+    mir::optional_value<int> max_width;
+    mir::optional_value<int> max_height;
 };
 
 struct MirSurface

=== modified file 'src/protobuf/mir_protobuf.proto'
--- src/protobuf/mir_protobuf.proto	2015-04-09 14:09:19 +0000
+++ src/protobuf/mir_protobuf.proto	2015-04-10 08:43:04 +0000
@@ -30,19 +30,30 @@
 
 // If and when we break our protocol backward-compatibility, this could be
 // merged with SurfaceParameters...
+message SurfaceSpecification {
+  optional int32 width = 1;
+  optional int32 height = 2;
+  optional int32 pixel_format = 3;
+  optional int32 buffer_usage = 4;
+  optional string name = 5;
+  optional uint32 output_id = 6;
+  optional int32 type = 7;
+  optional int32 state = 8;
+  optional int32 preferred_orientation = 9;
+  optional int32 parent_id = 10;
+  optional Rectangle aux_rect = 11;
+  optional int32 edge_attachment = 12;
+
+  optional int32 min_width = 13;
+  optional int32 min_height = 14;
+  optional int32 max_width = 15;
+  optional int32 max_height = 16;
+}
+
+
 message SurfaceModifications {
   required SurfaceId surface_id = 1;
-
-  optional string name = 2;
-
-  // TODO: Client API for resizing (LP: #1420573)
-  // optional int32 width = 3;
-  // optional int32 height = 4;
-
-  // TODO: We could use these to replace SurfaceSetting
-  // optional int32 type = 5;
-  // optional int32 state = 6;
-  // ...
+  required SurfaceSpecification surface_specification = 2;
 }
 
 message SurfaceId {

=== modified file 'src/server/frontend/session_mediator.cpp'
--- src/server/frontend/session_mediator.cpp	2015-04-09 06:20:31 +0000
+++ src/server/frontend/session_mediator.cpp	2015-04-10 08:43:04 +0000
@@ -420,17 +420,45 @@
     mir::protobuf::Void* /*response*/,
     google::protobuf::Closure* done)
 {
+    auto const& surface_specification = request->surface_specification();
+
     {
         std::unique_lock<std::mutex> lock(session_mutex);
 
-        auto session = weak_session.lock();
+        auto const session = weak_session.lock();
         if (!session)
             BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));
 
         msh::SurfaceSpecification mods;
-        if (request->has_name())
-            mods.name = request->name();
-        // TODO: More fields soon (LP: #1422522) (LP: #1420573)
+
+        #define COPY_IF_SET(name)\
+            if (surface_specification.has_##name())\
+                mods.name = decltype(mods.name.value())(surface_specification.name())
+
+        COPY_IF_SET(width);
+        COPY_IF_SET(height);
+        COPY_IF_SET(pixel_format);
+        COPY_IF_SET(buffer_usage);
+        COPY_IF_SET(name);
+        COPY_IF_SET(output_id);
+        COPY_IF_SET(type);
+        COPY_IF_SET(state);
+        COPY_IF_SET(preferred_orientation);
+        COPY_IF_SET(parent_id);
+        // aux_rect is a special case (below)
+        COPY_IF_SET(edge_attachment);
+        COPY_IF_SET(min_width);
+        COPY_IF_SET(min_height);
+        COPY_IF_SET(max_width);
+        COPY_IF_SET(max_height);
+
+        #undef COPY_IF_SET
+
+        if (surface_specification.has_aux_rect())
+        {
+            auto const& rect = surface_specification.aux_rect();
+            mods.aux_rect = {{rect.left(), rect.top()}, {rect.width(), rect.height()}};
+        }
 
         auto const id = mf::SurfaceId(request->surface_id().value());