Merge lp:~alan-griffiths/mir/fix-1447886 into lp:mir

[Alan Griffiths]

Update the resize logic in CanonicalWindowManagerPolicy following the discussion on lp:1447886

Update the resize logic in CanonicalWindowManagerPolicy following the discussion on lp:1447886

It now looks at the gesture as a whole and only does position checking at the start of the resize gesture. Thereafter the gesture continues until the button is released, regardless of whether the cursor is still inside the surface/titlebar.

[Mir CI Bot (mir-ci-bot)]

PASSED: Continuous integration, rev:3483
https://mir-jenkins.ubuntu.com/job/mir-ci/910/
Executed test runs:
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-mir/956
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-0-fetch/997
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=vivid+overlay/988
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-1-sourcepkg/release=xenial/988
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/966
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=clang,platform=mesa,release=vivid+overlay/966/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/966
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=amd64,compiler=gcc,platform=mesa,release=xenial/966/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/966
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=cross-armhf,compiler=gcc,platform=android,release=vivid+overlay/966/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/966
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=android,release=vivid+overlay/966/artifact/output/*zip*/output.zip
    SUCCESS: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/966
        deb: https://mir-jenkins.ubuntu.com/job/build-2-binpkg-mir/arch=i386,compiler=gcc,platform=mesa,release=xenial/966/artifact/output/*zip*/output.zip

Click here to trigger a rebuild:
https://mir-jenkins.ubuntu.com/job/mir-ci/910/rebuild

review: Approve (continuous-integration)

[Cemil Azizoglu (cemil-azizoglu)]

lgtm

review: Approve

[Daniel van Vugt (vanvugt)]

Feels like an improvement. I don't want to be a blocker on this for too long, but things worth noting:

New bug: Dragging a corner past the window's minimum size and back again results in the cursor losing its original position within the window. The cursor keeping the same relative position within the window for the duration of the resize feels most professional and least frustrating. Just try it in Compiz/Unity7 to get a feel for what should happen. Admittedly mir_proving_server has the same bug right now.

Old bug: I just noticed you're dividing the window into quarters instead of thirds. So grabbing a window from the middle of an edge incorrectly resizes two axes instead of one. See Compiz/Unity or mir_proving_server for examples of what should happen. This one would represent a regression for mir_proving_server, but also isn't the bug being fixed here. Just a recommended prerequisite before abolishing DefaultWindowManager.

review: Approve

=== modified file 'examples/server_example_canonical_window_manager.cpp'
--- examples/server_example_canonical_window_manager.cpp	2016-04-15 03:41:28 +0000
+++ examples/server_example_canonical_window_manager.cpp	2016-04-28 09:10:37 +0000
@@ -90,7 +90,7 @@
 
 void me::CanonicalWindowManagerPolicyCopy::resize(Point cursor)
 {
-    select_active_surface(tools->surface_at(old_cursor));
+    if (!resizing) select_active_surface(tools->surface_at(old_cursor));
     resize(active_surface(), cursor, old_cursor, display_area);
 }
 
@@ -733,6 +733,7 @@
         mir_pointer_event_axis_value(event, mir_pointer_axis_y)};
 
     bool consumes_event = false;
+    bool resize_event = false;
 
     if (action == mir_pointer_action_button_down)
     {
@@ -750,6 +751,7 @@
         if (mir_pointer_event_button_state(event, mir_pointer_button_tertiary))
         {
             resize(cursor);
+            resize_event = active_surface_.lock().get();
             consumes_event = true;
         }
     }
@@ -768,6 +770,7 @@
         }
     }
 
+    resizing = resize_event;
     old_cursor = cursor;
     return consumes_event;
 }
@@ -852,40 +855,63 @@
 
 bool me::CanonicalWindowManagerPolicyCopy::resize(std::shared_ptr<ms::Surface> const& surface, Point cursor, Point old_cursor, Rectangle bounds)
 {
-    if (!surface || !surface->input_area_contains(old_cursor))
+    if (!surface)
         return false;
 
+    auto const& surface_info = tools->info_for(surface);
+
     auto const top_left = surface->top_left();
     Rectangle const old_pos{top_left, surface->size()};
 
-    auto anchor = top_left;
-
-    for (auto const& corner : {
-        old_pos.top_right(),
-        old_pos.bottom_left(),
-        old_pos.bottom_right()})
+    if (!resizing)
     {
-        if ((old_cursor - anchor).length_squared() <
-            (old_cursor - corner).length_squared())
+        auto anchor = old_pos.bottom_right();
+
+        for (auto const& corner : {
+            old_pos.top_right(),
+            old_pos.bottom_left(),
+            top_left})
         {
-            anchor = corner;
+            if ((old_cursor - anchor).length_squared() <
+                (old_cursor - corner).length_squared())
+            {
+                anchor = corner;
+            }
         }
+
+        left_resize = anchor.x != top_left.x;
+        top_resize  = anchor.y != top_left.y;
     }
 
-    bool const left_resize = anchor.x != top_left.x;
-    bool const top_resize  = anchor.y != top_left.y;
     int const x_sign = left_resize? -1 : 1;
     int const y_sign = top_resize?  -1 : 1;
 
-    auto const delta = cursor-old_cursor;
-
-    Size new_size{old_pos.size.width + x_sign*delta.dx, old_pos.size.height + y_sign*delta.dy};
-
+    auto delta = cursor-old_cursor;
+
+    auto new_width = old_pos.size.width + x_sign * delta.dx;
+    auto new_height = old_pos.size.height + y_sign * delta.dy;
+
+    auto const min_width  = std::max(surface_info.min_width, Width{5});
+    auto const min_height = std::max(surface_info.min_height, Height{5});
+
+    if (new_width < min_width)
+    {
+        new_width = min_width;
+        if (delta.dx > DeltaX{0})
+            delta.dx = DeltaX{0};
+    }
+
+    if (new_height < min_height)
+    {
+        new_height = min_height;
+        if (delta.dy > DeltaY{0})
+            delta.dy = DeltaY{0};
+    }
+
+    Size new_size{new_width, new_height};
     Point new_pos = top_left + left_resize*delta.dx + top_resize*delta.dy;
 
 
-    auto const& surface_info = tools->info_for(surface);
-
     surface_info.constrain_resize(surface, new_pos, new_size, left_resize, top_resize, bounds);
 
     apply_resize(surface, surface_info.titlebar, new_pos, new_size);

=== modified file 'examples/server_example_canonical_window_manager.h'
--- examples/server_example_canonical_window_manager.h	2016-04-15 03:41:28 +0000
+++ examples/server_example_canonical_window_manager.h	2016-04-28 09:10:37 +0000
@@ -125,6 +125,10 @@
     using FullscreenSurfaces = std::set<std::weak_ptr<scene::Surface>, std::owner_less<std::weak_ptr<scene::Surface>>>;
 
     FullscreenSurfaces fullscreen_surfaces;
+
+    bool resizing = false;
+    bool left_resize = false;
+    bool top_resize = false;
 };
 }
 }

=== modified file 'src/include/server/mir/shell/canonical_window_manager.h'
--- src/include/server/mir/shell/canonical_window_manager.h	2016-03-23 06:39:56 +0000
+++ src/include/server/mir/shell/canonical_window_manager.h	2016-04-28 09:10:37 +0000
@@ -110,6 +110,10 @@
     using FullscreenSurfaces = std::set<std::weak_ptr<scene::Surface>, std::owner_less<std::weak_ptr<scene::Surface>>>;
 
     FullscreenSurfaces fullscreen_surfaces;
+
+    bool resizing = false;
+    bool left_resize = false;
+    bool top_resize = false;
 };
 
 using CanonicalWindowManager = WindowManagerConstructor<CanonicalWindowManagerPolicy>;

=== modified file 'src/server/shell/canonical_window_manager.cpp'
--- src/server/shell/canonical_window_manager.cpp	2016-04-05 11:59:12 +0000
+++ src/server/shell/canonical_window_manager.cpp	2016-04-28 09:10:37 +0000
@@ -113,7 +113,7 @@
 
 void msh::CanonicalWindowManagerPolicy::resize(Point cursor)
 {
-    select_active_surface(tools->surface_at(old_cursor));
+    if (!resizing) select_active_surface(tools->surface_at(old_cursor));
     resize(active_surface(), cursor, old_cursor, display_area);
     old_cursor = cursor;
 }
@@ -703,6 +703,7 @@
         mir_pointer_event_axis_value(event, mir_pointer_axis_y)};
 
     bool consumes_event = false;
+    bool resize_event = false;
 
     if (action == mir_pointer_action_button_down)
     {
@@ -720,10 +721,12 @@
         if (mir_pointer_event_button_state(event, mir_pointer_button_tertiary))
         {
             resize(cursor);
+            resize_event = active_surface_.lock().get();
             consumes_event = true;
         }
     }
 
+    resizing = resize_event;
     old_cursor = cursor;
     return consumes_event;
 }
@@ -786,40 +789,63 @@
 
 bool msh::CanonicalWindowManagerPolicy::resize(std::shared_ptr<ms::Surface> const& surface, Point cursor, Point old_cursor, Rectangle bounds)
 {
-    if (!surface || !surface->input_area_contains(old_cursor))
+    if (!surface)
         return false;
 
+    auto const& surface_info = tools->info_for(surface);
+
     auto const top_left = surface->top_left();
     Rectangle const old_pos{top_left, surface->size()};
 
-    auto anchor = top_left;
-
-    for (auto const& corner : {
-        old_pos.top_right(),
-        old_pos.bottom_left(),
-        old_pos.bottom_right()})
+    if (!resizing)
     {
-        if ((old_cursor - anchor).length_squared() <
-            (old_cursor - corner).length_squared())
+        auto anchor = old_pos.bottom_right();
+
+        for (auto const& corner : {
+            old_pos.top_right(),
+            old_pos.bottom_left(),
+            top_left})
         {
-            anchor = corner;
+            if ((old_cursor - anchor).length_squared() <
+                (old_cursor - corner).length_squared())
+            {
+                anchor = corner;
+            }
         }
+
+        left_resize = anchor.x != top_left.x;
+        top_resize  = anchor.y != top_left.y;
     }
 
-    bool const left_resize = anchor.x != top_left.x;
-    bool const top_resize  = anchor.y != top_left.y;
     int const x_sign = left_resize? -1 : 1;
     int const y_sign = top_resize?  -1 : 1;
 
-    auto const delta = cursor-old_cursor;
-
-    Size new_size{old_pos.size.width + x_sign*delta.dx, old_pos.size.height + y_sign*delta.dy};
-
+    auto delta = cursor-old_cursor;
+
+    auto new_width = old_pos.size.width + x_sign * delta.dx;
+    auto new_height = old_pos.size.height + y_sign * delta.dy;
+
+    auto const min_width  = std::max(surface_info.min_width, Width{5});
+    auto const min_height = std::max(surface_info.min_height, Height{5});
+
+    if (new_width < min_width)
+    {
+        new_width = min_width;
+        if (delta.dx > DeltaX{0})
+            delta.dx = DeltaX{0};
+    }
+
+    if (new_height < min_height)
+    {
+        new_height = min_height;
+        if (delta.dy > DeltaY{0})
+            delta.dy = DeltaY{0};
+    }
+
+    Size new_size{new_width, new_height};
     Point new_pos = top_left + left_resize*delta.dx + top_resize*delta.dy;
 
 
-    auto const& surface_info = tools->info_for(surface);
-
     surface_info.constrain_resize(surface, new_pos, new_size, left_resize, top_resize, bounds);
 
     apply_resize(surface, new_pos, new_size);