Merge lp:~robert-ancell/mir/abstract-session-container into lp:~mir-team/mir/trunk

[Robert Ancell]

Make mir::shell::SessionContainer an abstract class with a default implementation in DefaultSessionManager

Make mir::shell::SessionContainer an abstract class with a default implementation in DefaultSessionManager - the current class isn't useful if you want to access the session list in dynamic ways not present in the interface. The convention in Mir seems to be for the class to be abstract and allow the shell to implement this class if it needs more complex behaviour.

[Kevin DuBois (kdub)]

well, looks good to me.

83 + virtual ~SessionContainer() {};
could be virtual ~SessionContainer() = default; ,which gives the stronger noexcept guarantee. (some of our classes have this, some don't but I think we're moving more towards this convention)

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:600
http://jenkins.qa.ubuntu.com/job/mir-ci/418/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/426
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/309
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/423
        deb: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/423/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Robert Ancell (robert-ancell)]

> well, looks good to me.
>
> 83 + virtual ~SessionContainer() {};
> could be virtual ~SessionContainer() = default; ,which gives the stronger
> noexcept guarantee. (some of our classes have this, some don't but I think
> we're moving more towards this convention)

I copied ServerConfiguration here in doing it this way. When I originally had the default destructor it stops the mock SessionContainer classes from working (not sure why).

[Robert Ancell (robert-ancell)]

I managed to use the noexcept destructor - it required a change from lp:~kdub/mir/nicemock-improvements ⇒ lp:mir.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:601
http://jenkins.qa.ubuntu.com/job/mir-ci/425/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-raring-i386-build/437
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-raring-amd64-build/320
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/430
        deb: http://jenkins.qa.ubuntu.com/job/mir-quantal-amd64-ci/430/artifact/work/output/*zip*/output.zip

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

Intent is reasonable - although it would be good to have a use-case/acceptance test for "if you want to access the session list in dynamic ways not present in the interface".

There's an unnecessary header:

122 #include "mir/shell/session_container.h"

My doubts about the change from lp:~kdub/mir/nicemock-improvements are expressed there. (Shouldn't that be a prerequisite?)

review: Abstain

=== modified file '3rd_party/gmock-1.6.0/include/gmock/gmock-generated-nice-strict.h'
--- 3rd_party/gmock-1.6.0/include/gmock/gmock-generated-nice-strict.h	2012-08-08 08:07:55 +0000
+++ 3rd_party/gmock-1.6.0/include/gmock/gmock-generated-nice-strict.h	2013-04-18 08:57:29 +0000
@@ -151,7 +151,7 @@
         internal::ImplicitCast_<MockClass*>(this));
   }
 
-  virtual ~NiceMock() {
+  virtual ~NiceMock() noexcept {
     ::testing::Mock::UnregisterCallReaction(
         internal::ImplicitCast_<MockClass*>(this));
   }
@@ -245,7 +245,7 @@
         internal::ImplicitCast_<MockClass*>(this));
   }
 
-  virtual ~StrictMock() {
+  virtual ~StrictMock() noexcept {
     ::testing::Mock::UnregisterCallReaction(
         internal::ImplicitCast_<MockClass*>(this));
   }

=== added file 'include/server/mir/shell/default_session_container.h'
--- include/server/mir/shell/default_session_container.h	1970-01-01 00:00:00 +0000
+++ include/server/mir/shell/default_session_container.h	2013-04-18 08:57:29 +0000
@@ -0,0 +1,54 @@
+/*
+ * Copyright © 2012 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_DEFAULT_SESSION_CONTAINER_H_
+#define MIR_SHELL_DEFAULT_SESSION_CONTAINER_H_
+
+#include <vector>
+#include <memory>
+#include <mutex>
+
+#include "mir/shell/session_container.h"
+
+namespace mir
+{
+namespace frontend
+{
+class Session;
+}
+
+namespace shell
+{
+
+class DefaultSessionContainer : public SessionContainer
+{
+public:
+    void insert_session(std::shared_ptr<frontend::Session> const& session);
+    void remove_session(std::shared_ptr<frontend::Session> const& session);
+    void for_each(std::function<void(std::shared_ptr<frontend::Session> const&)> f) const;
+
+private:
+    std::vector<std::shared_ptr<frontend::Session>> apps;
+    mutable std::mutex guard;
+};
+
+}
+}
+
+
+#endif // MIR_SHELL_DEFAULT_SESSION_CONTAINER_H_

=== modified file 'include/server/mir/shell/session_container.h'
--- include/server/mir/shell/session_container.h	2013-04-16 09:08:29 +0000
+++ include/server/mir/shell/session_container.h	2013-04-18 08:57:29 +0000
@@ -36,20 +36,17 @@
 class SessionContainer
 {
 public:
-    SessionContainer();
-    ~SessionContainer();
-
-    virtual void insert_session(std::shared_ptr<frontend::Session> const& session);
-    virtual void remove_session(std::shared_ptr<frontend::Session> const& session);
-
-    void for_each(std::function<void(std::shared_ptr<frontend::Session> const&)> f) const;
-
-private:
+    virtual void insert_session(std::shared_ptr<frontend::Session> const& session) = 0;
+    virtual void remove_session(std::shared_ptr<frontend::Session> const& session) = 0;
+
+    virtual void for_each(std::function<void(std::shared_ptr<frontend::Session> const&)> f) const = 0;
+
+protected:
+    SessionContainer() = default;
+    virtual ~SessionContainer() = default;
+
     SessionContainer(const SessionContainer&) = delete;
     SessionContainer& operator=(const SessionContainer&) = delete;
-
-    std::vector<std::shared_ptr<frontend::Session>> apps;
-    mutable std::mutex guard;
 };
 
 }

=== modified file 'src/server/default_server_configuration.cpp'
--- src/server/default_server_configuration.cpp	2013-04-17 16:57:53 +0000
+++ src/server/default_server_configuration.cpp	2013-04-18 08:57:29 +0000
@@ -36,6 +36,7 @@
 #include "mir/shell/registration_order_focus_sequence.h"
 #include "mir/shell/single_visibility_focus_mechanism.h"
 #include "mir/shell/session_container.h"
+#include "mir/shell/default_session_container.h"
 #include "mir/shell/consuming_placement_strategy.h"
 #include "mir/shell/organising_surface_factory.h"
 #include "mir/graphics/display.h"
@@ -300,7 +301,7 @@
 mir::DefaultServerConfiguration::the_shell_session_container()
 {
     return shell_session_container(
-        []{ return std::make_shared<msh::SessionContainer>(); });
+        []{ return std::make_shared<msh::DefaultSessionContainer>(); });
 }
 
 std::shared_ptr<msh::FocusSetter>

=== modified file 'src/server/shell/CMakeLists.txt'
--- src/server/shell/CMakeLists.txt	2013-03-21 03:32:59 +0000
+++ src/server/shell/CMakeLists.txt	2013-04-18 08:57:29 +0000
@@ -2,7 +2,7 @@
   SHELL_SOURCES
 
   application_session.cpp
-  session_container.cpp
+  default_session_container.cpp
   session_manager.cpp
   registration_order_focus_sequence.cpp
   single_visibility_focus_mechanism.cpp

=== renamed file 'src/server/shell/session_container.cpp' => 'src/server/shell/default_session_container.cpp'
--- src/server/shell/session_container.cpp	2013-04-16 09:08:29 +0000
+++ src/server/shell/default_session_container.cpp	2013-04-18 08:57:29 +0000
@@ -16,7 +16,7 @@
  * Authored By: Robert Carr <[e-mail]>
  */
 
-#include "mir/shell/session_container.h"
+#include "mir/shell/default_session_container.h"
 #include "mir/frontend/session.h"
 
 #include <boost/throw_exception.hpp>
@@ -26,27 +26,17 @@
 #include <algorithm>
 #include <stdexcept>
 
-
 namespace mf = mir::frontend;
 namespace msh = mir::shell;
 
-msh::SessionContainer::SessionContainer()
-{
-
-}
-
-msh::SessionContainer::~SessionContainer()
-{
-}
-
-void msh::SessionContainer::insert_session(std::shared_ptr<mf::Session> const& session)
+void msh::DefaultSessionContainer::insert_session(std::shared_ptr<mf::Session> const& session)
 {
     std::unique_lock<std::mutex> lk(guard);
 
     apps.push_back(session);
 }
 
-void msh::SessionContainer::remove_session(std::shared_ptr<mf::Session> const& session)
+void msh::DefaultSessionContainer::remove_session(std::shared_ptr<mf::Session> const& session)
 {
     std::unique_lock<std::mutex> lk(guard);
 
@@ -61,7 +51,7 @@
     }
 }
 
-void msh::SessionContainer::for_each(std::function<void(std::shared_ptr<mf::Session> const&)> f) const
+void msh::DefaultSessionContainer::for_each(std::function<void(std::shared_ptr<mf::Session> const&)> f) const
 {
     std::unique_lock<std::mutex> lk(guard);
 

=== modified file 'tests/integration-tests/shell/test_session_manager.cpp'
--- tests/integration-tests/shell/test_session_manager.cpp	2013-03-22 16:41:59 +0000
+++ tests/integration-tests/shell/test_session_manager.cpp	2013-04-18 08:57:29 +0000
@@ -21,7 +21,7 @@
 #include "mir/shell/focus_sequence.h"
 #include "mir/shell/focus_setter.h"
 #include "mir/shell/registration_order_focus_sequence.h"
-#include "mir/shell/session_container.h"
+#include "mir/shell/default_session_container.h"
 #include "mir/surfaces/buffer_bundle.h"
 #include "mir/surfaces/surface.h"
 #include "mir/compositor/buffer_swapper.h"
@@ -45,7 +45,7 @@
 {
     using namespace ::testing;
     mtd::MockSurfaceFactory surface_factory;
-    std::shared_ptr<msh::SessionContainer> container(new msh::SessionContainer());
+    std::shared_ptr<msh::DefaultSessionContainer> container(new msh::DefaultSessionContainer());
     msh::RegistrationOrderFocusSequence sequence(container);
     mtd::MockFocusSetter focus_changer;
     std::shared_ptr<mf::Session> new_session;
@@ -78,7 +78,7 @@
 {
     using namespace ::testing;
     mtd::MockSurfaceFactory surface_factory;
-    std::shared_ptr<msh::SessionContainer> model(new msh::SessionContainer());
+    std::shared_ptr<msh::DefaultSessionContainer> model(new msh::DefaultSessionContainer());
     msh::RegistrationOrderFocusSequence sequence(model);
     mtd::MockFocusSetter focus_changer;
     std::shared_ptr<mf::Session> new_session;

=== modified file 'tests/unit-tests/shell/test_registration_order_focus_sequence.cpp'
--- tests/unit-tests/shell/test_registration_order_focus_sequence.cpp	2013-03-29 16:51:35 +0000
+++ tests/unit-tests/shell/test_registration_order_focus_sequence.cpp	2013-04-18 08:57:29 +0000
@@ -18,7 +18,7 @@
 
 #include "mir/surfaces/buffer_bundle.h"
 #include "mir/shell/application_session.h"
-#include "mir/shell/session_container.h"
+#include "mir/shell/default_session_container.h"
 #include "mir/shell/registration_order_focus_sequence.h"
 #include "mir/frontend/surface_creation_parameters.h"
 #include "mir/surfaces/surface.h"
@@ -41,10 +41,10 @@
     void SetUp()
     {
         factory = std::make_shared<mtd::MockSurfaceFactory>();
-        container = std::make_shared<msh::SessionContainer>();
+        container = std::make_shared<msh::DefaultSessionContainer>();
     }
     std::shared_ptr<mtd::MockSurfaceFactory> factory;
-    std::shared_ptr<msh::SessionContainer> container;
+    std::shared_ptr<msh::DefaultSessionContainer> container;
 
     static std::string const testing_app_name1;
     static std::string const testing_app_name2;

=== modified file 'tests/unit-tests/shell/test_session_manager.cpp'
--- tests/unit-tests/shell/test_session_manager.cpp	2013-04-01 16:11:03 +0000
+++ tests/unit-tests/shell/test_session_manager.cpp	2013-04-18 08:57:29 +0000
@@ -19,7 +19,7 @@
 #include "mir/surfaces/buffer_bundle.h"
 #include "mir/shell/focus_sequence.h"
 #include "mir/shell/session_manager.h"
-#include "mir/shell/session_container.h"
+#include "mir/shell/default_session_container.h"
 #include "mir/shell/session.h"
 #include "mir/frontend/session.h"
 #include "mir/frontend/surface_creation_parameters.h"
@@ -48,12 +48,13 @@
 
 namespace
 {
-struct MockSessionContainer : public msh::SessionContainer
+struct MockSessionContainer : public msh::DefaultSessionContainer
 {
     MOCK_METHOD1(insert_session, void(std::shared_ptr<mf::Session> const&));
     MOCK_METHOD1(remove_session, void(std::shared_ptr<mf::Session> const&));
     MOCK_METHOD0(lock, void());
     MOCK_METHOD0(unlock, void());
+    ~MockSessionContainer() noexcept {}
 };
 
 struct MockFocusSequence: public msh::FocusSequence

=== modified file 'tests/unit-tests/shell/test_single_visibility_focus_mechanism.cpp'
--- tests/unit-tests/shell/test_single_visibility_focus_mechanism.cpp	2013-04-16 17:42:26 +0000
+++ tests/unit-tests/shell/test_single_visibility_focus_mechanism.cpp	2013-04-18 08:57:29 +0000
@@ -18,7 +18,7 @@
 
 #include "mir/surfaces/buffer_bundle.h"
 #include "mir/shell/application_session.h"
-#include "mir/shell/session_container.h"
+#include "mir/shell/default_session_container.h"
 #include "mir/shell/registration_order_focus_sequence.h"
 #include "mir/shell/single_visibility_focus_mechanism.h"
 #include "mir/shell/session.h"
@@ -67,7 +67,7 @@
     NiceMock<mtd::MockInputFocusSelector> input_focus_selector;
 
     MockShellSession app1, app2, app3;
-    msh::SessionContainer model;
+    msh::DefaultSessionContainer model;
 
     ON_CALL(app1, default_surface()).WillByDefault(Return(std::shared_ptr<msh::Surface>()));
     ON_CALL(app2, default_surface()).WillByDefault(Return(std::shared_ptr<msh::Surface>()));
@@ -95,7 +95,7 @@
     using namespace ::testing;
 
     mtd::MockInputFocusSelector input_focus_selector;
-    msh::SessionContainer model;
+    msh::DefaultSessionContainer model;
     auto session = std::make_shared<MockShellSession>();
     auto surface = std::make_shared<mtd::MockSurface>(std::make_shared<mtd::StubSurfaceBuilder>());
 

=== modified file 'tests/unit-tests/shell/test_the_session_container_implementation.cpp'
--- tests/unit-tests/shell/test_the_session_container_implementation.cpp	2013-03-22 16:41:59 +0000
+++ tests/unit-tests/shell/test_the_session_container_implementation.cpp	2013-04-18 08:57:29 +0000
@@ -18,7 +18,7 @@
 
 #include "mir/surfaces/buffer_bundle.h"
 #include "mir/shell/application_session.h"
-#include "mir/shell/session_container.h"
+#include "mir/shell/default_session_container.h"
 #include "mir/frontend/surface_creation_parameters.h"
 #include "mir/surfaces/surface.h"
 #include "mir_test_doubles/mock_buffer_bundle.h"
@@ -32,11 +32,11 @@
 namespace msh = mir::shell;
 namespace mtd = mir::test::doubles;
 
-TEST(SessionContainer, for_each)
+TEST(DefaultSessionContainer, for_each)
 {
     using namespace ::testing;
     auto factory = std::make_shared<mtd::MockSurfaceFactory>();
-    msh::SessionContainer container;
+    msh::DefaultSessionContainer container;
 
     container.insert_session(std::make_shared<msh::ApplicationSession>(factory, "Visual Studio 7"));
     container.insert_session(std::make_shared<msh::ApplicationSession>(factory, "Visual Studio 8"));
@@ -58,11 +58,11 @@
     container.for_each(std::ref(functor));
 }
 
-TEST(SessionContainer, invalid_session_throw_behavior)
+TEST(DefaultSessionContainer, invalid_session_throw_behavior)
 {
     using namespace ::testing;
     auto factory = std::make_shared<mtd::MockSurfaceFactory>();
-    msh::SessionContainer container;
+    msh::DefaultSessionContainer container;
 
     auto session = std::make_shared<msh::ApplicationSession>(factory,
                                                                "Visual Studio 7");