Merge lp:~charon030/compiz-core/fix-751605 into lp:compiz-core

[Charon]

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.

I ported the same code changes Daniel van Vugt did in

https://code.launchpad.net/~vanvugt/compiz/fix-751605

to compiz-core (Compiz 0.9.7.x). This will allow users of Ubuntu 12.04 LTS to benefit from Daniel's bugfix.

[Daniel van Vugt (vanvugt)]

Looks almost perfect, however I notice some whitespace changes on lines 117 and 119 compared to the original fix:
https://code.launchpad.net/~vanvugt/compiz/fix-751605/+merge/134404

Please ensure the whitespace etc are identical to the original fix so we don't encounter conflicts in future.

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

Also, I don't want to rush a fix into precise until the fix has been tested for a little while by raring/quantal users. So we should backport to 0.9.8 for quantal first.

It's becoming a common mistake to backport fixes to LTS before they have had real-world testing in the latest release(s) first. As annoying as it is to delay a fix, we should always ensure fixes have had user testing in newer releases first.

[Charon (charon030)]

Daniel, thanks for your comments. I wasn't aware of that policy. I will fix the patch with respect to the spaces.

[Daniel van Vugt (vanvugt)]

Please re-propose for lp:compiz/0.9.8 instead.

I would rather risk testing it on quantal before it's accepted for precise.

review: Needs Resubmitting

[Charon (charon030)]

Any chance that this patch could be accepted now? I backported the patch also to 0.9.8 but since then no new release was published. Anyway the patch has been for quite a while in raring.
I know priority is on Mir now. Will there be there still be bugfix releases of compiz in the future?

[Christopher Townsend (townsend)]

Hi Charon,

I'm going to see about getting this accepted into lp:compiz-core. That said, I noticed that the original merge had some tests. Can those tests also be backported to this MP?

Seeing as this has been in Raring since late November of last year and the code is still intact, I say otherwise this is good.

Thanks!

review: Needs Information

[Charon (charon030)]

Hi Christopher,

thanks for having a look at this. The tests aren't here more or less by intention. See also the comments from Daniel van Vugt when backporting the patch to compiz 0.9.8:

https://code.launchpad.net/~charon030/compiz/fix-751605/+merge/138867

Thanks!

[Christopher Townsend (townsend)]

Hi Charon,

Ah, ok, got it! I'm going to go ahead and approve this MP into lp:compiz-core and see about getting it SRU'd for 12.04.

Thanks!

review: Approve

[Unity Merger (unity-merger)]

No commit message specified.

[Christopher Townsend (townsend)]

I set the commit message and re-approved.

=== modified file 'src/screen.cpp'
--- src/screen.cpp	2012-06-20 15:32:11 +0000
+++ src/screen.cpp	2013-03-20 19:10:25 +0000
@@ -4095,28 +4095,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 () % width ();
-	centerX = (x + (geomRect.width () / 2));
-	if (centerX < 0)
-	    x += width ();
-	else if (centerX > width ())
-	    x -= width ();
-	geomRect.setX (x);
-
-	y = gm.y () % height ();
-	centerY = (y + (geomRect.height () / 2));
-	if (centerY < 0)
-	    y += height ();
-	else if (centerY > height ())
-	    y -= 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/window.cpp'
--- src/window.cpp	2012-04-19 03:51:42 +0000
+++ src/window.cpp	2013-03-20 19:10:25 +0000
@@ -3781,71 +3781,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 ();