Merge lp:~vanvugt/mir/fix-1398294 into lp:mir

[Daniel van Vugt]

Ensure fullscreen'ing always succeeds in choosing a screen, if there
are any. (LP: #1398294)

Also ensure we pick the most appropriate one.

[Alexandros Frantzis (afrantzis)]

33 + geometry::Rectangle best = rect;

Don't we want get_output_for() to return an empty rectangle (i.e. no output) if the window doesn't intersect with any screen?

review: Needs Information

[Alan Griffiths (alan-griffiths)]

19 rect.size =
22 + geom::Size{br_closed.x.as_int() - tl_closed.x.as_int() + 1,
23 + br_closed.y.as_int() - tl_closed.y.as_int() + 1};

Isn't that easier to read as:

     rect.size = geom::Point{1, 1} + (br_closed - tl_closed);

[Daniel van Vugt (vanvugt)]

Alexandros:
Nope. In the worst evil case where there are no displays, I think it's better to fail to fullscreen (leave the window unchanged) than to resize to 0x0 (which becomes 1x1 due to the resize policy). Although noutputs==0 is not an important use case. I'll make fullscreening-to-zero-outputs a resize to 0x0 again if you think it's really preferable.

Alan:
I noticed that too. There might be room for simplification but I was going for the smallest diff being a bug fix.

[Daniel van Vugt (vanvugt)]

Alexandros:
To answer your question more precisely - The case of the window not intersecting any screen (where nscreens > 0) is handled already. In that case it goes fullscreen on the first screen (because an overlap area of 0 is greater than the default max_area of -1).

[Alan Griffiths (alan-griffiths)]

I guess the needless verbosity mentioned above and that in the tests (which is also rife with "magic numbers") can be argued as "pre-existing". (I still think it ought to be fixed though - even if not on this MP.)

review: Approve

[Cemil Azizoglu (cemil-azizoglu)]

Ok.

review: Approve

=== modified file 'src/server/shell/graphics_display_layout.cpp'
--- src/server/shell/graphics_display_layout.cpp	2014-12-04 03:22:52 +0000
+++ src/server/shell/graphics_display_layout.cpp	2014-12-05 09:23:19 +0000
@@ -44,17 +44,19 @@
     if (output.size.width > geom::Width{0} && output.size.height > geom::Height{0} &&
         rect.size.width > geom::Width{0} && rect.size.height > geom::Height{0})
     {
-        auto tl = rect.top_left;
+        auto tl_closed = rect.top_left;
         auto br_closed = rect.bottom_right() - geom::Displacement{1,1};
 
         geom::Rectangles rectangles;
         rectangles.add(output);
 
+        rectangles.confine(tl_closed);
         rectangles.confine(br_closed);
 
+        rect.top_left = tl_closed;
         rect.size =
-            geom::Size{br_closed.x.as_int() - tl.x.as_int() + 1,
-                       br_closed.y.as_int() - tl.y.as_int() + 1};
+            geom::Size{br_closed.x.as_int() - tl_closed.x.as_int() + 1,
+                       br_closed.y.as_int() - tl_closed.y.as_int() + 1};
     }
     else
     {
@@ -93,19 +95,23 @@
 
 geom::Rectangle msh::GraphicsDisplayLayout::get_output_for(geometry::Rectangle& rect)
 {
-    geom::Rectangle output;
+    int max_area = -1;
+    geometry::Rectangle best = rect;
 
-    /*
-     * TODO: We need a better heuristic to decide in which output a
-     * rectangle/surface belongs.
-     */
     display->for_each_display_buffer(
-        [&output,&rect](mg::DisplayBuffer const& db)
+        [&](mg::DisplayBuffer const& db)
         {
-            auto view_area = db.view_area();
-            if (view_area.contains(rect.top_left))
-                output = view_area;
+            auto const& screen = db.view_area();
+            auto const& overlap = rect.intersection_with(screen);
+            int area = overlap.size.width.as_int() *
+                       overlap.size.height.as_int();
+
+            if (area > max_area)
+            {
+                best = screen;
+                max_area = area;
+            }
         });
 
-    return output;
+    return best;
 }

=== modified file 'tests/unit-tests/shell/test_graphics_display_layout.cpp'
--- tests/unit-tests/shell/test_graphics_display_layout.cpp	2014-12-05 03:26:03 +0000
+++ tests/unit-tests/shell/test_graphics_display_layout.cpp	2014-12-05 09:23:19 +0000
@@ -66,7 +66,7 @@
             geom::Rectangle{geom::Point{899,99}, geom::Size{1,1}}),
         std::make_tuple(
             geom::Rectangle{geom::Point{-1,-1}, geom::Size{100,100}},
-            geom::Rectangle{geom::Point{-1,-1}, geom::Size{0,0}})
+            geom::Rectangle{geom::Point{0,0}, geom::Size{99,99}})
     };
 
     for (auto const& t : rect_tuples)
@@ -95,8 +95,20 @@
             geom::Rectangle{geom::Point{899,99}, geom::Size{30,40}},
             geom::Rectangle{geom::Point{800,0}, geom::Size{100,100}}),
         std::make_tuple(
+            geom::Rectangle{geom::Point{850,-10}, geom::Size{30,40}},
+            geom::Rectangle{geom::Point{800,0}, geom::Size{100,100}}),
+        std::make_tuple(
+            geom::Rectangle{geom::Point{749,20}, geom::Size{60,60}},
+            geom::Rectangle{geom::Point{0,0}, geom::Size{800,600}}),
+        std::make_tuple(
+            geom::Rectangle{geom::Point{790,20}, geom::Size{60,60}},
+            geom::Rectangle{geom::Point{800,0}, geom::Size{100,100}}),
+        std::make_tuple(
+            geom::Rectangle{geom::Point{-1000,-1000}, geom::Size{100,100}},
+            geom::Rectangle{geom::Point{0,0}, geom::Size{800,600}}),
+        std::make_tuple(
             geom::Rectangle{geom::Point{-1,-1}, geom::Size{100,100}},
-            geom::Rectangle{geom::Point{0,0}, geom::Size{0,0}})
+            geom::Rectangle{geom::Point{0,0}, geom::Size{800,600}})
     };
 
     for (auto const& t : rect_tuples)