Code review comment for lp:~vanvugt/compiz-core/fix-764330-trunk

Revision history for this message
Sam Spilsbury (smspillaz) wrote :

Hey Daniel,

I fixed this bug properly in precise already, have a look at https://code.launchpad.net/~smspillaz/compiz-core/fix-893467/+merge/84054

The reason why window movement sometimes becomes choppy in oneiric is because core didn't differentiate between synchronous and asynchronous position updates. Because of that, the following race condition could occurr:

1 => 2 => 3 => 4 => server
.... 1 => 2 => 3 => 4=> client

What the move plugin was doing was this:

1 => 2 => 3 => 4 server
1 => 2 => 3 => 4 client

At which point, the 1, 2, 3 that we sent to the server are now redundant because we already updated it on the client side. However, because clients can move themselves if they are unmanaged, that 1 => 2 => 3 =>4 will end up calling moveNotify and updating w->geometry () again. So you'll get

1 => 2 => 3 => 4 => 1 => 2 => 3 => 4 client

That generally causes chaos, and caused problems with window stacking because I meant that I couldn't track pending requests correctly, which was necessary in order to fix an edge case where a window had been restacked, and any restack relative to it would be a no-op (in error).

The move plugin immediately updates the client side position in order to create a perceived immediacy to movement because we're not waiting on the server to deliver the new position to us.

In oneiric at least, the only way I could reconcile these two differences was to disallow synchronous updates when it is known that there are pending configure notifies on that window. If there happens to be a condition where we don't match a ConfigureNotify event to an XConfigureWindow request, that event queue becomes stale and needs to be unblocked after a few seconds. (obviously that's a hack, but given time constraints at the time, it was the only way it could feasibly be worked around).

The correct fix that was implemented in the branch I mentioned earlier was to separate the concept of the paint position and the real window position with an API break. Now the window position can be immediately updated to the user while we wait for the server to catch up, without having the side effect of every position update coming in twice and filling the pending event buffer which the stacking code needed to search with bogus events.

As such, part two of this branch is more-or-less redundant in lieu of that. If you're not getting warnings about unhandled ConfigureNotify events on the stdout, I'm happy to SRU it though.

I think that part 1 is an interesting idea, but it should be implemented in a more direct way - instead of not processing movement we should make the event queue in core fill up once every 25ms if there are pending motion notify events, and discard all motion notify events. At the moment, we're discarding all but the last motion event every single time the event handler source is called, and theoretically, if compiz is running fast enough, it will wake up and return to poll () for every single event processed rather than doing them all at once.

review: Needs Fixing

« Back to merge proposal