feedback: replacing z_order with is_above

src/include/server/mir/shell/abstract_shell.h

@@ -132,7 +132,7 @@ class AbstractShell : public virtual Shell, public virtual FocusController
     void raise(SurfaceSet const& surfaces) override;
     void swap_z_order(SurfaceSet const& first, SurfaceSet const& second) override;
     void send_to_back(SurfaceSet const& surfaces) override;
-    auto z_order(std::weak_ptr<scene::Surface> const& surface) const -> unsigned int override;
+    auto is_above(std::weak_ptr<scene::Surface> const& a, std::weak_ptr<scene::Surface> const& b) const -> bool override;
 /** @} */
 
     void add_display(geometry::Rectangle const& area) override;

src/include/server/mir/shell/focus_controller.h

@@ -59,7 +59,9 @@ class FocusController
 
     virtual void send_to_back(SurfaceSet const& surfaces) = 0;
 
-    virtual auto z_order(std::weak_ptr<scene::Surface> const& surface) const -> unsigned int = 0;
+    /// Returns true if surface [a] is above surface [b].
+    virtual auto is_above(std::weak_ptr<scene::Surface> const& a, std::weak_ptr<scene::Surface> const& b) const
+        -> bool = 0;
 
 protected:
     FocusController() = default;

src/include/server/mir/shell/shell_wrapper.h

@@ -49,7 +49,7 @@ class ShellWrapper : public Shell
 
     void send_to_back(SurfaceSet const& surfaces) override;
 
-    auto z_order(std::weak_ptr<scene::Surface> const& surface) const -> unsigned int override;
+    auto is_above(std::weak_ptr<scene::Surface> const& a, std::weak_ptr<scene::Surface> const& b) const -> bool override;
 
     auto open_session(
         pid_t client_pid,

src/include/server/mir/shell/surface_stack.h

@@ -55,7 +55,8 @@ class SurfaceStack
 
     virtual void send_to_back(scene::SurfaceSet const& surfaces) = 0;
 
-    virtual auto z_order(std::weak_ptr<scene::Surface> const& surface) const -> unsigned int = 0;
+    virtual auto is_above(std::weak_ptr<scene::Surface> const& a, std::weak_ptr<scene::Surface> const& b) const
+        -> bool = 0;
 
 protected:
     SurfaceStack() = default;

src/include/server/mir/shell/surface_stack_wrapper.h

@@ -44,7 +44,7 @@ class SurfaceStackWrapper : public SurfaceStack
 
     void send_to_back(scene::SurfaceSet const& surfaces) override;
 
-    auto z_order(std::weak_ptr<scene::Surface> const& surface) const -> unsigned int override;
+    auto is_above(std::weak_ptr<scene::Surface> const& a, std::weak_ptr<scene::Surface> const& b) const -> bool override;
 
 protected:
     std::shared_ptr<SurfaceStack> const wrapped;

src/server/scene/surface_stack.cpp

@@ -493,27 +493,38 @@ void ms::SurfaceStack::send_to_back(const mir::scene::SurfaceSet &ss)
     }
 }
 
-auto ms::SurfaceStack::z_order(std::weak_ptr<scene::Surface> const& surface) const -> unsigned int
+auto ms::SurfaceStack::is_above(std::weak_ptr<scene::Surface> const& a, std::weak_ptr<scene::Surface> const& b) const -> bool
 {
     RecursiveReadLock lg(guard);
-    if (surface.expired())
-    {
-        mir::log_error("Could not find z_order for surface because surface is expired");
-        return 0;
-    }
-
-    auto const lock_surface = surface.lock();
-    auto const& layer = surface_layers[lock_surface->depth_layer()];
+    if (a.expired())
+        return false;
+    else if (b.expired())
+        return true;
+
+    auto const shared_a = a.lock();
+    auto const shared_b = b.lock();
+    if (shared_a->depth_layer() < shared_b->depth_layer())
+        return false;
+    else if (shared_a->depth_layer() > shared_b->depth_layer())
+        return true;
+
+    auto const& layer = surface_layers[shared_a->depth_layer()];
+    std::optional<size_t> a_index;
+    std::optional<size_t> b_index;
     for (size_t surface_index = 0; surface_index < layer.size(); surface_index++)
     {
-        auto const& other = layer[surface_index];
-        if (surface.lock() == other)
-            return surface_index;
+        if (shared_a == layer[surface_index])
+            a_index = surface_index;
+        else if (shared_b == layer[surface_index])
+            b_index = surface_index;
     }
 
+    if (!a_index.has_value())
+        return false;
+    else if (!b_index.has_value())
+        return true;
 
-    mir::log_error("Could not find z_order for surface in the stack");
-    return 0;
+    return a_index.value() > b_index.value();
 }
 
 void ms::SurfaceStack::create_rendering_tracker_for(std::shared_ptr<Surface> const& surface)

src/server/scene/surface_stack.h

@@ -101,7 +101,8 @@ class SurfaceStack :
     void raise(SurfaceSet const& surfaces) override;
     void swap_z_order(SurfaceSet const& first, SurfaceSet const& second) override;
     void send_to_back(SurfaceSet const& surfaces) override;
-    auto z_order(std::weak_ptr<scene::Surface> const& surface) const -> unsigned int override;
+    auto is_above(std::weak_ptr<scene::Surface> const& a, std::weak_ptr<scene::Surface> const& b) const
+        -> bool override;
 
     void add_surface(
         std::shared_ptr<Surface> const& surface,

src/server/shell/abstract_shell.cpp

@@ -736,7 +736,8 @@ void msh::AbstractShell::send_to_back(const mir::shell::SurfaceSet &surfaces)
     surface_stack->send_to_back(surfaces);
 }
 
-auto msh::AbstractShell::z_order(std::weak_ptr<scene::Surface> const& surface) const -> unsigned int
+auto msh::AbstractShell::is_above(std::weak_ptr<scene::Surface> const& a, std::weak_ptr<scene::Surface> const& b) const
+    -> bool
 {
-    return surface_stack->z_order(surface);
+   return surface_stack->is_above(a, b);
 }

src/server/shell/shell_wrapper.cpp

@@ -182,7 +182,8 @@ void msh::ShellWrapper::send_to_back(const mir::shell::SurfaceSet &surfaces)
     return wrapped->send_to_back(surfaces);
 }
 
-auto msh::ShellWrapper::z_order(std::weak_ptr<scene::Surface> const& surface) const -> unsigned int
+auto msh::ShellWrapper::is_above(std::weak_ptr<scene::Surface> const& a, std::weak_ptr<scene::Surface> const& b) const
+    -> bool
 {
-    return wrapped->z_order(surface);
-}
+    return wrapped->is_above(a, b);
+}

src/server/shell/surface_stack_wrapper.cpp

@@ -64,7 +64,9 @@ void msh::SurfaceStackWrapper::send_to_back(const scene::SurfaceSet &surfaces)
     wrapped->send_to_back(surfaces);
 }
 
-auto msh::SurfaceStackWrapper::z_order(std::weak_ptr<scene::Surface> const& surface) const -> unsigned int
+auto msh::SurfaceStackWrapper::is_above(
+    std::weak_ptr<scene::Surface> const& a,
+    std::weak_ptr<scene::Surface> const& b) const -> bool
 {
-    return wrapped->z_order(surface);
+    return wrapped->is_above(a, b);
 }

src/server/symbols.map

@@ -1407,14 +1407,14 @@ local: *;
 MIR_SERVER_INTERNAL_2.20 {
 global:
   extern "C++" {
-    mir::shell::AbstractShell::z_order*;
-    mir::shell::ShellWrapper::z_order*;
-    mir::shell::SurfaceStackWrapper::z_order*;
-    non-virtual?thunk?to?mir::shell::AbstractShell::z_order*;
-    non-virtual?thunk?to?mir::shell::ShellWrapper::z_order*;
-    non-virtual?thunk?to?mir::shell::SurfaceStackWrapper::z_order*;
-    virtual?thunk?to?mir::shell::AbstractShell::z_order*;
-    virtual?thunk?to?mir::shell::ShellWrapper::z_order*;
+    mir::shell::AbstractShell::is_above*;
+    mir::shell::ShellWrapper::is_above*;
+    mir::shell::SurfaceStackWrapper::is_above*;
+    non-virtual?thunk?to?mir::shell::AbstractShell::is_above*;
+    non-virtual?thunk?to?mir::shell::ShellWrapper::is_above*;
+    non-virtual?thunk?to?mir::shell::SurfaceStackWrapper::is_above*;
+    virtual?thunk?to?mir::shell::AbstractShell::is_above*;
+    virtual?thunk?to?mir::shell::ShellWrapper::is_above*;
   };
 } MIR_SERVER_INTERNAL_2.19;
 

tests/include/mir/test/doubles/mock_surface_stack.h

@@ -40,7 +40,7 @@ struct MockSurfaceStack : public shell::SurfaceStack
     MOCK_CONST_METHOD1(surface_at, std::shared_ptr<scene::Surface>(geometry::Point));
     MOCK_METHOD2(swap_z_order, void(scene::SurfaceSet const&, scene::SurfaceSet const&));
     MOCK_METHOD1(send_to_back, void(scene::SurfaceSet const&));
-    MOCK_CONST_METHOD1(z_order, unsigned int(std::weak_ptr<scene::Surface> const& surface));
+    MOCK_CONST_METHOD2(is_above, bool(std::weak_ptr<scene::Surface> const& a, std::weak_ptr<scene::Surface> const& b));
 };
 
 }

tests/include/mir/test/doubles/stub_shell.h

@@ -96,9 +96,9 @@ struct StubShell : public shell::Shell
     {
     }
 
-    auto z_order(std::weak_ptr<scene::Surface> const& /*surface*/) const -> unsigned int override
+    auto is_above(std::weak_ptr<scene::Surface> const& /*a*/, std::weak_ptr<scene::Surface> const& /*b*/) const -> bool override
     {
-        return 0;
+        return false;
     }
     /// @}
 

tests/include/mir_test_framework/window_management_test_harness.h

@@ -60,7 +60,7 @@ class WindowManagementTestHarness : public mir_test_framework::HeadlessInProcess
 
     auto focused_surface() -> std::shared_ptr<mir::scene::Surface>;
     auto tools() -> miral::WindowManagerTools const&;
-    auto z_order(miral::Window const&) const -> unsigned int;
+    auto is_above(miral::Window const& a, miral::Window const& b) const -> bool;
 
     virtual auto get_builder() -> WindowManagementPolicyBuilder = 0;
     virtual auto get_output_rectangles() -> std::vector<mir::geometry::Rectangle> = 0;

tests/mir_test_framework/window_management_test_harness.cpp

@@ -205,7 +205,8 @@ auto mir_test_framework::WindowManagementTestHarness::tools() -> miral::WindowMa
     return self->tools;
 }
 
-auto mir_test_framework::WindowManagementTestHarness::z_order(miral::Window const& window) const -> unsigned int
+auto mir_test_framework::WindowManagementTestHarness::is_above(
+    miral::Window const& a, miral::Window const& b) const -> bool
 {
-    return server.the_shell()->z_order(window.operator std::shared_ptr<ms::Surface>());
-}
+    return server.the_shell()->is_above(a.operator std::shared_ptr<ms::Surface>(), b.operator std::shared_ptr<ms::Surface>());
+}

tests/miral/test_window_manager_tools.cpp

@@ -65,8 +65,10 @@ struct StubFocusController : mir::shell::FocusController
 
     void send_to_back(mir::shell::SurfaceSet const& /*windows*/) override {}
 
-    auto z_order(std::weak_ptr<mir::scene::Surface> const&) const -> unsigned int override
-    { return 0; }
+    auto is_above(std::weak_ptr<mir::scene::Surface> const& /*a*/, std::weak_ptr<mir::scene::Surface> const& /*b*/) const -> bool override
+    {
+        return false;
+    }
 };
 
 struct StubDisplayLayout : mir::shell::DisplayLayout

tests/unit-tests/scene/test_application_session.cpp

@@ -94,9 +94,9 @@ struct StubSurfaceStack : public msh::SurfaceStack
     {
     }
 
-    auto z_order(std::weak_ptr<ms::Surface> const&) const -> unsigned int override
+    auto is_above(std::weak_ptr<ms::Surface> const& /*a*/, std::weak_ptr<ms::Surface> const& /*b*/) const -> bool override
     {
-        return 0;
+        return false;
     }
 };
 

tests/window_management_tests/test_minimal_window_manager.cpp

@@ -499,7 +499,7 @@ TEST_F(MinimalWindowManagerTest, can_resize_south_east_with_touch)
     EXPECT_EQ(window.size(), geom::Size(50, 50));
 }
 
-TEST_F(MinimalWindowManagerTest,  new_windows_are_inserted_at_a_higher_z_index)
+TEST_F(MinimalWindowManagerTest,  new_windows_are_inserted_above_older_windows)
 {
     auto const app1 = open_application("app1");
     auto const app2 = open_application("app2");
@@ -512,16 +512,11 @@ TEST_F(MinimalWindowManagerTest,  new_windows_are_inserted_at_a_higher_z_index)
     auto window2 = create_window(app2, spec);
     auto window3 = create_window(app3, spec);
 
-    auto window1_position = z_order(window1);
-    auto window2_position = z_order(window2);
-    auto window3_position = z_order(window3);
-
-    EXPECT_EQ(window1_position, 0);
-    EXPECT_EQ(window2_position, 1);
-    EXPECT_EQ(window3_position, 2);
+    EXPECT_TRUE(is_above(window3, window2));
+    EXPECT_TRUE(is_above(window2, window1));
 }
 
-TEST_F(MinimalWindowManagerTest,  when_a_window_is_focused_then_it_appears_higher_in_the_z_order)
+TEST_F(MinimalWindowManagerTest,  when_a_window_is_focused_then_it_appears_above_other_windows)
 {
     auto const app1 = open_application("app1");
     auto const app2 = open_application("app2");
@@ -539,10 +534,6 @@ TEST_F(MinimalWindowManagerTest,  when_a_window_is_focused_then_it_appears_highe
     auto surface1 = window1.operator std::shared_ptr<ms::Surface>();
     EXPECT_EQ(focused_surface(), surface1);
 
-    auto window1_position = z_order(window1);
-    auto window2_position = z_order(window2);
-    auto window3_position = z_order(window3);
-    EXPECT_EQ(window1_position, 2);
-    EXPECT_EQ(window2_position, 0);
-    EXPECT_EQ(window3_position, 1);
+    EXPECT_TRUE(is_above(window1, window3));
+    EXPECT_TRUE(is_above(window3, window2));
 }