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

[Alan Griffiths]

[libmiral] top-level window is now raised along with its child (LP: #1658085)

[Daniel d'Andrada (dandrader)]

In the case described in the bug report, I get two raises:

- First one containing kate's top-level window and its "Open File" dialog.
- Then another containing juts the "Open File" dialog.

I should receive only the first raise notification. The second one is superfluous.

I suppose it should be easy for you to check if a window is already top level and thus avoid raising it?

review: Needs Fixing

[Alan Griffiths (alan-griffiths)]

> In the case described in the bug report, I get two raises:
>
> - First one containing kate's top-level window and its "Open File" dialog.
> - Then another containing juts the "Open File" dialog.
>
> I should receive only the first raise notification. The second one is
> superfluous.
>
> I suppose it should be easy for you to check if a window is already top level
> and thus avoid raising it?

In this simple case, yes. But consider:

A parents B,C

Z order is DCBA

Click on B

I expect Z order to be: BCAD

Which transformation, AFAICS can only be expressed with multiple notifications (both to the Mir model and to the Qt model). Vis:

advise_raise (ABC)
advise_raise (B)

[Daniel d'Andrada (dandrader)]

On 20/01/2017 15:53, Alan Griffiths wrote:
>> In the case described in the bug report, I get two raises:
>>
>> - First one containing kate's top-level window and its "Open File" dialog.
>> - Then another containing juts the "Open File" dialog.
>>
>> I should receive only the first raise notification. The second one is
>> superfluous.
>>
>> I suppose it should be easy for you to check if a window is already top level
>> and thus avoid raising it?
> In this simple case, yes. But consider:
>
> A parents B,C
>
> Z order is DCBA
>
> Click on B
>
> I expect Z order to be: BCAD
>
> Which transformation, AFAICS can only be expressed with multiple notifications (both to the Mir model and to the Qt model). Vis:
>
> advise_raise (ABC)
> advise_raise (B)

Indeed. But the question remains: Could you check if a raise is indeed
needed (ie, window in question not top-level) before issuing the raise?
A noop raise notification will bother the upper layers of the stack for
nothing.

[Alan Griffiths (alan-griffiths)]

To do that I would need to change the Mir interface to determine if a "raise" is a NoOp. I don't have direct access to the model.

[Daniel d'Andrada (dandrader)]

> To do that I would need to change the Mir interface to determine if a "raise"
> is a NoOp. I don't have direct access to the model.

Fair enough.

review: Approve

=== modified file 'debian/changelog'
--- debian/changelog	2017-01-20 11:17:36 +0000
+++ debian/changelog	2017-01-20 15:10:26 +0000
@@ -8,6 +8,7 @@
         enums.
       . Logging of exceptions added to --window-management-trace
     - Bugs fixed:
+      . top-level window is not raised along with its child (LP: #1658085)
 
  -- Alan Griffiths <[e-mail]>  Wed, 14 Dec 2016 16:31:08 +0000
 

=== modified file 'miral/basic_window_manager.cpp'
--- miral/basic_window_manager.cpp	2017-01-20 11:17:36 +0000
+++ miral/basic_window_manager.cpp	2017-01-20 15:10:26 +0000
@@ -496,6 +496,11 @@
 
 void miral::BasicWindowManager::raise_tree(Window const& root)
 {
+    auto const& info = info_for(root);
+
+    if (auto parent = info.parent())
+        raise_tree(parent);
+
     std::vector<Window> windows;
 
     std::function<void(WindowInfo const& info)> const add_children =
@@ -509,7 +514,7 @@
             };
 
     windows.push_back(root);
-    add_children(info_for(root));
+    add_children(info);
 
     policy->advise_raise(windows);
     focus_controller->raise({begin(windows), end(windows)});

=== modified file 'test/CMakeLists.txt'
--- test/CMakeLists.txt	2017-01-18 10:14:04 +0000
+++ test/CMakeLists.txt	2017-01-20 15:10:26 +0000
@@ -54,6 +54,7 @@
     test_window_manager_tools.h
     display_reconfiguration.cpp
     active_window.cpp
+    raise_tree.cpp
 )
 
 target_link_libraries(miral-test

=== added file 'test/raise_tree.cpp'
--- test/raise_tree.cpp	1970-01-01 00:00:00 +0000
+++ test/raise_tree.cpp	2017-01-20 15:10:26 +0000
@@ -0,0 +1,95 @@
+/*
+ * Copyright © 2017 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: Alan Griffiths <[e-mail]>
+ */
+
+#include "test_window_manager_tools.h"
+
+using namespace miral;
+using namespace testing;
+namespace mt = mir::test;
+
+namespace
+{
+X const display_left{0};
+Y const display_top{0};
+Width  const display_width{640};
+Height const display_height{480};
+
+Rectangle const display_area{{display_left, display_top}, {display_width, display_height}};
+
+struct RaiseTree : TestWindowManagerTools
+{
+    Size const initial_parent_size{600, 400};
+    Size const initial_child_size{300, 300};
+    Rectangle const rectangle_away_from_rhs{{20, 20}, {20, 20}};
+    Rectangle const rectangle_near_rhs{{590, 20}, {10, 20}};
+    Rectangle const rectangle_away_from_bottom{{20, 20}, {20, 20}};
+    Rectangle const rectangle_near_bottom{{20, 380}, {20, 20}};
+    Rectangle const rectangle_near_both_sides{{0, 20}, {600, 20}};
+    Rectangle const rectangle_near_both_sides_and_bottom{{0, 380}, {600, 20}};
+    Rectangle const rectangle_near_all_sides{{0, 20}, {600, 380}};
+    Rectangle const rectangle_near_both_bottom_right{{400, 380}, {200, 20}};
+
+    Window parent;
+    Window child;
+    Window another_window;
+
+    WindowSpecification modification;
+
+    void SetUp() override
+    {
+        basic_window_manager.add_display(display_area);
+
+        mir::scene::SurfaceCreationParameters creation_parameters;
+        basic_window_manager.add_session(session);
+
+        EXPECT_CALL(*window_manager_policy, advise_new_window(_))
+            .WillOnce(Invoke([this](WindowInfo const& window_info){ parent = window_info.window(); }))
+            .WillOnce(Invoke([this](WindowInfo const& window_info){ child = window_info.window(); }))
+            .WillOnce(Invoke([this](WindowInfo const& window_info){ another_window = window_info.window(); }));
+
+        creation_parameters.size = initial_parent_size;
+        basic_window_manager.add_surface(session, creation_parameters, &create_surface);
+
+        creation_parameters.type = mir_window_type_menu;
+        creation_parameters.parent = parent;
+        creation_parameters.size = initial_child_size;
+        basic_window_manager.add_surface(session, creation_parameters, &create_surface);
+
+        creation_parameters.type = mir_window_type_normal;
+        creation_parameters.parent.reset();
+        creation_parameters.size = display_area.size;
+        basic_window_manager.add_surface(session, creation_parameters, &create_surface);
+
+        // Clear the expectations used to capture parent & child
+        Mock::VerifyAndClearExpectations(window_manager_policy);
+    }
+};
+}
+
+TEST_F(RaiseTree, when_parent_is_raised_child_is_raised)
+{
+    EXPECT_CALL(*window_manager_policy, advise_raise(ElementsAre(parent, child)));
+    basic_window_manager.raise_tree(parent);
+}
+
+TEST_F(RaiseTree, when_child_is_raised_parent_is_raised)
+{
+    EXPECT_CALL(*window_manager_policy, advise_raise(ElementsAre(parent, child)));
+    EXPECT_CALL(*window_manager_policy, advise_raise(ElementsAre(child)));
+    basic_window_manager.raise_tree(child);
+}

=== modified file 'test/test_window_manager_tools.h'
--- test/test_window_manager_tools.h	2017-01-18 14:56:39 +0000
+++ test/test_window_manager_tools.h	2017-01-20 15:10:26 +0000
@@ -140,6 +140,7 @@
     MOCK_METHOD1(advise_new_window, void (miral::WindowInfo const& window_info));
     MOCK_METHOD2(advise_move_to, void(miral::WindowInfo const& window_info, mir::geometry::Point top_left));
     MOCK_METHOD2(advise_resize, void(miral::WindowInfo const& window_info, mir::geometry::Size const& new_size));
+    MOCK_METHOD1(advise_raise, void(std::vector<miral::Window> const&));
 };
 
 struct TestWindowManagerTools : testing::Test
@@ -158,7 +159,7 @@
         mir::test::fake_shared(persistent_surface_store),
         [this](miral::WindowManagerTools const& tools) -> std::unique_ptr<miral::WindowManagementPolicy>
             {
-                auto policy = std::make_unique<MockWindowManagerPolicy>(tools);
+                auto policy = std::make_unique<testing::NiceMock<MockWindowManagerPolicy>>(tools);
                 window_manager_policy = policy.get();
                 window_manager_tools = tools;
                 return std::move(policy);