Merge lp:~alan-griffiths/mir/MVC-cleanup-of-FocusSetter into lp:mir

[Alan Griffiths]

shell: merge the FocusSetter strategy into DefaultShell

shell: merge the FocusSetter strategy into DefaultShell

This doesn't make the "DefaultShell" controller customizable - that, and other cleanup is for a future MP

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2248
http://jenkins.qa.ubuntu.com/job/mir-ci/2720/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/941
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/941
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/903
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/717
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/717/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/903
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/903/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/4005
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/17246

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

review: Approve (continuous-integration)

[Alberto Aguirre (albaguirre)]

What is the advantage of folding this into DefaultShell?

Is it mainly to expose only a single point of customization by shells?

review: Needs Information

[Daniel van Vugt (vanvugt)]

BTW, reviewing the latest Surface Management design we now finally do distinguish between the focussed and active windows. Keep it in mind that they will be different things.

[Alexandros Frantzis (afrantzis)]

Looks good.

698 +TEST_F(TestDefaultShellAndFocusSelectionStrategy, sets_input_focus)

I find the expectations in this test difficult to follow. The test would be clearer if it was split into separate tests for the 3 cases.

review: Approve

[Alan Griffiths (alan-griffiths)]

> What is the advantage of folding this into DefaultShell?

FocusSetter is demonstrably not a useful abstraction - nothing (not the unity downstreams, not our proving server, nor our demo server) has ever made use of it.

> Is it mainly to expose only a single point of customization by shells?

Partly, also having all the "window management" stuff in one place will make it easier to separate out the different concerns.

As Daniel notes "focus" is not as simple a concept as the existing implementation assumes - I think that the FocusSetter interface would get in the way of changing that.

[Alan Griffiths (alan-griffiths)]

> Looks good.
>
> 698 +TEST_F(TestDefaultShellAndFocusSelectionStrategy, sets_input_focus)
>
> I find the expectations in this test difficult to follow. The test would be
> clearer if it was split into separate tests for the 3 cases.

True, but this make it clear that I have not changed the test during refactoring.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2250
http://jenkins.qa.ubuntu.com/job/mir-ci/2740/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/965
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/965
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/927
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/737
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/737/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/927
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/927/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/4023
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/17278

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

review: Approve (continuous-integration)

[Alberto Aguirre (albaguirre)]

OK.

review: Approve

[Robert Carr (robertcarr)]

LGTM

review: Approve

[Kevin DuBois (kdub)]

I was downstream yesterday, and noticed that I had to remove stuff related to the shell focus setter to get qtmir to compile, is there a branch floating around to fix qtmir with the focus setting changes? (all it wants to do is make the FocusSetter a NullFocusSetter)

review: Needs Information

[Alan Griffiths (alan-griffiths)]

> I was downstream yesterday, and noticed that I had to remove stuff related to
> the shell focus setter to get qtmir to compile, is there a branch floating
> around to fix qtmir with the focus setting changes? (all it wants to do is
> make the FocusSetter a NullFocusSetter)

Oops! Somehow I missed that in my trawl of downstream.

This isn't directly affected by this branch (as FocusSetter has been unpublished on trunk).

I do intend a better mechanism. So we can either revert -c 2238 or land this and hope to publish the new approach soon.

review: Needs Information

[Alberto Aguirre (albaguirre)]

"So we can either revert -c 2238 or land this and hope to publish the new approach soon."

I would say go for the latter.

[Chris Halse Rogers (raof)]

Looks sensible to me.

review: Approve

=== modified file 'src/include/server/mir/default_server_configuration.h'
--- src/include/server/mir/default_server_configuration.h	2015-01-21 10:29:57 +0000
+++ src/include/server/mir/default_server_configuration.h	2015-01-21 10:29:57 +0000
@@ -249,7 +249,6 @@
     /** @name shell configuration - customization
      * configurable interfaces for modifying shell
      *  @{ */
-    virtual std::shared_ptr<shell::FocusSetter>         the_shell_focus_setter();
     virtual std::shared_ptr<scene::PlacementStrategy>   the_placement_strategy();
     virtual std::shared_ptr<scene::SessionListener>     the_session_listener();
     virtual std::shared_ptr<shell::DisplayLayout>       the_shell_display_layout();
@@ -399,7 +398,6 @@
     CachedPtr<scene::SurfaceFactory> surface_factory;
     CachedPtr<scene::SessionContainer>  session_container;
     CachedPtr<scene::SurfaceCoordinator> surface_coordinator;
-    CachedPtr<shell::FocusSetter>       shell_focus_setter;
     CachedPtr<scene::PlacementStrategy> shell_placement_strategy;
     CachedPtr<scene::SessionListener> session_listener;
     CachedPtr<scene::PixelBuffer>       pixel_buffer;

=== removed file 'src/include/server/mir/shell/focus_setter.h'
--- src/include/server/mir/shell/focus_setter.h	2015-01-19 11:40:40 +0000
+++ src/include/server/mir/shell/focus_setter.h	1970-01-01 00:00:00 +0000
@@ -1,50 +0,0 @@
-/*
- * 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_FOCUS_SETTER_H_
-#define MIR_SHELL_FOCUS_SETTER_H_
-
-#include <memory>
-
-namespace mir
-{
-namespace scene { class Session; }
-
-
-namespace shell
-{
-/// Interface used by the Shell to propagate changes in the focus model to interested views
-/// e.g. Input, or Surfaces.
-class FocusSetter
-{
-public:
-    virtual ~FocusSetter() {}
-
-    virtual void set_focus_to(std::shared_ptr<scene::Session> const& new_focus) = 0;
-
-protected:
-    FocusSetter() = default;
-    FocusSetter(const FocusSetter&) = delete;
-    FocusSetter& operator=(const FocusSetter&) = delete;
-};
-
-}
-}
-
-
-#endif // MIR_SHELL_FOCUS_SETTER_H_

=== modified file 'src/server/shell/CMakeLists.txt'
--- src/server/shell/CMakeLists.txt	2015-01-21 10:29:57 +0000
+++ src/server/shell/CMakeLists.txt	2015-01-21 10:29:57 +0000
@@ -1,7 +1,6 @@
 set(
   SHELL_SOURCES
 
-  default_focus_mechanism.cpp
   default_placement_strategy.cpp
   graphics_display_layout.cpp
   default_configuration.cpp

=== modified file 'src/server/shell/default_configuration.cpp'
--- src/server/shell/default_configuration.cpp	2015-01-21 10:29:57 +0000
+++ src/server/shell/default_configuration.cpp	2015-01-21 10:29:57 +0000
@@ -21,7 +21,6 @@
 
 
 #include "default_placement_strategy.h"
-#include "default_focus_mechanism.h"
 #include "default_shell.h"
 #include "graphics_display_layout.h"
 
@@ -34,7 +33,8 @@
     return default_shell([this]
         {
             return std::make_shared<msh::DefaultShell>(
-                the_shell_focus_setter(),
+                the_input_targeter(),
+                the_surface_coordinator(),
                 the_session_coordinator());
         });
 }
@@ -63,18 +63,6 @@
         });
 }
 
-std::shared_ptr<msh::FocusSetter>
-mir::DefaultServerConfiguration::the_shell_focus_setter()
-{
-    return shell_focus_setter(
-        [this]
-        {
-            return std::make_shared<msh::DefaultFocusMechanism>(
-                the_input_targeter(),
-                the_surface_coordinator());
-        });
-}
-
 std::shared_ptr<msh::DisplayLayout>
 mir::DefaultServerConfiguration::the_shell_display_layout()
 {

=== removed file 'src/server/shell/default_focus_mechanism.cpp'
--- src/server/shell/default_focus_mechanism.cpp	2015-01-14 06:39:13 +0000
+++ src/server/shell/default_focus_mechanism.cpp	1970-01-01 00:00:00 +0000
@@ -1,66 +0,0 @@
-/*
- * 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]>
- */
-
-#include "default_focus_mechanism.h"
-#include "mir/frontend/session.h"
-#include "mir/scene/surface_coordinator.h"
-#include "mir/scene/surface.h"
-#include "mir/shell/input_targeter.h"
-#include "mir/scene/session.h"
-#include "mir/scene/surface.h"
-
-namespace mf = mir::frontend;
-namespace ms = mir::scene;
-namespace msh = mir::shell;
-
-msh::DefaultFocusMechanism::DefaultFocusMechanism(std::shared_ptr<msh::InputTargeter> const& input_targeter,
-                                                  std::shared_ptr<ms::SurfaceCoordinator> const& surface_coordinator)
-  : input_targeter(input_targeter),
-    surface_coordinator(surface_coordinator)
-{
-}
-
-void msh::DefaultFocusMechanism::set_focus_to(std::shared_ptr<ms::Session> const& focus_session)
-{
-    // TODO: This path should be encapsulated in a seperate clear_focus message
-    if (!focus_session)
-    {
-        input_targeter->focus_cleared();
-        return;
-    }
-
-    auto surface = focus_session->default_surface();
-    if (surface)
-    {
-        std::lock_guard<std::mutex> lg(surface_focus_lock);
-
-        // Ensure the surface has really taken the focus before notifying it that it is focused
-        surface_coordinator->raise(surface);
-        surface->take_input_focus(input_targeter);
-
-        auto current_focus = currently_focused_surface.lock();
-        if (current_focus)
-            current_focus->configure(mir_surface_attrib_focus, mir_surface_unfocused);
-        surface->configure(mir_surface_attrib_focus, mir_surface_focused);
-        currently_focused_surface = surface;
-    }
-    else
-    {
-        input_targeter->focus_cleared();
-    }
-}

=== removed file 'src/server/shell/default_focus_mechanism.h'
--- src/server/shell/default_focus_mechanism.h	2015-01-14 06:39:13 +0000
+++ src/server/shell/default_focus_mechanism.h	1970-01-01 00:00:00 +0000
@@ -1,60 +0,0 @@
-/*
- * 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_SINGLE_VISIBILITY_FOCUS_MECHANISM_H_
-#define MIR_SHELL_SINGLE_VISIBILITY_FOCUS_MECHANISM_H_
-
-#include "mir/shell/focus_setter.h"
-
-#include <memory>
-#include <mutex>
-
-namespace mir
-{
-namespace scene { class SurfaceCoordinator; class Surface; }
-
-namespace shell
-{
-class InputTargeter;
-
-class DefaultFocusMechanism : public FocusSetter
-{
-public:
-    explicit DefaultFocusMechanism(std::shared_ptr<InputTargeter> const& input_targeter,
-                                   std::shared_ptr<scene::SurfaceCoordinator> const& surface_coordinator);
-    virtual ~DefaultFocusMechanism() = default;
-
-    void set_focus_to(std::shared_ptr<scene::Session> const& new_focus);
-
-protected:
-    DefaultFocusMechanism(const DefaultFocusMechanism&) = delete;
-    DefaultFocusMechanism& operator=(const DefaultFocusMechanism&) = delete;
-
-private:
-    std::shared_ptr<InputTargeter> const input_targeter;
-    std::shared_ptr<scene::SurfaceCoordinator> const surface_coordinator;
-
-    std::mutex surface_focus_lock;
-    std::weak_ptr<scene::Surface> currently_focused_surface;
-};
-
-}
-}
-
-
-#endif // MIR_SHELL_SINGLE_VISIBILITY_FOCUS_MECHANISM_H_

=== modified file 'src/server/shell/default_shell.cpp'
--- src/server/shell/default_shell.cpp	2015-01-21 10:29:57 +0000
+++ src/server/shell/default_shell.cpp	2015-01-21 10:29:57 +0000
@@ -17,11 +17,12 @@
  */
 
 #include "default_shell.h"
-#include "mir/shell/focus_setter.h"
+#include "mir/shell/input_targeter.h"
 #include "mir/scene/prompt_session.h"
 #include "mir/scene/session_coordinator.h"
 #include "mir/scene/session.h"
 #include "mir/scene/surface.h"
+#include "mir/scene/surface_coordinator.h"
 
 #include <boost/throw_exception.hpp>
 
@@ -32,9 +33,11 @@
 namespace msh = mir::shell;
 
 msh::DefaultShell::DefaultShell(
-    std::shared_ptr<FocusSetter> const& focus_setter,
+    std::shared_ptr<InputTargeter> const& input_targeter,
+    std::shared_ptr<scene::SurfaceCoordinator> const& surface_coordinator,
     std::shared_ptr<scene::SessionCoordinator> const& session_coordinator) :
-    focus_setter(focus_setter),
+    input_targeter(input_targeter),
+    surface_coordinator(surface_coordinator),
     session_coordinator(session_coordinator)
 {
 }
@@ -164,7 +167,30 @@
 {
     auto old_focus = focus_application.lock();
 
-    focus_setter->set_focus_to(session);
+    std::shared_ptr<ms::Surface> surface;
+
+    if (session)
+        surface = session->default_surface();
+
+    if (surface)
+    {
+        std::lock_guard<std::mutex> lg(surface_focus_lock);
+
+        // Ensure the surface has really taken the focus before notifying it that it is focused
+        surface_coordinator->raise(surface);
+        surface->take_input_focus(input_targeter);
+
+        auto current_focus = currently_focused_surface.lock();
+        if (current_focus)
+            current_focus->configure(mir_surface_attrib_focus, mir_surface_unfocused);
+        surface->configure(mir_surface_attrib_focus, mir_surface_focused);
+        currently_focused_surface = surface;
+    }
+    else
+    {
+        input_targeter->focus_cleared();
+    }
+
     focus_application = session;
 
     if (session)

=== modified file 'src/server/shell/default_shell.h'
--- src/server/shell/default_shell.h	2015-01-21 10:29:57 +0000
+++ src/server/shell/default_shell.h	2015-01-21 10:29:57 +0000
@@ -26,17 +26,18 @@
 
 namespace mir
 {
-namespace scene { class SessionCoordinator; }
+namespace scene { class SessionCoordinator; class Surface; class SurfaceCoordinator; }
 
 namespace shell
 {
-class FocusSetter;
+class InputTargeter;
 
 class DefaultShell : public frontend::Shell, public FocusController
 {
 public:
     DefaultShell(
-        std::shared_ptr<FocusSetter> const& focus_setter,
+        std::shared_ptr<InputTargeter> const& input_targeter,
+        std::shared_ptr<scene::SurfaceCoordinator> const& surface_coordinator,
         std::shared_ptr<scene::SessionCoordinator> const& session_coordinator);
 
     void focus_next() override;
@@ -80,9 +81,13 @@
         MirSurfaceAttrib attrib) override;
 
 private:
-    std::shared_ptr<FocusSetter> const focus_setter;
+    std::shared_ptr<InputTargeter> const input_targeter;
+    std::shared_ptr<scene::SurfaceCoordinator> const surface_coordinator;
     std::shared_ptr<scene::SessionCoordinator> const session_coordinator;
 
+    std::mutex surface_focus_lock;
+    std::weak_ptr<scene::Surface> currently_focused_surface;
+
     std::mutex mutex;
     std::weak_ptr<scene::Session> focus_application;
 

=== modified file 'tests/acceptance-tests/throwback/test_focus_selection.cpp'
--- tests/acceptance-tests/throwback/test_focus_selection.cpp	2015-01-14 06:39:13 +0000
+++ tests/acceptance-tests/throwback/test_focus_selection.cpp	2015-01-21 10:29:57 +0000
@@ -18,19 +18,18 @@
 
 #include "clients.h"
 
-#include "src/server/scene/session_container.h"
-#include "src/server/scene/session_manager.h"
+#include "mir/shell/session_coordinator_wrapper.h"
 #include "mir/graphics/display.h"
 #include "mir/shell/input_targeter.h"
 
 #include "mir_test_framework/display_server_test_fixture.h"
-#include "mir_test_doubles/mock_focus_setter.h"
 #include "mir_test_doubles/mock_input_targeter.h"
 
 #include <gtest/gtest.h>
 #include <gmock/gmock.h>
 
 namespace mf = mir::frontend;
+namespace ms = mir::scene;
 namespace msh = mir::shell;
 namespace mi = mir::input;
 namespace mt = mir::test;
@@ -38,39 +37,59 @@
 namespace mt = mir::test;
 namespace mtf = mir_test_framework;
 
+using namespace testing;
+
 namespace
 {
 MATCHER(NonNullSession, "")
 {
     return arg.operator bool();
 }
+
+struct MockSessionManager : msh::SessionCoordinatorWrapper
+{
+    explicit MockSessionManager(std::shared_ptr<ms::SessionCoordinator> const& wrapped) :
+        msh::SessionCoordinatorWrapper{wrapped}
+    {
+        ON_CALL(*this, set_focus_to(_)).
+            WillByDefault(Invoke(this, &MockSessionManager::unmocked_set_focus_to));
+
+        ON_CALL(*this, unset_focus()).
+            WillByDefault(Invoke(this, &MockSessionManager::unmocked_unset_focus));
+}
+
+    MOCK_METHOD1(set_focus_to, void(std::shared_ptr<ms::Session> const& focus));
+    MOCK_METHOD0(unset_focus, void());
+
+private:
+    void unmocked_set_focus_to(std::shared_ptr<ms::Session> const& focus)
+    { msh::SessionCoordinatorWrapper::set_focus_to(focus); }
+
+    void unmocked_unset_focus()
+    { msh::SessionCoordinatorWrapper::unset_focus(); }
+};
 }
 
 TEST_F(BespokeDisplayServerTestFixture, sessions_creating_surface_receive_focus)
 {
     struct ServerConfig : TestingServerConfiguration
     {
-        std::shared_ptr<msh::FocusSetter>
-        the_shell_focus_setter() override
+        std::shared_ptr<ms::SessionCoordinator>
+        wrap_session_coordinator(
+            std::shared_ptr<ms::SessionCoordinator> const& wrapped) override
         {
-            return shell_focus_setter(
-            []
-            {
-                using namespace ::testing;
-
-                auto focus_setter = std::make_shared<mtd::MockFocusSetter>();
-                {
-                    InSequence seq;
-                    // Once on application registration and once on surface creation
-                    EXPECT_CALL(*focus_setter, set_focus_to(NonNullSession())).Times(2);
-                    // Focus is cleared when the session is closed
-                    EXPECT_CALL(*focus_setter, set_focus_to(_)).Times(1);
-                }
-                // TODO: Counterexample ~racarr
-
-                return focus_setter;
-            });
+            sm = std::make_shared<MockSessionManager>(wrapped);
+
+            InSequence seq;
+            // Once on application registration and once on surface creation
+            EXPECT_CALL(*sm, set_focus_to(NonNullSession())).Times(2);
+            // Focus is cleared when the session is closed
+            EXPECT_CALL(*sm, unset_focus()).Times(1);
+
+            return sm;
         }
+
+        std::shared_ptr<MockSessionManager> sm;
     } server_config;
 
     launch_server_process(server_config);

=== removed file 'tests/include/mir_test_doubles/mock_focus_setter.h'
--- tests/include/mir_test_doubles/mock_focus_setter.h	2015-01-14 06:39:13 +0000
+++ tests/include/mir_test_doubles/mock_focus_setter.h	1970-01-01 00:00:00 +0000
@@ -1,42 +0,0 @@
-/*
- * 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_FOCUS_SETTER_H_
-#define MIR_TEST_DOUBLES_MOCK_FOCUS_SETTER_H_
-
-#include "mir/shell/focus_setter.h"
-
-#include <gmock/gmock.h>
-
-namespace mir
-{
-namespace test
-{
-namespace doubles
-{
-
-struct MockFocusSetter : public shell::FocusSetter
-{
-    MOCK_METHOD1(set_focus_to, void(std::shared_ptr<scene::Session> const&));
-};
-
-}
-}
-} // namespace mir
-
-#endif // MIR_TEST_DOUBLES_MOCK_FOCUS_SETTER_H_

=== modified file 'tests/include/mir_test_doubles/mock_surface_coordinator.h'
--- tests/include/mir_test_doubles/mock_surface_coordinator.h	2015-01-14 06:39:13 +0000
+++ tests/include/mir_test_doubles/mock_surface_coordinator.h	2015-01-21 10:29:57 +0000
@@ -21,6 +21,7 @@
 #define MIR_TEST_DOUBLES_MOCK_SURFACE_COORDINATOR_H_
 
 #include "mir/scene/surface_coordinator.h"
+#include "mir/scene/surface_creation_parameters.h"
 
 #include <gmock/gmock.h>
 

=== modified file 'tests/integration-tests/test_default_shell.cpp'
--- tests/integration-tests/test_default_shell.cpp	2015-01-21 10:29:57 +0000
+++ tests/integration-tests/test_default_shell.cpp	2015-01-21 10:29:57 +0000
@@ -19,43 +19,48 @@
 #include "src/server/shell/default_shell.h"
 #include "src/server/scene/session_manager.h"
 #include "mir/scene/session.h"
-#include "mir/shell/focus_setter.h"
 #include "src/server/scene/default_session_container.h"
 #include "mir/scene/null_session_listener.h"
-#include "mir/compositor/buffer_stream.h"
-#include "src/server/scene/basic_surface.h"
-#include "mir/scene/surface_creation_parameters.h"
 
 #include "mir_test/gmock_fixes.h"
 #include "mir_test/fake_shared.h"
+#include "mir_test_doubles/mock_scene_session.h"
+#include "mir_test_doubles/mock_surface.h"
 #include "mir_test_doubles/mock_surface_coordinator.h"
-#include "mir_test_doubles/mock_focus_setter.h"
 #include "mir_test_doubles/null_snapshot_strategy.h"
 #include "mir_test_doubles/null_event_sink.h"
 #include "mir_test_doubles/null_session_event_sink.h"
 #include "mir_test_doubles/null_prompt_session_manager.h"
+#include "mir_test_doubles/mock_input_targeter.h"
 
 #include <gmock/gmock.h>
 #include <gtest/gtest.h>
 
-namespace mc = mir::compositor;
 namespace mf = mir::frontend;
 namespace msh = mir::shell;
 namespace ms = mir::scene;
 namespace mt = mir::test;
 namespace mtd = mir::test::doubles;
 
+using namespace ::testing;
+
 namespace
 {
+struct MockSessionManager : ms::SessionManager
+{
+    using ms::SessionManager::SessionManager;
+
+    MOCK_METHOD1(set_focus_to, void (std::shared_ptr<ms::Session> const& focus));
+};
 
 struct TestDefaultShellAndFocusSelectionStrategy : public testing::Test
 {
-    mtd::MockSurfaceCoordinator surface_coordinator; // TODO this isn't used as a mock
+    NiceMock<mtd::MockSurfaceCoordinator> surface_coordinator;
     ms::DefaultSessionContainer container;
-    mtd::MockFocusSetter focus_setter;
+    NiceMock<mtd::MockInputTargeter> input_targeter;
     std::shared_ptr<mf::Session> new_session;
     ms::NullSessionListener session_listener;
-    ms::SessionManager session_manager
+    NiceMock<MockSessionManager> session_manager
         {
             mt::fake_shared(surface_coordinator),
             mt::fake_shared(container),
@@ -65,62 +70,128 @@
             std::make_shared<mtd::NullPromptSessionManager>()
         };
 
-    msh::DefaultShell shell{mt::fake_shared(focus_setter), mt::fake_shared(session_manager)};
+    msh::DefaultShell shell{
+        mt::fake_shared(input_targeter),
+        mt::fake_shared(surface_coordinator),
+        mt::fake_shared(session_manager)};
 };
 }
 
 TEST_F(TestDefaultShellAndFocusSelectionStrategy, cycle_focus)
 {
-    using namespace ::testing;
-
-    EXPECT_CALL(focus_setter, set_focus_to(_)).Times(3);
+    EXPECT_CALL(session_manager, set_focus_to(_)).Times(3);
 
     auto session1 = shell.open_session(__LINE__, "Visual Basic Studio", std::make_shared<mtd::NullEventSink>());
     auto session2 = shell.open_session(__LINE__, "Microsoft Access", std::make_shared<mtd::NullEventSink>());
     auto session3 = shell.open_session(__LINE__, "WordPerfect", std::make_shared<mtd::NullEventSink>());
 
-    Mock::VerifyAndClearExpectations(&focus_setter);
+    Mock::VerifyAndClearExpectations(&session_manager);
 
     {
       InSequence seq;
-      EXPECT_CALL(focus_setter, set_focus_to(Eq(session1))).Times(1);
-      EXPECT_CALL(focus_setter, set_focus_to(Eq(session2))).Times(1);
-      EXPECT_CALL(focus_setter, set_focus_to(Eq(session3))).Times(1);
+      EXPECT_CALL(session_manager, set_focus_to(Eq(session1))).Times(1);
+      EXPECT_CALL(session_manager, set_focus_to(Eq(session2))).Times(1);
+      EXPECT_CALL(session_manager, set_focus_to(Eq(session3))).Times(1);
     }
 
     shell.focus_next();
     shell.focus_next();
     shell.focus_next();
 
-    Mock::VerifyAndClearExpectations(&focus_setter);
+    Mock::VerifyAndClearExpectations(&session_manager);
 
     // Possible change of focus while sessions are closed on shutdown
-    EXPECT_CALL(focus_setter, set_focus_to(_)).Times(AtLeast(0));
+    EXPECT_CALL(session_manager, set_focus_to(_)).Times(AnyNumber());
 }
 
 TEST_F(TestDefaultShellAndFocusSelectionStrategy, closing_applications_transfers_focus)
 {
     using namespace ::testing;
 
-    EXPECT_CALL(focus_setter, set_focus_to(_)).Times(3);
+    EXPECT_CALL(session_manager, set_focus_to(_)).Times(3);
 
     auto session1 = shell.open_session(__LINE__, "Visual Basic Studio", std::make_shared<mtd::NullEventSink>());
     auto session2 = shell.open_session(__LINE__, "Microsoft Access", std::make_shared<mtd::NullEventSink>());
     auto session3 = shell.open_session(__LINE__, "WordPerfect", std::make_shared<mtd::NullEventSink>());
 
-    Mock::VerifyAndClearExpectations(&focus_setter);
+    Mock::VerifyAndClearExpectations(&session_manager);
 
     {
       InSequence seq;
-      EXPECT_CALL(focus_setter, set_focus_to(Eq(session2))).Times(1);
-      EXPECT_CALL(focus_setter, set_focus_to(Eq(session1))).Times(1);
+      EXPECT_CALL(session_manager, set_focus_to(Eq(session2))).Times(1);
+      EXPECT_CALL(session_manager, set_focus_to(Eq(session1))).Times(1);
     }
 
     shell.close_session(session3);
     shell.close_session(session2);
 
-    Mock::VerifyAndClearExpectations(&focus_setter);
+    Mock::VerifyAndClearExpectations(&session_manager);
 
     // Possible change of focus while sessions are closed on shutdown
-    EXPECT_CALL(focus_setter, set_focus_to(_)).Times(AtLeast(0));
+    EXPECT_CALL(session_manager, set_focus_to(_)).Times(AtLeast(0));
+}
+
+TEST_F(TestDefaultShellAndFocusSelectionStrategy, mechanism_notifies_default_surface_of_focus_changes)
+{
+    using namespace ::testing;
+
+    NiceMock<mtd::MockSceneSession> app1, app2;
+    auto const mock_surface1 = std::make_shared<NiceMock<mtd::MockSurface>>();
+    auto const mock_surface2 = std::make_shared<NiceMock<mtd::MockSurface>>();
+
+    ON_CALL(app1, default_surface()).WillByDefault(Return(mock_surface1));
+    ON_CALL(app2, default_surface()).WillByDefault(Return(mock_surface2));
+
+    InSequence seq;
+    EXPECT_CALL(surface_coordinator, raise(_)).Times(1);
+    EXPECT_CALL(*mock_surface1, configure(mir_surface_attrib_focus, mir_surface_focused)).Times(1);
+    EXPECT_CALL(surface_coordinator, raise(_)).Times(1);
+    EXPECT_CALL(*mock_surface1, configure(mir_surface_attrib_focus, mir_surface_unfocused)).Times(1);
+    EXPECT_CALL(*mock_surface2, configure(mir_surface_attrib_focus, mir_surface_focused)).Times(1);
+
+    shell.set_focus_to(mt::fake_shared(app1));
+    shell.set_focus_to(mt::fake_shared(app2));
+}
+
+TEST_F(TestDefaultShellAndFocusSelectionStrategy, sets_input_focus)
+{
+    NiceMock<mtd::MockSceneSession> app1;
+    NiceMock<mtd::MockSurface> mock_surface;
+
+    {
+        InSequence seq;
+        EXPECT_CALL(app1, default_surface()).Times(1)
+            .WillOnce(Return(mt::fake_shared(mock_surface)));
+        EXPECT_CALL(app1, default_surface()).Times(1)
+            .WillOnce(Return(std::shared_ptr<ms::Surface>()));
+    }
+
+    {
+        InSequence seq;
+        EXPECT_CALL(mock_surface, take_input_focus(_)).Times(1);
+        // When we have no default surface.
+        EXPECT_CALL(input_targeter, focus_cleared()).Times(1);
+        // When we have no session.
+        EXPECT_CALL(input_targeter, focus_cleared()).Times(1);
+    }
+    EXPECT_CALL(surface_coordinator, raise(_)).Times(1);
+
+    shell.set_focus_to(mt::fake_shared(app1));
+    shell.set_focus_to(mt::fake_shared(app1));
+    shell.set_focus_to(std::shared_ptr<ms::Session>());
+}
+
+TEST_F(TestDefaultShellAndFocusSelectionStrategy, notifies_surface_of_focus_change_after_it_has_taken_the_focus)
+{
+    NiceMock<mtd::MockSceneSession> app;
+    auto const mock_surface = std::make_shared<NiceMock<mtd::MockSurface>>();
+
+    ON_CALL(app, default_surface()).WillByDefault(Return(mock_surface));
+
+
+    InSequence seq;
+    EXPECT_CALL(*mock_surface, take_input_focus(_));
+    EXPECT_CALL(*mock_surface, configure(mir_surface_attrib_focus, mir_surface_focused)).Times(1);
+
+    shell.set_focus_to(mt::fake_shared(app));
 }

=== modified file 'tests/unit-tests/scene/test_session_manager.cpp'
--- tests/unit-tests/scene/test_session_manager.cpp	2015-01-21 10:29:57 +0000
+++ tests/unit-tests/scene/test_session_manager.cpp	2015-01-21 10:29:57 +0000
@@ -224,14 +224,23 @@
 // TODO but as I'm reworking that interaction and they use mocks that only exist
 // TODO in this file (e.g. MockSessionContainer) I've left them here temporarily.
 #include "src/server/shell/default_shell.h"
-#include "src/server/shell/default_focus_mechanism.h"
-#include "mir_test_doubles/mock_focus_setter.h"
+#include "mir_test_doubles/stub_input_targeter.h"
 
 namespace msh = mir::shell;
 using namespace ::testing;
 
 namespace
 {
+struct MockSessionManager : ms::SessionManager
+{
+    using ms::SessionManager::SessionManager;
+
+    MOCK_METHOD1(set_focus_to, void (std::shared_ptr<ms::Session> const& focus));
+
+    void unmocked_set_focus_to(std::shared_ptr<ms::Session> const& focus)
+    { ms::SessionManager::set_focus_to(focus); }
+};
+
 struct DefaultShell : Test
 {
     mtd::MockSurfaceCoordinator surface_coordinator;
@@ -239,7 +248,7 @@
     NiceMock<MockSessionEventSink> session_event_sink;
     NiceMock<mtd::MockSessionListener> session_listener;
 
-    ms::SessionManager session_manager{
+    NiceMock<MockSessionManager> session_manager{
         mt::fake_shared(surface_coordinator),
         mt::fake_shared(container),
         std::make_shared<mtd::NullSnapshotStrategy>(),
@@ -247,12 +256,17 @@
         mt::fake_shared(session_listener),
         std::make_shared<mtd::NullPromptSessionManager>()};
 
-    NiceMock<mtd::MockFocusSetter> focus_setter;
-    msh::DefaultShell shell{mt::fake_shared(focus_setter), mt::fake_shared(session_manager)};
+    mtd::StubInputTargeter input_targeter;
+    msh::DefaultShell shell{
+        mt::fake_shared(input_targeter),
+        mt::fake_shared(surface_coordinator),
+        mt::fake_shared(session_manager)};
 
     void SetUp() override
     {
         ON_CALL(container, successor_of(_)).WillByDefault(Return((std::shared_ptr<ms::Session>())));
+        ON_CALL(session_manager, set_focus_to(_)).
+            WillByDefault(Invoke(&session_manager, &MockSessionManager::unmocked_set_focus_to));
     }
 };
 }
@@ -263,7 +277,7 @@
     std::shared_ptr<ms::Session> new_session;
 
     EXPECT_CALL(container, insert_session(_)).Times(1);
-    EXPECT_CALL(focus_setter, set_focus_to(_)).WillOnce(SaveArg<0>(&new_session));
+    EXPECT_CALL(session_manager, set_focus_to(_)).WillOnce(SaveArg<0>(&new_session));
 
     auto session = shell.open_session(__LINE__, "Visual Basic Studio", std::shared_ptr<mf::EventSink>());
     EXPECT_EQ(session, new_session);

=== modified file 'tests/unit-tests/shell/CMakeLists.txt'
--- tests/unit-tests/shell/CMakeLists.txt	2015-01-14 06:39:13 +0000
+++ tests/unit-tests/shell/CMakeLists.txt	2015-01-21 10:29:57 +0000
@@ -1,7 +1,6 @@
 list(APPEND UNIT_TEST_SOURCES
   ${CMAKE_CURRENT_SOURCE_DIR}/test_default_placement_strategy.cpp
   ${CMAKE_CURRENT_SOURCE_DIR}/test_graphics_display_layout.cpp
-  ${CMAKE_CURRENT_SOURCE_DIR}/test_default_focus_mechanism.cpp
 )
 
 set(

=== removed file 'tests/unit-tests/shell/test_default_focus_mechanism.cpp'
--- tests/unit-tests/shell/test_default_focus_mechanism.cpp	2015-01-14 06:39:13 +0000
+++ tests/unit-tests/shell/test_default_focus_mechanism.cpp	1970-01-01 00:00:00 +0000
@@ -1,126 +0,0 @@
-/*
- * 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]>
- */
-
-#include "src/server/scene/application_session.h"
-#include "src/server/shell/default_focus_mechanism.h"
-#include "mir/scene/session.h"
-#include "mir/scene/surface_creation_parameters.h"
-#include "src/server/scene/basic_surface.h"
-#include "mir/graphics/display_configuration.h"
-
-#include "mir_test/fake_shared.h"
-#include "mir_test_doubles/mock_buffer_stream.h"
-#include "mir_test_doubles/mock_scene_session.h"
-#include "mir_test_doubles/mock_surface.h"
-#include "mir_test_doubles/mock_surface_coordinator.h"
-#include "mir_test_doubles/stub_input_targeter.h"
-#include "mir_test_doubles/mock_input_targeter.h"
-
-#include <gmock/gmock.h>
-#include <gtest/gtest.h>
-#include <string>
-
-namespace mc = mir::compositor;
-namespace msh = mir::shell;
-namespace ms = mir::scene;
-namespace mf = mir::frontend;
-namespace mt = mir::test;
-namespace mtd = mir::test::doubles;
-
-TEST(DefaultFocusMechanism, mechanism_notifies_default_surface_of_focus_changes)
-{
-    using namespace ::testing;
-
-    NiceMock<mtd::MockSceneSession> app1, app2;
-    auto const mock_surface1 = std::make_shared<NiceMock<mtd::MockSurface>>();
-    auto const mock_surface2 = std::make_shared<NiceMock<mtd::MockSurface>>();
-    auto const surface_coordinator = std::make_shared<mtd::MockSurfaceCoordinator>();
-
-    ON_CALL(app1, default_surface()).WillByDefault(Return(mock_surface1));
-    ON_CALL(app2, default_surface()).WillByDefault(Return(mock_surface2));
-
-    msh::DefaultFocusMechanism focus_mechanism(
-        std::make_shared<mtd::StubInputTargeter>(),
-        surface_coordinator);
-
-    InSequence seq;
-    EXPECT_CALL(*surface_coordinator, raise(_)).Times(1);
-    EXPECT_CALL(*mock_surface1, configure(mir_surface_attrib_focus, mir_surface_focused)).Times(1);
-    EXPECT_CALL(*surface_coordinator, raise(_)).Times(1);
-    EXPECT_CALL(*mock_surface1, configure(mir_surface_attrib_focus, mir_surface_unfocused)).Times(1);
-    EXPECT_CALL(*mock_surface2, configure(mir_surface_attrib_focus, mir_surface_focused)).Times(1);
-
-    focus_mechanism.set_focus_to(mt::fake_shared(app1));
-    focus_mechanism.set_focus_to(mt::fake_shared(app2));
-}
-
-TEST(DefaultFocusMechanism, sets_input_focus)
-{
-    using namespace ::testing;
-
-    NiceMock<mtd::MockSceneSession> app1;
-    NiceMock<mtd::MockSurface> mock_surface;
-    auto const surface_coordinator = std::make_shared<mtd::MockSurfaceCoordinator>();
-
-    {
-        InSequence seq;
-        EXPECT_CALL(app1, default_surface()).Times(1)
-            .WillOnce(Return(mt::fake_shared(mock_surface)));
-        EXPECT_CALL(app1, default_surface()).Times(1)
-            .WillOnce(Return(std::shared_ptr<ms::Surface>()));
-    }
-
-    NiceMock<mtd::MockInputTargeter> targeter;
-
-    msh::DefaultFocusMechanism focus_mechanism(mt::fake_shared(targeter), surface_coordinator);
-
-    {
-        InSequence seq;
-        EXPECT_CALL(mock_surface, take_input_focus(_)).Times(1);
-        // When we have no default surface.
-        EXPECT_CALL(targeter, focus_cleared()).Times(1);
-        // When we have no session.
-        EXPECT_CALL(targeter, focus_cleared()).Times(1);
-    }
-    EXPECT_CALL(*surface_coordinator, raise(_)).Times(1);
-
-    focus_mechanism.set_focus_to(mt::fake_shared(app1));
-    focus_mechanism.set_focus_to(mt::fake_shared(app1));
-    focus_mechanism.set_focus_to(std::shared_ptr<ms::Session>());
-}
-
-TEST(DefaultFocusMechanism, notifies_surface_of_focus_change_after_it_has_taken_the_focus)
-{
-    using namespace ::testing;
-
-    NiceMock<mtd::MockSceneSession> app;
-    auto const mock_surface = std::make_shared<NiceMock<mtd::MockSurface>>();
-    auto const surface_coordinator = std::make_shared<NiceMock<mtd::MockSurfaceCoordinator>>();
-
-    ON_CALL(app, default_surface()).WillByDefault(Return(mock_surface));
-
-    msh::DefaultFocusMechanism focus_mechanism(
-        std::make_shared<mtd::StubInputTargeter>(),
-        surface_coordinator);
-
-    InSequence seq;
-    EXPECT_CALL(*mock_surface, take_input_focus(_));
-    EXPECT_CALL(*mock_surface, configure(mir_surface_attrib_focus, mir_surface_focused)).Times(1);
-
-    focus_mechanism.set_focus_to(mt::fake_shared(app));
-}