Merge lp:~vanvugt/compiz/fix-751605 into lp:compiz/0.9.9

[Daniel van Vugt]

Prevent windows from maximizing on the wrong monitor (LP: #751605)

This fixes two causes I have found:
  1. outputDeviceForGeometry was wrapping to the wrong monitor (back to the
     top or left) if a window was mostly off the bottom/right of the screen.
  2. Even when outputDeviceForGeometry returned the correct output,
     PrivateWindow::addWindowSizeChanges was sometimes changing it to an
     incorrect output in the case where the old size of a window exceeds
     the dimensions of the smaller monitor you're trying to maximize it on.

[MC Return (mc-return)]

+1

Although I already was used to resizing windows to smaller size, before hitting the maximize button to maximize those on my smaller screen, I prefer not having to do it :)

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:3487
http://jenkins.qa.ubuntu.com/job/compiz-ci/249/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-ci/./build=pbuilder,distribution=quantal,flavor=amd64/249/console

review: Approve (continuous-integration)

[Sam Spilsbury (smspillaz)]

+ * There used to be a lot more logic here to handle the rare special
+ * case of maximizing a window whose hints say it is too large to fit
+ * the output and choose a different one. However that logic was a bad
+ * idea because:
+ * (1) It's confusing to the user to auto-magically move a window
+ * between monitors when they didn't ask for it. So don't.
+ * (2) In the worst case where the window can't go small enough to fit
+ * the output, they can simply move it with Alt+drag, Alt+F7 or
+ * expo.

I'm a little skeptical that this is actually the correct solution, and in fact it reintroduces the regression it was trying to solve.

The problem r2775.2.3 was trying to solve was that maximizing a window on a monitor where there was no room for it (eg, the monitor was smaller than the actual window size). In that case, the "next best" solution was to go to the next monitor.

Was bug 751605 about this behaviour specifically? Or was it about a general problem where windows may occasionally maximize over the wrong monitor (and its possible to maximize over both).

Having a window maximized on a monitor where the entire window is made not visible as a result is a really bad situation to get into IMO. First of all, the constrainment code is not really designed to handle that situation and so what you end up with is a window that tends to jump up and down on that monitor on input as the constrainment code tries to get it within the monitor bounds. Second of all, we can't assume that all users know about alt-drag or even the workspace switcher. When the window titlebar is thus made invisible because it is offscreen, the user is effectively not able to get the window back into its original state. That's an even more buggy state to get into because instead of doing something that might initially seem against the user's intention, you've essentially gotten them into a situation they can't get out of.

Reading the comments of bug 751605 it seems like the /real/ gripe is with the fact that the grid plugin displays the window maximization preview on the first monitor and then the maximization happens on the second. I think, just brainstorming a little that there are some better solutions to this problem:

a) Don't allow maximization of windows that might be bigger than one of the available monitors
b) Allow maximization of windows when they are already on the monitor that they can be maximized on (though this could be a little tricky to implement - and could look a bit weird if a window suddenly gains a maximize button as a result of moving it between the monitors)
c) Display the grid preview in the place where the window is about to go.

Is it possible that we can consider those alternatives? Trust me - if you've ever seen what happens when you maximize a window to a smaller size then its minimize size the result is really really bad.

review: Needs Information

[Sam Spilsbury (smspillaz)]

Because this is a weird corner case I had a look at what the other window managers are doing to deal with it:

 * GNOME-Shell has the benefit of never showing the maximize button in the first place, so when you try and maximize a window by dragging it to the top of the screen, it just does not allow you to do that on the small screen
 * KWin will top-center the window as best as it can, so that the titlebar is visible but other parts of the window aren't
 * Metacity just marks the window "maximized" and keeps it in the same place.

[Daniel van Vugt (vanvugt)]

I did have a nice simple solution that did not involve deleting that chunk but just changed a few lines. However I tried to avoid it initially because it was very hard to get test coverage of. And after a while I decided (in my opinion) that it should probably not be there at all.

I can reintroduce the code and just fix the lines pertaining to bug 751605, but I couldn't find an elegant way to give the change test coverage without breaking the core ABI (which is bad given how much this bug needs fixing in precise too).

[Daniel van Vugt (vanvugt)]

Yes I was thinking refusing to maximize in some cases would be a nice solution. Not sure if that's easy to do with compiz as it is.

[MC Return (mc-return)]

> + * (1) It's confusing to the user to auto-magically move a window
> + * between monitors when they didn't ask for it. So don't.
I agree 100%.

> + * (2) In the worst case where the window can't go small enough to fit
> + * the output, they can simply move it with Alt+drag, Alt+F7 or
> + * expo.
>
True also.

> Reading the comments of bug 751605 it seems like the /real/ gripe is with the
> fact that the grid plugin displays the window maximization preview on the
> first monitor and then the maximization happens on the second. I think, just
> brainstorming a little that there are some better solutions to this problem:
>
This is a big problem as the action taken should always follow the preview displayed.
Currently this works correctly, btw if the windows' x- or y-size is larger than the largest resolution of your monitors it is resized on the correct one via grid.
To test this simply open a window, resize it to a huge x or y resolution (larger than the respective highest resolution of your monitors) and then activate the grid on the smaller monitor - it will recale and go fullscreen correctly.

> a) Don't allow maximization of windows that might be bigger than one of the
> available monitors

Bad solution IMHO. Compiz should do what the user orders him to do (via grid for example).

> b) Allow maximization of windows when they are already on the monitor that
> they can be maximized on (though this could be a little tricky to implement -
> and could look a bit weird if a window suddenly gains a maximize button as a
> result of moving it between the monitors)

I cannot notice a problem with making a window smaller when maximizing it. We should make sure though that the size gets correctly restored afterwards (which is not happening in all cases, but that is another story...)

> c) Display the grid preview in the place where the window is about to go.
>

This should always be the case, but it would feel strange if I want to activate the grid on the smaller monitor and the preview shows me it would go fullscreen on the larger one...

> Is it possible that we can consider those alternatives? Trust me - if you've
> ever seen what happens when you maximize a window to a smaller size then its
> minimize size the result is really really bad.

You can easily test what happens then by creating a window larger than your largest monitor's x and y resolution and then maximize it on your smaller monitor (via grid for example). I cannot notice anything bad...

[Sam Spilsbury (smspillaz)]

I quickly had a test of this branch and noticed that the jumpiness problem that used to be quite bad in older versions of the code is no longer a problem. So that's probably a good thing. Things can still get jumpy when you alt-drag, but it seems for now that its not possible to get into a situation where the window titlebar is not visible.

I also tested this branch with the chunk that removes the part of the code which considered other monitors in that case reverted and it works as expected when you have variable size windows and two monitors stacked one above the other. That was primarily what bug 751605 was about.

I think it might be good to get some design feedback as to exactly what to do in this case though. I suspect that the design team only intended the "maximize on the same monitor that the user asked for the maximize on" statement for the cases where it was actually possible to do so. I'm sure that the design team didn't intend for that to extend to cases where it would be impossible to do so - for example where maximizing the window would actually make it larger than the screen size and thus parts of the window were offscreen.

However, considering that the "jumping behaviour" is no longer the problem that it once was when I first fixed this problem, I'm happy for it to go either way.

[Sam Spilsbury (smspillaz)]

> I did have a nice simple solution that did not involve deleting that chunk but
> just changed a few lines. However I tried to avoid it initially because it was
> very hard to get test coverage of. And after a while I decided (in my opinion)
> that it should probably not be there at all.
>
> I can reintroduce the code and just fix the lines pertaining to bug 751605,
> but I couldn't find an elegant way to give the change test coverage without
> breaking the core ABI (which is bad given how much this bug needs fixing in
> precise too).

Was it actually evident that this chunk was ever a part of the problem?

As far as I can tell, that chunk only ever comes into operation where constrainNewWindowSize fails and returns a width or height that we can't accept. bug 751605 was about cases where constrainNewWindowSize failed and the window still ended up on the wrong monitor.

As mentioned, reverting that change didn't seem to have any effect.

What was the problem with the test coverage?

[Sam Spilsbury (smspillaz)]

> > b) Allow maximization of windows when they are already on the monitor that
> > they can be maximized on (though this could be a little tricky to implement
> -
> > and could look a bit weird if a window suddenly gains a maximize button as a
> > result of moving it between the monitors)
>
> I cannot notice a problem with making a window smaller when maximizing it. We
> should make sure though that the size gets correctly restored afterwards
> (which is not happening in all cases, but that is another story...)

The problem with ignoring the size hint is that applications don't expect that, and there's a good chance that you'll break them. I tried hacking it to make it ignore the size hints for gtk windows and what happens is that gtk starts asserting like crazy and drawing widgets on top of each other. You really have to obey what the application tells you.

>
> > c) Display the grid preview in the place where the window is about to go.
> >
>
> This should always be the case, but it would feel strange if I want to
> activate the grid on the smaller monitor and the preview shows me it would go
> fullscreen on the larger one...

I agree, which is why I was hesitant to mention it. However for the reason I outlined below (eg, you can't fit a big thing into a small space, its impossible), I think it is the lesser of two evils rather than having this undefined behaviour where a large window occupies a small space.

>
> > Is it possible that we can consider those alternatives? Trust me - if you've
> > ever seen what happens when you maximize a window to a smaller size then its
> > minimize size the result is really really bad.
>
> You can easily test what happens then by creating a window larger than your
> largest monitor's x and y resolution and then maximize it on your smaller
> monitor (via grid for example). I cannot notice anything bad...

The main problem I'm trying to outline is where the window has a /minimum size/ that is larger in any dimention than the monitor that it is on. Try it with ccsm and a resolution of 640x480, and you'll see that the maximized window is actually larger than the monitor and some of it is inaccessible as a result (though thankfully the titlebar is accessible).

Making a window that has no minimum size, or a minimum size smaller than the size of the monitor larger than the monitor and then maximizing it isn't a problem here and hasn't been a problem.

There are some cases where doing exactly what the user specifies gets them into a broken experience. We should avoid that.

[Daniel van Vugt (vanvugt)]

OK, I'm not reading any more. Decide what you want and I'll do it next week.

[Sam Spilsbury (smspillaz)]

Well, I'd say have a look at what is necessary and then let me know.
If removing the second chunk is necessary to fix the original bug,
then just put it here and I'll be fine with it. I just want to make
sure that we don't regress the original thing that was trying to get
around without there being a good reason to.

On Thu, Nov 15, 2012 at 6:35 PM, Daniel van Vugt
<email address hidden> wrote:
> OK, I'm not reading any more. Decide what you want and I'll do it next week.
> --
> https://code.launchpad.net/~vanvugt/compiz/fix-751605/+merge/134404
> You are reviewing the proposed merge of lp:~vanvugt/compiz/fix-751605 into lp:compiz.

--
Sam Spilsbury

[John Lea (johnlea)]

@smspillaz; yes the gesture preview displaying on the wrong monitor is annoying, but the main issues is that "the window should always maximise on the the monitor that contains the majority of the window at the moment the user gives the maximise command".

Regarding the corner case of the minimum size (*not the current size*) of a window being larger than the available area when in the maximised state, I agree that a nice solution would be to prevent the user from trying to maximise the window in the first place. Yes having the 'maximise' window decoration disappear and reappear is a bit weird, but it is better than either the window maximising when their isn't enough space, or the user pressing the button and nothing happening. Given that this is a very rare corner case I am happy to go for "maximise button disappears when the minimum possible size of the window is larger than the available screen area when maximised" as a solution.

[Sam Spilsbury (smspillaz)]

Okay, lets go with this then.

I anticipate that it won't be that difficult to implement the
conditional maximize behaviour. All that needs to be done is to track
the window position and set the CompWindowActionMaximizeMask flag in
the window actions.

Might be more difficult to get under test though, but we should
probably make an exception for this.

On Thu, Nov 15, 2012 at 10:50 PM, John Lea <email address hidden> wrote:
> @smspillaz; yes the gesture preview displaying on the wrong monitor is annoying, but the main issues is that "the window should always maximise on the the monitor that contains the majority of the window at the moment the user gives the maximise command".
>
> Regarding the corner case of the minimum size (*not the current size*) of a window being larger than the available area when in the maximised state, I agree that a nice solution would be to prevent the user from trying to maximise the window in the first place. Yes having the 'maximise' window decoration disappear and reappear is a bit weird, but it is better than either the window maximising when their isn't enough space, or the user pressing the button and nothing happening. Given that this is a very rare corner case I am happy to go for "maximise button disappears when the minimum possible size of the window is larger than the available screen area when maximised" as a solution.
>
>
> --
> https://code.launchpad.net/~vanvugt/compiz/fix-751605/+merge/134404
> You are reviewing the proposed merge of lp:~vanvugt/compiz/fix-751605 into lp:compiz.

--
Sam Spilsbury

[MC Return (mc-return)]

@smspillaz: Sorry, I did not know about this potential problem with the minimum size of windows and so did not think about it. So thanks a lot for bringing me up-to-date on that one.

[Sam Spilsbury (smspillaz)]

Approve for now, lets deal with the (lack of) maximize button later.

review: Approve

[Daniel van Vugt (vanvugt)]

Sorry, I was on vacation as at my last comment so was not in the mood to discuss it further until returning today.

If there are any undesirable consequences of this merge, I will fix them.

=== modified file 'src/outputdevices.cpp'
--- src/outputdevices.cpp	2012-11-14 03:14:31 +0000
+++ src/outputdevices.cpp	2012-11-15 07:07:19 +0000
@@ -103,28 +103,11 @@
 
     if (strategy == CoreOptions::OverlappingOutputsSmartMode)
     {
-	int centerX, centerY;
-
-	/* for smart mode, calculate the overlap of the whole rectangle
-	   with the output device rectangle */
-	geomRect.setWidth (gm.width () + 2 * gm.border ());
-	geomRect.setHeight (gm.height () + 2 * gm.border ());
-
-	x = gm.x () % screen->width ();
-	centerX = (x + (geomRect.width () / 2));
-	if (centerX < 0)
-	    x += screen->width ();
-	else if (centerX > screen->width ())
-	    x -= screen->width ();
-	geomRect.setX (x);
-
-	y = gm.y () % screen->height ();
-	centerY = (y + (geomRect.height () / 2));
-	if (centerY < 0)
-	    y += screen->height ();
-	else if (centerY > screen->height ())
-	    y -= screen->height ();
-	geomRect.setY (y);
+	/* We're only going to use geomRect for overlapping area calculations,
+	   so the window rectangle is enough. We don't need to consider
+	   anything more like the border because it will never be significant
+	   to the result */
+	geomRect = gm;
     }
     else
     {
 
=== modified file 'src/tests/test_outputdevices.cpp'
--- src/tests/test_outputdevices.cpp	2012-11-14 05:45:36 +0000
+++ src/tests/test_outputdevices.cpp	2012-11-15 07:07:19 +0000
@@ -64,10 +64,8 @@
     w.set (50, 50, 100, 100, 0);
     EXPECT_EQ (0, d.outputDeviceForGeometry (w, 0, &s));
 
-    /* FIXME... but not until the initial refactoring has landed
     w.set (-50, 50, 10, 10, 0);
     EXPECT_EQ (0, d.outputDeviceForGeometry (w, 0, &s));
-    */
 
     w.set (-50, 50, 100, 10, 0);
     EXPECT_EQ (0, d.outputDeviceForGeometry (w, 0, &s));
@@ -78,8 +76,16 @@
     w.set (10, 0, 3000, 768, 0);
     EXPECT_EQ (1, d.outputDeviceForGeometry (w, 0, &s));
 
+    // Way off-screen to the right. Both outputs match equally with an area
+    // of zero. We don't care about distance so just choose the first.
     w.set (99999, 100, 123, 456, 0);
+    EXPECT_EQ (0, d.outputDeviceForGeometry (w, 0, &s));
+
+    w.set (1500, 100, 2000, 456, 0);
     EXPECT_EQ (1, d.outputDeviceForGeometry (w, 0, &s));
+
+    w.set (0, 0, 2048, 768, 0);
+    EXPECT_EQ (0, d.outputDeviceForGeometry (w, 0, &s));
 }
 
 TEST (OutputDevices, LaptopBelowMonitor)
@@ -102,5 +108,61 @@
 
     w.set (200, 1500, 20, 20, 0);
     EXPECT_EQ (1, d.outputDeviceForGeometry (w, 0, &s));
+
+    w.set (100, 1800, 700, 700, 0);
+    EXPECT_EQ (1, d.outputDeviceForGeometry (w, 0, &s));
+}
+
+TEST (OutputDevices, LaptopNextToMonitor)
+{
+    OutputDevices d;
+    CompSize s (3200, 1200);
+    CompWindow::Geometry w;
+
+    d.setGeometryOnDevice (0, 0, 400, 1280, 800);
+    d.setGeometryOnDevice (1, 1280, 0, 1920, 1200);
+
+    w.set (-10, -10, 1034, 778, 0);
+    EXPECT_EQ (0, d.outputDeviceForGeometry (w, 0, &s));
+
+    w.set (200, 300, 20, 20, 0);
+    EXPECT_EQ (1, d.outputDeviceForGeometry (w, 0, &s));
+
+    w.set (-10, 10, 1500, 500, 0);
+    EXPECT_EQ (0, d.outputDeviceForGeometry (w, 0, &s));
+
+    w.set (900, 50, 3000, 20, 0);
+    EXPECT_EQ (1, d.outputDeviceForGeometry (w, 0, &s));
+
+    w.set (10, 500, 2542, 100, 0);
+    EXPECT_EQ (1, d.outputDeviceForGeometry (w, 0, &s));
+}
+
+TEST (OutputDevices, FourSquare)
+{
+    OutputDevices d;
+    CompSize s (2000, 2000);
+    CompWindow::Geometry w;
+
+    d.setGeometryOnDevice (0,    0,    0, 1000, 1000);
+    d.setGeometryOnDevice (1, 1000,    0, 1000, 1000);
+    d.setGeometryOnDevice (2,    0, 1000, 1000, 1000);
+    d.setGeometryOnDevice (3, 1000, 1000, 1000, 1000);
+
+    w.set (-10, -10, 1034, 778, 0);
+    EXPECT_EQ (0, d.outputDeviceForGeometry (w, 0, &s));
+
+    w.set (900, 300, 300, 200, 0);
+    EXPECT_EQ (1, d.outputDeviceForGeometry (w, 0, &s));
+
+    w.set (900, 900, 201, 201, 0);
+    EXPECT_EQ (3, d.outputDeviceForGeometry (w, 0, &s));
+
+    w.set (-10, 1010, 2000, 500, 0);
+    EXPECT_EQ (2, d.outputDeviceForGeometry (w, 0, &s));
+
+    // When there are multiple canidates with equal scores, choose the first:
+    w.set (-5, -5, 3000, 3000, 0);
+    EXPECT_EQ (0, d.outputDeviceForGeometry (w, 0, &s));
 }
 
 
=== modified file 'src/window.cpp'
--- src/window.cpp	2012-10-29 14:34:58 +0000
+++ src/window.cpp	2012-11-15 07:07:19 +0000
@@ -3513,71 +3513,20 @@
      * but at least the user will be able to see all of the window */
     output   = &screen->outputDevs ().at (screen->outputDeviceForGeometry (old));
 
-    if (state & CompWindowStateFullscreenMask ||
-	state & CompWindowStateMaximizedHorzMask)
-    {
-	int width = (mask & CWWidth) ? xwc->width : old.width ();
-	int height = (mask & CWHeight) ? xwc->height : old.height ();
-
-	window->constrainNewWindowSize (width, height, &width, &height);
-
-	if (width > output->width ())
-	{
-	    int        distance = std::numeric_limits <int>::max ();
-	    CompOutput *selected = output;
-	    /* That's no good ... try and find the closest output device to this one
-	     * which has a large enough size */
-	    foreach (CompOutput &o, screen->outputDevs ())
-	    {
-		if (o.workArea ().width () > width)
-		{
-		    int tDistance = sqrt (pow (abs (o.x () - output->x ()), 2) +
-					  pow (abs (o.y () - output->y ()), 2));
-
-		    if (tDistance < distance)
-		    {
-			selected = &o;
-			tDistance = distance;
-		    }
-		}
-	    }
-
-	    output = selected;
-	}
-    }
-
-    if (state & CompWindowStateFullscreenMask ||
-	state & CompWindowStateMaximizedVertMask)
-    {
-	int width = (mask & CWWidth) ? xwc->width : old.width ();
-	int height = (mask & CWHeight) ? xwc->height : old.height ();
-
-	window->constrainNewWindowSize (width, height, &width, &height);
-
-	if (height > output->height ())
-	{
-	    int        distance = std::numeric_limits <int>::max ();
-	    CompOutput *selected = output;
-	    /* That's no good ... try and find the closest output device to this one
-	     * which has a large enough size */
-	    foreach (CompOutput &o, screen->outputDevs ())
-	    {
-		if (o.workArea ().height () > height)
-		{
-		    int tDistance = sqrt (pow (abs (o.x () - output->x ()), 2) +
-					  pow (abs (o.y () - output->y ()), 2));
-
-		    if (tDistance < distance)
-		    {
-			selected = &o;
-			tDistance = distance;
-		    }
-		}
-	    }
-
-	    output = selected;
-	}
-    }
+    /*
+     * output is now the correct output for the given geometry.
+     * There used to be a lot more logic here to handle the rare special
+     * case of maximizing a window whose hints say it is too large to fit
+     * the output and choose a different one. However that logic was a bad
+     * idea because:
+     *   (1) It's confusing to the user to auto-magically move a window
+     *       between monitors when they didn't ask for it. So don't.
+     *   (2) In the worst case where the window can't go small enough to fit
+     *       the output, they can simply move it with Alt+drag, Alt+F7 or
+     *       expo.
+     * Not moving the window at all is much less annoying than moving it when
+     * the user never asked to.
+     */
 
     workArea = output->workArea ();