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

[Sam Spilsbury]

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).

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)

[MC Return (mc-return)]

You did it :)

From a first look:

Typos:

332 + or colum

316 + or colum

I do not understand this step:

292 + and set up screen edges as appropriate

307 + and set up screen edges as appropriate

323 + and set up screen edges as appropriate

I think this ^^ could be removed.

Please add newlines after:

66 + break;

70 + break;

The rest LGTM. I will test this ASAP.

[MC Return (mc-return)]

Did you test this ?

It does not seem to work here, none of the edge-flipping modes... :(

[MC Return (mc-return)]

Or am I missing something ?

Ah -> probably I am missing this part:

260 - if (!(privateScreen.initialized || action->active ()))
261 + if (!privateScreen.initialized ||
262 + !action->active ())

hmm, but this change seems to be unrelated and does not change the logic ?!

(I just exchanged the plugin)

*clueless* at the moment

review: Needs Information

[MC Return (mc-return)]

Or is it this:

292 + and set up screen edges as appropriate

What exactly should I setup here and where ?

[MC Return (mc-return)]

But I have some good news as well:

I did not have to change anything regarding the cmake config to compile this MP, just cmake .. && make -j5 :)
This means for the first time ever here all the tests compiled :), which means recent trunk fixes seem to work :)

[Sam Spilsbury (smspillaz)]

Yes I did, it works fine here.

Its possible that when I took the patch off my main branch I may have
missed something. I can look into that.
On 27/04/2013 4:42 PM, "MC Return" <email address hidden> wrote:

> Did you test this ?
>
> It does not seem to work here, none of the edge-flipping modes... :(
> --
>
> https://code.launchpad.net/~smspillaz/compiz/compiz.merge-fix771448-desktop-wall-edge-flipping-broken.wall_edge_flip_logic/+merge/161279
> You are the owner of
> lp:~smspillaz/compiz/compiz.merge-fix771448-desktop-wall-edge-flipping-broken.wall_edge_flip_logic.
>

[MC Return (mc-return)]

> Yes I did, it works fine here.
>
> Its possible that when I took the patch off my main branch I may have
> missed something. I can look into that.

Thanks :)
I am pretty sure I made no mistake -> and I double-checked, so most probably
something's missing...

[MC Return (mc-return)]

Note: I just took the compiled libwall.so to test, so if this needs changes to core...

But this thing below seems unrelated and does not change anything, or does it ?

253 === modified file 'src/screen.cpp'
254 --- src/screen.cpp 2013-02-03 17:58:29 +0000
255 +++ src/screen.cpp 2013-04-27 06:17:25 +0000
256 @@ -3486,7 +3486,8 @@
257 void
258 CompScreenImpl::removeAction (CompAction *action)
259 {
260 - if (!(privateScreen.initialized || action->active ()))
261 + if (!privateScreen.initialized ||
262 + !action->active ())
263 return;
264
265 grabManager.setCurrentState(action->state());
266 @@ -3902,6 +3903,11 @@
267 SubstructureRedirectMask | SubstructureNotifyMask, &xev);
268 }
269
270 +/* These functions do not guard against negative decrements
271 + * as they are unable to determine the source of the reference
272 + * and as such they should only be called by functions that
273 + * actually determine the source of what was referencing the
274 + * edge and can guard against multiple-references-per-owner */
275 void
276 PrivateScreen::enableEdge (int edge)
277 {
278

[Sam Spilsbury (smspillaz)]

In testing I remember coming across something that would effectively
cause actions to be removed from the active actions list multiple
times, which would in term cause the screen edge reference count to go
below zero. The if condition was the problem. It read:

if NOT
   (privateScreen.initialized OR
    action->active ())
    return false;

The sub-condition is true if either privateScreen.initialized or
action->active () is true. This means that in order for it to be
false, both of them must be false. Eg,

!(A || B) -> !A && !B

That's incorrect, because privateScreen.initialized is usually always
true, so you'd run into situations like this:

A && !B

But that still satisfies A || B because A is still true.

The adjusted condition is correct:

if (!A || !B) -> !(A && B)

What we want is for the function to always return false if
action->active () is false, regardless of what the value of
privateScreen.initialized is.

On Sat, Apr 27, 2013 at 5:43 PM, MC Return <email address hidden> wrote:
> Note: I just took the compiled libwall.so to test, so if this needs changes to core...
>
> But this thing below seems unrelated and does not change anything, or does it ?
>
> 253 === modified file 'src/screen.cpp'
> 254 --- src/screen.cpp 2013-02-03 17:58:29 +0000
> 255 +++ src/screen.cpp 2013-04-27 06:17:25 +0000
> 256 @@ -3486,7 +3486,8 @@
> 257 void
> 258 CompScreenImpl::removeAction (CompAction *action)
> 259 {
> 260 - if (!(privateScreen.initialized || action->active ()))
> 261 + if (!privateScreen.initialized ||
> 262 + !action->active ())
> 263 return;
> 264
> 265 grabManager.setCurrentState(action->state());
> 266 @@ -3902,6 +3903,11 @@
> 267 SubstructureRedirectMask | SubstructureNotifyMask, &xev);
> 268 }
> 269
> 270 +/* These functions do not guard against negative decrements
> 271 + * as they are unable to determine the source of the reference
> 272 + * and as such they should only be called by functions that
> 273 + * actually determine the source of what was referencing the
> 274 + * edge and can guard against multiple-references-per-owner */
> 275 void
> 276 PrivateScreen::enableEdge (int edge)
> 277 {
> 278
>
> --
> https://code.launchpad.net/~smspillaz/compiz/compiz.merge-fix771448-desktop-wall-edge-flipping-broken.wall_edge_flip_logic/+merge/161279
> You are the owner of lp:~smspillaz/compiz/compiz.merge-fix771448-desktop-wall-edge-flipping-broken.wall_edge_flip_logic.

--
Sam Spilsbury

[MC Return (mc-return)]

> In testing I remember coming across something that would effectively
> cause actions to be removed from the active actions list multiple
> times, which would in term cause the screen edge reference count to go
> below zero. The if condition was the problem. It read:
>
> if NOT
> (privateScreen.initialized OR
> action->active ())
> return false;
>
> The sub-condition is true if either privateScreen.initialized or
> action->active () is true. This means that in order for it to be
> false, both of them must be false. Eg,
>
> !(A || B) -> !A && !B
>
> That's incorrect, because privateScreen.initialized is usually always
> true, so you'd run into situations like this:
>
> A && !B
>
> But that still satisfies A || B because A is still true.
>
> The adjusted condition is correct:
>
> if (!A || !B) -> !(A && B)
>
> What we want is for the function to always return false if
> action->active () is false, regardless of what the value of
> privateScreen.initialized is.
>
>
Aha -> I thought this might be the problem, but was too lazy to really rethink
the boolean logic (I am still missing my shower & coffee today ;))...

I will approve this blindly this time, because I hope and am sure you will
immediately take care of regressions should those occur...

review: Approve

[Sam Spilsbury (smspillaz)]

Can you verify it please?

On Sat, Apr 27, 2013 at 6:06 PM, MC Return <email address hidden> wrote:
> Review: Approve
>
>> In testing I remember coming across something that would effectively
>> cause actions to be removed from the active actions list multiple
>> times, which would in term cause the screen edge reference count to go
>> below zero. The if condition was the problem. It read:
>>
>> if NOT
>> (privateScreen.initialized OR
>> action->active ())
>> return false;
>>
>> The sub-condition is true if either privateScreen.initialized or
>> action->active () is true. This means that in order for it to be
>> false, both of them must be false. Eg,
>>
>> !(A || B) -> !A && !B
>>
>> That's incorrect, because privateScreen.initialized is usually always
>> true, so you'd run into situations like this:
>>
>> A && !B
>>
>> But that still satisfies A || B because A is still true.
>>
>> The adjusted condition is correct:
>>
>> if (!A || !B) -> !(A && B)
>>
>> What we want is for the function to always return false if
>> action->active () is false, regardless of what the value of
>> privateScreen.initialized is.
>>
>>
> Aha -> I thought this might be the problem, but was too lazy to really rethink
> the boolean logic (I am still missing my shower & coffee today ;))...
>
> I will approve this blindly this time, because I hope and am sure you will
> immediately take care of regressions should those occur...
>
>
> --
> https://code.launchpad.net/~smspillaz/compiz/compiz.merge-fix771448-desktop-wall-edge-flipping-broken.wall_edge_flip_logic/+merge/161279
> You are the owner of lp:~smspillaz/compiz/compiz.merge-fix771448-desktop-wall-edge-flipping-broken.wall_edge_flip_logic.

--
Sam Spilsbury

[MC Return (mc-return)]

> Can you verify it please?
>
If you ask me to, how can I say no ?
;)

But it'll take a few hours until I can get to it...

[MC Return (mc-return)]

I get this error, when trying to install this branch locally (probably not related to changes in this branch):

...
copying build/locale/or/ccsm.mo -> /home/mcr2010/staging/share/locale/or/LC_MESSAGES
copying build/locale/pl/ccsm.mo -> /home/mcr2010/staging/share/locale/pl/LC_MESSAGES
running install_egg_info
Removing /home/mcr2010/staging/lib/python2.7/site-packages/ccsm-0.9.10.0-py2.7.egg-info
Writing /home/mcr2010/staging/lib/python2.7/site-packages/ccsm-0.9.10.0-py2.7.egg-info
gtk-update-icon-cache: Cache file created successfully.
Updating Gtk icon cache.
CMake Error at compizconfig/gconf/cmake_install.cmake:50 (FILE):
  file cannot create directory: /compizconfig/backends. Maybe need
  administrative privileges.
Call Stack (most recent call first):
  compizconfig/cmake_install.cmake:41 (INCLUDE)
  cmake_install.cmake:85 (INCLUDE)

make: *** [install] Error 1

I do not know why I cannot install locally, but I do not want to try to sudo it through...

Installing this way worked not long ago, recently I was not forced to do it, because I was just replacing the compiled plugin binaries manually to test them...

review: Abstain

[MC Return (mc-return)]

Sam, I got this report (quote by bryonak, who is helping with testing):
"
@MC Return:
Sadly, this one doesn't work well. Window move and pointer flipping work flawlessly, but d&d has some issues. Moreover, after trying to d&d, the other two stop working. Here's a video of it: http://caleo.dyx.ch/stuff/.compiz/flipping.mp4
"

[Sam Spilsbury (smspillaz)]

Did the other tester also install the core patch or just the wall plugin.

The core patch is necessary to prevent he bug which the tester has referred
to.
On 28/04/2013 11:07 PM, "MC Return" <email address hidden> wrote:

> Sam, I got this report (quote by bryonak, who is helping with testing):
> "
> @MC Return:
> Sadly, this one doesn't work well. Window move and pointer flipping work
> flawlessly, but d&d has some issues. Moreover, after trying to d&d, the
> other two stop working. Here's a video of it:
> http://caleo.dyx.ch/stuff/.compiz/flipping.mp4
> "
> --
>
> https://code.launchpad.net/~smspillaz/compiz/compiz.merge-fix771448-desktop-wall-edge-flipping-broken.wall_edge_flip_logic/+merge/161279
> You are the owner of
> lp:~smspillaz/compiz/compiz.merge-fix771448-desktop-wall-edge-flipping-broken.wall_edge_flip_logic.
>

[MC Return (mc-return)]

Sam, could you merge trunk into this branch ? -> then I could test it myself as IIRC the bug I had above is already fixed in trunk...

[MC Return (mc-return)]

Forget it -> (I could do it locally myself)

[Sam Spilsbury (smspillaz)]

Ping.

[MC Return (mc-return)]

> Ping.

Sorry for the delay -> it is due to the core-changing nature of this MP.
I still had no chance to test this properly... :(
My suggestion would be this, to not hold up this any longer:

First we merge lp:~mc-return/compiz/compiz.merge-fix771448-desktop-wall-edge-flipping-broken into lp:compiz, then this branch here afterwards.

Currently I have this PPA running here locally: gebner/compiz-cms
It correctly updates and has the latest Compiz core from trunk in it.
I could then run the latest version of trunk here and would spot regressions or non-functionality of wall then. In the worst case we could revert this branch, which probably won't be necessary anyway...

How about this plan ?

review: Needs Information

[Sam Spilsbury (smspillaz)]

On Sat, May 11, 2013 at 5:07 PM, MC Return <email address hidden> wrote:
> Review: Needs Information
>
>> Ping.
>
> Sorry for the delay -> it is due to the core-changing nature of this MP.
> I still had no chance to test this properly... :(
> My suggestion would be this, to not hold up this any longer:
> ...
> Currently I have this PPA running here locally: gebner/compiz-cms
> It correctly updates and has the latest Compiz core from trunk in it.
> I could then run the latest version of trunk here and would spot regressions or non-functionality of wall then. In the worst case we could revert this branch, which probably won't be necessary anyway...

Huh?

What's stopping you from compiling compiz and unity locally?

The reason why I proposed this one is because
lp:~mc-return/compiz/compiz.merge-fix771448-desktop-wall-edge-flipping-broken
breaks edge-flip-dnd.

--
Sam Spilsbury

[MC Return (mc-return)]

>
> Huh?
>
> What's stopping you from compiling compiz and unity locally?
>
My additionally installed lenses/scopes, Compiz plugins and special CCSM settings.

> The reason why I proposed this one is because
> lp:~mc-return/compiz/compiz.merge-fix771448-desktop-wall-edge-flipping-broken
> breaks edge-flip-dnd.
>
For maximally 3 hours until your branch lands and just in trunk, while fixing all
those other things ;)
Come on, now you must be joking...

[Sam Spilsbury (smspillaz)]

> >
> > Huh?
> >
> > What's stopping you from compiling compiz and unity locally?
> >
> My additionally installed lenses/scopes, Compiz plugins and special CCSM
> settings.
>
> > The reason why I proposed this one is because
> > lp:~mc-return/compiz/compiz.merge-fix771448-desktop-wall-edge-flipping-
> broken
> > breaks edge-flip-dnd.
> >
> For maximally 3 hours until your branch lands and just in trunk, while fixing
> all
> those other things ;)
> Come on, now you must be joking...

No, I'm not.

1. I know its broken. It would be unconscionable for me to land something that I know is broken.
2. Landing this doesn't change the fact that you can't yet test the branch that fixes it because it requires a core change.

[Sam Spilsbury (smspillaz)]

Hmm, merge conflicts I see.

review: Needs Fixing

[MC Return (mc-return)]

> Hmm, merge conflicts I see.

It is just the ABI logic change...
I merged latest lp:compiz to the other branch already.

Please fix the conflict, I will test this ASAP to get it off the table finally...

[Sam Spilsbury (smspillaz)]

Back to needs-review again so this gets noticed.

Can you merge your branch with lp:compiz again? Just merged mine but yours is behind again.

[Sam Spilsbury (smspillaz)]

review: Needs Fixing

[MC Return (mc-return)]

> Back to needs-review again so this gets noticed.
>
I did not forget it, there's just so much to do...

> Can you merge your branch with lp:compiz again? Just merged mine but yours is
> behind again.
Just noticed, sorry...
I will do it right now.

[MC Return (mc-return)]

> Can you merge your branch with lp:compiz again? Just merged mine but yours is
> behind again.

Done and pushed...

=== modified file 'plugins/wall/src/wall.cpp'
--- plugins/wall/src/wall.cpp	2013-05-11 09:13:17 +0000
+++ plugins/wall/src/wall.cpp	2013-05-15 10:42:26 +0000
@@ -495,23 +495,19 @@
 
 	    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:
 	break;
@@ -523,20 +519,33 @@
     screen->handleEvent (event);
 }
 
+/*
+ * 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.
+ */
 void
-WallScreen::positionUpdate (const CompPoint &pos)
+WallWindow::windowNotify (CompWindowNotify n)
 {
-    if (edgeDrag)
-	return;
+    WallScreen *ws = WallScreen::get (screen);
+    bool toggleOnDnd = ws->optionGetEdgeflipDnd ();
 
-    if (edgeRegion.contains (pos))
-	toggleEdges (false);
-    else if (noEdgeRegion.contains (pos))
+    switch (n)
     {
-	if (!screen->grabbed ())
-	    poller.stop ();
-	toggleEdges (true);
+	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
@@ -599,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 ();
 }
@@ -760,30 +769,16 @@
     int dx, dy;
     int amountX, amountY;
 
-    if (screen->otherGrabExist ("wall", "move", "group-drag", NULL))
-	return false;
+    const bool allowFlipDnd = (state & CompAction::StateInitEdgeDnd) &&
+			      optionGetEdgeflipDnd ();
+    const bool allowFlipMove = (windowIsDragMoved &&
+				optionGetEdgeflipMove ());
+    const bool allowFlipPointer = optionGetEdgeflipPointer ();
 
-    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 ();
+    if (!allowFlipDnd &&
+	!allowFlipMove &&
+	!allowFlipPointer)
 	return false;
-    }
 
     switch (direction)
     {
@@ -1573,6 +1568,9 @@
 	    ww->isSliding = !optionGetNoSlideMatch ().evaluate (w);
 	}
 	break;
+    case WallOptions::EdgeflipPointer:
+	toggleEdges (optionGetEdgeflipPointer ());
+	break;
 
     default:
 	break;
@@ -1633,7 +1631,7 @@
     moveWindow (None),
     focusDefault (true),
     transform (NoTransformation),
-    edgeDrag (false)
+    windowIsDragMoved (false)
 {
     ScreenInterface::setHandler (screen);
     CompositeScreenInterface::setHandler (cScreen);
@@ -1700,9 +1698,6 @@
     setNotify (ArrowShadowColor);
     setNotify (NoSlideMatch);
     setNotify (EdgeflipPointer);
-
-    poller.setCallback (boost::bind (&WallScreen::positionUpdate, this,
-				     _1));
 }
 
 WallScreen::~WallScreen ()
@@ -1731,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-04-26 13:15:57 +0000
+++ plugins/wall/src/wall.h	2013-05-15 10:42:26 +0000
@@ -167,10 +167,7 @@
 	WallCairoContext highlightContext;
 	WallCairoContext arrowContext;
 
-	MousePoller	 poller;
-	bool		 edgeDrag;
-	CompRegion	 edgeRegion;
-	CompRegion	 noEdgeRegion;
+	bool             windowIsDragMoved;
 };
 
 class WallWindow :
@@ -184,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:42: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:42: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:42: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