Merge lp:~vanvugt/mir/wire-up-SurfaceController into lp:mir

[Daniel van Vugt]

Wire up SurfaceController to incoming configure_surface messages from
clients so that it may intercept them and implement WM policy... or
call something else to implement some WM policy.

This is an independent activity to the managed-surface branch. Regardless
of your school of thought, everyone seems to have suggested we will need
this for something eventually. As SurfaceController will have a place in
inter-surface policy in the least.

This branch is something like 10%-20% of the infamous 'states' branch that got shelved.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2213
http://jenkins.qa.ubuntu.com/job/mir-ci/2629/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/820
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/820
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/782
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/626
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/626/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/782
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/782/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3907
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/17027

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

review: Approve (continuous-integration)

[Robert Carr (robertcarr)]

This is the "wrong surface controller" so to speak.

We can see that it doesn't quite work as a controller when we look at ApplicationSession::configure_surface...if ApplicationSession is part of the model why is it acting as a "Controller-Proxy" in this case.

I think we'd like to reduce Session and Surface to real model components, and thus extract the surface coordinator from their dependencies. A caller (e.g. frontend, shell) looking to change surface state would then directly invoke the surface controller themselves.

As previously mentioned this allows for extraction of the control hierarchy from the Session and Surface dependencies (e.g. they no longer need a view of the surface controller or scene, as the external surface controller is responsible for maintaining this relationship). Furthermore it allows for a split of the implementation responsibilities in session...which currently acts as a Factory, a Model, and somewhat of a controller (or at least a controller proxy).

Obviously this is an existing problem but there is work in progress to fix it and this branch seems to be a step in the wrong direction.

review: Disapprove

[Daniel van Vugt (vanvugt)]

Ironic this code was ripped from the branch you said you preferred the most.

Yeah, I can see we probably can remove ApplicationSession from the code path here. Although it's hard to make any progress if every reviewer wants us to fix the whole server architecture before landing anything else. We've been on that path for 2+ years.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2214
http://jenkins.qa.ubuntu.com/job/mir-ci/2641/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/844
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/844
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/806
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/638
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/638/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/806
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/806/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3926
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/17062

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

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

Apparently I forgot the details that led me to this design a couple of months ago.

Getting the SurfaceCoordinator (SurfaceController) isn't so much the problem. The issue is to use SurfaceCoordinator we want a scene::Surface. And this being the frontend, we only have frontend::Surface. So to avoid a dynamic_cast, I chose to go through ApplicationSession which is the only class that knows how to cleanly map a SurfaceId (from client messages) to scene::Surface for the SurfaceController/SurfaceCoordinator.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2220
http://jenkins.qa.ubuntu.com/job/mir-ci/2657/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/867
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/867
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/829
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/654
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/654/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/829
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/829/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/3946
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/17106

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

This is the "wrong surface controller" so to speak.

Obviously this is an existing problem but there is work in progress to fix it and this branch seems to be a step in the wrong direction.

review: Disapprove

[Daniel van Vugt (vanvugt)]

For the current implementation, it is correct surface controller...

143 +++ src/server/scene/surface_controller.h 2015-01-15 08:10:56 +0000
148 -/// Will grow up to provide synchronization of model updates

SurfaceController is the implementation of SurfaceCoordinator. There is only one surface controller interface in Mir right now, so if it's the wrong one then we're blocking waiting for another architectural overhaul that hasn't happened yet.

I appreciate Mir is not perfect, and we all want to make it better, but it's a slow journey if we keep disapproving enhancements on the basis of the foundations being wrong and needing to be replaced first. We've been repeatedly rewriting those same foundations for around 3+ years. That said, I do endorse any attempts to improve the foundations. But we can't keep blocking feature development when it's the same two people repeatedly rewriting the same foundations. It's a bit of a bottleneck for a team that's bigger than two people and most of us are asked to add new features.

As for the Session stuff above, yeah I agree that's not quite right yet. That could benefit from a further simplification of the Surface hierarchy.

[Daniel van Vugt (vanvugt)]

And now we have unresolvable conflicts (Alan's branch changes the same code).

Unmerged revisions

2220. By Daniel van Vugt on 2015-01-15

Merge latest trunk

2219. By Daniel van Vugt on 2015-01-15

Revert back to r2216, which at least works.

The problem is only ApplicationSession knows how to convert a SurfaceId
into a scene::Surface to pass to SurfaceCoordinator/SurfaceController.
So we go through ApplicationSession.

2218. By Daniel van Vugt on 2015-01-15

More work in progress.

Still doesn't build because the frontend doesn't (and shouldn't?) know how
to convert a frontend::Surface into a scene::Surface for SurfaceController.

2217. By Daniel van Vugt on 2015-01-15

Start removing Session from the Controller path

2216. By Daniel van Vugt on 2015-01-15

Merge latest trunk

2215. By Daniel van Vugt on 2015-01-15

Merge latest trunk

2214. By Daniel van Vugt on 2015-01-14

Merge latest trunk and fix a conflict.

2213. By Daniel van Vugt on 2015-01-13

Update sha1sums

2212. By Daniel van Vugt on 2015-01-13

Backport SurfaceController wiring from the 'states' branch, with all
WM code removed.

=== modified file 'include/server/mir/frontend/session.h'
--- include/server/mir/frontend/session.h	2014-12-28 08:33:45 +0000
+++ include/server/mir/frontend/session.h	2015-01-15 08:10:56 +0000
@@ -49,6 +49,7 @@
     virtual SurfaceId create_surface(scene::SurfaceCreationParameters const& params) = 0;
     virtual void destroy_surface(SurfaceId surface) = 0;
     virtual std::shared_ptr<Surface> get_surface(SurfaceId surface) const = 0;
+    virtual int configure_surface(SurfaceId, MirSurfaceAttrib, int) = 0;
 
     virtual std::string name() const = 0;
 

=== modified file 'include/server/mir/scene/surface_coordinator.h'
--- include/server/mir/scene/surface_coordinator.h	2014-12-28 08:33:45 +0000
+++ include/server/mir/scene/surface_coordinator.h	2015-01-15 08:10:56 +0000
@@ -20,6 +20,7 @@
 #ifndef MIR_SCENE_SURFACE_COORDINATOR_H_
 #define MIR_SCENE_SURFACE_COORDINATOR_H_
 
+#include "mir_toolkit/common.h"
 #include <memory>
 
 namespace mir
@@ -39,7 +40,7 @@
         Session* session) = 0;
 
     virtual void raise(std::weak_ptr<Surface> const& surface) = 0;
-
+    virtual int configure_surface(Surface&, MirSurfaceAttrib, int) = 0;
     virtual void remove_surface(std::weak_ptr<Surface> const& surface) = 0;
 protected:
     SurfaceCoordinator() = default;

=== modified file 'include/server/mir/shell/surface_coordinator_wrapper.h'
--- include/server/mir/shell/surface_coordinator_wrapper.h	2014-05-23 10:00:46 +0000
+++ include/server/mir/shell/surface_coordinator_wrapper.h	2015-01-15 08:10:56 +0000
@@ -35,7 +35,7 @@
         scene::Session* session) override;
 
     void raise(std::weak_ptr<scene::Surface> const& surface) override;
-
+    int configure_surface(scene::Surface&, MirSurfaceAttrib, int) override;
     void remove_surface(std::weak_ptr<scene::Surface> const& surface) override;
 
 protected:

=== modified file 'server-ABI-sha1sums'
--- server-ABI-sha1sums	2015-01-14 12:48:51 +0000
+++ server-ABI-sha1sums	2015-01-15 08:10:56 +0000
@@ -62,7 +62,7 @@
 59d6703def2a0ff840e5eb75390a5af76fd93f58  include/server/mir/frontend/prompt_session.h
 b729a7710c37d9f1fba56a6f8e8eae1c2559f57a  include/server/mir/frontend/session_authorizer.h
 34ce482df448fd2fc5f0c4ae5ac8b7fecbd228c9  include/server/mir/frontend/session_credentials.h
-e5ea465ed7e05f0e1d6a837d0d6b3a04c2d7fc19  include/server/mir/frontend/session.h
+27a64aaf03b9df6dd2a0013d36b33e65142e628b  include/server/mir/frontend/session.h
 d754e5bb057f3018d7228170cf9a0c66d75d8cc2  include/server/mir/frontend/session_mediator_report.h
 68468aa2298c4e2cdc1bbb7cb5f250a914ae16c9  include/server/mir/frontend/shell.h
 1b55b05c39f6b38782cd333a1726d1fa20af6cef  include/server/mir/frontend/surface.h
@@ -95,7 +95,7 @@
 7f5f26000fd2312373817d05c488a6a950a47c82  include/server/mir/scene/snapshot.h
 5bab4dc0a6488b8b9d47e958c6bab94f1dcf2c57  include/server/mir/scene/surface_buffer_access.h
 bbc9e2e2bb71634cd6f1c5c0430093e10e74fa23  include/server/mir/scene/surface_configurator.h
-e5e4dd7bcaf186810043fa0f05be42d7e49b0843  include/server/mir/scene/surface_coordinator.h
+8cd7f2588cf721e578fbb8a7c3bf6f7ba40f2fd9  include/server/mir/scene/surface_coordinator.h
 8e3636b736700d72520709c8a61f5e85d9231d4c  include/server/mir/scene/surface_creation_parameters.h
 7da48dbedb4430ce322a50ef8f4e93ce08bdf147  include/server/mir/scene/surface.h
 587e22d751656ce2d9536afdf5659276ff9bbc46  include/server/mir/scene/surface_observer.h
@@ -108,7 +108,7 @@
 f8d415af54b4a477338fb9dbe20db01aa4544029  include/server/mir/shell/host_lifecycle_event_listener.h
 4f50c37bb8e36a1aa4918af6aa01b0f032ed0984  include/server/mir/shell/input_targeter.h
 de7d7ad64da45b4c5dedef8eaa1d720c9f307f4c  include/server/mir/shell/session_coordinator_wrapper.h
-18e869c5b87ab8be4bc185d2cd55a93d699cde72  include/server/mir/shell/surface_coordinator_wrapper.h
+3c75fa2a78674029d64969e7855d10f668937bcc  include/server/mir/shell/surface_coordinator_wrapper.h
 114cc8c0b12ee4fd78d86c8f3309546e4dc68c9f  include/server/mir/terminate_with_current_exception.h
 1b8c2a763c8f5f00b737c3a187db2304676f8076  include/server/mir/time/alarm.h
 a134c7b86e9c3b191f1004abe7bbdceed8ac42ee  include/server/mir/time/timer.h

=== modified file 'src/server/frontend/session_mediator.cpp'
--- src/server/frontend/session_mediator.cpp	2015-01-14 02:35:56 +0000
+++ src/server/frontend/session_mediator.cpp	2015-01-15 08:10:56 +0000
@@ -388,8 +388,7 @@
 
         auto const id = mf::SurfaceId(request->surfaceid().value());
         int value = request->ivalue();
-        auto const surface = session->get_surface(id);
-        int newvalue = surface->configure(attrib, value);
+        int newvalue = session->configure_surface(id, attrib, value);
 
         response->set_ivalue(newvalue);
     }

=== modified file 'src/server/scene/application_session.cpp'
--- src/server/scene/application_session.cpp	2014-12-28 08:33:45 +0000
+++ src/server/scene/application_session.cpp	2015-01-15 08:10:56 +0000
@@ -115,6 +115,15 @@
     return checked_find(id)->second;
 }
 
+int ms::ApplicationSession::configure_surface(mf::SurfaceId surface_id,
+                                              MirSurfaceAttrib attrib,
+                                              int value)
+{
+    std::unique_lock<std::mutex> lock(surfaces_mutex);
+    auto surface = checked_find(surface_id)->second;
+    return surface_coordinator->configure_surface(*surface, attrib, value);
+}
+
 void ms::ApplicationSession::take_snapshot(SnapshotCallback const& snapshot_taken)
 {
     if (auto surface = default_surface())

=== modified file 'src/server/scene/application_session.h'
--- src/server/scene/application_session.h	2014-12-28 08:33:45 +0000
+++ src/server/scene/application_session.h	2015-01-15 08:10:56 +0000
@@ -55,6 +55,7 @@
     frontend::SurfaceId create_surface(SurfaceCreationParameters const& params) override;
     void destroy_surface(frontend::SurfaceId surface) override;
     std::shared_ptr<frontend::Surface> get_surface(frontend::SurfaceId surface) const override;
+    int configure_surface(frontend::SurfaceId, MirSurfaceAttrib, int) override;
 
     void take_snapshot(SnapshotCallback const& snapshot_taken) override;
     std::shared_ptr<Surface> default_surface() const override;

=== modified file 'src/server/scene/surface_controller.cpp'
--- src/server/scene/surface_controller.cpp	2014-12-28 08:33:45 +0000
+++ src/server/scene/surface_controller.cpp	2015-01-15 08:10:56 +0000
@@ -54,3 +54,12 @@
 {
     surface_stack->raise(surface);
 }
+
+int ms::SurfaceController::configure_surface(Surface& surface,
+                                             MirSurfaceAttrib attrib,
+                                             int value)
+{
+    // TODO: Impose some policy here
+    return surface.configure(attrib, value);
+}
+

=== modified file 'src/server/scene/surface_controller.h'
--- src/server/scene/surface_controller.h	2014-12-28 08:33:45 +0000
+++ src/server/scene/surface_controller.h	2015-01-15 08:10:56 +0000
@@ -30,7 +30,7 @@
 class SurfaceStackModel;
 class SurfaceFactory;
 
-/// Will grow up to provide synchronization of model updates
+/// Will soon grow up to provide synchronization of model updates
 class SurfaceController : public SurfaceCoordinator
 {
 public:
@@ -47,6 +47,8 @@
 
     void raise(std::weak_ptr<Surface> const& surface) override;
 
+    int configure_surface(Surface&, MirSurfaceAttrib, int) override;
+
 private:
     std::shared_ptr<SurfaceFactory> const surface_factory;
     std::shared_ptr<PlacementStrategy> const placement_strategy;

=== modified file 'src/server/shell/surface_coordinator_wrapper.cpp'
--- src/server/shell/surface_coordinator_wrapper.cpp	2014-05-23 10:00:46 +0000
+++ src/server/shell/surface_coordinator_wrapper.cpp	2015-01-15 08:10:56 +0000
@@ -44,3 +44,10 @@
 {
     wrapped->remove_surface(surface);
 }
+
+int msh::SurfaceCoordinatorWrapper::configure_surface(ms::Surface& surface,
+                                                      MirSurfaceAttrib attrib,
+                                                      int value)
+{
+    return wrapped->configure_surface(surface, attrib, value);
+}

=== modified file 'src/server/symbols.map'
--- src/server/symbols.map	2015-01-15 07:52:30 +0000
+++ src/server/symbols.map	2015-01-15 08:10:56 +0000
@@ -498,6 +498,7 @@
     mir::shell::SessionCoordinatorWrapper::stop_prompt_session*;
     mir::shell::SurfaceCoordinatorWrapper::add_surface*;
     mir::shell::SurfaceCoordinatorWrapper::raise*;
+    mir::shell::SurfaceCoordinatorWrapper::configure_surface*;
     mir::shell::SurfaceCoordinatorWrapper::remove_surface*;
     mir::shell::SurfaceCoordinatorWrapper::SurfaceCoordinatorWrapper*;
     mir::terminate_with_current_exception*;

=== modified file 'tests/include/mir_test_doubles/mock_surface_coordinator.h'
--- tests/include/mir_test_doubles/mock_surface_coordinator.h	2014-12-28 08:33:45 +0000
+++ tests/include/mir_test_doubles/mock_surface_coordinator.h	2015-01-15 08:10:56 +0000
@@ -34,7 +34,8 @@
 struct MockSurfaceCoordinator : public scene::SurfaceCoordinator
 {
     MOCK_METHOD1(raise, void(std::weak_ptr<scene::Surface> const&));
-
+    MOCK_METHOD3(configure_surface,
+                 int(scene::Surface&, MirSurfaceAttrib, int));
     MOCK_METHOD2(add_surface, std::shared_ptr<scene::Surface>(
         scene::SurfaceCreationParameters const& params,
         scene::Session* session));

=== modified file 'tests/include/mir_test_doubles/stub_scene_session.h'
--- tests/include/mir_test_doubles/stub_scene_session.h	2014-12-28 08:33:45 +0000
+++ tests/include/mir_test_doubles/stub_scene_session.h	2015-01-15 08:10:56 +0000
@@ -43,6 +43,11 @@
     {
         return std::shared_ptr<frontend::Surface>();
     }
+    int configure_surface(frontend::SurfaceId, MirSurfaceAttrib,
+                          int value) override
+    {
+        return value;
+    }
     std::string name() const override
     {
         return std::string();

=== modified file 'tests/include/mir_test_doubles/stub_session.h'
--- tests/include/mir_test_doubles/stub_session.h	2014-12-28 08:33:45 +0000
+++ tests/include/mir_test_doubles/stub_session.h	2015-01-15 08:10:56 +0000
@@ -41,6 +41,11 @@
     {
         return std::shared_ptr<frontend::Surface>();
     }
+    int configure_surface(frontend::SurfaceId, MirSurfaceAttrib,
+                          int value) override
+    {
+        return value;
+    }
     std::string name() const override
     {
         return std::string();

=== modified file 'tests/unit-tests/scene/test_application_session.cpp'
--- tests/unit-tests/scene/test_application_session.cpp	2014-12-28 08:33:45 +0000
+++ tests/unit-tests/scene/test_application_session.cpp	2015-01-15 08:10:56 +0000
@@ -89,6 +89,10 @@
     void remove_surface(std::weak_ptr<ms::Surface> const&) override
     {
     }
+    int configure_surface(ms::Surface&, MirSurfaceAttrib, int val) override
+    {
+        return val;
+    }
 };
 
 struct ApplicationSession : public testing::Test