Merge lp:~smspillaz/compiz-core/compiz-core.fix_939228 into lp:compiz-core/0.9.8

[Sam Spilsbury]

== Problem ==

See LP#939288 - windows would jump if you had them on screen edges. Add tests for viewport picking behaviour.

== Solution ==

Don't return negative values for CompScreen::viewportForGeometry

== Test ==

Included.

[Alan Griffiths (alan-griffiths)]

why change "class" to "struct" when it behaves like a struct?

[Alan Griffiths (alan-griffiths)]

LGTM

review: Approve

[Daniel van Vugt (vanvugt)]

The fix works if I follow the test case that I just documented in bug 939228. However the opposite way around is still broken;

1. Move a window to the far left workspace so that it overlaps the left side of the screen a little.
2. Switch to the far right workspace where you can see a little of the overlapping window.
3. Alt-Tab to the window.

Observed: Window jumps to being entirely on the far left workspace.
Expected: Window stays where it was.

review: Needs Fixing

[Sam Spilsbury (smspillaz)]

Hi,

I am unable to reproduce the issue you mentioned. Are you running with lp:~smspillaz/compiz-wall-plugin/compiz-wall-plugin.fix_939228 ?

[Sam Spilsbury (smspillaz)]

conflicts fixed.

[Daniel van Vugt (vanvugt)]

Deduplicated bugs. This is now bug 755842.

[Daniel van Vugt (vanvugt)]

If the other branch is a prereq, can we mark it as such? (even though it's a different project)

[Daniel van Vugt (vanvugt)]

> conflicts fixed.

But not pushed to LP yet. The conflict is still there :)
Though it's simple enough for anyone to fix that I won't say Needs Fixing.

[Sam Spilsbury (smspillaz)]

Hum, I'll try again

[Alan Griffiths (alan-griffiths)]

review: Approve

[Daniel van Vugt (vanvugt)]

I'm still trying to test the prerequisite wall fix, but am delayed by new cmake/precise bugs.

[Daniel van Vugt (vanvugt)]

Contains a few nontrivial conflicts. Please fix.

review: Needs Fixing

[Daniel van Vugt (vanvugt)]

Needs Information:
As far as I can tell, the bug is already fixed by the prerequisite wall fix, which is now committed. I can't reproduce the bug any more so why do we need to complicate viewportForGeometry or change it at all?

Needs Fixing:
This branch introduces odd viewport bugs where windows suddenly change the viewport they're on when you select (zoom into) a new viewport using expo. lp:compiz-core has no such bugs.

So the bug doesn't exist any more, and this branch introduces a worse one.

If you would still like to submit this to keep the test cases, please:
1. Fix the regression; and
2. Disassociate the branch from the bug numbers.

review: Needs Fixing

[Sam Spilsbury (smspillaz)]

If the wall bug is fixed, then I guess it makes sense to keep the old behaviour and keep the tests. I'll resubmit to that end.

[Daniel van Vugt (vanvugt)]

The target branch is now retired. Please resubmit for: lp:compiz/0.9.8

(and ideally please remove references to bug numbers in the new proposal)

review: Needs Resubmitting

Unmerged revisions

3116. By Sam Spilsbury on 2012-05-17

Revert to the old behaviour with a note about possible bugs

3115. By Sam Spilsbury on 2012-05-15

Merged lp:compiz-core

3114. By Sam Spilsbury on 2012-05-15

Merge lp:compiz-core

3113. By Sam Spilsbury on 2012-04-27

Merge lp:compiz-core

3112. By Sam Spilsbury on 2012-04-24

s/class/struct/

3111. By Sam Spilsbury on 2012-04-24

Don't return a negative value for CompScreen::viewportForGeometry

Fix LP#939288 - If a window was partially on a viewport which was the last
one in its row or column, and we were on the viewport on the opposite side
of that row or column, compiz would work backwards to find the position
of the window (which would be negative) and return a negative viewport
position. This caused plugins like wall to behave strangely

=== modified file 'src/privatescreen.h'
--- src/privatescreen.h	2012-05-10 12:58:01 +0000
+++ src/privatescreen.h	2012-05-17 05:48:18 +0000
@@ -440,15 +440,41 @@
 	unsigned int activeNum;
 };
 
+class ViewportRetrievalInterface
+{
+    public:
+
+	virtual ~ViewportRetrievalInterface () {}
+
+	virtual const CompPoint & getCurrentViewport () const = 0;
+	virtual const CompSize & viewportDimentions () const = 0;
+};
+
 // Apart from a use by StartupSequence::addSequence this data
 // is only used by CompScreenImpl - like the OrphanData struct
-struct ViewPort
+struct ViewPort :
+    public ViewportRetrievalInterface
 {
-    ViewPort();
-    CompPoint    vp;
-    CompSize     vpSize;
+    public:
+
+	ViewPort();
+	CompPoint    vp;
+	CompSize     vpSize;
+
+    private:
+
+	const CompPoint & getCurrentViewport () const { return vp; }
+	const CompSize & viewportDimentions () const { return vpSize; }
 };
 
+namespace viewports
+{
+    void viewportForGeometry (const CompWindow::Geometry &gm,
+			      CompPoint                   &viewport,
+			      ViewportRetrievalInterface *viewports,
+			      const CompSize &screenSize);
+}
+
 class StartupSequence : boost::noncopyable
 {
     public:

=== modified file 'src/privatescreen/tests/test-privatescreen.cpp'
--- src/privatescreen/tests/test-privatescreen.cpp	2012-05-10 11:30:52 +0000
+++ src/privatescreen/tests/test-privatescreen.cpp	2012-05-17 05:48:18 +0000
@@ -10,6 +10,10 @@
 
 #include <stdlib.h>
 
+using ::testing::Return;
+using ::testing::ReturnRef;
+using ::testing::_;
+
 namespace {
 
 class MockCompScreen : public CompScreen
@@ -202,6 +206,15 @@
     MOCK_METHOD1(getWindowState, unsigned int (Window id));
 };
 
+class MockViewportRetreival :
+    public compiz::private_screen::ViewportRetrievalInterface
+{
+    public:
+
+	MOCK_CONST_METHOD0(getCurrentViewport, const CompPoint & ());
+	MOCK_CONST_METHOD0(viewportDimentions, const CompSize & ());
+};
+
 class StubActivePluginsOption : public CoreOptions
 {
 public:
@@ -751,3 +764,156 @@
     CoreOptions coreOptions(false);
     em.init();
 }
+
+TEST(privatescreen_ViewportGeometryTest, PickCurrent)
+{
+    CompPoint vp;
+    compiz::window::Geometry g (250, 250, 500, 500, 0);
+    MockViewportRetreival mvp;
+
+    CompPoint current (0, 0);
+    CompSize  dimentions (1, 1);
+
+    EXPECT_CALL (mvp, getCurrentViewport ()).WillOnce (ReturnRef (current));
+    EXPECT_CALL (mvp, viewportDimentions ()).WillOnce (ReturnRef (dimentions));
+
+    compiz::private_screen::viewports::viewportForGeometry (g, vp, &mvp, CompSize (1000, 1000));
+
+    EXPECT_EQ (vp, CompPoint (0, 0));
+}
+
+TEST(privatescreen_ViewportGeometryTest, PickRight)
+{
+    CompPoint vp;
+    compiz::window::Geometry g (1250, 0, 500, 500, 0);
+    MockViewportRetreival mvp;
+
+    CompPoint current (0, 0);
+    CompSize  dimentions (2, 1);
+
+    EXPECT_CALL (mvp, getCurrentViewport ()).WillOnce (ReturnRef (current));
+    EXPECT_CALL (mvp, viewportDimentions ()).WillOnce (ReturnRef (dimentions));
+
+    compiz::private_screen::viewports::viewportForGeometry (g, vp, &mvp, CompSize (1000, 1000));
+
+    EXPECT_EQ (vp, CompPoint (1, 0));
+}
+
+TEST(privatescreen_ViewportGeometryTest, PickLeft)
+{
+    CompPoint vp;
+    compiz::window::Geometry g (-750, 0, 500, 500, 0);
+    MockViewportRetreival mvp;
+
+    CompPoint current (1, 0);
+    CompSize  dimentions (2, 1);
+
+    EXPECT_CALL (mvp, getCurrentViewport ()).WillOnce (ReturnRef (current));
+    EXPECT_CALL (mvp, viewportDimentions ()).WillOnce (ReturnRef (dimentions));
+
+    compiz::private_screen::viewports::viewportForGeometry (g, vp, &mvp, CompSize (1000, 1000));
+
+    EXPECT_EQ (vp, CompPoint (0, 0));
+}
+
+TEST(privatescreen_ViewportGeometryTest, PickBottom)
+{
+    CompPoint vp;
+    compiz::window::Geometry g (0, 1250, 500, 500, 0);
+    MockViewportRetreival mvp;
+
+    CompPoint current (0, 0);
+    CompSize  dimentions (1, 2);
+
+    EXPECT_CALL (mvp, getCurrentViewport ()).WillOnce (ReturnRef (current));
+    EXPECT_CALL (mvp, viewportDimentions ()).WillOnce (ReturnRef (dimentions));
+
+    compiz::private_screen::viewports::viewportForGeometry (g, vp, &mvp, CompSize (1000, 1000));
+
+    EXPECT_EQ (vp, CompPoint (0, 1));
+}
+
+TEST(privatescreen_ViewportGeometryTest, PickTop)
+{
+    CompPoint vp;
+    compiz::window::Geometry g (0, -750, 500, 500, 0);
+    MockViewportRetreival mvp;
+
+    CompPoint current (0, 1);
+    CompSize  dimentions (1, 2);
+
+    EXPECT_CALL (mvp, getCurrentViewport ()).WillOnce (ReturnRef (current));
+    EXPECT_CALL (mvp, viewportDimentions ()).WillOnce (ReturnRef (dimentions));
+
+    compiz::private_screen::viewports::viewportForGeometry (g, vp, &mvp, CompSize (1000, 1000));
+
+    EXPECT_EQ (vp, CompPoint (0, 0));
+}
+
+TEST(privatescreen_ViewportGeometryTest, PickTopWhenJustAbove)
+{
+    CompPoint vp;
+    compiz::window::Geometry g (0, -251, 500, 500, 0);
+    MockViewportRetreival mvp;
+
+    CompPoint current (0, 1);
+    CompSize  dimentions (1, 2);
+
+    EXPECT_CALL (mvp, getCurrentViewport ()).WillOnce (ReturnRef (current));
+    EXPECT_CALL (mvp, viewportDimentions ()).WillOnce (ReturnRef (dimentions));
+
+    compiz::private_screen::viewports::viewportForGeometry (g, vp, &mvp, CompSize (1000, 1000));
+
+    EXPECT_EQ (vp, CompPoint (0, 0));
+}
+
+TEST(privatescreen_ViewportGeometryTest, PickRightWhenJustRight)
+{
+    CompPoint vp;
+    compiz::window::Geometry g (751, 0, 500, 500, 0);
+    MockViewportRetreival mvp;
+
+    CompPoint current (0, 0);
+    CompSize  dimentions (2, 1);
+
+    EXPECT_CALL (mvp, getCurrentViewport ()).WillOnce (ReturnRef (current));
+    EXPECT_CALL (mvp, viewportDimentions ()).WillOnce (ReturnRef (dimentions));
+
+    compiz::private_screen::viewports::viewportForGeometry (g, vp, &mvp, CompSize (1000, 1000));
+
+    EXPECT_EQ (vp, CompPoint (1, 0));
+}
+
+TEST(privatescreen_ViewportGeometryTest, PickLeftWhenJustLeft)
+{
+    CompPoint vp;
+    compiz::window::Geometry g (-251, 0, 500, 500, 0);
+    MockViewportRetreival mvp;
+
+    CompPoint current (1, 0);
+    CompSize  dimentions (2, 1);
+
+    EXPECT_CALL (mvp, getCurrentViewport ()).WillOnce (ReturnRef (current));
+    EXPECT_CALL (mvp, viewportDimentions ()).WillOnce (ReturnRef (dimentions));
+
+    compiz::private_screen::viewports::viewportForGeometry (g, vp, &mvp, CompSize (1000, 1000));
+
+    EXPECT_EQ (vp, CompPoint (0, 0));
+}
+
+TEST(privatescreen_ViewportGeometryTest, PickBottomWhenJustBelow)
+{
+    CompPoint vp;
+    compiz::window::Geometry g (0, 751, 500, 500, 0);
+    MockViewportRetreival mvp;
+
+    CompPoint current (0, 0);
+    CompSize  dimentions (1, 2);
+
+    EXPECT_CALL (mvp, getCurrentViewport ()).WillOnce (ReturnRef (current));
+    EXPECT_CALL (mvp, viewportDimentions ()).WillOnce (ReturnRef (dimentions));
+
+    compiz::private_screen::viewports::viewportForGeometry (g, vp, &mvp, CompSize (1000, 1000));
+
+    EXPECT_EQ (vp, CompPoint (0, 1));
+}

=== modified file 'src/screen.cpp'
--- src/screen.cpp	2012-05-10 11:37:42 +0000
+++ src/screen.cpp	2012-05-17 05:48:18 +0000
@@ -4135,27 +4135,42 @@
 }
 
 /* Returns default viewport for some window geometry. If the window spans
-   more than one viewport the most appropriate viewport is returned. How the
-   most appropriate viewport is computed can be made optional if necessary. It
-   is currently computed as the viewport where the center of the window is
-   located. */
+ * more than one viewport the most appropriate viewport is returned. How the
+ * most appropriate viewport is computed can be made optional if necessary. It
+ * is currently computed as the viewport where the center of the window is
+ * located.
+ *
+ * XXX: It is possible for this function to return a negative viewport, which
+ * definitely feels wrong, however it seems that some plugins depend on this behaviour
+ * so they need to be fixed first
+ */
 void
-CompScreenImpl::viewportForGeometry (const CompWindow::Geometry& gm,
-				 CompPoint&                  viewport)
+compiz::private_screen::viewports::viewportForGeometry (const CompWindow::Geometry &gm,
+							CompPoint                  &viewport,
+							ViewportRetrievalInterface *viewports,
+							const CompSize &           screenSize)
 {
     CompRect rect (gm);
     int      offset;
 
+    const CompPoint &vp = viewports->getCurrentViewport ();
+    const CompSize &vpSize = viewports->viewportDimentions ();
+
     rect.setWidth  (gm.widthIncBorders ());
     rect.setHeight (gm.heightIncBorders ());
 
     offset = rect.centerX () < 0 ? -1 : 0;
-    viewport.setX (privateScreen.viewPort.vp.x () + ((rect.centerX () / width ()) + offset) %
-		   privateScreen.viewPort.vpSize.width ());
+    viewport.setX (vp.x () + ((rect.centerX () / screenSize.width ()) + offset) % vpSize.width ());
 
     offset = rect.centerY () < 0 ? -1 : 0;
-    viewport.setY (privateScreen.viewPort.vp.y () + ((rect.centerY () / height ()) + offset ) %
-		   privateScreen.viewPort.vpSize.height ());
+    viewport.setY (vp.y () + ((rect.centerY () / screenSize.height ()) + offset ) % vpSize.height ());
+}
+
+void
+CompScreenImpl::viewportForGeometry (const CompWindow::Geometry& gm,
+				     CompPoint&                  viewport)
+{
+    compiz::private_screen::viewports::viewportForGeometry (gm, privateScreen.viewPort.vp, &privateScreen.viewPort, *this);
 }
 
 int