This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102311/

On August 23rd, 2011, 6:41 p.m., Martin Gräßlin wrote:

very nice :-) Just see the two minor comments below.

Do you know whether GTK applications implement sync properly? All GTK apps I just tried (Firefox (yeah not the best example), GIMP and Inkscape) were still very jumpy compared to the Qt applications.
No idea whether they implement it "correctly" but they do need the sync request _before_ the XMoveResize while Qt doesn't seem to care (XSync? Implementation?)

I wasn't sure what's the "correct" order (isn't specified)
My understanding of a "sync request" was and is: align to the current state and yell when you're out of work
It's however (on gtk+ at least) rather: "i set a mutex, fire a bunch of instructions (probably only "next XMoveResize") and you remove the mutex when you're done"

Whatever. Swapping the two lines will prevent gtk+ applications to rely on the timeout and Qt ones won't break. *sigh*

Swapped, fixed commented parts and pushed.

- Thomas


On August 14th, 2011, 9:08 a.m., Thomas Lübking wrote:

Review request for kwin and Martin Gräßlin.
By Thomas Lübking.

Updated Aug. 14, 2011, 9:08 a.m.

Description

<assume some very angry rant about the "protocol", it's authors and the author of the original patch here>

I finally got along, looking depper into this.
So far, the kwin implementation did hardly more than wasting resources.

=> changes:
a) ensure to NEVER send a second sync request while one's still pending (leading to the issues back then)
b) trigger a resize of the targetted geometry before requesting the sync (so the client actually syncs to the proper size and is there when we release the mutex)
c) skip all moveResizeHandle calls while there's a pending sync (the client's not done anyway and we cannot send a sync request)
d) move the *request* from "performMoveResize" into handleMoveResize, replace "syncTimeout" (great name) with "performMoveResize" and have it do exactly that.
e) don't new QTimer; delete QTimer with every size change - it's on the heap....
f) reduce the timeout to 250ms. waiting 500ms for the client is _completely_ inacceptable. if it cannot keep pace: bad luck. but having the user to wait half a second makes him feel "oh, blast. crashed."
g) fix tileset #ifdef (the geometry _has_ to be set esp. if there's no tiling at all)

==> todo:
- I thought about using the compositor to perform a texture scale while we're waiting for the client (but the client/effectwindow would have to keep "realGeometry" vs. "visualGeometry" for this
- look around whether XSYNC preproc checks are sufficient
- attempt to drop the second XMoveResize call for the window() WId (since it happened with the sync request)
- add a checkbox: "don't use xsync protocol" - frankly, this thing is so much wonky by design that i'd nearly say to remove support completely.

For the records (maybe i should add a comment in the code?)
- two sync requests on the stack will break it. no way to re-sync.
- one should aparently trigger the client resize before starting the sync request. otherwise the entire thing is pointless.
- sending the configure notifications is important. don't ask me why, but w/o the client will stop sending sync events just like in the above case

Testing

yes.
no more white edges in konsole (but for timeouts).
no issues with clemens qclock testcase (which blocks painting for some hundred ms) - scaling is smooth, blocks (as expected) and smoothly continues afterwards.

nevertheless, i'd like to have this in master as soon as possible (also before "playing around" for performance locally) and hopefully a lot of testing...
Bugs: 160393

Diffs

  • kwin/client.h (93510ec)
  • kwin/client.cpp (46c8eff)
  • kwin/composite.cpp (edffac1)
  • kwin/events.cpp (f523bf7)
  • kwin/geometry.cpp (51ed034)
  • kwin/manage.cpp (47c8814)

View Diff