Merge lp:~alan-griffiths/miral/mru-window-visiblity into lp:miral

[Alan Griffiths]

Make the MRU window list aware of visiblity, instead of faking it

Make the MRU window list aware of visiblity, instead of faking it

So far we "got away" with removing and re-adding windows when they became invisible. But the existing code didn't scale to more than one window. (This became painfully obvious in the workspace example branch.)

[lp:~alan-griffiths/miral/mru-window-visiblity]

515. By Alan Griffiths on 2017-02-17

Fix Xenial build

[Gerry Boland (gerboland)]

Is there a specific bug this is fixing?

review: Needs Information

[Gerry Boland (gerboland)]

It looks reasonable, am just a bit unclear on exactly the problem case this is fixing. A test case would help me understand.

[Alan Griffiths (alan-griffiths)]

> It looks reasonable, am just a bit unclear on exactly the problem case this is
> fixing. A test case would help me understand.

Yeah, adding a test case makes sense. I was just wanting to split this out of the "example" MP.

The issue is that when we hide a bunch of surfaces and then unhide them we don't want to lose track of their order. That's a existing problem but became very obvious when playing with workspaces.

[lp:~alan-griffiths/miral/mru-window-visiblity]

516. By Alan Griffiths on 2017-02-20

merge :parent

517. By Alan Griffiths on 2017-02-20

Some tests

[Alan Griffiths (alan-griffiths)]

> It looks reasonable, am just a bit unclear on exactly the problem case this is
> fixing. A test case would help me understand.

Without this the workspaces-example gets very confused when switching back to a workspace with multiple windows. So to see the problem you'd need to extract this code from that branch.

Under the hood, the mru list gets out of step with the Z order in the Mir "model" (as we don't guarantee to make windows visible in that order).

Medium term I think this logic needs to be better integrated with the Mir "model", but this papers over the cracks.

We need a lot more tests around the focus logic, I'm tempted to suggest leaving that for a follow-up branch.

[Gerry Boland (gerboland)]

Ok, I think I understand the issue & where you're going, so IMO you've added enough tests to cover this bit. Agreed deeper integration with Mir's model would be desirable in future.

review: Approve

[Gerry Boland (gerboland)]

"visiblity" or visibility?

=== modified file 'miral/basic_window_manager.cpp'
--- miral/basic_window_manager.cpp	2017-02-13 16:23:29 +0000
+++ miral/basic_window_manager.cpp	2017-02-20 18:05:53 +0000
@@ -1041,16 +1041,11 @@
                             window_info.state() == mir_window_state_minimized;
 
     policy->advise_state_change(window_info, value);
-    window_info.state(value);
-
-    mir_surface->configure(mir_window_attrib_state, value);
 
     switch (value)
     {
     case mir_window_state_hidden:
     case mir_window_state_minimized:
-        mir_surface->hide();
-
         if (window == active_window())
         {
             auto const workspaces_containing_window = workspaces_containing(window);
@@ -1087,15 +1082,23 @@
 
             if (window == active_window())
                 select_active_window({});
-
-            mru_active_windows.erase(window);
         }
+
+        window_info.state(value);
+        mir_surface->configure(mir_window_attrib_state, value);
+        mir_surface->hide();
+
         break;
 
     default:
+        auto const none_active = !active_window();
+        window_info.state(value);
+        mir_surface->configure(mir_window_attrib_state, value);
         mir_surface->show();
-        if (was_hidden && !active_window())
+        if (was_hidden && none_active)
+        {
             select_active_window(window);
+        }
     }
 }
 
@@ -1920,13 +1923,13 @@
 
     std::function<void(WindowInfo const& info)> const add_children =
         [&,this](WindowInfo const& info)
+        {
+            for (auto const& child : info.children())
             {
-                for (auto const& child : info.children())
-                {
-                    windows.push_back(child);
-                    add_children(info_for(child));
-                }
-            };
+                windows.push_back(child);
+                add_children(info_for(child));
+            }
+        };
 
     windows.push_back(root);
     add_children(*info);

=== modified file 'miral/mru_window_list.cpp'
--- miral/mru_window_list.cpp	2017-02-02 11:31:52 +0000
+++ miral/mru_window_list.cpp	2017-02-20 18:05:53 +0000
@@ -18,8 +18,28 @@
 
 #include "mru_window_list.h"
 
+#include <mir/client/detail/mir_forward_compatibility.h>
+#include <mir/scene/surface.h>
+
 #include <algorithm>
 
+namespace
+{
+bool visible(miral::Window const& window)
+{
+    std::shared_ptr<mir::scene::Surface> const& surface{window};
+
+    switch (surface->state())
+    {
+    case mir_window_state_hidden:
+    case mir_window_state_minimized:
+        return false;
+    default:
+        return surface->visible();
+    }
+}
+}
+
 void miral::MRUWindowList::push(Window const& window)
 {
     windows.erase(remove(begin(windows), end(windows), window), end(windows));
@@ -33,12 +53,14 @@
 
 auto miral::MRUWindowList::top() const -> Window
 {
-    return (!windows.empty()) ? windows.back() : Window{};
+    auto const& found = std::find_if(rbegin(windows), rend(windows), visible);
+    return (found != rend(windows)) ? *found: Window{};
 }
 
 void miral::MRUWindowList::enumerate(Enumerator const& enumerator) const
 {
     for (auto i = windows.rbegin(); i != windows.rend(); ++i)
-        if (!enumerator(const_cast<Window&>(*i)))
-            break;
+        if (visible(*i))
+            if (!enumerator(const_cast<Window&>(*i)))
+                break;
 }

=== modified file 'test/mru_window_list.cpp'
--- test/mru_window_list.cpp	2016-08-05 08:53:45 +0000
+++ test/mru_window_list.cpp	2017-02-20 18:05:53 +0000
@@ -24,17 +24,23 @@
 #include <gtest/gtest.h>
 #include <gmock/gmock.h>
 
-using StubSurface = mir::test::doubles::StubSurface;
 using namespace testing;
 
 namespace
 {
+struct StubSurface : mir::test::doubles::StubSurface
+{
+    bool visible() const override { return visible_; }
+
+    bool visible_ = true;
+};
+
 struct StubSession : mir::test::doubles::StubSession
 {
     StubSession(int number_of_surfaces)
     {
         for (auto i = 0; i != number_of_surfaces; ++i)
-            surfaces.push_back(std::make_shared<mir::test::doubles::StubSurface>());
+            surfaces.push_back(std::make_shared<StubSurface>());
     }
 
     std::shared_ptr<mir::scene::Surface> surface(mir::frontend::SurfaceId surface) const override
@@ -53,13 +59,26 @@
 
 struct MRUWindowList : testing::Test
 {
+    static auto const window_a_id = 0;
+    static auto const window_b_id = 1;
+
     miral::MRUWindowList mru_list;
 
     std::shared_ptr<StubSession> const stub_session{std::make_shared<StubSession>(3)};
     miral::Application app{stub_session};
-    miral::Window window_a{app, stub_session->surface(mir::frontend::SurfaceId{0})};
-    miral::Window window_b{app, stub_session->surface(mir::frontend::SurfaceId{1})};
+    miral::Window window_a{app, stub_session->surface(mir::frontend::SurfaceId{window_a_id})};
+    miral::Window window_b{app, stub_session->surface(mir::frontend::SurfaceId{window_b_id})};
     miral::Window window_c{app, stub_session->surface(mir::frontend::SurfaceId{2})};
+
+    void hide_window(int window_id)
+    {
+        stub_session->surfaces[window_id]->visible_ = false;
+    }
+
+    void show_window(int window_id)
+    {
+        stub_session->surfaces[window_id]->visible_ = true;
+    }
 };
 
 TEST_F(MRUWindowList, when_created_is_empty)
@@ -137,3 +156,38 @@
 
     EXPECT_THAT(count, Eq(1));
 }
+
+TEST_F(MRUWindowList, a_hidden_window_is_not_enumerated)
+{
+    mru_list.push(window_a);
+    mru_list.push(window_b);
+    mru_list.push(window_c);
+
+    int count{0};
+
+    hide_window(window_a_id);
+    mru_list.enumerate([&](miral::Window& window)
+       { if (window == window_a) ++count; return true; });
+
+    EXPECT_THAT(count, Eq(0));
+}
+
+TEST_F(MRUWindowList, hiding_then_showing_windows_retains_order)
+{
+    mru_list.push(window_a);
+    mru_list.push(window_b);
+    mru_list.push(window_c);
+
+    hide_window(window_a_id);
+    hide_window(window_b_id);
+    show_window(window_a_id);
+    show_window(window_b_id);
+
+    std::vector<miral::Window> as_enumerated;
+
+    mru_list.enumerate([&](miral::Window& window)
+       { as_enumerated.push_back(window); return true; });
+
+    EXPECT_THAT(as_enumerated, ElementsAre(window_c, window_b, window_a));
+}
+