Merge lp:~alan-griffiths/mir/fix-1454128-by-getting-select_active_surface-right into lp:mir

[Alan Griffiths]

shell, examples: Fix the select_active_surface() implementation(s) in msh::CanonicalWindowManagerPolicy and me::CanonicalWindowManagerPolicyCopy (fixes lp:1454128)

shell, examples: Fix the select_active_surface() implementation(s) in msh::CanonicalWindowManagerPolicy and me::CanonicalWindowManagerPolicyCopy (fixes lp:1454128)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:2568
http://jenkins.qa.ubuntu.com/job/mir-ci/3808/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2442
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/2441/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2390/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1805/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2390
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2390/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5270/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/20401

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

review: Needs Fixing (continuous-integration)

[Alan Griffiths (alan-griffiths)]

I made some changes to TestDefaultWindowManager.sets_input_focus because:

/1/ using two different shared_ptr<>s for the one mock_surface is just wrong.
/2/ And the second call to input_targeter.clear_focus()) when focus is already cleared shouldn't have been happening.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:2571
http://jenkins.qa.ubuntu.com/job/mir-ci/3815/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-android-vivid-i386-build/2451
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-clang-vivid-amd64-build/2450
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-vivid-touch/2399
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1812
        deb: http://jenkins.qa.ubuntu.com/job/mir-vivid-amd64-ci/1812/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2399
        deb: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-builder-vivid-armhf/2399/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/mir-mediumtests-runner-mako/5279
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/20421

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

review: Approve (continuous-integration)

[Alexandros Frantzis (afrantzis)]

Looks good.

review: Approve

[Kevin DuBois (kdub)]

lgtm

review: Approve

[Daniel van Vugt (vanvugt)]

Confirmed bug fixed.

Ran into bug 1452579 again, but that's unrelated.

review: Approve

=== modified file 'examples/server_example_canonical_window_manager.cpp'
--- examples/server_example_canonical_window_manager.cpp	2015-05-08 15:50:18 +0000
+++ examples/server_example_canonical_window_manager.cpp	2015-05-12 16:14:17 +0000
@@ -418,6 +418,7 @@
 
     if (!--tools->info_for(session).surfaces && session == tools->focused_session())
     {
+        active_surface_.reset();
         tools->focus_next_session();
         select_active_surface(tools->focused_surface());
     }
@@ -690,16 +691,19 @@
     if (surface == active_surface_.lock())
         return;
 
-    if (auto const active_surface = active_surface_.lock())
+    if (!surface)
     {
-        if (auto const titlebar = tools->info_for(active_surface).titlebar)
+        if (auto const active_surface = active_surface_.lock())
         {
-            paint_titlebar(titlebar, tools->info_for(titlebar), 0x3F);
+            if (auto const titlebar = tools->info_for(active_surface).titlebar)
+            {
+                paint_titlebar(titlebar, tools->info_for(titlebar), 0x3F);
+            }
         }
-    }
-
-    if (!surface)
-    {
+
+        if (active_surface_.lock())
+            tools->set_focus_to({}, {});
+
         active_surface_.reset();
         return;
     }
@@ -715,6 +719,13 @@
     case mir_surface_type_freestyle:
     case mir_surface_type_menu:
     case mir_surface_type_inputmethod:  /**< AKA "OSK" or handwriting etc.       */
+        if (auto const active_surface = active_surface_.lock())
+        {
+            if (auto const titlebar = tools->info_for(active_surface).titlebar)
+            {
+                paint_titlebar(titlebar, tools->info_for(titlebar), 0x3F);
+            }
+        }
         if (auto const titlebar = tools->info_for(surface).titlebar)
         {
             paint_titlebar(titlebar, tools->info_for(titlebar), 0xFF);

=== modified file 'src/server/shell/abstract_shell.cpp'
--- src/server/shell/abstract_shell.cpp	2015-04-17 14:33:09 +0000
+++ src/server/shell/abstract_shell.cpp	2015-05-12 16:14:17 +0000
@@ -169,35 +169,41 @@
     std::shared_ptr<ms::Session> const& session,
     std::shared_ptr<ms::Surface> const& surface)
 {
-    if (surface)
+    auto const current_focus = focus_surface.lock();
+
+    if (surface != current_focus)
     {
-        auto current_focus = focus_surface.lock();
-
-        if (surface != current_focus)
+        focus_surface = surface;
+
+        if (current_focus)
+            current_focus->configure(mir_surface_attrib_focus, mir_surface_unfocused);
+
+        if (surface)
         {
             // Ensure the surface has really taken the focus before notifying it that it is focused
             input_targeter->set_focus(surface);
-            if (current_focus)
-                current_focus->configure(mir_surface_attrib_focus, mir_surface_unfocused);
-
             surface->configure(mir_surface_attrib_focus, mir_surface_focused);
-            focus_surface = surface;
-        }
-    }
-    else
-    {
-        input_targeter->clear_focus();
-    }
-
-    focus_session = session;
-
-    if (session)
-    {
-        session_coordinator->set_focus_to(session);
-    }
-    else
-    {
-        session_coordinator->unset_focus();
+        }
+        else
+        {
+            input_targeter->clear_focus();
+        }
+    }
+
+    auto const current_session = focus_session.lock();
+
+    if (session != current_session)
+    {
+        focus_session = session;
+
+        if (session)
+        {
+            session_coordinator->set_focus_to(session);
+        }
+        else
+        {
+            session_coordinator->unset_focus();
+        }
     }
 }
 

=== modified file 'src/server/shell/canonical_window_manager.cpp'
--- src/server/shell/canonical_window_manager.cpp	2015-05-08 23:34:48 +0000
+++ src/server/shell/canonical_window_manager.cpp	2015-05-12 16:14:17 +0000
@@ -196,11 +196,15 @@
     public std::enable_shared_from_this<SurfaceReadyObserver>
 {
 public:
+    using ActivateFunction = std::function<void(
+        std::shared_ptr<ms::Session> const& session,
+        std::shared_ptr<ms::Surface> const& surface)>;
+
     SurfaceReadyObserver(
-        msh::CanonicalWindowManagerPolicy::Tools* const focus_controller,
+        ActivateFunction const& activate,
         std::shared_ptr<ms::Session> const& session,
         std::shared_ptr<ms::Surface> const& surface) :
-        focus_controller{focus_controller},
+        activate{activate},
         session{session},
         surface{surface}
     {
@@ -211,12 +215,12 @@
     {
         if (auto const s = surface.lock())
         {
-            focus_controller->set_focus_to(session.lock(), s);
+            activate(session.lock(), s);
             s->remove_observer(shared_from_this());
         }
     }
 
-    msh::CanonicalWindowManagerPolicy::Tools* const focus_controller;
+    ActivateFunction const activate;
     std::weak_ptr<ms::Session> const session;
     std::weak_ptr<ms::Surface> const surface;
 };
@@ -243,8 +247,14 @@
         // TODO There's currently no way to insert surfaces into an active (or inactive)
         // TODO window tree while keeping the order stable or consistent with spec.
         // TODO Nor is there a way to update the "default surface" when appropriate!!
-        surface->add_observer(std::make_shared<SurfaceReadyObserver>(tools, session, surface));
-        active_surface_ = surface;
+        surface->add_observer(std::make_shared<SurfaceReadyObserver>(
+            [this](std::shared_ptr<scene::Session> const& /*session*/,
+                   std::shared_ptr<scene::Surface> const& surface)
+                {
+                    select_active_surface(surface);
+                },
+            session,
+            surface));
         break;
 
     case mir_surface_type_gloss:
@@ -317,12 +327,11 @@
         }
     }
 
-
     if (!--tools->info_for(session).surfaces && session == tools->focused_session())
     {
+        active_surface_.reset();
         tools->focus_next_session();
-        if (auto const surface = tools->focused_surface())
-            tools->raise({surface});
+        select_active_surface(tools->focused_surface());
     }
 }
 
@@ -550,6 +559,9 @@
 {
     if (!surface)
     {
+        if (active_surface_.lock())
+            tools->set_focus_to({}, {});
+
         active_surface_.reset();
         return;
     }

=== modified file 'tests/integration-tests/test_default_window_manager.cpp'
--- tests/integration-tests/test_default_window_manager.cpp	2015-05-04 19:18:26 +0000
+++ tests/integration-tests/test_default_window_manager.cpp	2015-05-12 16:14:17 +0000
@@ -157,16 +157,15 @@
     mtd::StubSceneSession app1;
     NiceMock<mtd::MockSurface> mock_surface;
 
+    auto const surface = mt::fake_shared(mock_surface);
     {
         InSequence seq;
-        EXPECT_CALL(input_targeter, set_focus(Eq(mt::fake_shared(mock_surface)))).Times(1);
+        EXPECT_CALL(input_targeter, set_focus(Eq(surface))).Times(1);
         // When we have no default surface.
         EXPECT_CALL(input_targeter, clear_focus()).Times(1);
-        // When we have no session.
-        EXPECT_CALL(input_targeter, clear_focus()).Times(1);
     }
 
-    shell.set_focus_to(mt::fake_shared(app1), mt::fake_shared(mock_surface));
+    shell.set_focus_to(mt::fake_shared(app1), surface);
     shell.set_focus_to(mt::fake_shared(app1), std::shared_ptr<ms::Surface>());
     shell.set_focus_to(std::shared_ptr<ms::Session>(), std::shared_ptr<ms::Surface>());
 }