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

[Daniel van Vugt]

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

This work will also form the foundation of supporting surface states and
necessitates a prototype shell interface.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:498
http://s-jenkins:8080/job/mir-ci/1116/
Executed test runs:
    SUCCESS: http://s-jenkins:8080/job/mir-ci/./build=pbuilder,distribution=quantal,flavor=amd64/1116/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/mir-ci/1116//rebuild/?

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

87 === modified file 'include/mir/graphics/display.h'
...
94 +#include "mir/shell/shell.h"

This feels very wrong - I don't believe that graphics::Display needs to know about shell::Shell.

Unless there is a compelling reason for something different, there should be an interface in *graphics* that determines the functionality that graphics::Display needs, and that will be implemented by shell::Shell.

~~~~

102 + void set_shell(std::shared_ptr<mir::Shell> s);
103 + bool start_shell();
104 + void stop_shell();
105 + std::shared_ptr<mir::Shell> current_shell() const;

I find it very hard to believe that these are functions that belong to graphics::Display.

In addition, graphics::Display is an interface and should not provide an implementation. (It is used in compositing - it should probably be there, but that's an existing issue.)

~~~~

252 +#include <mir/surface.h>

Don't use system includes for our files.

~~~~

298 +#ifndef __MIR_COMMON_SURFACE_H__
299 +#define __MIR_COMMON_SURFACE_H__

Does not match project naming style

~~~~

307 + MIR_SURFACE_TYPE,
308 + MirSurfaceAttrib_ARRAYSIZE

Does not match project naming style

~~~~

608 === added file 'src/display.cpp'

This is a confusing place to implement mir::graphics::Display

~~~~

633 +using namespace mir;
634 +using namespace mir::graphics;
635 +using namespace mir::shell;

This is a very different style to the rest of the codebase. I don't like it as it isn't clear where the classes mentioned in the code come from. (It can also lead to free functions being defined in global namespace and not the intended namespace.)

~~~~

735 + auto const shell = graphics_display->current_shell();
736 + if (!shell->supports(attrib))
737 + BOOST_THROW_EXCEPTION(std::logic_error("Current shell does not "
738 + "support the specified surface "
739 + "attribute"));

Application logic doesn't belong in ApplicationMediator.

~~~

814 +message SurfaceSetting {
815 + required SurfaceId surfaceid = 1;
816 + required int32 attrib = 2;
817 + optional int32 ivalue = 3;
818 + // optional string svalue = 4; // Expected for future use
819 +
820 + optional string error = 127;
821 +}

As this is returned by the RPC framework all the fields should be optional (as, in the event of an exception, it may be the only field populated).

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

Alan,

"Shell in Display"
I kind of agree. This was the last part I wrote and I later saw one or two alternative places to attach Shell other than inside Display. But from a conceptional standpoint, can we discuss why the "shell" is not a 1:1 part of a "display"?

"#include <mir/surface.h>"
Yes this is correct because it is included from a public system header (shell.h). Hence angle brackets are required.

"__MIR_COMMON_SURFACE_H__"
Yeah I can change the style no problem.

"MIR_SURFACE_TYPE"
This is C code. Not C++. What is the established convention for C-compatible enums that I should change to?

"src/display.cpp"
It's not really a confusing place because it's the same directory where other sources relating to it already reside. If I put display.cpp in a subdir then it has to be built into a static library and then bundled into the server library. Seems like an overkill for one small source file right now.

"using namespace mir"
I prefer this because it makes the code much more readable, and will argue to the very end that the convention seen in existing mir code is ugly, hard to read and more time-consuming to maintain. It is never confusing where a symbol comes from if it is only defined in one place in the project.

"Application logic doesn't belong in ApplicationMediator."
Yeah I had the same feeling. Just haven't yet found a cleaner place to put it where the logic had full access to all the objects in question.

"required vs optional"
I don't care enough to argue. Will change it.

All in all, I think that's a good initial review. Good only because I only disagree deeply with one of the points made.

[PS Jenkins bot (ps-jenkins)]

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

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

review: Approve (continuous-integration)

[Alexandros Frantzis (afrantzis)]

> But from a conceptional standpoint, can we discuss why the "shell" is not a 1:1 part of a "display"?

The display subsystem (at least as it stands now) offers low-level display/graphics services. Why should it have any knowledge about how those services are being used? Also, as Alan noted, if it turns out that display actually needs to have more information, this should happen through an interface that the display itself defines, and which is expressed in terms of what the display actually needs, not what the shell happens to provide.

> "MIR_SURFACE_TYPE"
> This is C code. Not C++. What is the established convention for C-compatible enums that I should change to?

See, for example, mir_client_library.h:

MirPixelFormat{ mir_pixel_format_invalid, mir_pixel_format_abgr_8888, ...}

> "src/display.cpp"
> It's not really a confusing place because it's the same directory where other sources
> relating to it already reside. If I put display.cpp in a subdir then it has to be built
> into a static library and then bundled into the server library. Seems like an overkill
> for one small source file right now.

I think the main issue is that we needed to have this file because mg::Display was not an interface anymore.

> "using namespace mir"
> I prefer this because it makes the code much more readable, and will argue to the very
> end that the convention seen in existing mir code is ugly, hard to read and more
> time-consuming to maintain. It is never confusing where a symbol comes from if it is only
> defined in one place in the project.

That's not always the case, e.g., shell::Surface vs surfaces::Surface

119 + std::shared_ptr<mir::Shell> sh = nullptr);

It would be cleaner to remove the optional parameter, and make invocations of ApplicationSession() that don't need a shell, use a NullShell.

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

Alexandros,

Yeah forget about the "Display" argument. I don't use "Display" any more. The above discussion in yellow is very out of date and should be mostly ignored.

Ok, I'll change the enum format to lower case as described. There will be a lot of string substitution today...

Yeah I know parts of Mir distinguish foo::Surface from bar::Surface. I'm familiar with that convention but don't like it. I think it's a crutch for when you can't think of a really good accurate class name. A namespace should be used as a namespace, and not used as an explicit part of the class name.

Yeah I can change the "=nullshell" into two separate constructors. I don't think that's cleaner but it's not worth arguing about.

[Alexandros Frantzis (afrantzis)]

> Yeah I can change the "=nullshell" into two separate constructors. I don't think that's cleaner but it's not worth
> arguing about.

Sorry, I didn't phrase my proposal very clearly. So here is take 2:

My proposal is to use a single constructor with a non-optional shell argument, and all invocations that don't need a shell should explicitly pass a NullShell. The reason is that other ways of doing this introduce (unnecessarily, IMO) two code paths in the code, any one of which could be less tested than the other. There is also the matter of desing clarity: ApplicationSession either has a shell dependency, or it doesn't. I feel that hiding that dependency by either having an optional parameter, or a second constructor, trades clarity for coding convenience, which I don't find to be a good bargain.

Thoughts?

[Daniel van Vugt (vanvugt)]

That makes the test cases even more verbose and ugly. I tried it already.

[PS Jenkins bot (ps-jenkins)]

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

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

833 + if (!shell->supports(attrib))
834 + BOOST_THROW_EXCEPTION(std::logic_error("Current shell does not "
835 + "support the specified surface "
836 + "attribute"));
837 +
838 + if (!shell->supports(attrib, value))
839 + BOOST_THROW_EXCEPTION(std::logic_error("Current shell does not "
840 + "support the specified value "
841 + "for this attribute"));
842 +
843 + std::unique_lock<std::mutex> lock(surfaces_mutex);
844 + std::shared_ptr<msh::Surface> surf(checked_find(id)->second);
845 +
846 + return surf->configure(attrib, value);

I would expect all this precondition checking to occur inside the surf->configure() call. ApplicationSession doesn't need to know about surface attributes or "Shell".

~~~~

"Shell" is still a bad name for an interface that validates surface attributes (even if the discussion in yellow is believed to be "mostly obsolete"). But I don't really see the need for this interface at all - why doesn't Surface::configure() just implement the validation?

~~~~

195 +#include "mir/surface.h" // for MirSurfaceAttrib

use a forward declaration?

~~~~

344 === added file 'include/mir/surface.h'

I don't believe this is the correct place - this is for namespace mir {}

~~~~

Am I right in thinking that Surface attributes are meaningful to the shell, but not to the surfaces library? If that is correct, they belong on shell::Surface, but not on surfaces::Surface (and that supports my growing suspicion that ProxySurface belongs in shell).

review: Needs Fixing

[Robert Carr (robertcarr)]

Haven't processed the rest of the discussion but:

>> (and that supports my growing suspicion that ProxySurface belongs in shell).

Agree completely here.

[Daniel van Vugt (vanvugt)]

"Shell" will in time represent a whole "shell" (i.e. everything a shell implementer needs to implement). I'm just not defining parts of the interface we don't need yet.

And yes it is correct for the Shell class to validate attributes and types, because only the shell implementer knows what attributes and types are valid for his/her shell. It would be wrong to push that functionality into 'shell::Surface' (which did not exist while I was writing this branch), because the result of supports() is invariant across surface instances. The supports() methods are simply to ask the shell author what types of surfaces/attributes their shell supports. You could argue therefore that supports() should be static to shell::Surface. However limitations in the C++ language prevent us from being able to virtualize static methods (last I checked). Calling current_shell.supports(some-surface-type) is the next best option. Consider also that we will often want to know if a surface type is supported _before_ we create the surface. So "supports()" would have to be a method of some other object that exists before the surface does, if not a static virtual method of shell::Surface (which wasn't possible last I checked).

Forward declaration or MirSurfaceAttrib:
Is there a type-safe way to forward declare an enum (from a C header) into C++11 ? That is other than using "int"? Which I was until recently... What's the syntax (that's also clang-friendly)?

'include/mir/surface.h'
I think '#include <mir/foo.h>' is the perfect place for shared headers. But of course if your header re-org branch lands first, then I'll use that proposed layout of <mir/shared/foo.h> and argue later.

[PS Jenkins bot (ps-jenkins)]

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

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

> "Shell" will in time represent a whole "shell" (i.e. everything a shell
> implementer needs to implement). I'm just not defining parts of the interface
> we don't need yet.

Even if this were a good idea (which needs to be argued), we shouldn't make parts of mir that only require a specific part of this functionality dependent on the whole Shell interface.

> And yes it is correct for the Shell class to validate attributes and types,
> because only the shell implementer knows what attributes and types are valid
> for his/her shell. It would be wrong to push that functionality into
> 'shell::Surface' (which did not exist while I was writing this branch),

sessions::Surface existed before "sessions" was renamed "shell".

> because the result of supports() is invariant across surface instances. The
> supports() methods are simply to ask the shell author what types of
> surfaces/attributes their shell supports. You could argue therefore that
> supports() should be static to shell::Surface. However limitations in the C++
> language prevent us from being able to virtualize static methods (last I
> checked). Calling current_shell.supports(some-surface-type) is the next best
> option. Consider also that we will often want to know if a surface type is
> supported _before_ we create the surface. So "supports()" would have to be a
> method of some other object that exists before the surface does, if not a
> static virtual method of shell::Surface (which wasn't possible last I
> checked).

It makes more sense that an implementation of shell::Surface maintains the invariant of having valid attributes (by throwing from configure()) than that it requires from co-operation by ApplicationMediator and Shell. (If you want to separate out the validation logic then make shell::SurfaceAttributeValidator a dependency of that implementation.)

[Daniel van Vugt (vanvugt)]

P.S. Thank you (racarr?) for renaming sessions::Surface to shell::Surface. A single change in wording clarifies the design intention significantly, which helps.

=== modified file 'debian/control'
--- debian/control	2013-03-12 03:50:42 +0000
+++ debian/control	2013-03-13 08:24:26 +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-13 08:24:26 +0000
@@ -0,0 +1,1 @@
+usr/include/mir/*.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-13 08:24:26 +0000
@@ -18,6 +18,8 @@
 #ifndef MIR_CLIENT_LIBRARY_H
 #define MIR_CLIENT_LIBRARY_H
 
+#include <mir/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-13 08:24:26 +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-13 08:24:26 +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-13 08:24:26 +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-13 08:24:26 +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-13 08:24:26 +0000
@@ -19,6 +19,7 @@
 #ifndef MIR_SHELL_SESSION_H_
 #define MIR_SHELL_SESSION_H_
 
+#include "mir/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-13 08:24:26 +0000
@@ -23,6 +23,7 @@
 #include "mir/geometry/pixel_format.h"
 #include "mir/geometry/point.h"
 #include "mir/geometry/size.h"
+#include "mir/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-13 08:24:26 +0000
@@ -22,6 +22,7 @@
 #include "mir/geometry/pixel_format.h"
 #include "mir/graphics/renderable.h"
 #include "mir/compositor/buffer_properties.h"
+#include "mir/common.h"
 
 #include <memory>
 #include <string>
@@ -71,6 +72,10 @@
     std::shared_ptr<compositor::Buffer> client_buffer() const;
     void shutdown();
 
+    int configure(MirSurfaceAttrib attrib, int value);
+    MirSurfaceType type() const;
+    bool set_type(MirSurfaceType t);
+
 private:
     std::string surface_name;
     std::shared_ptr<BufferBundle> buffer_bundle;
@@ -81,6 +86,8 @@
     float alpha_value;
 
     bool is_hidden;
+
+    MirSurfaceType type_value;
 };
 
 }

=== added file 'include/shared/mir/common.h'
--- include/shared/mir/common.h	1970-01-01 00:00:00 +0000
+++ include/shared/mir/common.h	2013-03-13 08:24:26 +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-13 08:24:26 +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-13 08:24:26 +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-13 08:24:26 +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-13 08:24:26 +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-13 08:24:26 +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-13 08:24:26 +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/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-13 08:24:26 +0000
@@ -24,6 +24,7 @@
 #include "mir/shell/surface_creation_parameters.h"
 #include "mir/frontend/resource_cache.h"
 
+#include "mir/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-13 08:24:26 +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-13 08:24:26 +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-13 08:24:26 +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-13 08:24:26 +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-13 08:24:26 +0000
@@ -109,6 +109,17 @@
     surface_stack->destroy_surface(surface);
 }
 
+int ms::BasicProxySurface::configure(MirSurfaceAttrib attrib, int value)
+{
+    if (auto const& s = surface.lock())
+    {
+        return s->configure(attrib, value);
+    }
+    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-13 08:24:26 +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-13 08:24:26 +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,44 @@
 {
     return client_buffer_resource;
 }
+
+int ms::Surface::configure(MirSurfaceAttrib attrib, int value)
+{
+    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 (!set_type(static_cast<MirSurfaceType>(value)))
+            BOOST_THROW_EXCEPTION(std::logic_error("Invalid surface type."));
+        result = type_value;
+        break;
+    default:
+        BOOST_THROW_EXCEPTION(std::logic_error("Invalid surface attribute."));
+        break;
+    }
+
+    return result;
+}
+
+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-11 17:42:55 +0000
+++ src/shared/CMakeLists.txt	2013-03-13 08:24:26 +0000
@@ -6,3 +6,11 @@
   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/common.h
+)
+
+install (FILES ${MIR_COMMON_HEADERS} DESTINATION "include/mir/")

=== 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-13 08:24:26 +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-13 08:24:26 +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 < 1000; 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-13 08:24:26 +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-13 08:24:26 +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-13 08:24:26 +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-13 08:24:26 +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);
+}