Merge lp:~vanvugt/mir/surface-types into lp:~mir-team/mir/trunk

[Daniel van Vugt]

Add support for surface types (client API, protocol and server).

This work will also form the foundation of supporting surface states.

Resubmitted. This new version is much smaller, dropping about 500 lines and all mentions of "shell" which we could not agree on before.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:554
http://jenkins.qa.ubuntu.com/job/mir-ci/69/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/70//console

Click here to trigger a rebuild:
http://jenkins.qa.ubuntu.com/job/mir-ci/69//rebuild/?

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

I don't think that surfaces::Surface needs configure(MirSurfaceAttrib attrib, int value) - AFICS attributes are a feature of shell::Surface. (And, as discussed elsewhere, surfaces::ProxySurface belongs in shell, possibly as shell::Window.)

~~~~

90 +

Trailing whitespace.

~~~~

216 === added file 'include/shared/mir/common.h'

This is not the place for parts of the toolkit API - this is for namespace mir

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

Is there a clear distinction between SurfaceSetting and SurfaceParameters? (And is it worth supplying SurfaceSetting to create_surface and avoiding a round-trip?)

[Kevin DuBois (kdub)]

the new surface type get/set for client api looks ok to me

The acceptance test takes quite a while on the galaxy nexus, perhaps it can be shortened by using less iterations in line 757?
[ RUN ] DefaultDisplayServerTestFixture.surface_types
[ OK ] DefaultDisplayServerTestFixture.surface_types (1384 ms)
(i see about 180ms on my desktop)

small nit but with
615 +int ms::Surface::configure(MirSurfaceAttrib attrib, int value)
the name 'configure' didn't make it obvious what is going on in the function.

Also with this function, (related to the comment in it) is the surface type something that mir cares about, or the shell? This is tricky to talk about without a 'shell api', but its still a bit hazy to me what our requirements for tracking the data that the shell cares about.

although the tests look good, they do have multiple testing expectations in them because they are long.

[Daniel van Vugt (vanvugt)]

Alan,

Extraneous "configure" methods:
I only added methods where I absolutely needed to, to make the code compile and run. However that was many revisions ago so I'll check to see if it's still required...

Trailing whitespace at line 90:
Will fix now.

216 === added file 'include/shared/mir/common.h'
"common.h" is not part of the "toolkit". It is used by the client toolkit and in the server.

SurfaceSetting vs SurfaceParameters
Yes they will always be different. SurfaceParameters are required information to create a surface. SurfaceSetting is something you might optionally change later; like type, state, hidden, width, height (most of which are not implemented yet but require this branch to land first).

Kevin,

Stress test takes too long:
I think 1.3 seconds is OK for a stress test, but I can shorten it if you really want.

"configure" is not self explanatory:
I kind of agree but have not yet thought of a better method name other than "set_attribute" (making the protocol call something like "set_surface_attribute"). I prefer "configure_surface". More suggestions?

Who cares about surface type?...
You're absolutely right that surface type is something only the shell will care about and act upon. However for consistency between different shells and avoiding duplication, it is reasonable for the server to store the type value in each server surface instance. Even though it will only be used by the shell. This will change as new attributes are added like "state" which will potentially be used by the server directly.

Test length:
Yes, I will always make code as concise as possible without hurting readability.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:557
http://jenkins.qa.ubuntu.com/job/mir-ci/78/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/79/console
        deb: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/79/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://jenkins.qa.ubuntu.com/job/mir-ci/78/rebuild

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

Minor fixes done.

Alan -
I can put "common.h" anywhere of course. Just remember:
  1. It's not part of the "toolkit". "toolkit" means client-only but "common.h" is also used in the server and shell.
  2. common.h has to be installed under "include/mir/..." to make it clear to external users that it's part of mir. And it is implicitly in the "mir" namespace by virtue of the "Mir" type prefixes. We can't use "namespace mir" however because it is C code, not C++.

Alan & Kevin -
I agree that "type" is not a deep server-level attribute. It only makes sense at the shell level. I could move it out of surfaces::Surface now, and into a proxy class. However I think that should be done later because the "Proxy" classes clearly need some refactoring, everyone seems to agree. I'd rather not make assumptions about what form that will take and move any members into *ProxySurface until that's done.

Also keep in mind that _most_ future configurable attributes (state, hidden, width, height etc) will be low-level and live in the server surfaces::Surface rather than the shell.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:558
http://jenkins.qa.ubuntu.com/job/mir-ci/79/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/80/console
        deb: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/80/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://jenkins.qa.ubuntu.com/job/mir-ci/79/rebuild

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

> Minor fixes done.
>
> Alan -
> I can put "common.h" anywhere of course. Just remember:
> 1. It's not part of the "toolkit". "toolkit" means client-only but
> "common.h" is also used in the server and shell.
> 2. common.h has to be installed under "include/mir/..." to make it clear to
> external users that it's part of mir. And it is implicitly in the "mir"
> namespace by virtue of the "Mir" type prefixes. We can't use "namespace mir"
> however because it is C code, not C++.

As it is included by toolkit I think it is part of toolkit. If it were really "part of mir", then there would be no reason for it to be C code. (Is it part of some hypothetical "common" that we've just identified - if so, is "common.h" isn't a good name.)

> Alan & Kevin -
> I agree that "type" is not a deep server-level attribute. It only makes sense
> at the shell level. I could move it out of surfaces::Surface now, and into a
> proxy class. However I think that should be done later because the "Proxy"
> classes clearly need some refactoring, everyone seems to agree. I'd rather not
> make assumptions about what form that will take and move any members into
> *ProxySurface until that's done.

Perhaps it is best to wait on that refactoring and reassess after that?

> Also keep in mind that _most_ future configurable attributes (state, hidden,
> width, height etc) will be low-level and live in the server surfaces::Surface
> rather than the shell.

Are you envisaging using the same mechanism?

[Daniel van Vugt (vanvugt)]

Absolutely, all attributes that can be changed after construction will use configure() eventually.

Please consider landing this branch before any ProxySurface refactoring. I am tired of fixing conflicts and refactoring it every day.

[Daniel van Vugt (vanvugt)]

Also, these arguments are blocking me from starting on two other deliverables which will depend on this work.

[Alan Griffiths (alan-griffiths)]

> Absolutely, all attributes that can be changed after construction will use
> configure() eventually.

Does that imply that e.g. you see resizing happening by setting the width and height in two configure calls? I'm not sure that is a good plan.

Why wouldn't there be a single resize(graphics::Size) call?

> Please consider landing this branch before any ProxySurface refactoring. I am
> tired of fixing conflicts and refactoring it every day.

I'm equally tired of reviewing it every day - there has to be a better way to communicate.

[Daniel van Vugt (vanvugt)]

Good point. Width/height are bad examples. But I will reuse configure() for surface states soon.

[Daniel van Vugt (vanvugt)]

I tried moving common.h to: include/shared/mir_toolkit/common.h
But that fails for reasons alluded to earlier. That is such common headers need to have the same relative path in the source tree as in the install tree. Because mir_client_library.h has to be able to find common.h in both situations.

Any suggestions?

[Alan Griffiths (alan-griffiths)]

> I tried moving common.h to: include/shared/mir_toolkit/common.h
> But that fails for reasons alluded to earlier. That is such common headers
> need to have the same relative path in the source tree as in the install tree.
> Because mir_client_library.h has to be able to find common.h in both
> situations.
>
> Any suggestions?

I guess this comes as part of the various problems with directory names.

IMO include/shared/mir_toolkit and include/client/mir_toolkit should be installed to <target>/mir_toolkit - and that would facilitate #include <mir_toolkit/common.h>

[Kevin DuBois (kdub)]

>
> I guess this comes as part of the various problems with directory names.
>
> IMO include/shared/mir_toolkit and include/client/mir_toolkit should be
> installed to <target>/mir_toolkit - and that would facilitate #include
> <mir_toolkit/common.h>

+1

[Alexandros Frantzis (afrantzis)]

== include/shared/mir vs include/shared/mir_toolkit ==

The concept is that whatever is under */mir_toolkit (and only that) will be part of the toolkit interface. Since a toolkit/client needs this, mir_toolkit is the way to go. Keep in mind that this is still under shared/.

So, this is how I see things currently:

1. server/*, client/*, shared/* => which mir components need to access the definitions in the file
2. */mir, */mir_toolkit => whether the definitions are internal to mir, or (also) exposed to the toolkit API

== surfaces::Surface::set_type(),type() ==

I agree that this doesn't belong there. I don't have a problem putting it there *temporarily* to unblock work, if the related surface refactorings aren't expected to be done soon.

== configure() ==

At the Surface level, and for the limited number of attributes I am aware of, I would prefer to have explicit calls : app_session->set_surface_state(id, ...) or app_session->surface(id)->set_state(...) (since we are already exposing the surface) etc

At the RPC level: one approach is to use configure(attrib, value), another is to have <attrib>(value) calls. A third approach (not neccessarily preferable for this use case, just mentioning it for completeness) is to have, e.g.:

message SurfaceSettings {
    optional SurfaceId surfaceid = 1;
    optional int32 type = 2;
    optional int32 state = 3;
    optional bool visible = 4;
    ...
}

The frontend could then act depending on which values are actually included in the message. This method allows the client to set and retrieve all of the settings in one go.

For the number of attributes and the use cases I am aware of, my preference would be once again to be explicit, and have separate calls <attrib>().

Marking as "Needs information" => "Needs discussion"

review: Needs Information

[Daniel van Vugt (vanvugt)]

I thought it was implicit that having two different top-level include directories:
  <mir/foo>
  <mir_toolkit/bar>
was obviously a bad idea. I thought that was obvious but clearly it's just my opinion now. I think eventually everything should be under "mir/" for consistency:
  <mir/foo>
  <mir/toolkit/bar>
But that's a cleanup for a another day and a different proposal.

Alexandros:
configure() needs to be generic because there are so many layers of classes in the Mir code, it does not scale to add a new function for every attribute at every layer. When I added configure() I was surprised at how many classes I had to modify. It becomes unmaintainable and excessively over-complicated if you have to do that again and again for each new attribute. Consider also reporting -- we don't want to change every type of report to add new methods to those every time a new attribute is added.
Also, we should not assume to have designed all the required attributes any time soon. New attributes will be added and some even removed over time. We don't want to go changing the protocol and half a dozen foundational classes each time.

[Daniel van Vugt (vanvugt)]

I've now been working on this branch for a month and it's been under review for two weeks. It has worked perfectly without bugs for the entire duration. Remember we're only arguing about style issues that can be fixed later.

=== modified file 'debian/control'
--- debian/control	2013-03-12 03:50:42 +0000
+++ debian/control	2013-03-15 03:39:20 +0000
@@ -58,11 +58,22 @@
  .
  Contains the shared library needed by server applications for Mir.
 
+Package: mircommon-dev
+Section: libdevel
+Architecture: any
+Depends: ${misc:Depends},
+Description: Display server for Ubuntu - development headers
+ Mir is a display server running on linux systems, with a focus on efficiency, 
+ robust operation and a well-defined driver model.
+ .
+ Contains header files required for server and/or client development.
+
 Package: libmirserver-dev
 Section: libdevel
 Architecture: any
 Depends: libmirserver0 (= ${binary:Version}),
          libmirprotobuf-dev (= ${source:Version}),
+         mircommon-dev (= ${source:Version}),
          ${misc:Depends},
 Description: Display server for Ubuntu - development headers
  Mir is a display server running on linux systems, with a focus on efficiency,
@@ -86,6 +97,7 @@
 Architecture: any
 Depends: libmirclient0 (= ${binary:Version}),
          libmirprotobuf-dev (= ${source:Version}),
+         mircommon-dev (= ${source:Version}),
          ${misc:Depends},
 Description: Display server for Ubuntu - development headers
  Mir is a display server running on linux systems, with a focus on efficiency,

=== added file 'debian/mircommon-dev.install'
--- debian/mircommon-dev.install	1970-01-01 00:00:00 +0000
+++ debian/mircommon-dev.install	2013-03-15 03:39:20 +0000
@@ -0,0 +1,1 @@
+usr/include/mir_toolkit/*.h

=== modified file 'include/client/mir_toolkit/mir_client_library.h'
--- include/client/mir_toolkit/mir_client_library.h	2013-03-07 08:04:05 +0000
+++ include/client/mir_toolkit/mir_client_library.h	2013-03-15 03:39:20 +0000
@@ -18,6 +18,8 @@
 #ifndef MIR_CLIENT_LIBRARY_H
 #define MIR_CLIENT_LIBRARY_H
 
+#include <mir_toolkit/common.h>
+
 #ifdef __cplusplus
 /** The C client API
  */
@@ -322,6 +324,23 @@
  */
 int mir_debug_surface_id(MirSurface *surface);
 
+/**
+ * Set the type (purpose) of a surface. This is not guaranteed to always work
+ * with some shell types (e.g. phone/tablet UIs). As such, you may have to
+ * wait on the function and check the result using mir_surface_get_type.
+ *   \param [in] surface  The surface to operate on
+ *   \param [in] type     The new type of the surface
+ *   \return              A wait handle that can be passed to mir_wait_for
+ */
+MirWaitHandle* mir_surface_set_type(MirSurface *surface, MirSurfaceType type);
+
+/**
+ * Get the type (purpose) of a surface.
+ *   \param [in] surface  The surface to query
+ *   \return              The type of the surface
+ */
+MirSurfaceType mir_surface_get_type(MirSurface *surface);
+
 #ifdef __cplusplus
 }
 }

=== modified file 'include/server/mir/frontend/application_mediator.h'
--- include/server/mir/frontend/application_mediator.h	2013-03-07 08:04:05 +0000
+++ include/server/mir/frontend/application_mediator.h	2013-03-15 03:39:20 +0000
@@ -103,6 +103,11 @@
                         mir::protobuf::DRMAuthMagicStatus* response,
                         google::protobuf::Closure* done);
 
+    void configure_surface(google::protobuf::RpcController* controller,
+                           const mir::protobuf::SurfaceSetting*,
+                           mir::protobuf::SurfaceSetting*,
+                           google::protobuf::Closure* done);
+
 private:
     std::shared_ptr<shell::SessionStore> const session_store;
     std::shared_ptr<graphics::Platform> const graphics_platform;

=== modified file 'include/server/mir/frontend/application_mediator_report.h'
--- include/server/mir/frontend/application_mediator_report.h	2013-03-07 08:04:05 +0000
+++ include/server/mir/frontend/application_mediator_report.h	2013-03-15 03:39:20 +0000
@@ -41,6 +41,7 @@
     virtual void application_disconnect_called(std::string const& app_name) = 0;
 
     virtual void application_drm_auth_magic_called(std::string const& app_name) = 0;
+    virtual void application_configure_surface_called(std::string const& app_name) = 0;
 
     virtual void application_error(
         std::string const& app_name,
@@ -63,6 +64,8 @@
 
     virtual void application_drm_auth_magic_called(std::string const& app_name);
 
+    virtual void application_configure_surface_called(std::string const& app_name);
+
     virtual void application_error(
         std::string const& app_name,
         char const* method,

=== modified file 'include/server/mir/logging/application_mediator_report.h'
--- include/server/mir/logging/application_mediator_report.h	2013-03-04 16:02:02 +0000
+++ include/server/mir/logging/application_mediator_report.h	2013-03-15 03:39:20 +0000
@@ -47,6 +47,8 @@
 
     virtual void application_drm_auth_magic_called(std::string const& app_name);
 
+    virtual void application_configure_surface_called(std::string const& app_name);
+
     virtual void application_error(
         std::string const& app_name,
         char const* method,

=== modified file 'include/server/mir/shell/application_session.h'
--- include/server/mir/shell/application_session.h	2013-03-07 08:04:05 +0000
+++ include/server/mir/shell/application_session.h	2013-03-15 03:39:20 +0000
@@ -46,6 +46,8 @@
     void hide();
     void show();
 
+    int configure_surface(SurfaceId id, MirSurfaceAttrib attrib, int value);
+
 protected:
     ApplicationSession(ApplicationSession const&) = delete;
     ApplicationSession& operator=(ApplicationSession const&) = delete;

=== modified file 'include/server/mir/shell/session.h'
--- include/server/mir/shell/session.h	2013-03-07 08:04:05 +0000
+++ include/server/mir/shell/session.h	2013-03-15 03:39:20 +0000
@@ -19,6 +19,7 @@
 #ifndef MIR_SHELL_SESSION_H_
 #define MIR_SHELL_SESSION_H_
 
+#include "mir_toolkit/common.h"
 #include "mir/shell/surface_id.h"
 
 #include <mutex>
@@ -50,6 +51,8 @@
     virtual void hide() = 0;
     virtual void show() = 0;
 
+    virtual int configure_surface(SurfaceId id, MirSurfaceAttrib attrib, int value) = 0;
+
 protected:
     Session() = default;
     Session(Session const&) = delete;

=== modified file 'include/server/mir/shell/surface.h'
--- include/server/mir/shell/surface.h	2013-03-07 08:04:05 +0000
+++ include/server/mir/shell/surface.h	2013-03-15 03:39:20 +0000
@@ -23,6 +23,7 @@
 #include "mir/geometry/pixel_format.h"
 #include "mir/geometry/point.h"
 #include "mir/geometry/size.h"
+#include "mir_toolkit/common.h"
 
 #include <memory>
 
@@ -55,6 +56,8 @@
     virtual void advance_client_buffer() = 0;
     virtual std::shared_ptr<compositor::Buffer> client_buffer() const = 0;
 
+    virtual int configure(MirSurfaceAttrib attrib, int value) = 0;
+
 protected:
     Surface() = default;
     Surface(Surface const&) = delete;

=== modified file 'include/server/mir/surfaces/surface.h'
--- include/server/mir/surfaces/surface.h	2013-03-07 08:04:05 +0000
+++ include/server/mir/surfaces/surface.h	2013-03-15 03:39:20 +0000
@@ -22,6 +22,7 @@
 #include "mir/geometry/pixel_format.h"
 #include "mir/graphics/renderable.h"
 #include "mir/compositor/buffer_properties.h"
+#include "mir_toolkit/common.h"
 
 #include <memory>
 #include <string>
@@ -71,6 +72,9 @@
     std::shared_ptr<compositor::Buffer> client_buffer() const;
     void shutdown();
 
+    MirSurfaceType type() const;
+    bool set_type(MirSurfaceType t);
+
 private:
     std::string surface_name;
     std::shared_ptr<BufferBundle> buffer_bundle;
@@ -81,6 +85,8 @@
     float alpha_value;
 
     bool is_hidden;
+
+    MirSurfaceType type_value;
 };
 
 }

=== added file 'include/shared/mir_toolkit/common.h'
--- include/shared/mir_toolkit/common.h	1970-01-01 00:00:00 +0000
+++ include/shared/mir_toolkit/common.h	2013-03-15 03:39:20 +0000
@@ -0,0 +1,49 @@
+/*
+ * Simple definitions common to client and server.
+ *
+ * Copyright © 2013 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 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Author: Daniel van Vugt <[e-mail]>
+ */
+
+#ifndef MIR_COMMON_H_
+#define MIR_COMMON_H_
+
+/* This is C code. Not C++. */
+
+/*
+ * Attributes of a surface that the client and server/shell may wish to
+ * get or set over the wire.
+ */
+typedef enum MirSurfaceAttrib
+{
+    mir_surface_attrib_type,
+    mir_surface_attrib_arraysize_
+} MirSurfaceAttrib;
+
+typedef enum MirSurfaceType
+{
+    mir_surface_type_normal,
+    mir_surface_type_utility,
+    mir_surface_type_dialog,
+    mir_surface_type_overlay,
+    mir_surface_type_freestyle,
+    mir_surface_type_popover,
+    mir_surface_type_arraysize_
+} MirSurfaceType;
+
+/* TODO: Surface states here */
+
+#endif

=== modified file 'include/test/mir_test_doubles/mock_session.h'
--- include/test/mir_test_doubles/mock_session.h	2013-03-11 20:18:11 +0000
+++ include/test/mir_test_doubles/mock_session.h	2013-03-15 03:39:20 +0000
@@ -41,6 +41,8 @@
     
     MOCK_METHOD0(hide, void());
     MOCK_METHOD0(show, void());
+
+    MOCK_METHOD3(configure_surface, int(shell::SurfaceId, MirSurfaceAttrib, int));
 };
 
 }

=== modified file 'include/test/mir_test_doubles/mock_surface.h'
--- include/test/mir_test_doubles/mock_surface.h	2013-03-11 20:18:11 +0000
+++ include/test/mir_test_doubles/mock_surface.h	2013-03-15 03:39:20 +0000
@@ -41,6 +41,8 @@
     MOCK_CONST_METHOD0(size, geometry::Size ());
     MOCK_CONST_METHOD0(pixel_format, geometry::PixelFormat ());
     MOCK_CONST_METHOD0(client_buffer, std::shared_ptr<compositor::Buffer> ());
+
+    MOCK_METHOD2(configure, int(MirSurfaceAttrib, int));
 };
 
 }

=== modified file 'include/test/mir_test_doubles/stub_session.h'
--- include/test/mir_test_doubles/stub_session.h	2013-03-11 20:12:42 +0000
+++ include/test/mir_test_doubles/stub_session.h	2013-03-15 03:39:20 +0000
@@ -54,6 +54,10 @@
     void show()
     {
     }
+    int configure_surface(shell::SurfaceId, MirSurfaceAttrib, int)
+    {
+        return 0;
+    }
 };
 
 }

=== modified file 'src/client/mir_client_library.cpp'
--- src/client/mir_client_library.cpp	2013-03-07 08:04:05 +0000
+++ src/client/mir_client_library.cpp	2013-03-15 03:39:20 +0000
@@ -227,3 +227,25 @@
 {
     // Ignore
 }
+
+mir_toolkit::MirWaitHandle* mir_toolkit::mir_surface_set_type(MirSurface *surf,
+                                                           MirSurfaceType type)
+{
+    return surf ? surf->configure(mir_surface_attrib_type, type) : NULL;
+}
+
+MirSurfaceType mir_toolkit::mir_surface_get_type(MirSurface *surf)
+{
+    MirSurfaceType type = mir_surface_type_normal;
+
+    if (surf)
+    {
+        // I assume the type can only change from the client side. Otherwise
+        // we would have to send off a message to retrieve the latest...
+
+        int t = surf->attrib(mir_surface_attrib_type);
+        type = static_cast<MirSurfaceType>(t);
+    }
+
+    return type;
+}

=== modified file 'src/client/mir_surface.cpp'
--- src/client/mir_surface.cpp	2013-03-07 08:04:05 +0000
+++ src/client/mir_surface.cpp	2013-03-15 03:39:20 +0000
@@ -50,6 +50,10 @@
     message.set_buffer_usage(params.buffer_usage);
 
     server.create_surface(0, &message, &surface, gp::NewCallback(this, &MirSurface::created, callback, context));
+
+    for (int i = 0; i < mir_surface_attrib_arraysize_; i++)
+        attrib_cache[i] = -1;
+    attrib_cache[mir_surface_attrib_type] = mir_surface_type_normal;
 }
 
 mir_toolkit::MirSurface::~MirSurface()
@@ -226,3 +230,46 @@
 {
     return *accelerated_window;
 }
+
+MirWaitHandle* MirSurface::configure(MirSurfaceAttrib at, int value)
+{
+    mp::SurfaceSetting setting;
+    setting.mutable_surfaceid()->CopyFrom(surface.id());
+    setting.set_attrib(at);
+    setting.set_ivalue(value);
+
+    configure_wait_handle.expect_result();
+    server.configure_surface(0, &setting, &configure_result, 
+              google::protobuf::NewCallback(this, &MirSurface::on_configured));
+
+    return &configure_wait_handle;
+}
+
+void MirSurface::on_configured()
+{
+    if (configure_result.has_surfaceid() &&
+        configure_result.surfaceid().value() == surface.id().value() &&
+        configure_result.has_attrib())
+    {
+        switch (configure_result.attrib())
+        {
+        case mir_surface_attrib_type:
+            if (configure_result.has_ivalue())
+            {
+                int t = configure_result.ivalue();
+                attrib_cache[mir_surface_attrib_type] = t;
+            } // else error is probably set due to an unsupported attrib/value
+            break;
+        default:
+            assert(false);
+            break;
+        }
+
+        configure_wait_handle.result_received();
+    }
+}
+
+int MirSurface::attrib(MirSurfaceAttrib at) const
+{
+    return attrib_cache[at];
+}

=== modified file 'src/client/mir_surface.h'
--- src/client/mir_surface.h	2013-03-07 08:04:05 +0000
+++ src/client/mir_surface.h	2013-03-15 03:39:20 +0000
@@ -23,6 +23,7 @@
 #include "mir/geometry/pixel_format.h"
 #include "mir/geometry/dimensions.h"
 #include "mir_toolkit/mir_client_library.h"
+#include "mir_toolkit/common.h"
 #include "client_buffer_depository.h"
 #include "mir_wait_handle.h"
 #include "mir_client_surface.h"
@@ -72,7 +73,11 @@
     void release_cpu_region();
     EGLNativeWindowType generate_native_window();
 
+    MirWaitHandle* configure(MirSurfaceAttrib a, int value);
+    int attrib(MirSurfaceAttrib a) const;
+
 private:
+    void on_configured();
     void process_incoming_buffer();
     void populate(MirBufferPackage& buffer_package);
     void created(mir_surface_lifecycle_callback callback, void * context);
@@ -88,6 +93,7 @@
     MirConnection *connection;
     MirWaitHandle create_wait_handle;
     MirWaitHandle next_buffer_wait_handle;
+    MirWaitHandle configure_wait_handle;
 
     int last_buffer_id;
 
@@ -96,6 +102,11 @@
 
     std::shared_ptr<mir::client::Logger> logger;
     std::shared_ptr<EGLNativeWindowType> accelerated_window;
+
+    mir::protobuf::SurfaceSetting configure_result;
+
+    // Cache of latest SurfaceSettings returned from the server
+    int attrib_cache[mir_surface_attrib_arraysize_];
 };
 
 #endif /* MIR_CLIENT_PRIVATE_MIR_WAIT_HANDLE_H_ */

=== modified file 'src/server/frontend/application_mediator.cpp'
--- src/server/frontend/application_mediator.cpp	2013-03-07 08:04:05 +0000
+++ src/server/frontend/application_mediator.cpp	2013-03-15 03:39:20 +0000
@@ -24,6 +24,7 @@
 #include "mir/shell/surface_creation_parameters.h"
 #include "mir/frontend/resource_cache.h"
 
+#include "mir_toolkit/common.h"
 #include "mir/compositor/buffer_ipc_package.h"
 #include "mir/compositor/buffer_id.h"
 #include "mir/compositor/buffer.h"
@@ -220,3 +221,29 @@
     done->Run();
 }
 
+void mir::frontend::ApplicationMediator::configure_surface(
+    google::protobuf::RpcController*, // controller,
+    const mir::protobuf::SurfaceSetting* request,
+    mir::protobuf::SurfaceSetting* response,
+    google::protobuf::Closure* done)
+{
+    MirSurfaceAttrib attrib = static_cast<MirSurfaceAttrib>(request->attrib());
+
+    // Required response fields:
+    response->mutable_surfaceid()->CopyFrom(request->surfaceid());
+    response->set_attrib(attrib);
+
+    if (application_session.get() == nullptr)
+        BOOST_THROW_EXCEPTION(std::logic_error("Invalid application session"));
+
+    report->application_configure_surface_called(application_session->name());
+
+    auto const id = shell::SurfaceId(request->surfaceid().value());
+    int value = request->ivalue();
+    int newvalue = application_session->configure_surface(id, attrib, value);
+
+    response->set_ivalue(newvalue);
+
+    done->Run();
+}
+

=== modified file 'src/server/frontend/null_application_mediator_report.cpp'
--- src/server/frontend/null_application_mediator_report.cpp	2013-03-07 08:04:05 +0000
+++ src/server/frontend/null_application_mediator_report.cpp	2013-03-15 03:39:20 +0000
@@ -42,6 +42,10 @@
 {
 }
 
+void mir::frontend::NullApplicationMediatorReport::application_configure_surface_called(std::string const&)
+{
+}
+
 void mir::frontend::NullApplicationMediatorReport::application_error(
         std::string const&,
         char const* ,

=== modified file 'src/server/frontend/protobuf_message_processor.cpp'
--- src/server/frontend/protobuf_message_processor.cpp	2013-03-07 08:04:05 +0000
+++ src/server/frontend/protobuf_message_processor.cpp	2013-03-15 03:39:20 +0000
@@ -176,6 +176,10 @@
         {
             invoke(&protobuf::DisplayServer::select_focus_by_lightdm_id, invocation);
         }
+        else if ("configure_surface" == invocation.method_name())
+        {
+            invoke(&protobuf::DisplayServer::configure_surface, invocation);
+        }
         else if ("disconnect" == invocation.method_name())
         {
             invoke(&protobuf::DisplayServer::disconnect, invocation);

=== modified file 'src/server/logging/application_mediator_report.cpp'
--- src/server/logging/application_mediator_report.cpp	2013-03-04 16:02:02 +0000
+++ src/server/logging/application_mediator_report.cpp	2013-03-15 03:39:20 +0000
@@ -63,6 +63,11 @@
     log->log<Logger::informational>("application_drm_auth_magic_called(\"" + app_name + "\")", component);
 }
 
+void ml::ApplicationMediatorReport::application_configure_surface_called(std::string const& app_name)
+{
+    log->log<Logger::informational>("application_configure_surface_called(\"" + app_name + "\")", component);
+}
+
 void ml::ApplicationMediatorReport::application_error(
         std::string const& app_name,
         char const* method,

=== modified file 'src/server/shell/application_session.cpp'
--- src/server/shell/application_session.cpp	2013-03-07 08:04:05 +0000
+++ src/server/shell/application_session.cpp	2013-03-15 03:39:20 +0000
@@ -122,3 +122,13 @@
         id_s.second->show();
     }
 }
+
+int msh::ApplicationSession::configure_surface(msh::SurfaceId id,
+                                               MirSurfaceAttrib attrib,
+                                               int value)
+{
+    std::unique_lock<std::mutex> lock(surfaces_mutex);
+    std::shared_ptr<msh::Surface> surf(checked_find(id)->second);
+
+    return surf->configure(attrib, value);
+}

=== modified file 'src/server/surfaces/proxy_surface.cpp'
--- src/server/surfaces/proxy_surface.cpp	2013-03-07 08:04:05 +0000
+++ src/server/surfaces/proxy_surface.cpp	2013-03-15 03:39:20 +0000
@@ -109,6 +109,37 @@
     surface_stack->destroy_surface(surface);
 }
 
+int ms::BasicProxySurface::configure(MirSurfaceAttrib attrib, int value)
+{
+    if (auto const& s = surface.lock())
+    {
+        int result = 0;
+
+        /*
+         * TODO: In future, query the shell implementation for the subset of
+         *       attributes/types it implements.
+         */
+        switch (attrib)
+        {
+        case mir_surface_attrib_type:
+            if (!s->set_type(static_cast<MirSurfaceType>(value)))
+                BOOST_THROW_EXCEPTION(std::logic_error("Invalid surface "
+                                                       "type."));
+            result = s->type();
+            break;
+        default:
+            BOOST_THROW_EXCEPTION(std::logic_error("Invalid surface "
+                                                   "attribute."));
+            break;
+        }
+
+        return result;
+    }
+    else
+    {
+        BOOST_THROW_EXCEPTION(std::runtime_error("Invalid surface"));
+    }
+}
 
 ms::ProxySurface::ProxySurface(
         SurfaceStackModel* const surface_stack_,

=== modified file 'src/server/surfaces/proxy_surface.h'
--- src/server/surfaces/proxy_surface.h	2013-03-07 08:04:05 +0000
+++ src/server/surfaces/proxy_surface.h	2013-03-15 03:39:20 +0000
@@ -56,6 +56,8 @@
 
     std::shared_ptr<compositor::Buffer> client_buffer() const;
 
+    int configure(MirSurfaceAttrib attrib, int value);
+
 protected:
     void destroy_surface(SurfaceStackModel* const surface_stack) const;
 

=== modified file 'src/server/surfaces/surface.cpp'
--- src/server/surfaces/surface.cpp	2013-03-07 08:04:05 +0000
+++ src/server/surfaces/surface.cpp	2013-03-15 03:39:20 +0000
@@ -22,6 +22,8 @@
 #include "mir/compositor/buffer_ipc_package.h"
 #include "mir/surfaces/buffer_bundle.h"
 
+#include <stdexcept>
+#include <boost/throw_exception.hpp>
 #include <cassert>
 #include <glm/gtc/matrix_transform.hpp>
 
@@ -36,7 +38,8 @@
     surface_name(name),
     buffer_bundle(buffer_bundle),
     alpha_value(1.0f),
-    is_hidden(false)
+    is_hidden(false),
+    type_value(mir_surface_type_normal)
 {
     // TODO(tvoss,kdub): Does a surface without a buffer_bundle make sense?
     assert(buffer_bundle);
@@ -128,3 +131,21 @@
 {
     return client_buffer_resource;
 }
+
+MirSurfaceType ms::Surface::type() const
+{
+    return type_value;
+}
+
+bool ms::Surface::set_type(MirSurfaceType t)
+{
+    bool valid = false;
+
+    if (t >= 0 && t < mir_surface_type_arraysize_)
+    {
+        type_value = t;
+        valid = true;
+    }
+
+    return valid;
+}

=== modified file 'src/shared/CMakeLists.txt'
--- src/shared/CMakeLists.txt	2013-03-13 17:54:59 +0000
+++ src/shared/CMakeLists.txt	2013-03-15 03:39:20 +0000
@@ -5,3 +5,13 @@
   MIR_GENERATED_INCLUDE_DIRECTORIES
   ${MIR_GENERATED_INCLUDE_DIRECTORIES}
   PARENT_SCOPE)
+
+# Some headers contain common definitions required for both client and
+# server development...
+set (MIR_COMMON_HEADERS
+  ${CMAKE_SOURCE_DIR}/include/shared/mir_toolkit/common.h
+)
+
+# TODO: Find a more elegant include/ structure. Ideally one in which everything
+#       lives under "include/mir/".
+install (FILES ${MIR_COMMON_HEADERS} DESTINATION "include/mir_toolkit/")

=== modified file 'src/shared/protobuf/mir_protobuf.proto'
--- src/shared/protobuf/mir_protobuf.proto	2013-01-10 16:58:01 +0000
+++ src/shared/protobuf/mir_protobuf.proto	2013-03-15 03:39:20 +0000
@@ -111,6 +111,14 @@
   optional string error = 127;
 }
 
+message SurfaceSetting {
+  optional SurfaceId surfaceid = 1;
+  optional int32     attrib = 2;
+  optional int32     ivalue = 3;
+  // optional string    svalue = 4;  // Expected for future use
+  optional string error = 127;
+}
+
 service DisplayServer {
   // Platform independent requests
   rpc connect(ConnectParameters) returns (Connection);
@@ -124,5 +132,7 @@
   rpc drm_auth_magic(DRMMagic) returns (DRMAuthMagicStatus);
 
   rpc test_file_descriptors(Void) returns (Buffer);
+
+  rpc configure_surface(SurfaceSetting) returns (SurfaceSetting);
 }
 

=== modified file 'tests/acceptance-tests/test_client_library.cpp'
--- tests/acceptance-tests/test_client_library.cpp	2013-03-07 08:04:05 +0000
+++ tests/acceptance-tests/test_client_library.cpp	2013-03-15 03:39:20 +0000
@@ -168,6 +168,80 @@
     launch_client_process(client_config);
 }
 
+TEST_F(DefaultDisplayServerTestFixture, surface_types)
+{
+    struct ClientConfig : ClientConfigCommon
+    {
+        void exec()
+        {
+
+            mir_wait_for(mir_connect(mir_test_socket, __PRETTY_FUNCTION__, connection_callback, this));
+
+            ASSERT_TRUE(connection != NULL);
+            EXPECT_TRUE(mir_connection_is_valid(connection));
+            EXPECT_STREQ(mir_connection_get_error_message(connection), "");
+
+            MirSurfaceParameters const request_params =
+            {
+                __PRETTY_FUNCTION__,
+                640, 480,
+                mir_pixel_format_abgr_8888,
+                mir_buffer_usage_hardware
+            };
+
+            mir_wait_for(mir_surface_create(connection, &request_params, create_surface_callback, this));
+
+            ASSERT_TRUE(surface != NULL);
+            EXPECT_TRUE(mir_surface_is_valid(surface));
+            EXPECT_STREQ(mir_surface_get_error_message(surface), "");
+
+            EXPECT_EQ(mir_surface_get_type(surface),
+                      mir_surface_type_normal);
+
+            mir_wait_for(mir_surface_set_type(surface,
+                                              mir_surface_type_freestyle));
+            EXPECT_EQ(mir_surface_get_type(surface),
+                      mir_surface_type_freestyle);
+
+            mir_wait_for(mir_surface_set_type(surface,
+                                            static_cast<MirSurfaceType>(999)));
+            EXPECT_EQ(mir_surface_get_type(surface),
+                      mir_surface_type_freestyle);
+
+            mir_wait_for(mir_surface_set_type(surface,
+                                              mir_surface_type_dialog));
+            EXPECT_EQ(mir_surface_get_type(surface),
+                      mir_surface_type_dialog);
+
+            mir_wait_for(mir_surface_set_type(surface,
+                                            static_cast<MirSurfaceType>(888)));
+            EXPECT_EQ(mir_surface_get_type(surface),
+                      mir_surface_type_dialog);
+
+            // Stress-test synchronization logic with some flooding
+            for (int i = 0; i < 100; i++)
+            {
+                mir_surface_set_type(surface, mir_surface_type_normal);
+                mir_surface_set_type(surface, mir_surface_type_utility);
+                mir_surface_set_type(surface, mir_surface_type_dialog);
+                mir_surface_set_type(surface, mir_surface_type_overlay);
+                mir_surface_set_type(surface, mir_surface_type_freestyle);
+                mir_wait_for(mir_surface_set_type(surface,
+                                                  mir_surface_type_popover));
+                ASSERT_EQ(mir_surface_get_type(surface),
+                          mir_surface_type_popover);
+            }
+
+            mir_wait_for(mir_surface_release(surface, release_surface_callback,
+                                             this));
+
+            mir_connection_release(connection);
+        }
+    } client_config;
+
+    launch_client_process(client_config);
+}
+
 TEST_F(DefaultDisplayServerTestFixture, client_library_creates_multiple_surfaces)
 {
     int const n_surfaces = 13;

=== modified file 'tests/behavior-tests/session_management_context.cpp'
--- tests/behavior-tests/session_management_context.cpp	2013-03-13 07:41:46 +0000
+++ tests/behavior-tests/session_management_context.cpp	2013-03-15 03:39:20 +0000
@@ -75,6 +75,11 @@
     {
         return std::shared_ptr<mc::Buffer>();
     }
+
+    virtual int configure(MirSurfaceAttrib, int)
+    {
+        return 0;
+    }
 };
 
 struct SizedDummySurface : public DummySurface

=== modified file 'tests/integration-tests/cucumber/test_session_management_context.cpp'
--- tests/integration-tests/cucumber/test_session_management_context.cpp	2013-03-13 07:41:46 +0000
+++ tests/integration-tests/cucumber/test_session_management_context.cpp	2013-03-15 03:39:20 +0000
@@ -64,6 +64,8 @@
     MOCK_CONST_METHOD0(size, mir::geometry::Size ());
     MOCK_CONST_METHOD0(pixel_format, mir::geometry::PixelFormat ());
     MOCK_CONST_METHOD0(client_buffer, std::shared_ptr<mc::Buffer> ());
+
+    MOCK_METHOD2(configure, int(MirSurfaceAttrib, int));
 };
 
 MATCHER_P(NamedWindowWithNoGeometry, name, "")

=== modified file 'tests/unit-tests/client/test_client_mir_surface.cpp'
--- tests/unit-tests/client/test_client_mir_surface.cpp	2013-03-07 08:04:05 +0000
+++ tests/unit-tests/client/test_client_mir_surface.cpp	2013-03-15 03:39:20 +0000
@@ -376,3 +376,22 @@
     EXPECT_EQ(pf, geom::PixelFormat::abgr_8888);
 }
 
+TEST_F(MirClientSurfaceTest, default_surface_type)
+{
+    using namespace testing;
+    using namespace mir::protobuf;
+
+    EXPECT_CALL(*mock_depository, deposit_package_rv(_,_,_,_))
+        .Times(1);
+
+    auto surface = std::make_shared<MirSurface> (connection.get(),
+                                                 *client_comm_channel,
+                                                 logger,
+                                                 mock_depository,
+                                                 params,
+                                                 &empty_callback,
+                                                 (void*) NULL);
+    surface->get_create_wait_handle()->wait_for_result();
+
+    EXPECT_EQ(surface->attrib(mir_surface_attrib_type), mir_surface_type_normal);
+}

=== modified file 'tests/unit-tests/surfaces/test_surface.cpp'
--- tests/unit-tests/surfaces/test_surface.cpp	2013-03-07 08:04:05 +0000
+++ tests/unit-tests/surfaces/test_surface.cpp	2013-03-15 03:39:20 +0000
@@ -306,3 +306,21 @@
 
     EXPECT_EQ(alpha, ret_alpha);
 }
+
+TEST_F(SurfaceCreation, types)
+{
+    using namespace testing;
+
+    ms::Surface surf{surface_name, mock_buffer_bundle};
+
+    EXPECT_EQ(mir_surface_type_normal, surf.type());
+
+    EXPECT_TRUE(surf.set_type(mir_surface_type_utility));
+    EXPECT_EQ(surf.type(), mir_surface_type_utility);
+
+    EXPECT_FALSE(surf.set_type(static_cast<MirSurfaceType>(999)));
+    EXPECT_EQ(surf.type(), mir_surface_type_utility);
+
+    EXPECT_TRUE(surf.set_type(mir_surface_type_dialog));
+    EXPECT_EQ(surf.type(), mir_surface_type_dialog);
+}