Code review comment for lp:~smspillaz/compiz/compiz.merge-fix771448-desktop-wall-edge-flipping-broken.wall_edge_flip_logic

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