Merge lp:~alan-griffiths/miral/fix-focus-when-active-window-closes into lp:miral

[Alan Griffiths]

Fix focus loss when the active window closes

Fix focus loss when the active window closes

Easiest example:

$ miral-shell --startup gnome-mahjongg

Show the "About" window and close it. Expect focus to return to the main window - and now it does.

[Cemil Azizoglu (cemil-azizoglu)]

LGTM

review: Approve

[lp:~alan-griffiths/miral/fix-focus-when-active-window-closes]

155. By Alan Griffiths on 2016-04-19

Hoist common code from XXX::handle_delete_window() to calling context in BasicWindowManager::remove_surface()

156. By Alan Griffiths on 2016-04-19

Make selecting an active window part of the interface

157. By Alan Griffiths on 2016-04-19

Put refocus-on-close logic into BasicWindowManager::remove_surface()

158. By Alan Griffiths on 2016-04-19

Make clang happy

=== modified file 'include/miral/window_management_policy.h'
--- include/miral/window_management_policy.h	2016-04-14 13:55:48 +0000
+++ include/miral/window_management_policy.h	2016-04-19 16:34:41 +0000
@@ -46,6 +46,8 @@
     virtual bool handle_pointer_event(MirPointerEvent const* event) = 0;
     virtual void handle_raise_window(WindowInfo& window_info) = 0;
 
+    virtual auto select_active_window(miral::Window const& hint) -> miral::Window = 0;
+
     virtual ~WindowManagementPolicy() = default;
     WindowManagementPolicy() = default;
     WindowManagementPolicy(WindowManagementPolicy const&) = delete;

=== modified file 'miral-kiosk/kiosk_window_manager.cpp'
--- miral-kiosk/kiosk_window_manager.cpp	2016-04-15 09:39:08 +0000
+++ miral-kiosk/kiosk_window_manager.cpp	2016-04-19 16:34:41 +0000
@@ -137,43 +137,8 @@
         window_info.window.rename(modifications.name.value());
 }
 
-void KioskWindowManagerPolicy::handle_delete_window(WindowInfo& window_info)
+void KioskWindowManagerPolicy::handle_delete_window(WindowInfo& /*window_info*/)
 {
-    auto const session = window_info.window.session();
-    auto const& window = window_info.window;
-
-    if (auto const parent = window_info.parent)
-    {
-        auto& siblings = tools->info_for(parent).children;
-
-        for (auto i = begin(siblings); i != end(siblings); ++i)
-        {
-            if (window == *i)
-            {
-                siblings.erase(i);
-                break;
-            }
-        }
-    }
-
-    auto& windows = tools->info_for(session).windows;
-
-    for (auto i = begin(windows); i != end(windows); ++i)
-    {
-        if (window == *i)
-        {
-            windows.erase(i);
-            break;
-        }
-    }
-
-    window_info.window.destroy_surface();
-
-    if (windows.empty() && session == tools->focused_application())
-    {
-        tools->focus_next_application();
-        select_active_window(tools->focused_window());
-    }
 }
 
 auto KioskWindowManagerPolicy::handle_set_state(WindowInfo& window_info, MirSurfaceState value)

=== modified file 'miral-kiosk/kiosk_window_manager.h'
--- miral-kiosk/kiosk_window_manager.h	2016-04-15 09:39:08 +0000
+++ miral-kiosk/kiosk_window_manager.h	2016-04-19 16:34:41 +0000
@@ -59,6 +59,8 @@
 
     void generate_decorations_for(miral::WindowInfo& window_info) override;
 
+    auto select_active_window(miral::Window const& window) -> miral::Window override;
+
 private:
     static const int modifier_mask =
         mir_input_event_modifier_alt |
@@ -67,7 +69,6 @@
         mir_input_event_modifier_ctrl |
         mir_input_event_modifier_meta;
 
-    auto select_active_window(miral::Window const& window) -> miral::Window;
     auto transform_set_state(miral::WindowInfo& window_info, MirSurfaceState value) -> MirSurfaceState;
     void raise_splash_session() const;
 

=== modified file 'miral-shell/canonical_window_manager.cpp'
--- miral-shell/canonical_window_manager.cpp	2016-04-15 10:20:08 +0000
+++ miral-shell/canonical_window_manager.cpp	2016-04-19 16:34:41 +0000
@@ -381,24 +381,7 @@
 
 void CanonicalWindowManagerPolicy::handle_delete_window(WindowInfo& window_info)
 {
-    auto const& session = window_info.window.session();
-    auto& window = window_info.window;
-
-    fullscreen_surfaces.erase(window);
-
-    if (auto const parent = window_info.parent)
-    {
-        auto& siblings = tools->info_for(parent).children;
-
-        for (auto i = begin(siblings); i != end(siblings); ++i)
-        {
-            if (window == *i)
-            {
-                siblings.erase(i);
-                break;
-            }
-        }
-    }
+    fullscreen_surfaces.erase(window_info.window);
 
     if (auto const titlebar = std::static_pointer_cast<CanonicalWindowManagementPolicyData>(window_info.userdata))
     {
@@ -406,25 +389,7 @@
         tools->forget(titlebar->window);
     }
 
-    auto& windows = tools->info_for(session).windows;
-
-    for (auto i = begin(windows); i != end(windows); ++i)
-    {
-        if (window == *i)
-        {
-            windows.erase(i);
-            break;
-        }
-    }
-
-    window.destroy_surface();
-
-    if (windows.empty() && session == tools->focused_application())
-    {
-        active_window_.reset();
-        tools->focus_next_application();
-        select_active_window(tools->focused_window());
-    }
+    active_window_.reset();
 }
 
 auto CanonicalWindowManagerPolicy::handle_set_state(WindowInfo& window_info, MirSurfaceState value)
@@ -756,12 +721,12 @@
     }
 }
 
-void CanonicalWindowManagerPolicy::select_active_window(Window const& window)
+auto CanonicalWindowManagerPolicy::select_active_window(Window const& hint) -> miral::Window
 {
-    if (window == active_window_)
-        return;
+    if (hint == active_window_)
+        return hint ;
 
-    if (!window)
+    if (!hint)
     {
         if (auto const active_surface = active_window_)
         {
@@ -776,10 +741,10 @@
             tools->set_focus_to({});
 
         active_window_.reset();
-        return;
+        return hint;
     }
 
-    auto const& info_for = tools->info_for(window);
+    auto const& info_for = tools->info_for(hint);
 
     if (info_for.can_be_active())
     {
@@ -791,7 +756,7 @@
                 titlebar->paint_titlebar(0x3F);
             }
         }
-        auto& info = tools->info_for(window);
+        auto& info = tools->info_for(hint);
         if (auto const titlebar = std::static_pointer_cast<CanonicalWindowManagementPolicyData>(info.userdata))
         {
             titlebar->paint_titlebar(0xFF);
@@ -808,13 +773,17 @@
             if (spinner_info.windows.size() > 0)
                 tools->raise_tree(spinner_info.windows[0]);
         }
+
+        return hint;
     }
     else
     {
         // Cannot have input focus - try the parent
         if (auto const parent = info_for.parent)
-            select_active_window(parent);
+            return select_active_window(parent);
     }
+
+    return {};
 }
 
 auto CanonicalWindowManagerPolicy::active_window() const

=== modified file 'miral-shell/canonical_window_manager.h'
--- miral-shell/canonical_window_manager.h	2016-04-15 10:20:08 +0000
+++ miral-shell/canonical_window_manager.h	2016-04-19 16:34:41 +0000
@@ -75,6 +75,8 @@
 
     void generate_decorations_for(miral::WindowInfo& window_info) override;
 
+    auto select_active_window(miral::Window const& hint) -> miral::Window override;
+
 private:
     static const int modifier_mask =
         mir_input_event_modifier_alt |
@@ -88,7 +90,6 @@
     void resize(Point cursor);
     void toggle(MirSurfaceState state);
 
-    void select_active_window(miral::Window const& window);
     auto active_window() const -> miral::Window;
 
     bool resize(miral::Window const& window, Point cursor, Point old_cursor);

=== modified file 'miral-shell/tiling_window_manager.cpp'
--- miral-shell/tiling_window_manager.cpp	2016-04-14 14:37:35 +0000
+++ miral-shell/tiling_window_manager.cpp	2016-04-19 16:34:41 +0000
@@ -177,43 +177,8 @@
         window_info.window.rename(modifications.name.value());
 }
 
-void TilingWindowManagerPolicy::handle_delete_window(WindowInfo& window_info)
+void TilingWindowManagerPolicy::handle_delete_window(WindowInfo& /*window_info*/)
 {
-    auto const session = window_info.window.session();
-    auto const& window = window_info.window;
-
-    if (auto const parent = window_info.parent)
-    {
-        auto& siblings = tools->info_for(parent).children;
-
-        for (auto i = begin(siblings); i != end(siblings); ++i)
-        {
-            if (window == *i)
-            {
-                siblings.erase(i);
-                break;
-            }
-        }
-    }
-
-    auto& windows = tools->info_for(session).windows;
-
-    for (auto i = begin(windows); i != end(windows); ++i)
-    {
-        if (window == *i)
-        {
-            windows.erase(i);
-            break;
-        }
-    }
-
-    window_info.window.destroy_surface();
-
-    if (windows.empty() && session == tools->focused_application())
-    {
-        tools->focus_next_application();
-        select_active_window(tools->focused_window());
-    }
 }
 
 auto TilingWindowManagerPolicy::handle_set_state(WindowInfo& window_info, MirSurfaceState value)

=== modified file 'miral-shell/tiling_window_manager.h'
--- miral-shell/tiling_window_manager.h	2016-04-14 14:16:54 +0000
+++ miral-shell/tiling_window_manager.h	2016-04-19 16:34:41 +0000
@@ -67,6 +67,8 @@
 
     void generate_decorations_for(miral::WindowInfo& window_info) override;
 
+    auto select_active_window(miral::Window const& window) -> miral::Window override;
+
 private:
     static const int modifier_mask =
         mir_input_event_modifier_alt |
@@ -85,7 +87,6 @@
     void update_tiles(Rectangles const& displays);
     void update_surfaces(miral::ApplicationInfo& info, Rectangle const& old_tile, Rectangle const& new_tile);
     void drag(miral::WindowInfo& window_info, Point to, Point from, Rectangle bounds);
-    auto select_active_window(miral::Window const& window) -> miral::Window;
     auto transform_set_state(miral::WindowInfo& window_info, MirSurfaceState value) -> MirSurfaceState;
 
     static void clip_to_tile(mir::scene::SurfaceCreationParameters& parameters, Rectangle const& tile);

=== modified file 'miral/basic_window_manager.cpp'
--- miral/basic_window_manager.cpp	2016-04-15 09:11:31 +0000
+++ miral/basic_window_manager.cpp	2016-04-19 16:34:41 +0000
@@ -103,13 +103,68 @@
 }
 
 void miral::BasicWindowManager::remove_surface(
-    std::shared_ptr<scene::Session> const& /*session*/,
+    std::shared_ptr<scene::Session> const& session,
     std::weak_ptr<scene::Surface> const& surface)
 {
     std::lock_guard<decltype(mutex)> lock(mutex);
-    policy->handle_delete_window(info_for(surface));
-
+    bool const is_active_window{surface.lock() == focus_controller->focused_surface()};
+
+    auto& info = info_for(surface);
+
+    if (auto const parent = info.parent)
+    {
+        auto& siblings = info_for(parent).children;
+
+        for (auto i = begin(siblings); i != end(siblings); ++i)
+        {
+            if (info.window == *i)
+            {
+                siblings.erase(i);
+                break;
+            }
+        }
+    }
+
+    auto& windows = info_for(session).windows;
+
+    for (auto i = begin(windows); i != end(windows); ++i)
+    {
+        if (info.window == *i)
+        {
+            windows.erase(i);
+            break;
+        }
+    }
+
+    policy->handle_delete_window(info);
+
+    session->destroy_surface(surface);
+
+    auto const parent = info.parent;
+
+    // NB this invalidates info, but we want to keep access to "parent".
     window_info.erase(surface);
+
+    if (is_active_window)
+    {
+        // Try to make the parent active
+        if (parent)
+        {
+            if (policy->select_active_window(parent))
+                return;
+        }
+
+        // Ought to find top window of same application, but we don't
+        // have the API (yet), so find any suitable top-level-window
+        for (auto const& tlw : windows)
+        {
+            if (policy->select_active_window(tlw))
+                return;
+        }
+
+        focus_next_application();
+        policy->select_active_window(focused_window());
+    }
 }
 
 void miral::BasicWindowManager::forget(Window const& window)