Compiz

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

  1. compiz.merge-fix771448-desktop-wall-edge-flipping-broken.wall_edge_flip_logic
  2. Merge into compiz.merge-fix771448-des...
Proposed by Sam Spilsbury
Status: Merged
Merged at revision: 3680
Proposed branch: lp://qastaging/~smspillaz/compiz/compiz.merge-fix771448-desktop-wall-edge-flipping-broken.wall_edge_flip_logic
Merge into: lp://qastaging/~mc-return/compiz/compiz.merge-fix771448-desktop-wall-edge-flipping-broken
Diff against target: 331 lines (+111/-61)
5 files modified
plugins/wall/src/wall.cpp (+49/-55)
plugins/wall/src/wall.h (+2/-4)
plugins/wall/wall.xml.in (+4/-1)
src/screen.cpp (+7/-1)
tests/manual/plugins/wall.txt (+49/-0)
To merge this branch: bzr merge lp://qastaging/~smspillaz/compiz/compiz.merge-fix771448-desktop-wall-edge-flipping-broken.wall_edge_flip_logic
Reviewer Review Type Date Requested Status
Sam Spilsbury Pending
MC Return Pending
Review via email: mp+163887@code.qastaging.launchpad.net

This proposal supersedes a proposal from 2013-05-13.

Commit message

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

Description of the change

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)

To post a comment you must log in.
Revision history for this message
MC Return (mc-return) wrote : Posted in a previous version of this proposal

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.

Revision history for this message
MC Return (mc-return) wrote : Posted in a previous version of this proposal

Did you test this ?

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

Revision history for this message
MC Return (mc-return) wrote : Posted in a previous version of this proposal

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
Revision history for this message
MC Return (mc-return) wrote : Posted in a previous version of this proposal

Or is it this:

292 + and set up screen edges as appropriate

What exactly should I setup here and where ?

Revision history for this message
MC Return (mc-return) wrote : Posted in a previous version of this proposal

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

Revision history for this message
Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal

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

Revision history for this message
MC Return (mc-return) wrote : Posted in a previous version of this proposal

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

Revision history for this message
MC Return (mc-return) wrote : Posted in a previous version of this proposal

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

Revision history for this message
Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal

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

Revision history for this message
MC Return (mc-return) wrote : Posted in a previous version of this proposal

> 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
Revision history for this message
Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal

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

Revision history for this message
MC Return (mc-return) wrote : Posted in a previous version of this proposal

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

Revision history for this message
MC Return (mc-return) wrote : Posted in a previous version of this proposal

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
Revision history for this message
MC Return (mc-return) wrote : Posted in a previous version of this proposal

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
"

Revision history for this message
Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal

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

Revision history for this message
MC Return (mc-return) wrote : Posted in a previous version of this proposal

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

Revision history for this message
MC Return (mc-return) wrote : Posted in a previous version of this proposal

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

Revision history for this message
Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal

Ping.

Revision history for this message
MC Return (mc-return) wrote : Posted in a previous version of this proposal

> 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
Revision history for this message
Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal

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

Revision history for this message
MC Return (mc-return) wrote : Posted in a previous version of this proposal

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

Revision history for this message
Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal

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

Revision history for this message
Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal

Hmm, merge conflicts I see.

review: Needs Fixing
Revision history for this message
MC Return (mc-return) wrote : Posted in a previous version of this proposal

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

Revision history for this message
Sam Spilsbury (smspillaz) wrote : Posted in a previous version of this proposal

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.

Revision history for this message
Sam Spilsbury (smspillaz) : Posted in a previous version of this proposal
review: Needs Fixing
Revision history for this message
MC Return (mc-return) wrote : Posted in a previous version of this proposal

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

Revision history for this message
MC Return (mc-return) wrote : Posted in a previous version of this proposal

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

Done and pushed...

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes: