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

== 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 ?

[lp:~smspillaz/compiz-core/compiz-core.fix_939228]

3113. By Sam Spilsbury on 2012-04-27

Merge lp:compiz-core

3114. By Sam Spilsbury on 2012-05-15

Merge lp:compiz-core

3115. By Sam Spilsbury on 2012-05-15

Merged lp:compiz-core

3116. By Sam Spilsbury on 2012-05-17

Revert to the old behaviour with a note about possible bugs

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-04-25 09:21:24 +0000
+++ src/privatescreen.h	2012-04-26 11:00:27 +0000
@@ -478,15 +478,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
+class 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 ViewPort
 {

=== modified file 'src/privatescreen/tests/test-privatescreen.cpp'
--- src/privatescreen/tests/test-privatescreen.cpp	2012-04-24 14:34:19 +0000
+++ src/privatescreen/tests/test-privatescreen.cpp	2012-04-26 11:00:27 +0000
@@ -10,6 +10,10 @@
 
 #include <stdlib.h>
 
+using ::testing::Return;
+using ::testing::ReturnRef;
+using ::testing::_;
+
 namespace {
 
 class MockCompScreen : public CompScreen
@@ -175,7 +179,20 @@
     MOCK_CONST_METHOD0(createFailed, bool ());
 };
 
+<<<<<<< TREE
 class StubActivePluginsOption : public CoreOptions
+=======
+class MockViewportRetreival :
+    public compiz::private_screen::ViewportRetrievalInterface
+{
+    public:
+
+	MOCK_CONST_METHOD0(getCurrentViewport, const CompPoint & ());
+	MOCK_CONST_METHOD0(viewportDimentions, const CompSize & ());
+};
+
+class StubActivePluginsOption
+>>>>>>> MERGE-SOURCE
 {
 public:
     StubActivePluginsOption() : CoreOptions(false)
@@ -723,3 +740,156 @@
 
     em.init(0);
 }
+
+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-04-25 09:21:24 +0000
+++ src/screen.cpp	2012-04-26 11:00:27 +0000
@@ -4065,22 +4065,32 @@
    is currently computed as the viewport where the center of the window is
    located. */
 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 (priv->vp.x () + ((rect.centerX () / width ()) + offset) %
-		   priv->vpSize.width ());
+    viewport.setX (compiz::core::screen::wraparound_mod (vp.x () + ((rect.centerX () / screenSize.width ()) + offset), vpSize.width ()));
 
     offset = rect.centerY () < 0 ? -1 : 0;
-    viewport.setY (priv->vp.y () + ((rect.centerY () / height ()) + offset ) %
-		   priv->vpSize.height ());
+    viewport.setY (compiz::core::screen::wraparound_mod (vp.y () + ((rect.centerY () / screenSize.height ()) + offset), vpSize.height ()));
+}
+
+void
+CompScreenImpl::viewportForGeometry (const CompWindow::Geometry& gm,
+				     CompPoint&                  viewport)
+{
+    compiz::private_screen::viewports::viewportForGeometry (gm, viewport, priv.get (), *this);
 }
 
 int