Merge lp:~robertcarr/mir/prepare-session-manager-for-input-focus into lp:~mir-team/mir/trunk

[Robert Carr]

Prepare the session manager for setting input focus through setting focus on creation of a sessions first surface.

This branch works to prepare the session manager for setting input focus.

The driving integration test is tests/integration-tests/shell/test_session_manager.cpp : TestSessionManagerAndSession, sessions_creating_first_surface_receive_focus.

The issue, is in order for the focus setter to set input focus in addition to visible focus, it will have to see the session at surface creation time (not just at session creation time).

===
To go a little deeper in to the motivation (to give context to the chosen interfaces), the input manager (or owner of the droidinput::InputDispatcher) needs to call (paraphrased from a list based API)

InputDispatcher::setInputFocus(droidinput::InputApplicationHandle, droidinput::InputWindowHandle)

where InputApplicationHandle is analogous to msh::Session and InputWindowHandle is analagous to msh::Surface. So mi::InputManager gains an interface:

InputFocusSelector::set_input_focus_to(std::shared_ptr<msh::Session> const& session, std::shared_ptr<msh::Surface> const& surface)

Now, the focus mechanism in shell may be modified to use the InputFocusSelector, to request focus for a session and it's default surface (a notion which will be added in the branch wiring this up).

This branch is just to allow msh::FocusSetter::set_focus_to be invoked when a session has created it's surface.
===

This branch is backed up by the design discussions so far but the full semantics for application focus v. window focus are still in progress in cucumber definition, so rather than try and sort all of that I am just trying to unblock input in a way that will also move us forward in a self contained view.

I think it's a little strange how the FocusArbitrator (session manager) is passed to the ApplicationSession by reference through usage of *this, but the ApplicationSession is passed to the FocusArbitrator (for request_focus) through std::enable_shared_from_this. I think this is justifiable though, as it is clear that an ApplicationSession must be owned by a shared_ptr (as they are only created by the session manager, who internally holds them with shared_ptrs).

There is some incidental cleanup to test_registration_order_focus_sequence.cpp and test_the_session_container_implementation.cpp to remove dependency on ApplicationSession implementation.

This branch depends on: https://code.launchpad.net/~robertcarr/mir/reduce-coupling-in-application-mediator-tests/+merge/152304 as without it there was much cumbersome test refactoring

[PS Jenkins bot (ps-jenkins)]

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

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

review: Approve (continuous-integration)

[Alan Griffiths (alan-griffiths)]

First let me say I like the style of most of this MP - it cleans up a lot of existing untidyness,

But I don't like the bit you felt uncomfortable enough to write about regarding ApplicationSession::focus_arbitrator& and enable_shared_from_this. (You should listen to the code telling you this isn't the right approach.)

I think what is actually needed is a function in shell (not sure which object yet) with the signature:

std::shared_ptr<shell::Surface>
create_surface_for(std::shared_ptr<Session> const& session, SurfaceCreationParameters const& params);

This can be called naturally from ApplicationMediator and has context for the call to request_focus() without the need for the above explanation.

Logically, it belongs with (or instead of) SessionStore::request_focus() - but SessionStore is growing functions that contradict its current name. Should shell::SessionStore become frontend::Shell?

review: Needs Fixing

Unmerged revisions

485. By Robert Carr on 2013-03-12

Reduce obtusity of test name

484. By Robert Carr on 2013-03-12

test_session_manager.cpp: Replace a should be stub mock with a stub

483. By Robert Carr on 2013-03-12

Remove overzealous whitespace deletion

482. By Robert Carr on 2013-03-12

Merge reduce-coupling-in-application-mediator-test

481. By Robert Carr on 2013-03-12

focus_arbitrator.h: Fix misnamed header guard comment

480. By Robert Carr on 2013-03-12

Implement request_focus, allowing the integration test to pass

479. By Robert Carr on 2013-03-12

Failing unit test to define SessionManager::request_focus

478. By Robert Carr on 2013-03-11

Pass application session to focus arbitrator through std::enable_shared_from_this

477. By Robert Carr on 2013-03-11

Define new msh::Session attribute has_appeared (see, ApplicationSession, session_has_appeared_after_creating_surface)

476. By Robert Carr on 2013-03-11

Hook the session manager up to the session via focus arbitrator

=== modified file 'include/mir/shell/application_session.h'
--- include/mir/shell/application_session.h	2013-03-06 12:25:34 +0000
+++ include/mir/shell/application_session.h	2013-03-12 00:40:38 +0000
@@ -29,11 +29,12 @@
 namespace shell
 {
 class SurfaceFactory;
+class FocusArbitrator;
 
 class ApplicationSession : public Session
 {
 public:
-    explicit ApplicationSession(std::shared_ptr<SurfaceFactory> const& surface_factory, std::string const& session_name);
+    explicit ApplicationSession(std::shared_ptr<SurfaceFactory> const& surface_factory, FocusArbitrator& focus_arbitrator, std::string const& session_name);
     ~ApplicationSession();
 
     SurfaceId create_surface(SurfaceCreationParameters const& params);
@@ -45,6 +46,8 @@
 
     void hide();
     void show();
+    
+    bool has_appeared() const;
 
 protected:
     ApplicationSession(ApplicationSession const&) = delete;
@@ -52,6 +55,8 @@
 
 private:
     std::shared_ptr<SurfaceFactory> const surface_factory;
+    FocusArbitrator &focus_arbitrator;
+
     std::string const session_name;
 
     SurfaceId next_id();

=== added file 'include/mir/shell/focus_arbitrator.h'
--- include/mir/shell/focus_arbitrator.h	1970-01-01 00:00:00 +0000
+++ include/mir/shell/focus_arbitrator.h	2013-03-12 00:40:38 +0000
@@ -0,0 +1,47 @@
+/*
+ * 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 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/>.
+ *
+ * Authored by: Robert Carr <[e-mail]>
+ */
+
+#ifndef MIR_SHELL_FOCUS_ARBITRATOR_H_
+#define MIR_SHELL_FOCUS_ARBITRATOR_H_
+
+#include <memory>
+
+namespace mir
+{
+namespace shell
+{
+class Session;
+
+class FocusArbitrator
+{
+public:
+    virtual ~FocusArbitrator() {}
+
+    virtual bool request_focus(std::shared_ptr<Session> const& session) = 0;
+
+protected:
+    FocusArbitrator() = default;
+    FocusArbitrator(const FocusArbitrator&) = delete;
+    FocusArbitrator& operator=(const FocusArbitrator&) = delete;
+};
+
+}
+}
+
+
+#endif // MIR_SHELL_FOCUS_ARBITRATOR_H_

=== modified file 'include/mir/shell/session.h'
--- include/mir/shell/session.h	2013-03-06 12:25:34 +0000
+++ include/mir/shell/session.h	2013-03-12 00:40:38 +0000
@@ -35,7 +35,7 @@
 class Surface;
 class SurfaceCreationParameters;
 
-class Session
+class Session : public std::enable_shared_from_this<Session>
 {
 public:
     virtual ~Session() {}
@@ -49,6 +49,8 @@
 
     virtual void hide() = 0;
     virtual void show() = 0;
+    
+    virtual bool has_appeared() const = 0;
 
 protected:
     Session() = default;

=== modified file 'include/mir/shell/session_manager.h'
--- include/mir/shell/session_manager.h	2013-03-06 12:25:34 +0000
+++ include/mir/shell/session_manager.h	2013-03-12 00:40:38 +0000
@@ -51,6 +51,8 @@
 
     virtual void tag_session_with_lightdm_id(std::shared_ptr<Session> const& session, int id);
     virtual void focus_session_with_lightdm_id(int id);
+    
+    bool request_focus(std::shared_ptr<Session> const& session);
 
     void focus_next();
 

=== modified file 'include/mir/shell/session_store.h'
--- include/mir/shell/session_store.h	2013-03-06 12:25:34 +0000
+++ include/mir/shell/session_store.h	2013-03-12 00:40:38 +0000
@@ -19,6 +19,8 @@
 #ifndef MIR_SHELL_SESSION_STORE_H_
 #define MIR_SHELL_SESSION_STORE_H_
 
+#include "mir/shell/focus_arbitrator.h"
+
 #include <memory>
 
 namespace mir
@@ -29,7 +31,7 @@
 
 class Session;
 
-class SessionStore
+class SessionStore : public FocusArbitrator
 {
 public:
     virtual ~SessionStore() {}
@@ -40,6 +42,8 @@
     virtual void tag_session_with_lightdm_id(std::shared_ptr<Session> const& session, int id) = 0;
     virtual void focus_session_with_lightdm_id(int id) = 0;
 
+    virtual bool request_focus(std::shared_ptr<Session> const& session) = 0;
+
     virtual void shutdown() = 0;
 
 protected:

=== modified file 'include/mir_test_doubles/mock_session.h'
--- include/mir_test_doubles/mock_session.h	2013-03-12 00:40:38 +0000
+++ include/mir_test_doubles/mock_session.h	2013-03-12 00:40:38 +0000
@@ -41,6 +41,8 @@
     
     MOCK_METHOD0(hide, void());
     MOCK_METHOD0(show, void());
+    
+    MOCK_CONST_METHOD0(has_appeared, bool());
 };
 
 }

=== modified file 'include/mir_test_doubles/mock_session_store.h'
--- include/mir_test_doubles/mock_session_store.h	2013-03-12 00:40:38 +0000
+++ include/mir_test_doubles/mock_session_store.h	2013-03-12 00:40:38 +0000
@@ -37,6 +37,8 @@
 
     MOCK_METHOD2(tag_session_with_lightdm_id, void(std::shared_ptr<shell::Session> const&, int));
     MOCK_METHOD1(focus_session_with_lightdm_id, void(int));
+    
+    MOCK_METHOD1(request_focus, bool(std::shared_ptr<shell::Session> const&));
 
     MOCK_METHOD0(shutdown, void());
 };

=== modified file 'include/mir_test_doubles/stub_session.h'
--- include/mir_test_doubles/stub_session.h	2013-03-12 00:40:38 +0000
+++ include/mir_test_doubles/stub_session.h	2013-03-12 00:40:38 +0000
@@ -54,6 +54,10 @@
     void show()
     {
     }
+    bool has_appeared() const
+    {
+        return false;
+    }
 };
 
 }

=== modified file 'include/mir_test_doubles/stub_session_store.h'
--- include/mir_test_doubles/stub_session_store.h	2013-03-12 00:40:38 +0000
+++ include/mir_test_doubles/stub_session_store.h	2013-03-12 00:40:38 +0000
@@ -44,6 +44,10 @@
     void focus_session_with_lightdm_id(int /* id */)
     {
     }
+    bool request_focus(std::shared_ptr<shell::Session> const& /* session */)
+    {
+        return false;
+    }
     void shutdown()
     {
     }

=== modified file 'src/shell/application_session.cpp'
--- src/shell/application_session.cpp	2013-03-06 12:54:33 +0000
+++ src/shell/application_session.cpp	2013-03-12 00:40:38 +0000
@@ -18,6 +18,7 @@
 
 #include "mir/shell/application_session.h"
 #include "mir/shell/surface.h"
+#include "mir/shell/focus_arbitrator.h"
 
 #include "mir/surfaces/surface_controller.h"
 
@@ -33,8 +34,10 @@
 
 msh::ApplicationSession::ApplicationSession(
     std::shared_ptr<msh::SurfaceFactory> const& surface_factory,
+    msh::FocusArbitrator &focus_arbitrator,
     std::string const& session_name) :
     surface_factory(surface_factory),
+    focus_arbitrator(focus_arbitrator),
     session_name(session_name),
     next_surface_id(0)
 {
@@ -60,8 +63,12 @@
     auto surf = surface_factory->create_surface(params);
     auto const id = next_id();
 
+    // Careful of ordering with respect to the focus arbitrators invocation of has_appeared()
+    focus_arbitrator.request_focus(shared_from_this());
+
     std::unique_lock<std::mutex> lock(surfaces_mutex);
     surfaces[id] = surf;
+    
     return id;
 }
 
@@ -122,3 +129,9 @@
         id_s.second->show();
     }
 }
+
+bool msh::ApplicationSession::has_appeared() const
+{
+    std::unique_lock<std::mutex> lock(surfaces_mutex);
+    return surfaces.size() > 0 ;
+}

=== modified file 'src/shell/session_manager.cpp'
--- src/shell/session_manager.cpp	2013-03-06 12:54:33 +0000
+++ src/shell/session_manager.cpp	2013-03-12 00:40:38 +0000
@@ -51,7 +51,7 @@
 
 std::shared_ptr<msh::Session> msh::SessionManager::open_session(std::string const& name)
 {
-    auto new_session = std::make_shared<msh::ApplicationSession>(surface_factory, name);
+    auto new_session = std::make_shared<msh::ApplicationSession>(surface_factory, *this, name);
 
     app_container->insert_session(new_session);
 
@@ -134,3 +134,12 @@
         set_focus_to(match->second);
     }
 }
+
+bool msh::SessionManager::request_focus(std::shared_ptr<msh::Session> const& session)
+{
+    if (session->has_appeared())
+        return false;
+    
+    set_focus_to(session);
+    return true;
+}

=== modified file 'tests/acceptance-tests/test_focus_management_api.cpp'
--- tests/acceptance-tests/test_focus_management_api.cpp	2013-03-06 12:25:34 +0000
+++ tests/acceptance-tests/test_focus_management_api.cpp	2013-03-12 00:40:38 +0000
@@ -19,6 +19,7 @@
 #include "mir_toolkit/mir_client_library_lightdm.h"
 
 #include "mir/shell/session_store.h"
+#include "mir/shell/session.h"
 
 #include "mir_test_doubles/mock_display.h"
 #include "mir_test_framework/display_server_test_fixture.h"
@@ -132,6 +133,8 @@
 
         ON_CALL(*this, tag_session_with_lightdm_id(_, _)).WillByDefault(Invoke(impl.get(), &SessionStore::tag_session_with_lightdm_id));
         ON_CALL(*this, focus_session_with_lightdm_id(_)).WillByDefault(Invoke(impl.get(), &SessionStore::focus_session_with_lightdm_id));
+        
+        ON_CALL(*this, request_focus(_)).WillByDefault(Invoke(impl.get(), &SessionStore::request_focus));
 
         ON_CALL(*this, shutdown()).WillByDefault(Invoke(impl.get(), &SessionStore::shutdown));
     }
@@ -141,6 +144,8 @@
 
     MOCK_METHOD2(tag_session_with_lightdm_id, void (std::shared_ptr<shell::Session> const& session, int id));
     MOCK_METHOD1(focus_session_with_lightdm_id, void (int id));
+    
+    MOCK_METHOD1(request_focus, bool (std::shared_ptr<shell::Session> const&));
 
     MOCK_METHOD0(shutdown, void ());
 

=== modified file 'tests/integration-tests/CMakeLists.txt'
--- tests/integration-tests/CMakeLists.txt	2013-03-07 10:50:43 +0000
+++ tests/integration-tests/CMakeLists.txt	2013-03-12 00:40:38 +0000
@@ -12,6 +12,7 @@
 add_subdirectory(client/)
 add_subdirectory(compositor/)
 add_subdirectory(frontend/)
+add_subdirectory(shell/)
 add_subdirectory(process/)
 if (NOT MIR_DISABLE_INPUT)
 add_subdirectory(input/)

=== modified file 'tests/integration-tests/frontend/CMakeLists.txt'
--- tests/integration-tests/frontend/CMakeLists.txt	2013-03-04 15:54:45 +0000
+++ tests/integration-tests/frontend/CMakeLists.txt	2013-03-12 00:40:38 +0000
@@ -1,7 +1,6 @@
 list(
   APPEND INTEGRATION_TESTS_SRCS
   ${CMAKE_CURRENT_SOURCE_DIR}/test_application_mediator_report.cpp
-  ${CMAKE_CURRENT_SOURCE_DIR}/test_session_manager.cpp
 )
 
 set(

=== added directory 'tests/integration-tests/shell'
=== added file 'tests/integration-tests/shell/CMakeLists.txt'
--- tests/integration-tests/shell/CMakeLists.txt	1970-01-01 00:00:00 +0000
+++ tests/integration-tests/shell/CMakeLists.txt	2013-03-12 00:40:38 +0000
@@ -0,0 +1,9 @@
+list(
+  APPEND INTEGRATION_TESTS_SRCS
+  ${CMAKE_CURRENT_SOURCE_DIR}/test_session_manager.cpp
+)
+
+set(
+  INTEGRATION_TESTS_SRCS
+  ${INTEGRATION_TESTS_SRCS}
+  PARENT_SCOPE)

=== renamed file 'tests/integration-tests/frontend/test_session_manager.cpp' => 'tests/integration-tests/shell/test_session_manager.cpp'
--- tests/integration-tests/frontend/test_session_manager.cpp	2013-03-06 12:54:33 +0000
+++ tests/integration-tests/shell/test_session_manager.cpp	2013-03-12 00:40:38 +0000
@@ -24,6 +24,8 @@
 #include "mir/compositor/buffer_swapper.h"
 #include "mir/shell/focus_sequence.h"
 #include "mir/shell/focus_setter.h"
+#include "mir/shell/session.h"
+#include "mir/shell/surface.h"
 #include "mir/shell/registration_order_focus_sequence.h"
 #include "mir/shell/session_container.h"
 #include "mir/shell/surface_creation_parameters.h"
@@ -37,6 +39,7 @@
 namespace mc = mir::compositor;
 namespace msh = mir::shell;
 namespace ms = mir::surfaces;
+namespace geom = mir::geometry;
 namespace mt = mir::test;
 namespace mtd = mir::test::doubles;
 
@@ -48,6 +51,37 @@
   MOCK_METHOD1(set_focus_to, void(std::shared_ptr<msh::Session> const&));
 };
 
+struct StubSurface : public msh::Surface
+{
+    void hide()
+    {
+    }
+    void show()
+    {
+    }
+    void destroy()
+    {
+    }
+    void shutdown()
+    {
+    }
+    geom::Size size() const
+    {
+        return geom::Size();
+    }
+    geom::PixelFormat pixel_format() const
+    {
+        return geom::PixelFormat();
+    }
+    void advance_client_buffer()
+    {
+    }
+    std::shared_ptr<mc::Buffer> client_buffer() const
+    {
+        return std::shared_ptr<mc::Buffer>();
+    }
+};
+
 }
 
 TEST(TestSessionManagerAndFocusSelectionStrategy, cycle_focus)
@@ -113,3 +147,33 @@
     session_manager.close_session(session3);
     session_manager.close_session(session2);
 }
+
+TEST(TestSessionManagerAndSession, sessions_creating_first_surface_receive_focus)
+{
+    using namespace ::testing;
+
+    StubSurface stub_surface;
+    mtd::MockSurfaceFactory surface_factory;
+    std::shared_ptr<msh::SessionContainer> model(new msh::SessionContainer());
+    msh::RegistrationOrderFocusSequence sequence(model);
+    MockFocusSetter focus_changer;
+    std::shared_ptr<msh::Session> new_session;
+    
+    EXPECT_CALL(surface_factory, create_surface(_)).Times(2).WillRepeatedly(Return(mt::fake_shared(stub_surface)));
+
+    msh::SessionManager session_manager(
+        mt::fake_shared(surface_factory),
+        model,
+        mt::fake_shared(sequence),
+        mt::fake_shared(focus_changer));
+    
+    // Once for creating the session and a second time at surface creation
+    EXPECT_CALL(focus_changer, set_focus_to(_)).Times(2);
+
+    // Triggers a focus set
+    auto session1 = session_manager.open_session("Yesterday");
+    // Also triggers a focus change
+    session1->create_surface(msh::a_surface());
+    // Should not trigger a focus change
+    session1->create_surface(msh::a_surface());
+}

=== modified file 'tests/unit-tests/shell/test_application_session.cpp'
--- tests/unit-tests/shell/test_application_session.cpp	2013-03-12 00:40:38 +0000
+++ tests/unit-tests/shell/test_application_session.cpp	2013-03-12 00:40:38 +0000
@@ -19,6 +19,7 @@
 #include "mir/shell/application_session.h"
 #include "mir/compositor/buffer.h"
 #include "mir/shell/surface_creation_parameters.h"
+#include "mir/shell/focus_arbitrator.h"
 #include "mir_test/fake_shared.h"
 #include "mir_test_doubles/mock_surface_factory.h"
 #include "mir_test_doubles/mock_surface.h"
@@ -34,23 +35,59 @@
 namespace mt = mir::test;
 namespace mtd = mir::test::doubles;
 
+namespace
+{
+
+struct MockFocusArbitrator : public msh::FocusArbitrator
+{
+    MOCK_METHOD1(request_focus, bool(std::shared_ptr<msh::Session> const&));
+};
+
+}
+
 TEST(ApplicationSession, create_and_destroy_surface)
 {
     using namespace ::testing;
 
     auto const mock_surface = std::make_shared<mtd::MockSurface>();
     mtd::MockSurfaceFactory surface_factory;
+    NiceMock<MockFocusArbitrator> focus_arbitrator;
+
     ON_CALL(surface_factory, create_surface(_)).WillByDefault(Return(mock_surface));
 
     EXPECT_CALL(surface_factory, create_surface(_));
     EXPECT_CALL(*mock_surface, destroy());
 
-    msh::ApplicationSession session(mt::fake_shared(surface_factory), "Foo");
+    auto session = std::make_shared<msh::ApplicationSession>(mt::fake_shared(surface_factory), focus_arbitrator, "Foo");
 
     msh::SurfaceCreationParameters params;
-    auto surf = session.create_surface(params);
-
-    session.destroy_surface(surf);
+    auto surf = session->create_surface(params);
+
+    session->destroy_surface(surf);
+}
+
+TEST(ApplicationSession, must_be_managed_by_shared_ptr_to_create_surface)
+{
+    using namespace ::testing;
+
+    auto const mock_surface = std::make_shared<NiceMock<mtd::MockSurface>>();
+    NiceMock<mtd::MockSurfaceFactory> surface_factory;
+    NiceMock<MockFocusArbitrator> focus_arbitrator;
+
+    ON_CALL(surface_factory, create_surface(_)).WillByDefault(Return(mock_surface));
+
+    auto session = std::make_shared<msh::ApplicationSession>(mt::fake_shared(surface_factory), focus_arbitrator, "Foo");
+    msh::ApplicationSession unmanaged_session(mt::fake_shared(surface_factory), focus_arbitrator, "Foo");
+    
+    EXPECT_NO_THROW({
+            session->create_surface(msh::a_surface());
+     });
+
+    // Since create surface uses std::enable_shared_from this to request focus from the focus arbitrator 
+    // creating a surface from an unmanaged session will throw
+    EXPECT_THROW({
+            unmanaged_session.create_surface(msh::a_surface());
+    }, std::bad_weak_ptr);
 }
 
 
@@ -60,9 +97,11 @@
 
     auto const mock_surface = std::make_shared<mtd::MockSurface>();
     mtd::MockSurfaceFactory surface_factory;
+    NiceMock<MockFocusArbitrator> focus_arbitrator;
+
     ON_CALL(surface_factory, create_surface(_)).WillByDefault(Return(mock_surface));
 
-    msh::ApplicationSession app_session(mt::fake_shared(surface_factory), "Foo");
+    auto app_session = std::make_shared<msh::ApplicationSession>(mt::fake_shared(surface_factory), focus_arbitrator, "Foo");
 
     EXPECT_CALL(surface_factory, create_surface(_));
 
@@ -74,36 +113,69 @@
     }
 
     msh::SurfaceCreationParameters params;
-    auto surf = app_session.create_surface(params);
-
-    app_session.hide();
-    app_session.show();
-
-    app_session.destroy_surface(surf);
-}
-
-TEST(Session, get_invalid_surface_throw_behavior)
-{
-    using namespace ::testing;
-
-    mtd::MockSurfaceFactory surface_factory;
-    msh::ApplicationSession app_session(mt::fake_shared(surface_factory), "Foo");
+    auto surf = app_session->create_surface(params);
+
+    app_session->hide();
+    app_session->show();
+
+    app_session->destroy_surface(surf);
+}
+
+TEST(ApplicationSession, creating_suface_requests_focus)
+{
+    using namespace ::testing;
+
+    mtd::MockSurfaceFactory surface_factory;
+    mtd::MockSurface mock_surface;
+    MockFocusArbitrator focus_arbitrator;
+
+    ON_CALL(surface_factory, create_surface(_)).WillByDefault(Return(mt::fake_shared(mock_surface)));
+    EXPECT_CALL(focus_arbitrator, request_focus(_)).Times(1).WillOnce(Return(true));
+
+    auto app_session = std::make_shared<msh::ApplicationSession>(mt::fake_shared(surface_factory), focus_arbitrator, "Foo");
+    app_session->create_surface(msh::a_surface());
+}
+
+TEST(ApplicationSession, session_has_appeared_after_creating_surface)
+{
+    using namespace ::testing;
+
+    mtd::MockSurfaceFactory surface_factory;
+    mtd::MockSurface mock_surface;
+    NiceMock<MockFocusArbitrator> focus_arbitrator;
+
+    ON_CALL(surface_factory, create_surface(_)).WillByDefault(Return(mt::fake_shared(mock_surface)));
+
+    auto app_session = std::make_shared<msh::ApplicationSession>(mt::fake_shared(surface_factory), focus_arbitrator, "Foo");
+    EXPECT_FALSE(app_session->has_appeared());
+    app_session->create_surface(msh::a_surface());
+    EXPECT_TRUE(app_session->has_appeared());
+}
+
+TEST(ApplicationSession, get_invalid_surface_throw_behavior)
+{
+    using namespace ::testing;
+
+    mtd::MockSurfaceFactory surface_factory;
+    NiceMock<MockFocusArbitrator> focus_arbitrator;
+    auto app_session = std::make_shared<msh::ApplicationSession>(mt::fake_shared(surface_factory), focus_arbitrator, "Foo");
     msh::SurfaceId invalid_surface_id = msh::SurfaceId{1};
 
     EXPECT_THROW({
-            app_session.get_surface(invalid_surface_id);
+            app_session->get_surface(invalid_surface_id);
     }, std::runtime_error);
 }
 
-TEST(Session, destroy_invalid_surface_throw_behavior)
+TEST(ApplicationSession, destroy_invalid_surface_throw_behavior)
 {
     using namespace ::testing;
 
     mtd::MockSurfaceFactory surface_factory;
-    msh::ApplicationSession app_session(mt::fake_shared(surface_factory), "Foo");
+    NiceMock<MockFocusArbitrator> focus_arbitrator;
+    auto app_session = std::make_shared<msh::ApplicationSession>(mt::fake_shared(surface_factory), focus_arbitrator, "Foo");
     msh::SurfaceId invalid_surface_id = msh::SurfaceId{1};
 
     EXPECT_THROW({
-            app_session.destroy_surface(invalid_surface_id);
+            app_session->destroy_surface(invalid_surface_id);
     }, std::runtime_error);
 }

=== modified file 'tests/unit-tests/shell/test_registration_order_focus_sequence.cpp'
--- tests/unit-tests/shell/test_registration_order_focus_sequence.cpp	2013-03-06 12:54:33 +0000
+++ tests/unit-tests/shell/test_registration_order_focus_sequence.cpp	2013-03-12 00:40:38 +0000
@@ -17,13 +17,15 @@
  */
 
 #include "mir/surfaces/buffer_bundle.h"
-#include "mir/shell/application_session.h"
+#include "mir/shell/session.h"
 #include "mir/shell/session_container.h"
 #include "mir/shell/registration_order_focus_sequence.h"
 #include "mir/shell/surface_creation_parameters.h"
 #include "mir/surfaces/surface.h"
+
+#include "mir_test/fake_shared.h"
 #include "mir_test_doubles/mock_buffer_bundle.h"
-#include "mir_test_doubles/mock_surface_factory.h"
+#include "mir_test_doubles/stub_session.h"
 
 #include <gmock/gmock.h>
 #include <gtest/gtest.h>
@@ -32,7 +34,8 @@
 namespace mc = mir::compositor;
 namespace msh = mir::shell;
 namespace ms = mir::surfaces;
-namespace mtd = mir::test::doubles;
+namespace mt = mir::test;
+namespace mtd = mt::doubles;
 
 namespace
 {
@@ -40,30 +43,19 @@
 {
     void SetUp()
     {
-        factory = std::make_shared<mtd::MockSurfaceFactory>();
         container = std::make_shared<msh::SessionContainer>();
     }
-    std::shared_ptr<mtd::MockSurfaceFactory> factory;
     std::shared_ptr<msh::SessionContainer> container;
-    
-    static std::string const testing_app_name1;
-    static std::string const testing_app_name2;
-    static std::string const testing_app_name3;
 };
-
-std::string const RegistrationOrderFocusSequenceSetup::testing_app_name1{"Visual Studio 7"};
-std::string const RegistrationOrderFocusSequenceSetup::testing_app_name2{"Visual Studio 8"};
-std::string const RegistrationOrderFocusSequenceSetup::testing_app_name3{"Visual Studio 9"};
 }
 
 TEST_F(RegistrationOrderFocusSequenceSetup, focus_order)
 {
     using namespace ::testing;
 
-    auto app1 = std::make_shared<msh::ApplicationSession>(factory, testing_app_name1);
-    auto app2 = std::make_shared<msh::ApplicationSession>(factory, testing_app_name2);
-    auto app3 = std::make_shared<msh::ApplicationSession>(factory, testing_app_name3);
-
+    auto app1 = std::make_shared<mtd::StubSession>();
+    auto app2 = std::make_shared<mtd::StubSession>();
+    auto app3 = std::make_shared<mtd::StubSession>();
     container->insert_session(app1);
     container->insert_session(app2);
     container->insert_session(app3);
@@ -78,9 +70,9 @@
 {
     using namespace ::testing;
 
-    auto app1 = std::make_shared<msh::ApplicationSession>(factory, testing_app_name1);
-    auto app2 = std::make_shared<msh::ApplicationSession>(factory, testing_app_name2);
-    auto app3 = std::make_shared<msh::ApplicationSession>(factory, testing_app_name3);
+    auto app1 = std::make_shared<mtd::StubSession>();
+    auto app2 = std::make_shared<mtd::StubSession>();
+    auto app3 = std::make_shared<mtd::StubSession>();
     container->insert_session(app1);
     container->insert_session(app2);
     container->insert_session(app3);
@@ -95,7 +87,7 @@
 {
     using namespace ::testing;
 
-    auto app1 = std::make_shared<msh::ApplicationSession>(factory, testing_app_name1);
+    auto app1 = std::make_shared<mtd::StubSession>();
     container->insert_session(app1);
 
     msh::RegistrationOrderFocusSequence focus_sequence(container);
@@ -107,9 +99,9 @@
 {
     using namespace ::testing;
 
-    auto app1 = std::make_shared<msh::ApplicationSession>(factory, testing_app_name1);
-    auto app2 = std::make_shared<msh::ApplicationSession>(factory, testing_app_name2);
-    auto null_session = std::shared_ptr<msh::ApplicationSession>();
+    auto app1 = std::make_shared<mtd::StubSession>();
+    auto app2 = std::make_shared<mtd::StubSession>();
+    auto null_session = std::shared_ptr<msh::Session>();
 
     msh::RegistrationOrderFocusSequence focus_sequence(container);
     
@@ -124,8 +116,8 @@
 {
     using namespace ::testing;
 
-    auto invalid_session = std::make_shared<msh::ApplicationSession>(factory, testing_app_name1);
-    auto null_session = std::shared_ptr<msh::ApplicationSession>();
+    auto invalid_session = std::make_shared<mtd::StubSession>();
+    auto null_session = std::shared_ptr<msh::Session>();
     
     msh::RegistrationOrderFocusSequence focus_sequence(container);
 

=== modified file 'tests/unit-tests/shell/test_session_manager.cpp'
--- tests/unit-tests/shell/test_session_manager.cpp	2013-03-06 18:12:43 +0000
+++ tests/unit-tests/shell/test_session_manager.cpp	2013-03-12 00:40:38 +0000
@@ -28,6 +28,7 @@
 #include "mir_test/fake_shared.h"
 #include "mir_test_doubles/mock_surface_factory.h"
 #include "mir_test_doubles/null_buffer_bundle.h"
+#include "mir_test_doubles/mock_session.h"
 
 #include "src/surfaces/proxy_surface.h"
 
@@ -114,8 +115,9 @@
 
     EXPECT_CALL(container, insert_session(_)).Times(1);
     EXPECT_CALL(container, remove_session(_)).Times(1);
-
-    EXPECT_CALL(focus_setter, set_focus_to(_)).Times(1);
+    
+    // Once for session creation and once for surface creation
+    EXPECT_CALL(focus_setter, set_focus_to(_)).Times(2);
     EXPECT_CALL(focus_setter, set_focus_to(std::shared_ptr<msh::Session>())).Times(1);
 
     EXPECT_CALL(sequence, default_focus()).WillOnce(Return((std::shared_ptr<msh::Session>())));
@@ -166,3 +168,21 @@
 
     session_manager.close_session(session1);
 }
+
+
+TEST_F(SessionManagerSetup, request_focus_sets_focus_for_sessions_which_have_not_appeared)
+{
+    using namespace ::testing;
+    // It is ok to use a session not managed by the session manager as the exceptional behavior is defined in the real focus setter
+    mtd::MockSession session;
+    
+    {
+        InSequence seq;
+        EXPECT_CALL(session, has_appeared()).Times(1).WillOnce(Return(false));
+        EXPECT_CALL(session, has_appeared()).Times(1).WillOnce(Return(true));
+    }
+    EXPECT_CALL(focus_setter, set_focus_to(_)).Times(1);
+
+    EXPECT_TRUE(session_manager.request_focus(mt::fake_shared(session)));
+    EXPECT_FALSE(session_manager.request_focus(mt::fake_shared(session)));
+}

=== modified file 'tests/unit-tests/shell/test_the_session_container_implementation.cpp'
--- tests/unit-tests/shell/test_the_session_container_implementation.cpp	2013-03-06 12:54:33 +0000
+++ tests/unit-tests/shell/test_the_session_container_implementation.cpp	2013-03-12 00:40:38 +0000
@@ -17,42 +17,43 @@
  */
 
 #include "mir/surfaces/buffer_bundle.h"
-#include "mir/shell/application_session.h"
+#include "mir/shell/session.h"
 #include "mir/shell/session_container.h"
 #include "mir/shell/surface_creation_parameters.h"
 #include "mir/surfaces/surface.h"
-#include "mir_test_doubles/mock_buffer_bundle.h"
-#include "mir_test_doubles/mock_surface_factory.h"
+
+#include "mir_test/fake_shared.h"
+#include "mir_test_doubles/mock_session.h"
 
 #include <gmock/gmock.h>
 #include <gtest/gtest.h>
 #include <string>
 
 namespace msh = mir::shell;
-namespace mtd = mir::test::doubles;
+namespace mt = mir::test;
+namespace mtd = mt::doubles;
 
 TEST(SessionContainer, for_each)
 {
     using namespace ::testing;
-    auto factory = std::make_shared<mtd::MockSurfaceFactory>();
     msh::SessionContainer 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"));
+    mtd::MockSession session1;
+    mtd::MockSession session2;
+    
+    container.insert_session(mt::fake_shared(session1));
+    container.insert_session(mt::fake_shared(session2));
 
     struct local
     {
-        MOCK_METHOD1(check_name, void (std::string const&));
-
         void operator()(std::shared_ptr<msh::Session> const& session)
         {
-            check_name(session->name());
+            session->name();
         }
     } functor;
 
-    InSequence seq;
-    EXPECT_CALL(functor, check_name("Visual Studio 7"));
-    EXPECT_CALL(functor, check_name("Visual Studio 8"));
+    EXPECT_CALL(session1, name()).WillOnce(Return("VS 7"));
+    EXPECT_CALL(session2, name()).WillOnce(Return("VS 8"));
 
     container.for_each(std::ref(functor));
 }
@@ -60,12 +61,10 @@
 TEST(SessionContainer, invalid_session_throw_behavior)
 {
     using namespace ::testing;
-    auto factory = std::make_shared<mtd::MockSurfaceFactory>();
     msh::SessionContainer container;
+    mtd::MockSession invalid_session;
 
-    auto session = std::make_shared<msh::ApplicationSession>(factory,
-                                                               "Visual Studio 7");
     EXPECT_THROW({
-        container.remove_session(session);
+        container.remove_session(mt::fake_shared(invalid_session));
     }, std::logic_error);
 }