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

[Alan Griffiths]

[libmiral] Allow the WM policy a say in inherited moves

Allow the WM policy a say in inherited moves

Movement of window "trees" is an area that needs more test around it, especially when tips and menus are involved. I aim to get to that, but landing this could be helpful already.

[Daniel d'Andrada (dandrader)]

I would prefer if shell could give miral enough context information (eg, the available desktop area) so that it could make correct decision on its own. But maybe that doesn't match miral's API style.

But confirm_inherited_move() works as well and is probably safer in case something other than the available desktop area happens to affect this decision.

What I wonder is how much value miral is adding in this specific case (moving child windows) with this API. What's more work for shell: validating every single child movement or implementing inherited child movement on its own?

review: Approve

[Alan Griffiths (alan-griffiths)]

I'd like Gerry's take on this too (see below).

> I would prefer if shell could give miral enough context information (eg, the
> available desktop area) so that it could make correct decision on its own. But
> maybe that doesn't match miral's API style.
>
> But confirm_inherited_move() works as well and is probably safer in case
> something other than the available desktop area happens to affect this
> decision.

Exactly, my concern is that the "available desktop area" isn't a simple concept that MirAL can handle.

In the "tiling" case it is the tile associated with the window.

In Unity8 it is the available region(s) on any outputs (but not associated with any specific windows).

In a lot of window managers there's no constraint - windows can be moved offscreen.

> What I wonder is how much value miral is adding in this specific case (moving
> child windows) with this API. What's more work for shell: validating every
> single child movement or implementing inherited child movement on its own?

I'm imagining that qtmir can implement the "available desktop area" concept as needed by Unity8 and that there's no other work needed further up the stack.

[Daniel d'Andrada (dandrader)]

On 05/12/2016 09:07, Alan Griffiths wrote:
> I'm imagining that qtmir can implement the "available desktop area" concept as needed by Unity8 and that there's no other work needed further up the stack.

Yes, that's how I plan to do it.

[Gerry Boland (gerboland)]

Having the "available desktop area" concept as something in the shell-specific policy makes sense to me. Unity8 does have different requirements to a tiling WM in this area.

I like Alan's suggestion, let's see how do-able it is.

=== modified file 'debian/libmiral1.symbols'
--- debian/libmiral1.symbols	2016-11-29 12:59:36 +0000
+++ debian/libmiral1.symbols	2016-12-05 09:52:38 +0000
@@ -315,3 +315,5 @@
  (c++)"miral::DebugExtension::DebugExtension()@MIRAL_0.5" 0.5.0
  (c++)"miral::DebugExtension::operator=(miral::DebugExtension const&)@MIRAL_0.5" 0.5.0
  (c++)"miral::DebugExtension::operator()(mir::Server&) const@MIRAL_0.5" 0.5.0
+ MIRAL_0.6@MIRAL_0.6 0.6.0
+ (c++)"miral::CanonicalWindowManagerPolicy::confirm_inherited_move(miral::WindowInfo const&, mir::geometry::Displacement)@MIRAL_0.6" 0.6.0

=== modified file 'include/miral/canonical_window_manager.h'
--- include/miral/canonical_window_manager.h	2016-08-02 16:20:54 +0000
+++ include/miral/canonical_window_manager.h	2016-12-05 09:52:38 +0000
@@ -49,6 +49,9 @@
     /// Raises the window (and any children)
     void advise_focus_gained(WindowInfo const& info) override;
 
+    /// Move the child window with the parent
+    auto confirm_inherited_move(WindowInfo const& window_info, Displacement movement) -> Rectangle override;
+
 protected:
     WindowManagerTools tools;
 };

=== modified file 'include/miral/window_management_policy.h'
--- include/miral/window_management_policy.h	2016-11-17 14:05:54 +0000
+++ include/miral/window_management_policy.h	2016-12-05 09:52:38 +0000
@@ -19,6 +19,7 @@
 #ifndef MIRAL_WINDOW_MANAGEMENT_POLICY_H
 #define MIRAL_WINDOW_MANAGEMENT_POLICY_H
 
+#include <mir/geometry/displacement.h>
 #include <mir/geometry/rectangles.h>
 #include <mir_toolkit/event.h>
 
@@ -172,6 +173,15 @@
     virtual void advise_raise(std::vector<Window> const& windows);
 /** @} */
 
+    /** Confirm (and optionally adjust) the motion of a child window when the parent is moved.
+     *
+     * @param window_info   the window
+     * @param movement      the movement of the parent
+     *
+     * @return the confirmed placement of the window
+     */
+    virtual auto confirm_inherited_move(WindowInfo const& window_info, Displacement movement) -> Rectangle = 0;
+
     virtual ~WindowManagementPolicy() = default;
     WindowManagementPolicy() = default;
     WindowManagementPolicy(WindowManagementPolicy const&) = delete;

=== modified file 'miral-shell/tiling_window_manager.cpp'
--- miral-shell/tiling_window_manager.cpp	2016-11-17 14:54:47 +0000
+++ miral-shell/tiling_window_manager.cpp	2016-12-05 09:52:38 +0000
@@ -615,6 +615,19 @@
     tiles.erase(application.userdata());
     dirty_tiles = true;
 }
+
+auto TilingWindowManagerPolicy::confirm_inherited_move(miral::WindowInfo const& window_info, Displacement movement)
+-> Rectangle
+{
+    auto const& window = window_info.window();
+    WindowSpecification mods;
+    mods.top_left() = window.top_left() + movement;
+    constrain_size_and_place(mods, window, tile_for(window_info));
+    auto pos  = mods.top_left().is_set() ? mods.top_left().value() : window.top_left();
+    auto size = mods.size().is_set()     ? mods.size().value()     : window.size();
+    return {pos, size};
+}
+
 void TilingWindowManagerPolicy::advise_end()
 {
     if (dirty_tiles)

=== modified file 'miral-shell/tiling_window_manager.h'
--- miral-shell/tiling_window_manager.h	2016-11-17 11:09:14 +0000
+++ miral-shell/tiling_window_manager.h	2016-12-05 09:52:38 +0000
@@ -73,6 +73,8 @@
     void advise_new_app(miral::ApplicationInfo& application) override;
     void advise_delete_app(miral::ApplicationInfo const& application) override;
 
+    auto confirm_inherited_move(miral::WindowInfo const& window_info, Displacement movement) -> Rectangle override;
+
 private:
     void advise_output_create(miral::Output const& output) override;
     void advise_output_update(miral::Output const& updated, miral::Output const& original) override;

=== modified file 'miral/basic_window_manager.cpp'
--- miral/basic_window_manager.cpp	2016-11-16 12:48:17 +0000
+++ miral/basic_window_manager.cpp	2016-12-05 09:52:38 +0000
@@ -528,7 +528,10 @@
     root.window().move_to(top_left);
 
     for (auto const& child: root.children())
-        move_tree(info_for(child), movement);
+    {
+        auto const& pos = policy->confirm_inherited_move(info_for(child), movement);
+        place_and_size(info_for(child), pos.top_left, pos.size);
+    }
 }
 
 void miral::BasicWindowManager::modify_window(WindowInfo& window_info, WindowSpecification const& modifications)

=== modified file 'miral/canonical_window_manager.cpp'
--- miral/canonical_window_manager.cpp	2016-07-27 15:43:19 +0000
+++ miral/canonical_window_manager.cpp	2016-12-05 09:52:38 +0000
@@ -59,3 +59,9 @@
 {
     tools.raise_tree(info.window());
 }
+
+auto miral::CanonicalWindowManagerPolicy::confirm_inherited_move(WindowInfo const& window_info, Displacement movement)
+-> Rectangle
+{
+    return {window_info.window().top_left()+movement, window_info.window().size()};
+}

=== modified file 'miral/symbols.map'
--- miral/symbols.map	2016-11-29 12:59:36 +0000
+++ miral/symbols.map	2016-12-05 09:52:38 +0000
@@ -342,3 +342,11 @@
     vtable?for?miral::DebugExtension;
   };
 } MIRAL_0.4;
+
+MIRAL_0.6 {
+global:
+  extern "C++" {
+    miral::CanonicalWindowManagerPolicy::confirm_inherited_move*;
+    non-virtual?thunk?to?miral::CanonicalWindowManagerPolicy::confirm_inherited_move*;
+  };
+} MIRAL_0.5;

=== modified file 'miral/window_management_trace.cpp'
--- miral/window_management_trace.cpp	2016-11-29 10:55:43 +0000
+++ miral/window_management_trace.cpp	2016-12-05 09:52:38 +0000
@@ -552,6 +552,16 @@
     return policy->handle_pointer_event(event);
 }
 
+auto miral::WindowManagementTrace::confirm_inherited_move(WindowInfo const& window_info, Displacement movement)
+-> Rectangle
+{
+    std::stringstream out;
+    out << movement;
+    mir::log_info("%s window_info=%s, movement=%s", __func__, dump_of(window_info).c_str(), out.str().c_str());
+
+    return policy->confirm_inherited_move(window_info, movement);
+}
+
 void miral::WindowManagementTrace::advise_begin()
 {
     log_input = []{};

=== modified file 'miral/window_management_trace.h'
--- miral/window_management_trace.h	2016-11-03 10:15:24 +0000
+++ miral/window_management_trace.h	2016-12-05 09:52:38 +0000
@@ -86,6 +86,8 @@
 
     virtual bool handle_pointer_event(MirPointerEvent const* event) override;
 
+    auto confirm_inherited_move(WindowInfo const& window_info, Displacement movement) -> Rectangle override;
+
 public:
     virtual void advise_begin() override;