Merge lp:~alan-griffiths/miral/fix-1671072 into lp:miral

[Alan Griffiths]

[libmiral] When a dialog is being hidden don't consider it a candidate active window

[Gerry Boland (gerboland)]

Why is this dialog specific? Is it the modality that is the cause?

review: Needs Information

[lp:~alan-griffiths/miral/fix-1671072]

540. By Alan Griffiths on 2017-03-16

Update changelog

[Alan Griffiths (alan-griffiths)]

> Why is this dialog specific? Is it the modality that is the cause?

Yes. When a window has a child dialog then we consider the dialog for focus first. And the logic there didn't adequately handle the scenario where the child is being hidden.

[lp:~alan-griffiths/miral/fix-1671072]

541. By Alan Griffiths on 2017-03-16

Reduce diff

542. By Alan Griffiths on 2017-03-16

ActiveWindow.when_another_window_is_about_hiding_active_dialog_makes_parent_active

543. By Alan Griffiths on 2017-03-16

Test passing

[Kevin DuBois (kdub)]

lgtm

review: Approve

[Brandon Schaefer (brandontschaefer)]

lgtm

review: Approve

[Brandon Schaefer (brandontschaefer)]

Confirmed fixes the bug

=== modified file 'debian/changelog'
--- debian/changelog	2017-03-15 17:44:36 +0000
+++ debian/changelog	2017-03-16 17:00:52 +0000
@@ -11,6 +11,8 @@
         miral-shell can crash, or hang on exit (LP: #1673038)
       . [miral-shell] if the specified font doesn't exist the server crashes
         (LP: #1671028)
+      . [libmiral] When a dialog is hidden ensure that the active window focus
+        goes to the parent. (LP: #1671072)
 
  -- Alan Griffiths <[e-mail]>  Mon, 06 Mar 2017 10:00:20 +0000
 
 
=== modified file 'miral/basic_window_manager.cpp'
--- miral/basic_window_manager.cpp	2017-03-06 09:11:47 +0000
+++ miral/basic_window_manager.cpp	2017-03-16 17:00:52 +0000
@@ -1090,29 +1090,35 @@
     {
     case mir_window_state_hidden:
     case mir_window_state_minimized:
+        window_info.state(value);
         if (window == active_window())
         {
-            auto const workspaces_containing_window = workspaces_containing(window);
-
-            // Try to activate to recently active window of any application
-            mru_active_windows.enumerate([&](Window& candidate)
-                {
-                    if (candidate == window)
-                        return true;
-                    auto const w = candidate;
-                    for (auto const& workspace : workspaces_containing(w))
+            select_active_window(window);
+
+            if (window == active_window() || !active_window())
+            {
+                auto const workspaces_containing_window = workspaces_containing(window);
+
+                // Try to activate to recently active window of any application
+                mru_active_windows.enumerate([&](Window& candidate)
                     {
-                        for (auto const& ww : workspaces_containing_window)
+                        if (candidate == window)
+                            return true;
+                        auto const w = candidate;
+                        for (auto const& workspace : workspaces_containing(w))
                         {
-                            if (ww == workspace)
+                            for (auto const& ww : workspaces_containing_window)
                             {
-                                return !(select_active_window(w));
+                                if (ww == workspace)
+                                {
+                                    return !(select_active_window(w));
+                                }
                             }
                         }
-                    }
 
-                    return true;
-                });
+                        return true;
+                    });
+            }
 
             // Try to activate to recently active window of any application
             if (window == active_window() || !active_window())
@@ -1128,7 +1134,6 @@
                 select_active_window({});
         }
 
-        window_info.state(value);
         mir_surface->configure(mir_window_attrib_state, value);
         mir_surface->hide();
 
@@ -1205,11 +1210,10 @@
 
     for (auto const& child : info_for_hint.children())
     {
-        if (std::shared_ptr<mir::scene::Surface> surface = child)
-        {
-            if (surface->type() == mir_window_type_dialog && surface->visible())
-                return select_active_window(child);
-        }
+        auto const& info_for_child = info_for(child);
+
+        if (info_for_child.type() == mir_window_type_dialog && info_for_child.is_visible())
+            return select_active_window(child);
     }
 
     if (info_for_hint.can_be_active() && info_for_hint.is_visible())
 
=== modified file 'test/active_window.cpp'
--- test/active_window.cpp	2017-03-03 16:17:37 +0000
+++ test/active_window.cpp	2017-03-16 17:00:52 +0000
@@ -360,3 +360,39 @@
 
     assert_active_window_is(test_name);
 }
+
+// lp:1671072
+TEST_F(ActiveWindow, hiding_active_dialog_makes_parent_active)
+{
+    char const* const parent_name = __PRETTY_FUNCTION__;
+    auto const dialog_name = "dialog";
+    auto const connection = connect_client(parent_name);
+
+    auto const parent = create_surface(connection, parent_name, sync1);
+    auto const dialog = create_dialog(connection, dialog_name, parent, sync2);
+
+    sync1.exec([&]{ mir_window_set_state(dialog, mir_window_state_hidden); });
+
+    EXPECT_TRUE(sync1.signal_raised());
+
+    assert_active_window_is(parent_name);
+}
+
+TEST_F(ActiveWindow, when_another_window_is_about_hiding_active_dialog_makes_parent_active)
+{
+    FocusChangeSync sync3;
+    char const* const parent_name = __PRETTY_FUNCTION__;
+    auto const dialog_name = "dialog";
+    auto const another_window_name = "another window";
+    auto const connection = connect_client(parent_name);
+
+    auto const parent = create_surface(connection, parent_name, sync1);
+    auto const another_window = create_surface(connection, another_window_name, sync2);
+    auto const dialog = create_dialog(connection, dialog_name, parent, sync3);
+
+    sync1.exec([&]{ mir_window_set_state(dialog, mir_window_state_hidden); });
+
+    EXPECT_TRUE(sync1.signal_raised());
+
+    assert_active_window_is(parent_name);
+}