Merge lp:~mc-return/compiz/compiz.merge-fix771448-desktop-wall-edge-flipping-broken into lp:compiz/0.9.10

[MC Return]

Wall:
Fixed broken wall edge flipping functions:
"Edge Flip Pointer"
"Edge Flip Move"

Removed void WallScreen::updateScreenEdgeRegions () completely.
This function not needed hereat all, no other plugin
needs to re-define the screen edge regions by itself -
just core should do that.
The screen edge region updating also confused the edge flipping functionality.
case ConfigureNotify: just breaks out now
Added missing default: case and a break (just a style issue).

Minor cleanup in bool WallScreen::initiateFlip (...).
One declaration per line.

Note: "Edge Flip DnD" still seems to be broken, but will be fixed in a follow-up
proposal.

(LP: #771448, LP: #858845)

[Sam Spilsbury (smspillaz)]

I figured the screen edge update stuff would be controversial when I added it a few years ago.

The reason why its there is because the screen edge detection windows had an annoying tendency to block input on, eg, panels. That was really, really, annoying.

Instead of removing them, I'd suggest trying to say if you can make it work with that code.

The way it's meant to work as I understand it is that when a window is being dragged, the screen edges will be dynamically re-enabled. Although it was really complicated to get right now I remember, because we had to detect when a grab happened and then figure out whether or not the edges should be enabled because of XDnD grabs being external to our own.

I might branch this off myself tonight and see if I can come up with something a bit better, as it was quite complicated.

review: Abstain

[Sam Spilsbury (smspillaz)]

So what I'm thinking is that we can probably remove the mousepoll + edge update stuff and do something like this:

1. On grab, turn all edges on (eg, toggleEdges)
2. Listen for xdndEnter and xdndLeave events
3. When an edge action is triggered, if we don't have "pointer" edge flipping enabled then:
   a. If "edge flip move" is enabled check if:
      1. There has been a grabbed window and
      2. Its grabbed for moving
   b. If "edge flip dnd" is enabled check if:
      1. We currently have the xdndEnter event for an edge.

This might require some core changes too - to check if an edge was entered for a DnD event.

[MC Return (mc-return)]

> I figured the screen edge update stuff would be controversial when I added it
> a few years ago.
>
> The reason why its there is because the screen edge detection windows had an
> annoying tendency to block input on, eg, panels. That was really, really,
> annoying.
>
Well, there are no regressions here, maybe there once were.

> Instead of removing them, I'd suggest trying to say if you can make it work
> with that code.
>
Why ?
It is not needed here -> look at rotate cube, where the edge flipping works
perfectly as well, completely without the plugin trying to take over core's work.

> The way it's meant to work as I understand it is that when a window is being
> dragged, the screen edges will be dynamically re-enabled. Although it was
> really complicated to get right now I remember, because we had to detect when
> a grab happened and then figure out whether or not the edges should be enabled
> because of XDnD grabs being external to our own.
>
I really do not see a reason to overcomplicate stuff in this case.

> I might branch this off myself tonight and see if I can come up with something
> a bit better, as it was quite complicated.

Sure, but I recommend to not reinvent the wheel for this, rather take some other
bug and fix it ;)

[MC Return (mc-return)]

> So what I'm thinking is that we can probably remove the mousepoll + edge
> update stuff and do something like this:
>
> 1. On grab, turn all edges on (eg, toggleEdges)
> 2. Listen for xdndEnter and xdndLeave events
> 3. When an edge action is triggered, if we don't have "pointer" edge flipping
> enabled then:
> a. If "edge flip move" is enabled check if:
> 1. There has been a grabbed window and
> 2. Its grabbed for moving
> b. If "edge flip dnd" is enabled check if:
> 1. We currently have the xdndEnter event for an edge.
>
> This might require some core changes too - to check if an edge was entered for
> a DnD event.

I really see no reason for this -> I have a multimonitor system and all edge-flip
cube stuff is always working here...

If you want to fix a thing regarding the screen edges, please-please-please fix
this one:

bug #1011167

[Sam Spilsbury (smspillaz)]

At least testing on my end shows that xdnd edge flipping is not working with this branch. Which is why I wanted to have a further look into it because I know the issues there were quite complicated.

Give me a little bit of time with this.

As for LP: #110167 - I commented on the bug report.

[Sam Spilsbury (smspillaz)]

Actually, I can't reproduce any of the issues you've mentioned in this branch on trunk. Edge flipping works fine in wall regardless.

But in any case I still think that the requirement to do mouse polling and a screen edge detection workaround is a bit of a kludge. So I'll see if I can come up with something better.

[MC Return (mc-return)]

> At least testing on my end shows that xdnd edge flipping is not working with
> this branch. Which is why I wanted to have a further look into it because I
> know the issues there were quite complicated.
>
That is really strange -> it is working perfectly here, I even tried 2x2 configurations.
I can use the pointer to flip and I can move windows around the 4 desktops, which was
not working before.

Damn - you are right -> DnD of icons does not seem to work, but note that this branch also
seems to fix: bug #858845

> Give me a little bit of time with this.
>
Sure. I will pause working on wall then...

[MC Return (mc-return)]

> Actually, I can't reproduce any of the issues you've mentioned in this branch
> on trunk. Edge flipping works fine in wall regardless.
>
I do not think that I am hallucinating, 26 other people marked the bug as affecting them ;)

Well, bug #1172881 was the reason I started looking into this issue, as I usually
have the cube running to rotate to my desktops and Compiz does not allow me to
use wall and cube together (bug #1173256).

When I started looking into it I noticed that it is a duplicate of bug #771448,
which affects 26 other people, was reported as regression on 2011-04-26 and has over 100 heat.
So I thought it is an issue worth looking into and when I discovered the strange,
duplicated core code in there, I remembered bug #858845, which I never could reproduce locally,
because wall was always disabled here, because... (see above)

Bug #858845 affects 136 other people, was reported on 2011-09-25.
Those people also seem credible to me...

> But in any case I still think that the requirement to do mouse polling and a
> screen edge detection workaround is a bit of a kludge. So I'll see if I can
> come up with something better.

As long as it is efficient, does not create regressions and works I'm fine with that solution also of course.

[MC Return (mc-return)]

My suggestion: Let us simply get this in first, then your corrections afterwards...

[lp:~mc-return/compiz/compiz.merge-fix771448-desktop-wall-edge-flipping-broken]

3678. By MC Return on 2013-05-11

Merged latest lp:compiz

[MC Return (mc-return)]

I've adjusted the commit message...

[Sam Spilsbury (smspillaz)]

Can you merge this with lp:compiz again after your other merges land?

review: Needs Resubmitting

[lp:~mc-return/compiz/compiz.merge-fix771448-desktop-wall-edge-flipping-broken]

3679. By MC Return on 2013-05-15

Merged latest lp:compiz

3680. By MC Return on 2013-05-15

wall: Remove mouse polling edge flip detection code, instead turn edges on
      in the following situations:

    1. Edge flip pointer is on (always on)
    2. Edge flip move is on, and the screen is grabbed (someone is moving
       a window)
    3. Edge flip dnd is on, and a dnd type window is mapped (someone is
       doing a dnd)

Fix a misconstructed boolean condition in removeAction that could cause
actions to be removed twice and as such cause the edge reference count to
go negative.

Note that dnd window detection is a bit imprecise, but I didn't want to resort
to using some exotic workarounds for the dnd edge cases (such as non-EWMH
compliant dnd windows which are still compliant with ICCCM).

[Sam Spilsbury (smspillaz)]

Excellent.

review: Approve

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

=== modified file 'plugins/wall/src/wall.cpp'
--- plugins/wall/src/wall.cpp	2013-05-09 13:43:07 +0000
+++ plugins/wall/src/wall.cpp	2013-05-15 10:56:26 +0000
@@ -495,29 +495,24 @@
 
 	    moveViewport (-dx, -dy, None);
 	}
-	if (event->xclient.message_type == Atoms::xdndEnter)
-	{
-	    toggleEdges (true);
-	    edgeDrag = true;
-	}
-	else if (event->xclient.message_type == Atoms::xdndLeave)
-	    edgeDrag = false;
 
 	break;
 
 	case FocusIn:
 	case FocusOut:
-	    if (event->xfocus.mode == NotifyGrab)
-		poller.start ();
-	    else if (event->xfocus.mode == NotifyUngrab)
-		poller.stop ();
-	break;
+	    /* Edges on when grabbed */
+	    if (!optionGetEdgeflipPointer ())
+	    {
+		if (event->xfocus.mode == NotifyGrab)
+		    toggleEdges (true);
+		else if (event->xfocus.mode == NotifyUngrab)
+		    toggleEdges (false);
+	    }
 
 	case ConfigureNotify:
-
-	     if (event->xconfigure.window == screen->root ())
-		updateScreenEdgeRegions ();
-
+	break;
+
+	default:
 	break;
     }
 
@@ -525,67 +520,32 @@
 }
 
 /*
- * Borrowed this from PrivateScreen::updateScreenEdges
- *
+ * When a dnd type window is mapped toggle edge flip dnd windows back on
+ * _NET_WM_WINDOW_TYPE_DND windows. This will not detect all dnd windows
+ * but it will detect the EWMH-compliant ones without resorting to
+ * hacks involving selection stealing.
  */
-
-#define SCREEN_EDGE_NUM		8
-
-void
-WallScreen::updateScreenEdgeRegions ()
-{
-    edgeRegion = CompRegion (0, 0, screen->width (), screen->height ());
-    noEdgeRegion = CompRegion (0, 0, screen->width (), screen->height ());
-
-    struct screenEdgeGeometry {
-	int xw, x0;
-	int yh, y0;
-	int ww, w0;
-	int hh, h0;
-    } geometry[SCREEN_EDGE_NUM] = {
-	{ 0,  0,   0,  2,   0,  2,   1, -4 }, /* left */
-	{ 1, -2,   0,  2,   0,  2,   1, -4 }, /* right */
-	{ 0,  2,   0,  0,   1, -4,   0,  2 }, /* top */
-	{ 0,  2,   1, -2,   1, -4,   0,  2 }, /* bottom */
-	{ 0,  0,   0,  0,   0,  2,   0,  2 }, /* top-left */
-	{ 1, -2,   0,  0,   0,  2,   0,  2 }, /* top-right */
-	{ 0,  0,   1, -2,   0,  2,   0,  2 }, /* bottom-left */
-	{ 1, -2,   1, -2,   0,  2,   0,  2 }  /* bottom-right */
-    };
-
-    for (unsigned int i = 0; i < SCREEN_EDGE_NUM; i++)
-    {
-	CompRegion edge (geometry[i].xw * screen->width () +
-			 geometry[i].x0,
-			 geometry[i].yh * screen->height () +
-			 geometry[i].y0,
-			 geometry[i].ww * screen->width () +
-			 geometry[i].w0,
-			 geometry[i].hh * screen->height () +
-			 geometry[i].h0);
-
-	noEdgeRegion -= edgeRegion;
-    }
-
-    edgeRegion -= noEdgeRegion;
-}
-
-#undef SCREEN_EDGE_NUM
-
-void
-WallScreen::positionUpdate (const CompPoint &pos)
-{
-    if (edgeDrag)
-	return;
-
-    if (edgeRegion.contains (pos))
-	toggleEdges (false);
-    else if (noEdgeRegion.contains (pos))
-    {
-	if (!screen->grabbed ())
-	    poller.stop ();
-	toggleEdges (true);
-    }
+void
+WallWindow::windowNotify (CompWindowNotify n)
+{
+    WallScreen *ws = WallScreen::get (screen);
+    bool toggleOnDnd = ws->optionGetEdgeflipDnd ();
+
+    switch (n)
+    {
+	case CompWindowNotifyMap:
+	    if (window->type () & CompWindowTypeDndMask && toggleOnDnd)
+		ws->toggleEdges (true);
+	    break;
+	case CompWindowNotifyUnmap:
+	    if (window->type () & CompWindowTypeDndMask && toggleOnDnd)
+		ws->toggleEdges (false);
+	    break;
+	default:
+	    break;
+    }
+
+    window->windowNotify (n);
 }
 
 void
@@ -648,19 +608,19 @@
 void
 WallWindow::grabNotify (int          x,
 			int          y,
-			unsigned int width,
-			unsigned int height)
+			unsigned int state,
+			unsigned int mask)
 {
-    WallScreen::get (screen)->toggleEdges (true);
-    WallScreen::get (screen)->edgeDrag = true;
+    if (mask & (CompWindowGrabMoveMask | CompWindowGrabButtonMask))
+	WallScreen::get (screen)->windowIsDragMoved = true;
 
-    window->grabNotify (x, y, width, height);
+    window->grabNotify (x, y, state, mask);
 }
 
 void
 WallWindow::ungrabNotify ()
 {
-    WallScreen::get (screen)->edgeDrag = false;
+    WallScreen::get (screen)->windowIsDragMoved = false;
 
     window->ungrabNotify ();
 }
@@ -804,49 +764,47 @@
 WallScreen::initiateFlip (Direction         direction,
 			  CompAction::State state)
 {
+    CompOption::Vector o (0);
+
     int dx, dy;
     int amountX, amountY;
 
-    if (screen->otherGrabExist ("wall", "move", "group-drag", 0))
-	return false;
-
-    if (state & CompAction::StateInitEdgeDnd)
-    {
-	if (!optionGetEdgeflipDnd ())
-	    return false;
-    }
-    else if (screen->grabExist ("move"))
-    {
-	if (!optionGetEdgeflipMove ())
-	    return false;
-    }
-    else if (screen->grabExist ("group-drag"))
-    {
-	if (!optionGetEdgeflipDnd ())
-	    return false;
-    }
-    else if (!optionGetEdgeflipPointer ())
-    {
-	toggleEdges (false);
-	poller.start ();
-	return false;
-    }
-
-    switch (direction) {
+    const bool allowFlipDnd = (state & CompAction::StateInitEdgeDnd) &&
+			      optionGetEdgeflipDnd ();
+    const bool allowFlipMove = (windowIsDragMoved &&
+				optionGetEdgeflipMove ());
+    const bool allowFlipPointer = optionGetEdgeflipPointer ();
+
+    if (!allowFlipDnd &&
+	!allowFlipMove &&
+	!allowFlipPointer)
+	return false;
+
+    switch (direction)
+    {
     case Left:
-	dx = -1; dy = 0;
+	dx = -1;
+	dy = 0;
 	break;
+
     case Right:
-	dx = 1; dy = 0;
+	dx = 1;
+	dy = 0;
 	break;
+
     case Up:
-	dx = 0; dy = -1;
+	dx = 0;
+	dy = -1;
 	break;
+
     case Down:
-	dx = 0; dy = 1;
+	dx = 0;
+	dy = 1;
 	break;
+
     default:
-	dx = 0; dy = 0;
+	dx = 0;
+	dy = 0;
 	break;
     }
 
@@ -1610,6 +1568,9 @@
 	    ww->isSliding = !optionGetNoSlideMatch ().evaluate (w);
 	}
 	break;
+    case WallOptions::EdgeflipPointer:
+	toggleEdges (optionGetEdgeflipPointer ());
+	break;
 
     default:
 	break;
@@ -1670,7 +1631,7 @@
     moveWindow (None),
     focusDefault (true),
     transform (NoTransformation),
-    edgeDrag (false)
+    windowIsDragMoved (false)
 {
     ScreenInterface::setHandler (screen);
     CompositeScreenInterface::setHandler (cScreen);
@@ -1737,11 +1698,6 @@
     setNotify (ArrowShadowColor);
     setNotify (NoSlideMatch);
     setNotify (EdgeflipPointer);
-
-    updateScreenEdgeRegions ();
-
-    poller.setCallback (boost::bind (&WallScreen::positionUpdate, this,
-				     _1));
 }
 
 WallScreen::~WallScreen ()
@@ -1770,8 +1726,7 @@
 {
     if (CompPlugin::checkPluginABI ("core", CORE_ABIVERSION)		&&
 	CompPlugin::checkPluginABI ("composite", COMPIZ_COMPOSITE_ABI)	&&
-	CompPlugin::checkPluginABI ("opengl", COMPIZ_OPENGL_ABI)	&&
-	CompPlugin::checkPluginABI ("mousepoll", COMPIZ_MOUSEPOLL_ABI))
+	CompPlugin::checkPluginABI ("opengl", COMPIZ_OPENGL_ABI))
 	return true;
 
     return false;

=== modified file 'plugins/wall/src/wall.h'
--- plugins/wall/src/wall.h	2013-01-04 08:43:45 +0000
+++ plugins/wall/src/wall.h	2013-05-15 10:56:26 +0000
@@ -124,7 +124,6 @@
 	void toggleEdges (bool);
 
 	void positionUpdate (const CompPoint &pos);
-	void updateScreenEdgeRegions ();
 
 	CompositeScreen *cScreen;
 	GLScreen        *glScreen;
@@ -168,10 +167,7 @@
 	WallCairoContext highlightContext;
 	WallCairoContext arrowContext;
 
-	MousePoller	 poller;
-	bool		 edgeDrag;
-	CompRegion	 edgeRegion;
-	CompRegion	 noEdgeRegion;
+	bool             windowIsDragMoved;
 };
 
 class WallWindow :
@@ -185,6 +181,7 @@
 	virtual void activate ();
 	void grabNotify (int, int, unsigned int, unsigned int);
 	void ungrabNotify ();
+	void windowNotify (CompWindowNotify);
 	bool glPaint (const GLWindowPaintAttrib &, const GLMatrix &,
 		      const CompRegion &, unsigned int);
 

=== modified file 'plugins/wall/wall.xml.in'
--- plugins/wall/wall.xml.in	2012-10-15 10:31:51 +0000
+++ plugins/wall/wall.xml.in	2013-05-15 10:56:26 +0000
@@ -17,7 +17,6 @@
 			</relation>
 			<requirement>
 				<plugin>opengl</plugin>
-				<plugin>mousepoll</plugin>
 			</requirement>
 		</deps>
 		<options>
@@ -290,6 +289,7 @@
 							<edge name="Left"/>
 						</default>
 						<allowed edgednd="true"/>
+                                                <passive_grab>false</passive_grab>
 					</option>
 					<option name="flip_right_edge" type="edge">
 						<_short>Flip Right</_short>
@@ -299,6 +299,7 @@
 							<edge name="Right"/>
 						</default>
 						<allowed edgednd="true"/>
+                                                <passive_grab>false</passive_grab>
 					</option>
 					<option name="flip_up_edge" type="edge">
 						<_short>Flip Up</_short>
@@ -308,6 +309,7 @@
 							<edge name="Top"/>
 						</default>
 						<allowed edgednd="true"/>
+                                                <passive_grab>false</passive_grab>
 					</option>
 					<option name="flip_down_edge" type="edge">
 						<_short>Flip Down</_short>
@@ -317,6 +319,7 @@
 							<edge name="Bottom"/>
 						</default>
 						<allowed edgednd="true"/>
+                                                <passive_grab>false</passive_grab>
 					</option>
 				</subgroup>
 			</group>

=== modified file 'src/screen.cpp'
--- src/screen.cpp	2013-05-12 08:20:10 +0000
+++ src/screen.cpp	2013-05-15 10:56:26 +0000
@@ -3487,7 +3487,8 @@
 void
 CompScreenImpl::removeAction (CompAction *action)
 {
-    if (!(privateScreen.initialized || action->active ()))
+    if (!privateScreen.initialized ||
+        !action->active ())
 	return;
 
     grabManager.setCurrentState(action->state());
@@ -3903,6 +3904,11 @@
 		SubstructureRedirectMask | SubstructureNotifyMask, &xev);
 }
 
+/* These functions do not guard against negative decrements
+ * as they are unable to determine the source of the reference
+ * and as such they should only be called by functions that
+ * actually determine the source of what was referencing the
+ * edge and can guard against multiple-references-per-owner */
 void
 PrivateScreen::enableEdge (int edge)
 {

=== added directory 'tests/manual/plugins'
=== added file 'tests/manual/plugins/wall.txt'
--- tests/manual/plugins/wall.txt	1970-01-01 00:00:00 +0000
+++ tests/manual/plugins/wall.txt	2013-05-15 10:56:26 +0000
@@ -0,0 +1,49 @@
+COMPIZ WALL PLUGIN MANUAL TESTS
+===============================
+Sam Spilsbury <[e-mail]>
+
+Edge Flip Pointer
+-----------------
+Setup:
+#. Enable "Edge Flip Pointer" in the wall plugin
+   and set up screen edges as appropriate
+
+Actions:
+#. Move cursor to relevant screen edge
+#. Viewport will change to the next one
+   over
+#. Viewport will not change if that
+   viewport is the last one in the row
+   or column
+
+Edge Flip Move
+--------------
+Setup:
+#. Disable "Edge Flip Pointer" in the wall plugin,
+   enable "Edge Flip Move" in the wall plugin,
+   and set up screen edges as appropriate
+
+Actions:
+#. Grab window by titlebar
+#. Move cursor to relevant screen edge
+#. Viewport will change to the next one
+   over while window is grabbed
+#. Viewport will not change if that
+   viewport is the last one in the row
+   or colum
+
+Edge Flip Dnd
+-------------
+Setup:
+#. Disable "Edge Flip Pointer" in the wall plugin,
+   enable "Edge Flip Dnd" in the wall plugin,
+   and set up screen edges as appropriate
+
+Actions:
+#. Grab a desktop icon
+#. Move cursor to relevant screen edge
+#. Viewport will change to the next one
+   over while icon is grabbed
+#. Viewport will not change if that
+   viewport is the last one in the row
+   or colum