Merge lp:~vorlon/compiz/lp.763148 into lp:compiz/0.9.10

[Steve Langasek]

Fix for bug #763148 (with added test cases): when the desktop is resized,
windows should stay on their original workspace.

Fix for bug #763148 (with added test cases): when the desktop is resized,
windows should stay on their original workspace.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:3647
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~vorlon/compiz/lp.763148/+merge/157534/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/compiz-ci/122/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-gles-ci/./build=pbuilder,distribution=raring,flavor=amd64/159/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-pbuilder/./build=pbuilder,distribution=raring,flavor=amd64/511/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/compiz-ci/122/rebuild

review: Needs Fixing (continuous-integration)

[Sam Spilsbury (smspillaz)]

Hey, thanks so much for doing this, and also adding test cases.

A quick note: I'm marking this as "resubmit" as there are merge conflicts. It appears that you've taken lp:compiz/raring, made changes there, and then submitted it for merging against lp:compiz . The two branches differ slightly (distro is cherry-picking fixes from lp:compiz to lp:compiz/0.9.9). Also - changes to debian/changelog are unnecessary and probably not wanted either. The automerger will automatically update the debian changelog, and updating it manually will probably confuse it.

As for some code review, here are some suggestions I have.

1. Thanks for refactoring some of the common setup code. I think the method could be renamed to assignStrutsWorkAreaAndGeoemtry
2. In TestScreenSizeChangePreviousViewport:

506 + /* Deal with the case where the position is negative, which means
507 + * it's actually wrapped around to the rightmost viewport
508 + */
509 + g.setPos (CompPoint (-300, 200));
510 + ms.setGeometry (g);
511 +
512 + expected = compiz::window::Geometry (-300, 200, 300, 400, 0);

It appears that the relevant assertion here is that the window will have the same geometry if wrapped to the rightmost viewport when the rightmost viewport is unplugged. In that case, it might be better to just set "expected" to "g", as they are both the same.

Everything else seems mostly fine to me.

Doing this review reminded me that both the tests and adjustForSize need some serious refactoring, because even in their simplified versions, there is way too much going on in both of them. I'm not going to block this review on it, however, because:
 a) They were monsters of my own creation, and I should have the responsibility to deal with them.
 b) I was going to suggest doing the refactoring in this review, but decided against it because articulating what needed to be done was so detailed and complicated that it would just cause miscommunication and lots of unrelated change.

[Sam Spilsbury (smspillaz)]

What you'll need to do from here is:

1. Branch lp:compiz
2. Cherry pick revisions 3643..3646 from your old branch into that one
3. Submit that branch.

review: Resubmit

[Sam Spilsbury (smspillaz)]

(Or at least 3646 from this branch)

[MC Return (mc-return)]

\o/
Stopping windows from jumping around weirdly should be numbero uno priority for us...
But I do not like this diff either...

review: Needs Fixing

[Steve Langasek (vorlon)]

On Sun, Apr 07, 2013 at 05:59:18AM -0000, Sam Spilsbury wrote:
> Hey, thanks so much for doing this, and also adding test cases.

> A quick note: I'm marking this as "resubmit" as there are merge conflicts.
> It appears that you've taken lp:compiz/raring, made changes there, and
> then submitted it for merging against lp:compiz

Yep, was betrayed by the default behavior of bzr lp-propose-merge here.
I've rebased now and resubmitted:

  https://code.launchpad.net/~vorlon/compiz/lp.763148/+merge/157537

> As for some code review, here are some suggestions I have.

> 1. Thanks for refactoring some of the common setup code. I think the
> method could be renamed to assignStrutsWorkAreaAndGeoemtry

I don't find that a very clear description of what the method does, since
it's also what calls adjustForSize(). But maybe I'm misunderstanding?

> 2. In TestScreenSizeChangePreviousViewport:

> 506 + /* Deal with the case where the position is negative, which means
> 507 + * it's actually wrapped around to the rightmost viewport
> 508 + */
> 509 + g.setPos (CompPoint (-300, 200));
> 510 + ms.setGeometry (g);
> 511 +
> 512 + expected = compiz::window::Geometry (-300, 200, 300, 400, 0);

> It appears that the relevant assertion here is that the window will have
> the same geometry if wrapped to the rightmost viewport when the rightmost
> viewport is unplugged. In that case, it might be better to just set
> "expected" to "g", as they are both the same.

Right, changed - thanks.

--
Steve Langasek Give me a lever long enough and a Free OS
Debian Developer to set it on, and I can move the world.
Ubuntu Developer http://www.debian.org/
<email address hidden> <email address hidden>

[Sam Spilsbury (smspillaz)]

This looks fine to me. I plan to do some rather large-ish refactoring on this section of the code, so lets not make the perfect the enemy of the good here.

review: Approve

[MC Return (mc-return)]

/offtopic on
During recent intensive testing I found another jumping window bug:
bug 1165695

Anyone an idea about the origin (in the code) of that one ?
/offtopic off

Steve, I could not test your branch that fast, but my "Approve" does
not count anyway ;)

review: Abstain

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:3651
http://jenkins.qa.ubuntu.com/job/compiz-ci/123/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-gles-ci/./build=pbuilder,distribution=raring,flavor=amd64/160
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-pbuilder/./build=pbuilder,distribution=raring,flavor=amd64/512

Click here to trigger a rebuild:
http://s-jenkins:8080/job/compiz-ci/123/rebuild

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

[Didier Roche (didrocks)]

@Steve: the destination branch is now 0.9.9 for raring (btw, I think we should update Vcs-Bzr). Mind havind a look for making a MP there as well?

=== modified file 'plugins/place/src/place.cpp'
--- plugins/place/src/place.cpp	2013-03-24 05:51:41 +0000
+++ plugins/place/src/place.cpp	2013-04-07 08:57:20 +0000
@@ -202,7 +202,9 @@
 PlaceScreen::handleEvent (XEvent *event)
 {
     if (event->type == ConfigureNotify &&
-	event->xconfigure.window == screen->root ())
+	event->xconfigure.window == screen->root () &&
+	(event->xconfigure.width != screen->width () ||
+	 event->xconfigure.height != screen->height ()))
     {
 	mPrevSize.setWidth (screen->width ());
 	mPrevSize.setHeight (screen->height ());

=== modified file 'plugins/place/src/screen-size-change/src/screen-size-change.cpp'
--- plugins/place/src/screen-size-change/src/screen-size-change.cpp	2012-07-30 10:01:45 +0000
+++ plugins/place/src/screen-size-change/src/screen-size-change.cpp	2013-04-07 08:57:20 +0000
@@ -40,8 +40,6 @@
     int            vpX, vpY;
     compiz::window::Geometry g, vpRelRect;
     int		   pivotX, pivotY;
-    int		   curVpOffsetX = getViewport ().x () * newSize.width ();
-    int		   curVpOffsetY = getViewport ().y () * newSize.height ();
 
     g = getGeometry ();
     compiz::window::Geometry og (g);
@@ -59,24 +57,14 @@
     if (pivotY < 0)
 	vpY -= 1;
 
-    /* if window's target vp is to the left of the leftmost viewport on that
-       row, assign its target vp column as 0 (-s->x rel. to current vp) */
-    if (getViewport ().x () + vpX < 0)
-	vpX = -getViewport ().x ();
-
-    /* if window's target vp is above the topmost viewport on that column,
-       assign its target vp row as 0 (-s->y rel. to current vp) */
-    if (getViewport ().y () + vpY < 0)
-	vpY = -getViewport ().y ();
-
     unsigned int mask = mSaver.pop (vpRelRect, CHANGE_X | CHANGE_Y |
 					       CHANGE_WIDTH | CHANGE_HEIGHT);
 
     if (mask)
     {
 	/* set position/size to saved original rectangle */
-	g.applyChange (compiz::window::Geometry (vpRelRect.x () + vpX * newSize.width (),
-						 vpRelRect.y () + vpY * newSize.height (),
+	g.applyChange (compiz::window::Geometry (vpRelRect.x (),
+						 vpRelRect.y (),
 						 vpRelRect.width (),
 						 vpRelRect.height (),
 						 vpRelRect.border ()), mask);
@@ -90,42 +78,24 @@
 	vpRelRect.setWidth (g.width ());
 	vpRelRect.setHeight (g.height ());
 
-	g.setPos (g.pos ());
-
-	int shiftX = vpX * (newSize.width () - oldSize.width ()),
-	    shiftY = vpY * (newSize.width () - oldSize.height ());
+	g = vpRelRect;
 
 	/* if coords. relative to viewport are outside new viewport area,
 	   shift window left/up so that it falls inside */
-	if (vpRelRect.x () >= newSize.width ())
-	    shiftX -= vpRelRect.x () - (newSize.width () - 1);
-	if (vpRelRect.y () >= newSize.height ())
-	    shiftY -= vpRelRect.y () - (newSize.height () - 1);
-
-	if (shiftX)
-	    g.setX (g.x () + shiftX);
-
-	if (shiftY)
-	    g.setY (g.y () + shiftY);
+	if (vpRelRect.x () + vpRelRect.width() >= newSize.width ())
+	    g.setX (g.x () - (vpRelRect.x () + vpRelRect.width () - newSize.width ()));
+	if (vpRelRect.y () + vpRelRect.height() >= newSize.height ())
+	    g.setY (g.y () - (vpRelRect.y () + vpRelRect.width () - newSize.height ()));
 
 	g.setWidth (vpRelRect.width ());
 	g.setHeight (vpRelRect.height ());
     }
 
-    /* Handle non-(0,0) current viewport by shifting by curVpOffsetX,Y,
-       and bring window to (0,0) by shifting by minus its vp offset */
-
-    g.setX (g.x () + curVpOffsetX - (getViewport ().x () + vpX) * newSize.width ());
-    g.setY (g.y () + curVpOffsetY - (getViewport ().y () + vpY) * newSize.height ());
-
     unsigned int flags = 0;
     const CompRect &workArea = getWorkarea (g);
 
     compiz::place::clampGeometryToWorkArea (g, workArea, getExtents (), flags, newSize);
 
-    g.setX (g.x () - curVpOffsetX + (getViewport ().x () + vpX) * newSize.width ());
-    g.setY (g.y () - curVpOffsetY + (getViewport ().y () + vpY) * newSize.height ());
-
     if (!mask)
     {
 	/* save window geometry (relative to viewport) so that it
@@ -151,6 +121,9 @@
 	mSaver.push (vpRelRect, remaining);
     }
 
+    g.setX (g.x () + vpX * newSize.width ());
+    g.setY (g.y () + vpY * newSize.height ());
+
     /* for maximized/fullscreen windows, update saved pos/size XXX,
      * also pull in the old code to handle maximized windows which
      * currently can't be implemented yet */

=== modified file 'plugins/place/src/screen-size-change/tests/screen-size-change/src/test-place-screen-size-change.cpp'
--- plugins/place/src/screen-size-change/tests/screen-size-change/src/test-place-screen-size-change.cpp	2012-07-26 09:10:09 +0000
+++ plugins/place/src/screen-size-change/tests/screen-size-change/src/test-place-screen-size-change.cpp	2013-04-07 08:57:20 +0000
@@ -57,6 +57,9 @@
 			 unsigned int bottom);
 
 	void setGeometry (const compiz::window::Geometry &g);
+	compiz::window::Geometry sizeAdjustTest (const CompSize &oldSize,
+						 const CompSize &newSize,
+						 CompRect &workArea);
 
     private:
 
@@ -162,21 +165,34 @@
     workArea.setBottom (workArea.bottom () - 24);
 }
 
+compiz::window::Geometry
+MockScreenSizeChangeObject::sizeAdjustTest (const CompSize &oldSize,
+					    const CompSize &newSize,
+					    CompRect &workArea)
+{
+    /* Reserve top, bottom and left parts of the screen for
+     * fake "24px" panels */
+    reserveStruts (workArea);
+
+    setWorkArea (workArea);
+
+    compiz::window::Geometry g = adjustForSize (oldSize, newSize);
+
+    return g;
+}
+
+
 TEST_F(CompPlaceScreenSizeChangeTestScreenSizeChange, TestScreenSizeChange)
 {
     CompSize		     current, old;
     compiz::window::Geometry g (200, 250, 300, 400, 0);
+    compiz::window::Geometry expected;
 
     MockScreenSizeChangeObject ms (g);
 
     current = CompSize (1280, 800);
 
-    /* Reserve top, bottom and left parts of the screen for
-     * fake "24px" panels */
-    CompRect workArea = CompRect (0, 0, current.width (), current.height ());
-    reserveStruts (workArea);
-
-    ms.setWorkArea (workArea);
+    CompRect workArea;
 
     /* First test that changing the screen size
      * to something smaller here doesn't cause our
@@ -184,114 +200,100 @@
 
     old = current;
     current = CompSize (1024, 768);
-
     workArea = CompRect (0, 0, current.width (), current.height ());
-    reserveStruts (workArea);
-
-    ms.setWorkArea (workArea);
-
-    g = ms.adjustForSize (old, current);
-
-    EXPECT_EQ (g, compiz::window::Geometry (200, 250, 300, 400, 0));
+
+    expected = compiz::window::Geometry (200, 250, 300, 400, 0);
+
+    g = ms.sizeAdjustTest (old, current, workArea);
+
+    EXPECT_EQ (expected, g);
 
     /* Making the screen size bigger with no
      * saved geometry should cause the window not to move */
-
     old = current;
     current = CompSize (2048, 768);
-
     workArea = CompRect (0, 0, current.width (), current.height ());
-    reserveStruts (workArea);
-
-    ms.setWorkArea (workArea);
-
-    g = ms.adjustForSize (old, current);
-
-    EXPECT_EQ (g, compiz::window::Geometry (200, 250, 300, 400, 0));
+
+    g = ms.sizeAdjustTest (old, current, workArea);
+
+    EXPECT_EQ (expected, g);
 
     /* Move the window to the other "monitor" */
-
     ms.setGeometry (compiz::window::Geometry (1025, 250, 300, 400, 0));
 
-    old = current;
-
     /* Unplug a "monitor" */
+    old = current;
     current = CompSize (1024, 768);
-
     workArea = CompRect (0, 0, current.width (), current.height ());
-    reserveStruts (workArea);
-
-    ms.setWorkArea (workArea);
-
-    g = ms.adjustForSize (old, current);
-
-    EXPECT_EQ (g, compiz::window::Geometry (724, 250, 300, 400, 0));
-
-    old = current;
+
+    expected = compiz::window::Geometry (724, 250, 300, 400, 0);
+
+    g = ms.sizeAdjustTest (old, current, workArea);
+
+    EXPECT_EQ (expected, g);
 
     /* Re-plug the monitor - window should go back
      * to the same position */
+    old = current;
     current = CompSize (2048, 768);
-
     workArea = CompRect (0, 0, current.width (), current.height ());
-    reserveStruts (workArea);
-
-    ms.setWorkArea (workArea);
-
-    g = ms.adjustForSize (old, current);
-
-    EXPECT_EQ (g, compiz::window::Geometry (1025, 250, 300, 400, 0));
-
-    old = current;
+
+    expected = compiz::window::Geometry (1025, 250, 300, 400, 0);
+
+    g = ms.sizeAdjustTest (old, current, workArea);
+
+    EXPECT_EQ (expected, g);
 
     /* Plug 2 monitors downwards, no change */
+    old = current;
     current = CompSize (2048, 1536);
-
     workArea = CompRect (0, 0, current.width (), current.height ());
-    reserveStruts (workArea);
-
-    ms.setWorkArea (workArea);
-
-    g = ms.adjustForSize (old, current);
-
-    EXPECT_EQ (g, compiz::window::Geometry (1025, 250, 300, 400, 0));
+
+    g = ms.sizeAdjustTest (old, current, workArea);
+
+    EXPECT_EQ (expected, g);
 
     /* Move the window to the bottom "monitor" */
-
     ms.setGeometry (compiz::window::Geometry (1025, 791, 300, 400, 0));
 
-    old = current;
-
     /* Unplug bottom "monitor" */
+    old = current;
     current = CompSize (2048, 768);
-
     workArea = CompRect (0, 0, current.width (), current.height ());
-    reserveStruts (workArea);
-
-    ms.setWorkArea (workArea);
-
-    g = ms.adjustForSize (old, current);
-
-    EXPECT_EQ (g, compiz::window::Geometry (1025, 344, 300, 400, 0));
-
-    old = current;
+
+    expected = compiz::window::Geometry (1025, 344, 300, 400, 0);
+
+    g = ms.sizeAdjustTest (old, current, workArea);
+
+    EXPECT_EQ (expected, g);
 
     /* Re-plug bottom "monitor" */
+    old = current;
     current = CompSize (2048, 1356);
-
     workArea = CompRect (0, 0, current.width (), current.height ());
-    reserveStruts (workArea);
-
-    ms.setWorkArea (workArea);
-
-    g = ms.adjustForSize (old, current);
-
-    EXPECT_EQ (g, compiz::window::Geometry (1025, 791, 300, 400, 0));
+
+    expected = compiz::window::Geometry (1025, 791, 300, 400, 0);
+
+    g = ms.sizeAdjustTest (old, current, workArea);
+
+    EXPECT_EQ (expected, g);
+
+}
+
+TEST_F(CompPlaceScreenSizeChangeTestScreenSizeChange, TestScreenChangeWindowsOnSecondViewport)
+{
+    CompSize		     current, old;
+    compiz::window::Geometry g (1025, 791, 300, 400, 0);
+    compiz::window::Geometry expected;
+
+    MockScreenSizeChangeObject ms (g);
+
+    current = CompSize (2048, 1356);
+
+    CompRect workArea;
 
     /* Move the entire window right a viewport */
-
     g.setPos (g.pos () + CompPoint (current.width (), 0));
-
     ms.setGeometry (g);
 
     /* Now change the screen resolution again - the window should
@@ -301,14 +303,115 @@
     /* Unplug a "monitor" */
     old = current;
     current = CompSize (1024, 1356);
-
-    workArea = CompRect (0, 0, current.width (), current.height ());
-    reserveStruts (workArea);
-
-    ms.setWorkArea (workArea);
-
-    g = ms.adjustForSize (old, current);
-
-    EXPECT_EQ (g, compiz::window::Geometry (current.width () + 724, 791, 300, 400, 0));
-}
-
+    workArea = CompRect (0, 0, current.width (), current.height ());
+
+    expected = compiz::window::Geometry (current.width () + 724, 791, 300, 400, 0);
+
+    g = ms.sizeAdjustTest (old, current, workArea);
+
+    EXPECT_EQ (expected, g);
+
+    /* Replug the monitor, make sure that the geometry is restored */
+    old = current;
+    current = CompSize (2048, 1356);
+    workArea = CompRect (0, 0, current.width (), current.height ());
+
+    expected = compiz::window::Geometry (current.width () + 1025, 791, 300, 400, 0);
+
+    g = ms.sizeAdjustTest (old, current, workArea);
+
+    EXPECT_EQ (expected, g);
+
+    /* Replug the monitor and move the window to where it fits on the first
+     * monitor on the second viewport, then make sure it doesn't move */
+    old = CompSize (2048, 1356);
+    current = CompSize (1024, 1356);
+    workArea = CompRect (0, 0, current.width (), current.height ());
+
+    g.setPos (CompPoint (old.width () + 25, 791));
+    ms.setGeometry (g);
+    /* clear the saved geometry */
+    ms.unset();
+
+    expected = compiz::window::Geometry (current.width () + 25, 791, 300, 400, 0);
+
+    g = ms.sizeAdjustTest (old, current, workArea);
+
+    EXPECT_EQ (expected, g);
+
+    old = current;
+    current = CompSize (2048, 1356);
+    workArea = CompRect (0, 0, current.width (), current.height ());
+
+    expected = compiz::window::Geometry (current.width () + 25, 791, 300, 400, 0);
+
+    g = ms.sizeAdjustTest (old, current, workArea);
+
+    EXPECT_EQ (expected, g);
+}
+
+TEST_F(CompPlaceScreenSizeChangeTestScreenSizeChange, TestScreenChangeWindowsOnPreviousViewport)
+{
+    CompSize		     current, old;
+    compiz::window::Geometry g (0, 0, 300, 400, 0);
+    compiz::window::Geometry expected;
+
+    MockScreenSizeChangeObject ms (g);
+
+    current = CompSize (2048, 1356);
+
+    CompRect workArea;
+
+    /* Deal with the case where the position is negative, which means
+     * it's actually wrapped around to the rightmost viewport
+     */
+    g.setPos (CompPoint (-300, 200));
+    ms.setGeometry (g);
+
+    expected = g;
+
+    /* Unplug the right "monitor" */
+    old = current;
+    current = CompSize (1024, 1356);
+    workArea = CompRect (0, 0, current.width (), current.height ());
+
+    g = ms.sizeAdjustTest (old, current, workArea);
+
+    EXPECT_EQ (expected, g);
+
+    /* Re-plug the right "monitor" */
+    old = current;
+    current = CompSize (2048, 1356);
+    workArea = CompRect (0, 0, current.width (), current.height ());
+
+    g = ms.sizeAdjustTest (old, current, workArea);
+
+    EXPECT_EQ (expected, g);
+
+    /* Move the window to the left monitor, verify that it survives an
+     * unplug/plug cycle
+     */
+    g.setPos (CompPoint (-1324, 200));
+    ms.setGeometry (g);
+
+    old = current;
+    current = CompSize (1024, 1356);
+    workArea = CompRect (0, 0, current.width (), current.height ());
+
+    expected = compiz::window::Geometry (-300, 200, 300, 400, 0);
+
+    g = ms.sizeAdjustTest (old, current, workArea);
+
+    EXPECT_EQ (expected, g);
+
+    old = current;
+    current = CompSize (2048, 1356);
+    workArea = CompRect (0, 0, current.width (), current.height ());
+
+    expected = compiz::window::Geometry (-1324, 200, 300, 400, 0);
+
+    g = ms.sizeAdjustTest (old, current, workArea);
+
+    EXPECT_EQ (expected, g);
+
+}