Merge lp:~alan-griffiths/miral/rework-handling-of-surface-state-changes into lp:miral

[Alan Griffiths]

Rework the handling of surface state changes:
1. shells now have the option of ignoring (or adjusting) the default placement.
2. fixes issues arising when the state of the active window changes.

Rework the handling of surface state changes:
1. shells now have the option of ignoring (or adjusting) the default placement.
2. fixes issues arising when the state of the active window changes.

It may seem odd to provide position_for_state() in tools, but the if the shell initiates a state change (as the Titlebar example does on [Ctrl|Shif|Alt]-F11) it may need the default placement.

[Daniel d'Andrada (dandrader)]

Fixes those two bugs I reported, so I'm happy with it.
No opinion on the code itself, just stating that it fixed those bugs.

review: Approve (test)

[Gerry Boland (gerboland)]

+ void position_for_state(WindowSpecification& modifications, WindowInfo const& window_info) const;
name doesn't quite match implementation, as it also impacts size, perhaps "geometry_for_state"?

review: Needs Fixing

[Gerry Boland (gerboland)]

+void miral::BasicWindowManager::validate_modification_request(WindowSpecification const& modifications, WindowInfo const& window_info) const

+ void position_for_state(WindowSpecification& modifications, WindowInfo const& window_info) const override;

Order of the arguments now do not match. Consistency would be nice. I'm unsure why you changed the first one here.

[Gerry Boland (gerboland)]

- policy->advise_state_change(window_info, value);
- window_info.state(value);
- mir_surface->hide();
- mir_surface->configure(mir_surface_attrib_state, value);
- if (window_info.window() == active_window())
- {
- mru_active_windows.erase(window_info.window());
-
- // Try to activate to recently active window of any application
- mru_active_windows.enumerate([&](Window& window)
- {
- auto const w = window;
- return !(select_active_window(w));
- });
- }
- return;

I wasn't expecting this to vanish in this MP. Is it dead code?

[Gerry Boland (gerboland)]

> +void
> miral::BasicWindowManager::validate_modification_request(WindowSpecification
> const& modifications, WindowInfo const& window_info) const
>
> + void position_for_state(WindowSpecification& modifications, WindowInfo
> const& window_info) const override;
>
> Order of the arguments now do not match. Consistency would be nice. I'm unsure
> why you changed the first one here.

Or the consistency you're developing is an (out, const in) design?

[Alan Griffiths (alan-griffiths)]

> - policy->advise_state_change(window_info, value);
> - window_info.state(value);
> - mir_surface->hide();
> - mir_surface->configure(mir_surface_attrib_state, value);
> - if (window_info.window() == active_window())
> - {
> - mru_active_windows.erase(window_info.window());
> -
> - // Try to activate to recently active window of any application
> - mru_active_windows.enumerate([&](Window& window)
> - {
> - auto const w = window;
> - return !(select_active_window(w));
> - });
> - }
> - return;
>
> I wasn't expecting this to vanish in this MP. Is it dead code?

Neither.

+ if (window == active_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;
+ return !(select_active_window(w));
+ });
+ }

Plus some duplicated code consolidated.

[lp:~alan-griffiths/miral/rework-handling-of-surface-state-changes]

384. By Alan Griffiths on 2016-09-29

position_for_state() => place_and_size_for_state()

[Alan Griffiths (alan-griffiths)]

> > +void
> > miral::BasicWindowManager::validate_modification_request(WindowSpecification
> > const& modifications, WindowInfo const& window_info) const
> >
> > + void position_for_state(WindowSpecification& modifications, WindowInfo
> > const& window_info) const override;
> >
> > Order of the arguments now do not match. Consistency would be nice. I'm
> unsure
> > why you changed the first one here.
>
> Or the consistency you're developing is an (out, const in) design?

Partly that, but also both functions are /about/ the first parameter, with the second parameter being secondary.

[Alan Griffiths (alan-griffiths)]

> + void position_for_state(WindowSpecification& modifications, WindowInfo
> const& window_info) const;
> name doesn't quite match implementation, as it also impacts size, perhaps
> "geometry_for_state"?

place_and_size_for_state()?

[Gerry Boland (gerboland)]

> > + void position_for_state(WindowSpecification& modifications, WindowInfo
> > const& window_info) const;
> > name doesn't quite match implementation, as it also impacts size, perhaps
> > "geometry_for_state"?
>
> place_and_size_for_state()?

You use similar elsewhere, so yeah.

[Gerry Boland (gerboland)]

review: Approve

=== modified file 'debian/libmiral1.symbols'
--- debian/libmiral1.symbols	2016-09-28 10:38:36 +0000
+++ debian/libmiral1.symbols	2016-09-29 14:41:47 +0000
@@ -180,6 +180,7 @@
  (c++)"miral::WindowManagerTools::invoke_under_lock(std::function<void ()> const&)@MIRAL_0.1" 0.1.0
  (c++)"miral::WindowManagerTools::modify_window(miral::WindowInfo&, miral::WindowSpecification const&)@MIRAL_0.1" 0.1.0
  (c++)"miral::WindowManagerTools::operator=(miral::WindowManagerTools const&)@MIRAL_0.1" 0.1.0
+ (c++)"miral::WindowManagerTools::place_and_size_for_state(miral::WindowSpecification&, miral::WindowInfo const&) const@MIRAL_0.2" 0.2.0
  (c++)"miral::WindowManagerTools::raise_tree(miral::Window const&)@MIRAL_0.1" 0.1.0
  (c++)"miral::WindowManagerTools::select_active_window(miral::Window const&)@MIRAL_0.1" 0.1.0
  (c++)"miral::WindowManagerTools::window_at(mir::geometry::Point) const@MIRAL_0.1" 0.1.0

=== modified file 'include/miral/window_manager_tools.h'
--- include/miral/window_manager_tools.h	2016-09-07 20:23:20 +0000
+++ include/miral/window_manager_tools.h	2016-09-29 14:41:47 +0000
@@ -146,6 +146,8 @@
     /// Apply modifications to a window
     void modify_window(WindowInfo& window_info, WindowSpecification const& modifications);
 
+    /// Set a default size and position to reflect state change
+    void place_and_size_for_state(WindowSpecification& modifications, WindowInfo const& window_info) const;
 /** @} */
 
     /** Multi-thread support

=== modified file 'miral-shell/titlebar_window_manager.cpp'
--- miral-shell/titlebar_window_manager.cpp	2016-09-28 11:42:35 +0000
+++ miral-shell/titlebar_window_manager.cpp	2016-09-29 14:41:47 +0000
@@ -455,7 +455,7 @@
         WindowSpecification modifications;
 
         modifications.state() = (info.state() == state) ? mir_surface_state_restored : state;
-
+        tools.place_and_size_for_state(modifications, info);
         tools.modify_window(info, modifications);
     }
 }

=== modified file 'miral/basic_window_manager.cpp'
--- miral/basic_window_manager.cpp	2016-09-23 16:55:46 +0000
+++ miral/basic_window_manager.cpp	2016-09-29 14:41:47 +0000
@@ -143,8 +143,10 @@
 {
     Locker lock{mutex, policy};
     auto& info = info_for(surface);
-    validate_modification_request(info, modifications);
-    policy->handle_modify_window(info, modifications);
+    WindowSpecification mods{modifications};
+    validate_modification_request(mods, info);
+    place_and_size_for_state(mods, info);
+    policy->handle_modify_window(info, mods);
 }
 
 void miral::BasicWindowManager::remove_surface(
@@ -321,7 +323,8 @@
     Locker lock{mutex, policy};
     auto& info = info_for(surface);
 
-    validate_modification_request(info, modification);
+    validate_modification_request(modification, info);
+    place_and_size_for_state(modification, info);
     policy->handle_modify_window(info, modification);
 
     switch (attrib)
@@ -601,6 +604,38 @@
     if (modifications.input_shape().is_set())
         std::shared_ptr<scene::Surface>(window)->set_input_region(modifications.input_shape().value());
 
+    if (modifications.state().is_set() && window_info.state() != modifications.state().value())
+    {
+        switch (window_info.state())
+        {
+        case mir_surface_state_restored:
+        case mir_surface_state_hidden:
+            window_info.restore_rect({window.top_left(), window.size()});
+            break;
+
+        case mir_surface_state_vertmaximized:
+        {
+            auto restore_rect = window_info.restore_rect();
+            restore_rect.top_left.x = window.top_left().x;
+            restore_rect.size.width = window.size().width;
+            window_info.restore_rect(restore_rect);
+            break;
+        }
+
+        case mir_surface_state_horizmaximized:
+        {
+            auto restore_rect = window_info.restore_rect();
+            restore_rect.top_left.y = window.top_left().y;
+            restore_rect.size.height= window.size().height;
+            window_info.restore_rect(restore_rect);
+            break;
+        }
+
+        default:
+            break;
+        }
+    }
+
     Point new_pos = modifications.top_left().is_set() ? modifications.top_left().value() : window.top_left();
 
     if (modifications.size().is_set())
@@ -687,48 +722,36 @@
     move_tree(root, new_pos - root.window().top_left());
 }
 
-void miral::BasicWindowManager::set_state(miral::WindowInfo& window_info, MirSurfaceState value)
+void miral::BasicWindowManager::place_and_size_for_state(
+    WindowSpecification& modifications, WindowInfo const& window_info) const
 {
-    auto const mir_surface = std::shared_ptr<scene::Surface>(window_info.window());
-
-    switch (value)
-    {
-    case mir_surface_state_restored:
-    case mir_surface_state_maximized:
-    case mir_surface_state_vertmaximized:
-    case mir_surface_state_horizmaximized:
-    case mir_surface_state_fullscreen:
-    case mir_surface_state_hidden:
-    case mir_surface_state_minimized:
-        break;
-
-    default:
-        mir_surface->configure(mir_surface_attrib_state, window_info.state());
+    if (!modifications.state().is_set() || modifications.state().value() == window_info.state())
         return;
-    }
-
+
+    auto const display_area = displays.bounding_rectangle();
+    auto const window = window_info.window();
+
+    auto restore_rect = window_info.restore_rect();
+
+    // window_info.restore_rect() was cached on last state change, update to reflect current window position 
     switch (window_info.state())
     {
     case mir_surface_state_restored:
     case mir_surface_state_hidden:
-        window_info.restore_rect({window_info.window().top_left(), window_info.window().size()});
+        restore_rect = {window.top_left(), window.size()};
         break;
 
     case mir_surface_state_vertmaximized:
     {
-        auto restore_rect = window_info.restore_rect();
-        restore_rect.top_left.x = window_info.window().top_left().x;
-        restore_rect.size.width = window_info.window().size().width;
-        window_info.restore_rect(restore_rect);
+        restore_rect.top_left.x = window.top_left().x;
+        restore_rect.size.width = window.size().width;
         break;
     }
 
     case mir_surface_state_horizmaximized:
     {
-        auto restore_rect = window_info.restore_rect();
-        restore_rect.top_left.y = window_info.window().top_left().y;
-        restore_rect.size.height= window_info.window().size().height;
-        window_info.restore_rect(restore_rect);
+        restore_rect.top_left.y = window.top_left().y;
+        restore_rect.size.height= window.size().height;
         break;
     }
 
@@ -736,29 +759,20 @@
         break;
     }
 
-    if (value != mir_surface_state_fullscreen)
-    {
-        window_info.output_id({});
-        fullscreen_surfaces.erase(window_info.window());
-    }
-    else
-    {
-        fullscreen_surfaces.insert(window_info.window());
-    }
+    // If the shell has also set position, that overrides restore_rect default
+    if (modifications.top_left().is_set())
+        restore_rect.top_left = modifications.top_left().value();
 
-    if (window_info.state() == value)
-    {
-        return;
-    }
+    // If the client or shell has also set size, that overrides restore_rect default 
+    if (modifications.size().is_set())
+        restore_rect.size = modifications.size().value();
 
     Rectangle rect;
 
-    auto const display_area = displays.bounding_rectangle();
-
-    switch (value)
+    switch (modifications.state().value())
     {
     case mir_surface_state_restored:
-        rect = window_info.restore_rect();
+        rect = restore_rect;
         break;
 
     case mir_surface_state_maximized:
@@ -766,18 +780,18 @@
         break;
 
     case mir_surface_state_horizmaximized:
-        rect.top_left = {display_area.top_left.x, window_info.restore_rect().top_left.y};
-        rect.size = {display_area.size.width, window_info.restore_rect().size.height};
+        rect.top_left = {display_area.top_left.x, restore_rect.top_left.y};
+        rect.size = {display_area.size.width, restore_rect.size.height};
         break;
 
     case mir_surface_state_vertmaximized:
-        rect.top_left = {window_info.restore_rect().top_left.x, display_area.top_left.y};
-        rect.size = {window_info.restore_rect().size.width, display_area.size.height};
+        rect.top_left = {restore_rect.top_left.x, display_area.top_left.y};
+        rect.size = {restore_rect.size.width, display_area.size.height};
         break;
 
     case mir_surface_state_fullscreen:
     {
-        rect = {(window_info.window().top_left()), window_info.window().size()};
+        rect = {(window.top_left()), window.size()};
 
         if (window_info.has_output_id())
         {
@@ -794,35 +808,59 @@
 
     case mir_surface_state_hidden:
     case mir_surface_state_minimized:
-        policy->advise_state_change(window_info, value);
-        window_info.state(value);
-        mir_surface->hide();
-        mir_surface->configure(mir_surface_attrib_state, value);
-        if (window_info.window() == active_window())
-        {
-            mru_active_windows.erase(window_info.window());
-
-            // Try to activate to recently active window of any application
-            mru_active_windows.enumerate([&](Window& window)
-                {
-                    auto const w = window;
-                    return !(select_active_window(w));
-                });
-        }
-        return;
-
     default:
-        break;
-    }
-
-    place_and_size(window_info, rect.top_left, rect.size);
+        return;
+    }
+
+    modifications.top_left() = rect.top_left;
+    modifications.size() = rect.size;
+}
+
+void miral::BasicWindowManager::set_state(miral::WindowInfo& window_info, MirSurfaceState value)
+{
+    auto const window = window_info.window();
+    auto const mir_surface = std::shared_ptr<scene::Surface>(window);
+
+    if (value != mir_surface_state_fullscreen)
+    {
+        window_info.output_id({});
+        fullscreen_surfaces.erase(window);
+    }
+    else
+    {
+        fullscreen_surfaces.insert(window);
+    }
+
+    if (window_info.state() == value)
+    {
+        return;
+    }
+
     policy->advise_state_change(window_info, value);
     window_info.state(value);
 
     if (window_info.is_visible())
+    {
         mir_surface->show();
-
-    mir_surface->configure(mir_surface_attrib_state, window_info.state());
+    }
+    else
+    {
+        mir_surface->hide();
+
+        if (window == active_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;
+                    return !(select_active_window(w));
+                });
+        }
+    }
+
+    mir_surface->configure(mir_surface_attrib_state, value);
 }
 
 
@@ -1434,9 +1472,7 @@
     return default_result;
 }
 
-void miral::BasicWindowManager::validate_modification_request(
-    WindowInfo const& window_info,
-    WindowSpecification const& modifications) const
+void miral::BasicWindowManager::validate_modification_request(WindowSpecification const& modifications, WindowInfo const& window_info) const
 {
     auto target_type = window_info.type();
 

=== modified file 'miral/basic_window_manager.h'
--- miral/basic_window_manager.h	2016-09-21 13:37:35 +0000
+++ miral/basic_window_manager.h	2016-09-29 14:41:47 +0000
@@ -133,6 +133,7 @@
     auto info_for_window_id(std::string const& id) const -> WindowInfo& override;
 
     auto id_for_window(Window const& window) const -> std::string override;
+    void place_and_size_for_state(WindowSpecification& modifications, WindowInfo const& window_info) const override;
 
     void invoke_under_lock(std::function<void()> const& callback) override;
 
@@ -167,7 +168,7 @@
 
     void move_tree(miral::WindowInfo& root, mir::geometry::Displacement movement);
     void erase(miral::WindowInfo const& info);
-    void validate_modification_request(WindowInfo const& window_info, WindowSpecification const& modifications) const;
+    void validate_modification_request(WindowSpecification const& modifications, WindowInfo const& window_info) const;
     void place_and_size(WindowInfo& root, Point const& new_pos, Size const& new_size);
     void set_state(miral::WindowInfo& window_info, MirSurfaceState value);
 };

=== modified file 'miral/set_window_managment_policy.cpp'
--- miral/set_window_managment_policy.cpp	2016-09-21 14:43:46 +0000
+++ miral/set_window_managment_policy.cpp	2016-09-29 14:41:47 +0000
@@ -57,7 +57,7 @@
             {
                 auto trace_builder = [this](WindowManagerTools const& tools) -> std::unique_ptr<miral::WindowManagementPolicy>
                     {
-                    return std::make_unique<WindowManagementTrace>(tools, builder);
+                        return std::make_unique<WindowManagementTrace>(tools, builder);
                     };
 
                 return std::make_shared<BasicWindowManager>(focus_controller, display_layout, persistent_surface_store, trace_builder);

=== modified file 'miral/symbols.map'
--- miral/symbols.map	2016-09-28 10:38:36 +0000
+++ miral/symbols.map	2016-09-29 14:41:47 +0000
@@ -289,6 +289,7 @@
     miral::CursorTheme::CursorTheme*;
     miral::CursorTheme::operator*;
     miral::WindowInfo::confine_pointer*;
+    miral::WindowManagerTools::place_and_size_for_state*;
     miral::WindowSpecification::confine_pointer*;
     typeinfo?for?miral::CursorTheme;
     vtable?for?miral::CursorTheme;

=== modified file 'miral/window_management_trace.cpp'
--- miral/window_management_trace.cpp	2016-09-23 16:54:30 +0000
+++ miral/window_management_trace.cpp	2016-09-29 14:41:47 +0000
@@ -401,6 +401,14 @@
     return result;
 }
 
+void miral::WindowManagementTrace::place_and_size_for_state(
+    WindowSpecification& modifications, WindowInfo const& window_info) const
+{
+    log_input();
+    mir::log_info("%s modifications=%s window_info=%s", __func__, dump_of(modifications).c_str(), dump_of(window_info).c_str());
+    wrapped.place_and_size_for_state(modifications, window_info);
+}
+
 void miral::WindowManagementTrace::drag_active_window(mir::geometry::Displacement movement)
 {
     log_input();

=== modified file 'miral/window_management_trace.h'
--- miral/window_management_trace.h	2016-09-23 10:28:25 +0000
+++ miral/window_management_trace.h	2016-09-29 14:41:47 +0000
@@ -56,6 +56,7 @@
     virtual auto active_display() -> mir::geometry::Rectangle const override;
     virtual auto info_for_window_id(std::string const& id) const -> WindowInfo& override;
     virtual auto id_for_window(Window const& window) const -> std::string override;
+    virtual void place_and_size_for_state(WindowSpecification& modifications, WindowInfo const& window_info) const override;
 
     virtual void drag_active_window(mir::geometry::Displacement movement) override;
 

=== modified file 'miral/window_manager_tools.cpp'
--- miral/window_manager_tools.cpp	2016-09-07 20:23:20 +0000
+++ miral/window_manager_tools.cpp	2016-09-29 14:41:47 +0000
@@ -85,3 +85,7 @@
 
 void miral::WindowManagerTools::invoke_under_lock(std::function<void()> const& callback)
 { tools->invoke_under_lock(callback); }
+
+void miral::WindowManagerTools::place_and_size_for_state(
+    WindowSpecification& modifications, WindowInfo const& window_info) const
+{ tools->place_and_size_for_state(modifications, window_info); }

=== modified file 'miral/window_manager_tools_implementation.h'
--- miral/window_manager_tools_implementation.h	2016-09-21 13:37:35 +0000
+++ miral/window_manager_tools_implementation.h	2016-09-29 14:41:47 +0000
@@ -65,6 +65,8 @@
     virtual void modify_window(WindowInfo& window_info, WindowSpecification const& modifications) = 0;
     virtual auto info_for_window_id(std::string const& id) const -> WindowInfo& = 0;
     virtual auto id_for_window(Window const& window) const -> std::string = 0;
+    virtual void place_and_size_for_state(WindowSpecification& modifications, WindowInfo const& window_info) const= 0;
+
 /** @} */
 
 /** @name Multi-thread support