Merge lp:~robertcarr/mir/surface-configuration-interface--for-shell into lp:~mir-team/mir/trunk

[Robert Carr]

Add an interface by which the shell may be notified of and interfere with surface configuration requests.

Add an interface by which the shell may be notified of and interfere with surface configuration requests.

[Robert Carr (robertcarr)]

This...already has 8 conflicts with trunk :(

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:880
http://jenkins.qa.ubuntu.com/job/mir-ci/1105/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1449/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1334/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/343/console

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

review: Needs Fixing (continuous-integration)

[Robert Carr (robertcarr)]

Merged trunk. The SessionManager->Session construction path is such a cludge (and always conflicting), maybe SessionManager needs to use a SessionFactory.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:881
http://jenkins.qa.ubuntu.com/job/mir-ci/1106/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1450
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1335
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/344
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/344/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=quantal,flavor=amd64/718

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

review: Approve (continuous-integration)

[Kevin DuBois (kdub)]

suggestion:
305 + surface_configurator->configure_surface(surf, attrib, configured_value);
306 + return surf->configure(attrib, configured_value);

skimming the requests like this seems a bit strange, I'd expect more of an Adaptor pattern that adapts the requests coming in. (and perhaps ms::Surface implements SurfaceConfigurator, and there's SurfaceConfigurationAuthorizer(ms::Surface)) or something like that. Its a bit tricky to crank on the unit tests for msh::Surface (we need to have a proxy object for ms::Surface), but maybe a unit test would tease out the class structure?

[Alan Griffiths (alan-griffiths)]

149 + virtual ~SurfaceConfigurator() {}

Don't introduce new interfaces with throwing destructors.

~~~~

151 + virtual void configure_surface(std::shared_ptr<Surface> const& surface, MirSurfaceAttrib attrib,
152 + int &requested_value) = 0;

I don't find this interface intuitive:

  o Why are we sharing ownership? (wouldn't ...Surface& surface... be better?)
  o is requested_value an oddly named out parameter or an in/out parameter? (Why not use a return value for the out value?)

// returns value actually set
int set_attribute(Surface& surface, MirSurfaceAttrib attribute, int value)

~~~~

238 + struct DefaultSurfaceConfigurator : public msh::SurfaceConfigurator
239 + {
240 + void configure_surface(std::shared_ptr<msh::Surface> const&, MirSurfaceAttrib, int&)
241 + {
242 + }

Surely a better default is to set the attrib to the value requested?

review: Needs Fixing

[Robert Carr (robertcarr)]

Reworked the interface

void configure_surface(std::shared_ptr<msh::Surface> const&, MirSurfaceAttrib, int&) ->
int select_attribute_value(msh::Surface const&, MirSurfaceAttrib, int)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:882
http://jenkins.qa.ubuntu.com/job/mir-ci/1117/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1463
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1348
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/355
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/355/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=quantal,flavor=amd64/725

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

review: Approve (continuous-integration)

[Robert Carr (robertcarr)]

To support the OSK we will have to allow client initiated hide/show, this means thinking of a way for the shell to respond to and implement the minimised/restored state transitions. In light of this I have updated SurfaceConfigurator to include a second callback (attribute_set), providing a place for the shell to insert mechanism for these transitions (as opposed to the select_attribute_value callback which handles policy).

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:883
http://jenkins.qa.ubuntu.com/job/mir-ci/1121/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1467
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1352
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/359
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/359/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=quantal,flavor=amd64/728

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

253 + void attribute_set(msh::Surface const&, MirSurfaceAttrib, int)
254 + {
255 + }

This confuses me. Are attributes never stored anywhere? (Clearly, not on the *const* surface supplied.)

~~~~

317 + auto applied_value = surf->configure(attrib,
318 + surface_configurator->select_attribute_value(*surf.get(), attrib, requested_value));
319 + surface_configurator->attribute_set(*surf.get(), attrib, applied_value);
320 + return applied_value;

s/*surf.get()/*surf/

review: Needs Fixing

[Robert Carr (robertcarr)]

Removed superfulous.get()

The attributes are still stored on the surface, rather the two methods of this interface are intended to have different roles

select_attribute_value: A place to implement policy for client state change request, i.e. client X wants to go fullscreen is that allowed.
attribute_set: A place to implement mechanism in response to client state change. I.e. this surface has now changed state to minimized, go ahead and hide it and update some shell objects or whatever you want to do.

It seems this isn't really clear from the interface though. So I will brainstorm on ways to make it more self documenting. Any ideas?

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:884
http://jenkins.qa.ubuntu.com/job/mir-ci/1138/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1494
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1379
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/376
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/376/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vm-ci-build/./distribution=quantal,flavor=amd64/739

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

> The attributes are still stored on the surface, rather the two methods of this
> interface are intended to have different roles
>
> select_attribute_value: A place to implement policy for client state change
> request, i.e. client X wants to go fullscreen is that allowed.
> attribute_set: A place to implement mechanism in response to client state
> change. I.e. this surface has now changed state to minimized, go ahead and
> hide it and update some shell objects or whatever you want to do.
>
> It seems this isn't really clear from the interface though. So I will
> brainstorm on ways to make it more self documenting. Any ideas?

I'm not sure if this is "merely" a naming issue - or if the SurfaceConfigurator has multiple roles (which makes it hard to describe).

In either case however:

316 - return surf->configure(attrib, value);
317 + auto applied_value = surf->configure(attrib,
318 + surface_configurator->select_attribute_value(*surf, attrib, requested_value));
319 + surface_configurator->attribute_set(*surf, attrib, applied_value);
320 + return applied_value;

Is hardly clear. Is it really useful for both surface_configurator AND surf to have an opportunity to override the requested value? (Surely that is a policy that should be implemented once in one place.)

I'd be much happier if it read:

return surface_configurator->configure(*surf, attrib, value);

Even if SurfaceConfigurator::configure() were a template method and called select_attribute_value() and [notify_]attribute_set() internally.

review: Needs Fixing

[Robert Carr (robertcarr)]

I could modify things to the Surface takes the SurfaceConfigurator directly and this becomes internal to Surface::configure rather than using the session. Conflicts with death-to-single-visibility though :(

[Robert Carr (robertcarr)]

I've modified this to not involve the session at all (now handled at the surface level), introduced a new unit test, and reworked some of the test doubles. I think, luckily, I've come up with a way to address the concerns on death-to-single-visibility that should not conflict.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:888
http://jenkins.qa.ubuntu.com/job/mir-ci/1155/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1521/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1406/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/393/console

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

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:889
http://jenkins.qa.ubuntu.com/job/mir-ci/1156/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1522/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1407/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/394/console

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

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:890
http://jenkins.qa.ubuntu.com/job/mir-ci/1159/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1526/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1411
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/397
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/397/artifact/work/output/*zip*/output.zip

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

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:891
http://jenkins.qa.ubuntu.com/job/mir-ci/1160/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1527
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1412
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/398
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/398/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

> I've modified this to not involve the session at all (now handled at the
> surface level), introduced a new unit test, and reworked some of the test
> doubles. I think, luckily, I've come up with a way to address the concerns on
> death-to-single-visibility that should not conflict.

That does seem a bit simpler, but I still feel that the SurfaceConfigurator has multiple roles (which makes it hard to describe).

"Needs discussion"

review: Needs Information

[Robert Carr (robertcarr)]

I'd like to reduce SurfaceConfigurator to a single method, but it's as the step intermediate to select_attribute_value and attribute_set is mutating internal surface state that we probably don't want to expose through public API.

What if SurfaceConfigurator is reduced to a single interface: (small example of a configurator which forces everything to fullscreen state)

SurfaceConfigurator::apply_attribute(msh::Surface const&, msh::Surface::AttributeState const& state, MirAttribute attrib, int value)
{
    if (attrib == mir_surface_type_state)
        value = mir_surface_state_fullscreen
    state.apply(attrib, value) // Updates msh::Surface members
    apply_surface_transformations(surface, attrib, value) // shell does things here to respond to state change, i.e. manipulate window geometry, hide/show, etc.
}
}

[Robert Carr (robertcarr)]

To be clear apply_surface_transformations is just a psuedocode snip not part of the API

[Robert Carr (robertcarr)]

We need to have a discussion on this as we are blocking the OSK.

Alan, is your concern that SurfaceConfigurator has multiple roles? I think that the attribute setting needs to follow a: filter->apply sort of pattern. It seem's then that there are two options:

1. Split in to two interfaces, i.e. SurfaceConfigurationDecider, SurfaceConfigurationApplicator
2. Merge in to single method using some sort of object to encapsulate the apply step.

Any ideas?

[Robert Carr (robertcarr)]

To clarify more on why the interface has two methods, i.e. a select, and apply step. I'm uncomfortable with the following flow:

msh::Surface::configure
{
     <SNIP>
     value = select_and_apply_attribute(*this, attribute, value)
     if (attribute == state)
        state = value
     <...SNIP>
}

Imagine the case, where a surface is switching state from mir_surface_state_minimized to restored. The surface becomes visibile during the body of select_and_apply_attribute but state still == mir_surface_state_minimized so it makes it hard for the shell to enforce state invariants (i.e. if msh::Surface::state == mir_surface_state_minimized, the surface is hidden). Maybe this isn't a serious problem? In particular given appropriate locking on msh::Surface

[Robert Carr (robertcarr)]

I came up with another way to reduce the interface to one method.

First, we protect the state/type members of msh::Surface with a mutex (needs to happen anyway!).

Then we ensure the mutex is held while calling to SurfaceConfigurator

// Returns the configured value
int msh::SurfaceConfigurator::configure_surface(msh::Surface &, MirAttribute, int)

is enough as a single mutex. However, it seems reasonable that the SurfaceConfigurator might want to act like:

if (set_attribute == state)
   if (surface->type() == one_thing)
      // do_one_thing
   else
      // do_another

surface->type() is a deadlock though so this would require a recursive mutex.

[Robert Carr (robertcarr)]

s/is enough as a single mutex/is enough as a single function/

[Robert Carr (robertcarr)]

Another verbose but perhaps more self documenting alternative:

SurfaceAttributeSelector
{
int select_attribute(msh::Surface &, MirAttribute, int)
}

SurfaceAttributeApplicator
{
void apply_attribute(msh::Surface &, MirAttribute, int)
}

Presumably as they are passed together, they are wrapped in a composite type:

SurfaceConfigurator
{
SurfaceAttributeSelector& selector()
SurfaceAttributeApplicator& applicator()
}

[Kevin DuBois (kdub)]

> I came up with another way to reduce the interface to one method.
>
> First, we protect the state/type members of msh::Surface with a mutex (needs
> to happen anyway!).
>
> Then we ensure the mutex is held while calling to SurfaceConfigurator
>
this isn't present in this MP, but I'm not a fan of holding a mutex as a part of the interface requirements

[Kevin DuBois (kdub)]

I think this is OK as it is... although I do think its one of those MP's where what we have to get done (redirecting client requests to the shell to approve) speaks to some funny code structure or data models in msh::. Like, where does the shell keep all the information it owns about the surface stack? Seems like the shell will want to inject tentacles all over like this to extract and modify state, when if we restructured a few things so the appropriate data flowed into the shell and owned it, we wouldn't have to coordinate injecting shell permission objects all over. (my 2c)

Seems like there is some time pressure here, and I do not object to the code as it is so I feel ok +1'ing

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:892
http://jenkins.qa.ubuntu.com/job/mir-ci/1246/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-saucy-i386-build/1646
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-saucy-amd64-build/1531
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/484
        deb: http://jenkins.qa.ubuntu.com/job/mir-saucy-amd64-ci/484/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

=== modified file 'include/server/mir/default_server_configuration.h'
--- include/server/mir/default_server_configuration.h	2013-08-06 10:39:53 +0000
+++ include/server/mir/default_server_configuration.h	2013-08-07 20:06:30 +0000
@@ -65,6 +65,7 @@
 class PixelBuffer;
 class SnapshotStrategy;
 class DisplayLayout;
+class SurfaceConfigurator;
 class DisplayChanger;
 }
 namespace time
@@ -161,16 +162,18 @@
     /** @name shell configuration - customization
      * configurable interfaces for modifying shell
      *  @{ */
-    virtual std::shared_ptr<shell::SurfaceFactory>    the_shell_surface_factory();
-    virtual std::shared_ptr<shell::SessionContainer>  the_shell_session_container();
-    virtual std::shared_ptr<shell::FocusSetter>       the_shell_focus_setter();
-    virtual std::shared_ptr<shell::FocusSequence>     the_shell_focus_sequence();
-    virtual std::shared_ptr<shell::PlacementStrategy> the_shell_placement_strategy();
-    virtual std::shared_ptr<shell::SessionListener>   the_shell_session_listener();
-    virtual std::shared_ptr<shell::PixelBuffer>       the_shell_pixel_buffer();
-    virtual std::shared_ptr<shell::SnapshotStrategy>  the_shell_snapshot_strategy();
-    virtual std::shared_ptr<shell::DisplayLayout>     the_shell_display_layout();
-    virtual std::shared_ptr<shell::DisplayChanger>    the_shell_display_changer();
+    virtual std::shared_ptr<shell::SurfaceFactory>      the_shell_surface_factory();
+    virtual std::shared_ptr<shell::SessionContainer>    the_shell_session_container();
+    virtual std::shared_ptr<shell::FocusSetter>         the_shell_focus_setter();
+    virtual std::shared_ptr<shell::FocusSequence>       the_shell_focus_sequence();
+    virtual std::shared_ptr<shell::PlacementStrategy>   the_shell_placement_strategy();
+    virtual std::shared_ptr<shell::SessionListener>     the_shell_session_listener();
+    virtual std::shared_ptr<shell::PixelBuffer>         the_shell_pixel_buffer();
+    virtual std::shared_ptr<shell::SnapshotStrategy>    the_shell_snapshot_strategy();
+    virtual std::shared_ptr<shell::DisplayLayout>       the_shell_display_layout();
+    virtual std::shared_ptr<shell::DisplayChanger>      the_shell_display_changer();
+    virtual std::shared_ptr<shell::SurfaceConfigurator> the_shell_surface_configurator();
+
     /** @} */
 
     /** @name shell configuration - dependencies
@@ -260,6 +263,7 @@
     CachedPtr<shell::PixelBuffer>       shell_pixel_buffer;
     CachedPtr<shell::SnapshotStrategy>  shell_snapshot_strategy;
     CachedPtr<shell::DisplayLayout>     shell_display_layout;
+    CachedPtr<shell::SurfaceConfigurator> shell_surface_configurator;
     CachedPtr<shell::DisplayChanger>     shell_display_changer;
     CachedPtr<compositor::DisplayBufferCompositorFactory> display_buffer_compositor_factory;
     CachedPtr<compositor::OverlayRenderer> overlay_renderer;

=== modified file 'include/server/mir/shell/surface.h'
--- include/server/mir/shell/surface.h	2013-07-31 22:53:11 +0000
+++ include/server/mir/shell/surface.h	2013-08-07 20:06:30 +0000
@@ -39,8 +39,8 @@
 {
 class InputTargeter;
 class SurfaceBuilder;
+class SurfaceConfigurator;
 class SurfaceController;
-
 struct SurfaceCreationParameters;
 
 class Surface : public frontend::Surface, public shell::SurfaceBufferAccess
@@ -48,6 +48,7 @@
 public:
     Surface(
         std::shared_ptr<SurfaceBuilder> const& builder,
+        std::shared_ptr<SurfaceConfigurator> const& configurator,
         SurfaceCreationParameters const& params,
         frontend::SurfaceId id,
         std::shared_ptr<frontend::EventSink> const& event_sink);
@@ -93,6 +94,7 @@
     void notify_change(MirSurfaceAttrib attrib, int value);
 
     std::shared_ptr<SurfaceBuilder> const builder;
+    std::shared_ptr<SurfaceConfigurator> const configurator;
     std::weak_ptr<mir::surfaces::Surface> const surface;
 
     frontend::SurfaceId const id;

=== added file 'include/server/mir/shell/surface_configurator.h'
--- include/server/mir/shell/surface_configurator.h	1970-01-01 00:00:00 +0000
+++ include/server/mir/shell/surface_configurator.h	2013-08-07 20:06:30 +0000
@@ -0,0 +1,54 @@
+/*
+ * Copyright © 2013 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU 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 General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Robert Carr <[e-mail]>
+ */
+
+#ifndef MIR_SHELL_SURFACE_CONFIGURATOR_H_
+#define MIR_SHELL_SURFACE_CONFIGURATOR_H_
+
+#include "mir_toolkit/common.h"
+
+#include <memory>
+
+namespace mir
+{
+
+namespace shell
+{
+class Surface;
+
+class SurfaceConfigurator
+{
+public:
+    virtual ~SurfaceConfigurator() = default;
+    
+    /// Returns the selected value.
+    virtual int select_attribute_value(Surface const& surface, MirSurfaceAttrib attrib,
+                                       int requested_value) = 0;
+
+    virtual void attribute_set(Surface const& surface, MirSurfaceAttrib attrib,
+                               int new_value) = 0;
+
+protected:
+    SurfaceConfigurator() = default;
+    SurfaceConfigurator(SurfaceConfigurator const&) = delete;
+    SurfaceConfigurator& operator=(SurfaceConfigurator const&) = delete;
+};
+
+}
+} // namespace mir
+
+#endif // MIR_SHELL_SURFACE_CONFIGURATOR_H_

=== modified file 'include/server/mir/shell/surface_source.h'
--- include/server/mir/shell/surface_source.h	2013-08-01 06:55:26 +0000
+++ include/server/mir/shell/surface_source.h	2013-08-07 20:06:30 +0000
@@ -29,12 +29,13 @@
 namespace shell
 {
 class SurfaceBuilder;
-class SurfaceController;
+class SurfaceConfigurator;
 
 class SurfaceSource : public SurfaceFactory
 {
 public:
-    SurfaceSource(std::shared_ptr<SurfaceBuilder> const& surface_builder);
+    SurfaceSource(std::shared_ptr<SurfaceBuilder> const& surface_builder,
+                  std::shared_ptr<SurfaceConfigurator> const& surface_configurator);
     virtual ~SurfaceSource() {}
 
     std::shared_ptr<Surface> create_surface(
@@ -48,6 +49,7 @@
 
 private:
     std::shared_ptr<SurfaceBuilder> const surface_builder;
+    std::shared_ptr<SurfaceConfigurator> const surface_configurator;
 };
 
 }

=== modified file 'include/test/mir_test_doubles/mock_surface.h'
--- include/test/mir_test_doubles/mock_surface.h	2013-07-31 22:53:11 +0000
+++ include/test/mir_test_doubles/mock_surface.h	2013-08-07 20:06:30 +0000
@@ -23,6 +23,7 @@
 
 #include "mir/shell/surface_creation_parameters.h"
 #include "null_event_sink.h"
+#include "null_surface_configurator.h"
 
 #include <gmock/gmock.h>
 
@@ -38,7 +39,8 @@
 struct MockSurface : public shell::Surface
 {
     MockSurface(std::shared_ptr<shell::SurfaceBuilder> const& builder) :
-        shell::Surface(builder, shell::a_surface(), frontend::SurfaceId{}, std::make_shared<NullEventSink>())
+        shell::Surface(builder, std::make_shared<NullSurfaceConfigurator>(), shell::a_surface(), 
+            frontend::SurfaceId{}, std::make_shared<NullEventSink>())
     {
     }
 

=== added file 'include/test/mir_test_doubles/mock_surface_configurator.h'
--- include/test/mir_test_doubles/mock_surface_configurator.h	1970-01-01 00:00:00 +0000
+++ include/test/mir_test_doubles/mock_surface_configurator.h	2013-08-07 20:06:30 +0000
@@ -0,0 +1,43 @@
+/*
+ * Copyright © 2013 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU 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 General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Robert Carr <[e-mail]>
+ */
+
+#ifndef MIR_TEST_DOUBLES_MOCK_SURFACE_CONFIGURATOR_H_
+#define MIR_TEST_DOUBLES_MOCK_SURFACE_CONFIGURATOR_H_
+
+#include "mir/shell/surface_configurator.h"
+
+#include <gmock/gmock.h>
+
+namespace mir
+{
+namespace test
+{
+namespace doubles
+{
+
+struct MockSurfaceConfigurator : public shell::SurfaceConfigurator
+{
+    MOCK_METHOD3(select_attribute_value, int(shell::Surface const&, MirSurfaceAttrib, int));
+    MOCK_METHOD3(attribute_set, void(shell::Surface const&, MirSurfaceAttrib, int));
+};
+
+}
+}
+}
+
+#endif /* MIR_TEST_DOUBLES_MOCK_SURFACE_CONFIGURATOR_H_ */

=== added file 'include/test/mir_test_doubles/null_surface_configurator.h'
--- include/test/mir_test_doubles/null_surface_configurator.h	1970-01-01 00:00:00 +0000
+++ include/test/mir_test_doubles/null_surface_configurator.h	2013-08-07 20:06:30 +0000
@@ -0,0 +1,48 @@
+/*
+ * Copyright © 2013 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify it
+ * under the terms of the GNU 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 General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Robert Carr <[e-mail]>
+ */
+
+#ifndef MIR_TEST_DOUBLES_NULL_SURFACE_CONFIGURATOR_H_
+#define MIR_TEST_DOUBLES_NULL_SURFACE_CONFIGURATOR_H_
+
+#include "mir/shell/surface_configurator.h"
+
+namespace mir
+{
+namespace test
+{
+namespace doubles
+{
+
+struct NullSurfaceConfigurator : public shell::SurfaceConfigurator
+{
+    int select_attribute_value(shell::Surface const&,
+                               MirSurfaceAttrib, int requested_value)
+    {
+        return requested_value;
+    }
+    void attribute_set(shell::Surface const&,
+                       MirSurfaceAttrib, int)
+    {
+    }
+};
+
+}
+}
+}
+
+#endif /* MIR_TEST_DOUBLES_NULL_SURFACE_CONFIGURATOR_H_ */

=== modified file 'src/server/default_server_configuration.cpp'
--- src/server/default_server_configuration.cpp	2013-08-06 10:39:53 +0000
+++ src/server/default_server_configuration.cpp	2013-08-07 20:06:30 +0000
@@ -44,6 +44,7 @@
 #include "mir/shell/organising_surface_factory.h"
 #include "mir/shell/threaded_snapshot_strategy.h"
 #include "mir/shell/graphics_display_layout.h"
+#include "mir/shell/surface_configurator.h"
 #include "mir/graphics/cursor.h"
 #include "mir/shell/null_session_listener.h"
 #include "mir/graphics/display.h"
@@ -607,6 +608,25 @@
         });
 }
 
+std::shared_ptr<msh::SurfaceConfigurator> mir::DefaultServerConfiguration::the_shell_surface_configurator()
+{
+    struct DefaultSurfaceConfigurator : public msh::SurfaceConfigurator
+    {
+        int select_attribute_value(msh::Surface const&, MirSurfaceAttrib, int requested_value)
+        {
+            return requested_value;
+        }
+        void attribute_set(msh::Surface const&, MirSurfaceAttrib, int)
+        {
+        }
+    };
+    return shell_surface_configurator(
+        [this]()
+        {
+            return std::make_shared<DefaultSurfaceConfigurator>();
+        });
+}
+
 std::shared_ptr<ms::SurfaceStackModel>
 mir::DefaultServerConfiguration::the_surface_stack_model()
 {
@@ -642,7 +662,7 @@
         [this]()
         {
             auto surface_source = std::make_shared<msh::SurfaceSource>(
-                the_surface_builder());
+                the_surface_builder(), the_shell_surface_configurator());
 
             return std::make_shared<msh::OrganisingSurfaceFactory>(
                 surface_source,

=== modified file 'src/server/shell/application_session.cpp'
--- src/server/shell/application_session.cpp	2013-07-31 22:53:11 +0000
+++ src/server/shell/application_session.cpp	2013-08-07 20:06:30 +0000
@@ -152,10 +152,10 @@
 
 int msh::ApplicationSession::configure_surface(mf::SurfaceId id,
                                                MirSurfaceAttrib attrib,
-                                               int value)
+                                               int requested_value)
 {
     std::unique_lock<std::mutex> lock(surfaces_mutex);
-    std::shared_ptr<mf::Surface> surf(checked_find(id)->second);
+    std::shared_ptr<msh::Surface> surf(checked_find(id)->second);
 
-    return surf->configure(attrib, value);
+    return surf->configure(attrib, requested_value);
 }

=== modified file 'src/server/shell/surface.cpp'
--- src/server/shell/surface.cpp	2013-07-31 22:53:11 +0000
+++ src/server/shell/surface.cpp	2013-08-07 20:06:30 +0000
@@ -18,6 +18,7 @@
 
 #include "mir/shell/surface.h"
 #include "mir/shell/surface_builder.h"
+#include "mir/shell/surface_configurator.h"
 #include "mir/shell/surface_controller.h"
 #include "mir/shell/input_targeter.h"
 #include "mir/input/input_channel.h"
@@ -40,10 +41,12 @@
 
 msh::Surface::Surface(
     std::shared_ptr<SurfaceBuilder> const& builder,
+    std::shared_ptr<SurfaceConfigurator> const& configurator,
     shell::SurfaceCreationParameters const& params,
     frontend::SurfaceId id,
     std::shared_ptr<mf::EventSink> const& event_sink)
   : builder(builder),
+    configurator(configurator),
     surface(builder->create_surface(params)),
     id(id),
     event_sink(event_sink),
@@ -223,6 +226,7 @@
      * TODO: In future, query the shell implementation for the subset of
      *       attributes/types it implements.
      */
+    value = configurator->select_attribute_value(*this, attrib, value);
     switch (attrib)
     {
     case mir_surface_attrib_type:
@@ -248,6 +252,8 @@
         break;
     }
 
+    configurator->attribute_set(*this, attrib, result);
+
     return result;
 }
 

=== modified file 'src/server/shell/surface_source.cpp'
--- src/server/shell/surface_source.cpp	2013-08-01 06:55:26 +0000
+++ src/server/shell/surface_source.cpp	2013-08-07 20:06:30 +0000
@@ -30,10 +30,13 @@
 namespace mf = mir::frontend;
 
 
-msh::SurfaceSource::SurfaceSource(std::shared_ptr<SurfaceBuilder> const& surface_builder)
-    : surface_builder(surface_builder)
+msh::SurfaceSource::SurfaceSource(std::shared_ptr<SurfaceBuilder> const& surface_builder,
+                                  std::shared_ptr<SurfaceConfigurator> const& surface_configurator)
+    : surface_builder(surface_builder),
+      surface_configurator(surface_configurator)
 {
     assert(surface_builder);
+    assert(surface_configurator);
 }
 
 std::shared_ptr<msh::Surface> msh::SurfaceSource::create_surface(
@@ -41,6 +44,6 @@
     frontend::SurfaceId id,
     std::shared_ptr<mf::EventSink> const& sender)
 {
-    return std::make_shared<Surface>(surface_builder, params, id, sender);
+    return std::make_shared<Surface>(surface_builder, surface_configurator, params, id, sender);
 }
 

=== modified file 'tests/acceptance-tests/CMakeLists.txt'
--- tests/acceptance-tests/CMakeLists.txt	2013-08-06 07:20:02 +0000
+++ tests/acceptance-tests/CMakeLists.txt	2013-08-07 20:06:30 +0000
@@ -11,6 +11,7 @@
   test_focus_selection.cpp
   test_server_shutdown.cpp
   test_client_authorization.cpp
+  test_shell_control_of_surface_configuration.cpp
   test_nested_mir.cpp
 )
 

=== added file 'tests/acceptance-tests/test_shell_control_of_surface_configuration.cpp'
--- tests/acceptance-tests/test_shell_control_of_surface_configuration.cpp	1970-01-01 00:00:00 +0000
+++ tests/acceptance-tests/test_shell_control_of_surface_configuration.cpp	2013-08-07 20:06:30 +0000
@@ -0,0 +1,139 @@
+/*
+ * Copyright © 2013 Canonical Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU 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 General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Authored by: Robert Carr <[e-mail]>
+ */
+
+#include "mir/shell/surface_configurator.h"
+#include "mir/shell/surface.h"
+
+#include "mir_test/fake_shared.h"
+#include "mir_test_doubles/mock_surface_configurator.h"
+#include "mir_test_framework/display_server_test_fixture.h"
+
+#include "mir_toolkit/mir_client_library.h"
+
+#include <gtest/gtest.h>
+#include <gmock/gmock.h>
+
+namespace msh = mir::shell;
+
+namespace mt = mir::test;
+namespace mtd = mt::doubles;
+namespace mtf = mir_test_framework;
+
+namespace
+{
+    char const* const mir_test_socket = mtf::test_socket_file().c_str();
+}
+
+namespace
+{
+
+struct SurfaceCreatingClient : public mtf::TestingClientConfiguration
+{
+    void exec() override
+    {
+        connection = mir_connect_sync(mir_test_socket, __PRETTY_FUNCTION__);
+        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
+            };
+
+        surface = mir_connection_create_surface_sync(connection,
+                                                     &request_params);
+    }
+
+    MirConnection* connection;
+    MirSurface* surface;
+};
+
+}
+
+TEST_F(BespokeDisplayServerTestFixture, the_shell_surface_configurator_is_notified_of_attribute_changes)
+{
+    struct ServerConfiguration : TestingServerConfiguration
+    {
+        std::shared_ptr<msh::SurfaceConfigurator> the_shell_surface_configurator() override
+        {
+            return mt::fake_shared(mock_configurator);
+        }
+
+        void exec() override
+        {
+            using namespace ::testing;
+
+            ON_CALL(mock_configurator, select_attribute_value(_, _, _)).WillByDefault(Return(mir_surface_type_freestyle));
+            EXPECT_CALL(mock_configurator, select_attribute_value(_, mir_surface_attrib_type, Eq(mir_surface_type_freestyle))).Times(1);
+            EXPECT_CALL(mock_configurator, attribute_set(_, mir_surface_attrib_type, Eq(mir_surface_type_freestyle))).Times(1);
+        }
+
+        mtd::MockSurfaceConfigurator mock_configurator;
+    } server_config;
+    launch_server_process(server_config);
+
+    struct ClientConfiguration : SurfaceCreatingClient
+    {
+        void exec() override
+        {
+            SurfaceCreatingClient::exec();
+            mir_wait_for(mir_surface_set_type(surface,
+                                               mir_surface_type_freestyle));
+            EXPECT_EQ(mir_surface_type_freestyle,
+                      mir_surface_get_type(surface));
+        }
+    } client_config;
+    launch_client_process(client_config);
+}
+
+TEST_F(BespokeDisplayServerTestFixture, the_shell_surface_configurator_may_interfere_with_attribute_changes)
+{
+    struct ServerConfiguration : TestingServerConfiguration
+    {
+        std::shared_ptr<msh::SurfaceConfigurator> the_shell_surface_configurator() override
+        {
+            return mt::fake_shared(mock_configurator);
+        }
+        void exec() override
+        {
+            using namespace ::testing;
+            EXPECT_CALL(mock_configurator, select_attribute_value(_, mir_surface_attrib_type, Eq(mir_surface_type_freestyle))).Times(1)
+                .WillOnce(Return(mir_surface_type_normal));
+            EXPECT_CALL(mock_configurator, attribute_set(_, mir_surface_attrib_type, Eq(mir_surface_type_normal))).Times(1);
+        }
+        mtd::MockSurfaceConfigurator mock_configurator;
+    } server_config;
+    launch_server_process(server_config);
+
+    struct ClientConfiguration : SurfaceCreatingClient
+    {
+        void exec() override
+        {
+            SurfaceCreatingClient::exec();
+            mir_wait_for(mir_surface_set_type(surface,
+                                               mir_surface_type_freestyle));
+            EXPECT_EQ(mir_surface_type_normal,
+                      mir_surface_get_type(surface));
+        }
+    } client_config;
+    launch_client_process(client_config);
+}

=== modified file 'tests/integration-tests/graphics/android/test_internal_client.cpp'
--- tests/integration-tests/graphics/android/test_internal_client.cpp	2013-08-02 03:13:38 +0000
+++ tests/integration-tests/graphics/android/test_internal_client.cpp	2013-08-07 20:06:30 +0000
@@ -39,6 +39,7 @@
 #include "mir/options/program_option.h"
 
 #include "mir_test_doubles/stub_input_registrar.h"
+#include "mir_test_doubles/null_surface_configurator.h"
 
 #include <EGL/egl.h>
 #include <gtest/gtest.h>
@@ -110,7 +111,7 @@
     auto surface_allocator = std::make_shared<ms::SurfaceAllocator>(buffer_stream_factory, stub_input_factory);
     auto ss = std::make_shared<ms::SurfaceStack>(surface_allocator, stub_input_registrar);
     auto surface_controller = std::make_shared<ms::SurfaceController>(ss);
-    auto surface_source = std::make_shared<msh::SurfaceSource>(surface_controller);
+    auto surface_source = std::make_shared<msh::SurfaceSource>(surface_controller, std::make_shared<mtd::NullSurfaceConfigurator>());
     auto mir_surface = std::make_shared<ForwardingInternalSurface>(
         surface_source->create_surface(params, id, std::shared_ptr<mf::EventSink>()));
 

=== modified file 'tests/unit-tests/shell/test_application_session.cpp'
--- tests/unit-tests/shell/test_application_session.cpp	2013-07-31 22:53:11 +0000
+++ tests/unit-tests/shell/test_application_session.cpp	2013-08-07 20:06:30 +0000
@@ -72,7 +72,8 @@
 
     msh::ApplicationSession session(mt::fake_shared(surface_factory), "Foo",
                                     std::make_shared<mtd::NullSnapshotStrategy>(),
-                                    mt::fake_shared(listener), mt::fake_shared(sender));
+                                    mt::fake_shared(listener),
+                                    mt::fake_shared(sender));
 
     msh::SurfaceCreationParameters params;
     auto surf = session.create_surface(params);

=== modified file 'tests/unit-tests/shell/test_session_manager.cpp'
--- tests/unit-tests/shell/test_session_manager.cpp	2013-07-31 22:53:11 +0000
+++ tests/unit-tests/shell/test_session_manager.cpp	2013-08-07 20:06:30 +0000
@@ -35,6 +35,7 @@
 #include "mir_test_doubles/stub_surface_builder.h"
 #include "mir_test_doubles/stub_surface_controller.h"
 #include "mir_test_doubles/null_snapshot_strategy.h"
+#include "mir_test_doubles/null_surface_configurator.h"
 
 #include <gmock/gmock.h>
 #include <gtest/gtest.h>
@@ -113,7 +114,7 @@
 
     ON_CALL(surface_factory, create_surface(_, _, _)).WillByDefault(
        Return(std::make_shared<msh::Surface>(
-           mt::fake_shared(surface_builder),
+           mt::fake_shared(surface_builder), std::make_shared<mtd::NullSurfaceConfigurator>(),
            msh::a_surface(),mf::SurfaceId{}, std::shared_ptr<mf::EventSink>())));
 
 
@@ -148,7 +149,7 @@
     using namespace ::testing;
     ON_CALL(surface_factory, create_surface(_, _, _)).WillByDefault(
         Return(std::make_shared<msh::Surface>(
-           mt::fake_shared(surface_builder),
+           mt::fake_shared(surface_builder), std::make_shared<mtd::NullSurfaceConfigurator>(),
            msh::a_surface(),mf::SurfaceId{}, std::shared_ptr<mf::EventSink>())));
 
     // Once for session creation and once for surface creation

=== modified file 'tests/unit-tests/shell/test_surface.cpp'
--- tests/unit-tests/shell/test_surface.cpp	2013-07-31 22:53:11 +0000
+++ tests/unit-tests/shell/test_surface.cpp	2013-08-07 20:06:30 +0000
@@ -32,6 +32,8 @@
 #include "mir_test_doubles/stub_input_targeter.h"
 #include "mir_test_doubles/null_event_sink.h"
 #include "mir_test_doubles/mock_surface_state.h"
+#include "mir_test_doubles/null_surface_configurator.h"
+#include "mir_test_doubles/mock_surface_configurator.h"
 #include "mir_test/fake_shared.h"
 
 #include <stdexcept>
@@ -143,7 +145,7 @@
     EXPECT_CALL(surface_builder, destroy_surface(_)).Times(1);
 
     msh::Surface test(
-        mt::fake_shared(surface_builder),
+        mt::fake_shared(surface_builder), std::make_shared<mtd::NullSurfaceConfigurator>(),
         params, stub_id, stub_sender);
 }
 
@@ -161,7 +163,7 @@
 
     EXPECT_THROW({
         msh::Surface test(
-            mt::fake_shared(surface_builder),
+            mt::fake_shared(surface_builder), std::make_shared<mtd::NullSurfaceConfigurator>(),
             params, stub_id, stub_sender);
     }, std::runtime_error);
 }
@@ -176,7 +178,7 @@
     EXPECT_CALL(surface_builder, destroy_surface(_)).Times(0);
 
     msh::Surface test(
-            mt::fake_shared(surface_builder),
+            mt::fake_shared(surface_builder), std::make_shared<mtd::NullSurfaceConfigurator>(),
             msh::a_surface(), stub_id, stub_sender);
 
     Mock::VerifyAndClearExpectations(&test);
@@ -192,7 +194,7 @@
 TEST_F(ShellSurface, size_throw_behavior)
 {
     msh::Surface test(
-            mt::fake_shared(surface_builder),
+            mt::fake_shared(surface_builder), std::make_shared<mtd::NullSurfaceConfigurator>(),
             msh::a_surface(), stub_id, stub_sender);
 
     EXPECT_NO_THROW({
@@ -209,7 +211,7 @@
 TEST_F(ShellSurface, top_left_throw_behavior)
 {
     msh::Surface test(
-            mt::fake_shared(surface_builder),
+            mt::fake_shared(surface_builder), std::make_shared<mtd::NullSurfaceConfigurator>(),
             msh::a_surface(), stub_id, stub_sender);
 
     EXPECT_NO_THROW({
@@ -226,7 +228,7 @@
 TEST_F(ShellSurface, name_throw_behavior)
 {
     msh::Surface test(
-            mt::fake_shared(surface_builder),
+            mt::fake_shared(surface_builder), std::make_shared<mtd::NullSurfaceConfigurator>(),
             msh::a_surface(), stub_id, stub_sender);
 
     EXPECT_NO_THROW({
@@ -243,7 +245,7 @@
 TEST_F(ShellSurface, pixel_format_throw_behavior)
 {
     msh::Surface test(
-            mt::fake_shared(surface_builder),
+            mt::fake_shared(surface_builder), std::make_shared<mtd::NullSurfaceConfigurator>(),
             msh::a_surface(), stub_id, stub_sender);
 
     EXPECT_NO_THROW({
@@ -260,7 +262,7 @@
 TEST_F(ShellSurface, hide_throw_behavior)
 {
     msh::Surface test(
-            mt::fake_shared(surface_builder),
+            mt::fake_shared(surface_builder), std::make_shared<mtd::NullSurfaceConfigurator>(),
             msh::a_surface(), stub_id, stub_sender);
 
     EXPECT_NO_THROW({
@@ -277,7 +279,7 @@
 TEST_F(ShellSurface, show_throw_behavior)
 {
     msh::Surface test(
-            mt::fake_shared(surface_builder),
+            mt::fake_shared(surface_builder), std::make_shared<mtd::NullSurfaceConfigurator>(),
             msh::a_surface(), stub_id, stub_sender);
 
     EXPECT_NO_THROW({
@@ -294,7 +296,7 @@
 TEST_F(ShellSurface, destroy_throw_behavior)
 {
     msh::Surface test(
-            mt::fake_shared(surface_builder),
+            mt::fake_shared(surface_builder), std::make_shared<mtd::NullSurfaceConfigurator>(),
             msh::a_surface(), stub_id, stub_sender);
 
     EXPECT_NO_THROW({
@@ -311,7 +313,7 @@
 TEST_F(ShellSurface, force_request_to_complete_throw_behavior)
 {
     msh::Surface test(
-            mt::fake_shared(surface_builder),
+            mt::fake_shared(surface_builder), std::make_shared<mtd::NullSurfaceConfigurator>(),
             msh::a_surface(), stub_id, stub_sender);
 
     EXPECT_NO_THROW({
@@ -328,7 +330,7 @@
 TEST_F(ShellSurface, advance_client_buffer_throw_behavior)
 {
     msh::Surface test(
-            mt::fake_shared(surface_builder),
+            mt::fake_shared(surface_builder), std::make_shared<mtd::NullSurfaceConfigurator>(),
             msh::a_surface(), stub_id, stub_sender);
 
     EXPECT_NO_THROW({
@@ -345,7 +347,7 @@
 TEST_F(ShellSurface, input_fds_throw_behavior)
 {
     msh::Surface test(
-            mt::fake_shared(surface_builder),
+            mt::fake_shared(surface_builder), std::make_shared<mtd::NullSurfaceConfigurator>(),
             msh::a_surface(), stub_id, stub_sender);
 
     surface_builder.reset_surface();
@@ -360,7 +362,7 @@
     using namespace testing;
 
     msh::Surface surf(
-            mt::fake_shared(surface_builder),
+            mt::fake_shared(surface_builder), std::make_shared<mtd::NullSurfaceConfigurator>(),
             msh::a_surface(), stub_id, stub_sender);
 
     EXPECT_THROW({
@@ -373,8 +375,8 @@
     using namespace testing;
 
     msh::Surface surf(
-        mt::fake_shared(surface_builder),
-        msh::a_surface(), stub_id, stub_sender);
+            mt::fake_shared(surface_builder), std::make_shared<mtd::NullSurfaceConfigurator>(),
+            msh::a_surface(), stub_id, stub_sender);
 
     EXPECT_EQ(mir_surface_type_normal, surf.type());
 
@@ -407,7 +409,7 @@
     using namespace testing;
 
     msh::Surface surf(
-        mt::fake_shared(surface_builder),
+        mt::fake_shared(surface_builder), std::make_shared<mtd::NullSurfaceConfigurator>(),
         msh::a_surface(), stub_id, stub_sender);
 
     EXPECT_EQ(mir_surface_state_restored, surf.state());
@@ -436,12 +438,29 @@
     EXPECT_EQ(mir_surface_state_fullscreen, surf.state());
 }
 
+TEST_F(ShellSurface, configurator_selects_attribute_values)
+{
+    using namespace testing;
+    
+    mtd::MockSurfaceConfigurator configurator;
+    
+    EXPECT_CALL(configurator, select_attribute_value(_, mir_surface_attrib_state, mir_surface_state_restored)).Times(1)
+        .WillOnce(Return(mir_surface_state_minimized));
+    EXPECT_CALL(configurator, attribute_set(_, mir_surface_attrib_state, mir_surface_state_minimized)).Times(1);
+
+    msh::Surface surf(
+            mt::fake_shared(surface_builder), mt::fake_shared(configurator),
+            msh::a_surface(), stub_id, stub_sender);
+
+    EXPECT_EQ(mir_surface_state_minimized, surf.configure(mir_surface_attrib_state, mir_surface_state_restored));
+}
+
 TEST_F(ShellSurface, take_input_focus)
 {
     using namespace ::testing;
 
     msh::Surface test(
-        mt::fake_shared(surface_builder),
+        mt::fake_shared(surface_builder), std::make_shared<mtd::NullSurfaceConfigurator>(),
         msh::a_surface(), stub_id, stub_sender);
     
     mtd::MockInputTargeter targeter;
@@ -455,7 +474,7 @@
     using namespace ::testing;
 
     msh::Surface test(
-        mt::fake_shared(surface_builder),
+        mt::fake_shared(surface_builder), std::make_shared<mtd::NullSurfaceConfigurator>(),
         msh::a_surface(), stub_id, stub_sender);
     surface_builder.reset_surface();
 
@@ -471,7 +490,7 @@
     using namespace ::testing;
     
     msh::Surface test(
-        mt::fake_shared(surface_builder),
+        mt::fake_shared(surface_builder), std::make_shared<mtd::NullSurfaceConfigurator>(),
         msh::a_surface(), stub_id, stub_sender);
     
     EXPECT_NO_THROW({
@@ -488,7 +507,7 @@
 TEST_F(ShellSurface, with_most_recent_buffer_do_uses_compositor_buffer)
 {
     msh::Surface test(
-        mt::fake_shared(surface_builder),
+        mt::fake_shared(surface_builder), std::make_shared<mtd::NullSurfaceConfigurator>(),
         msh::a_surface(), stub_id, stub_sender);
 
     mg::Buffer* buf_ptr{nullptr};
@@ -509,7 +528,7 @@
 
     mtd::MockSurfaceController surface_controller;
     msh::Surface test(
-        mt::fake_shared(surface_builder),
+        mt::fake_shared(surface_builder), std::make_shared<mtd::NullSurfaceConfigurator>(),
         msh::a_surface(), stub_id, stub_sender);
     
     EXPECT_CALL(surface_controller, raise(_)).Times(1);