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

On August 23rd, 2011, 6:41 p.m., Martin Gr= =C3=A4=C3=9Flin 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 whe=
ther they implement it "correctly" but they do need the sync requ=
est _before_ the XMoveResize while Qt doesn't seem to care (XSync? Impl=
ementation?)

I wasn't sure what's the "correct" order (isn't speci=
fied)
My understanding of a "sync request" was and is: align to the cur=
rent state and yell when you're out of work
It's however (on gtk+ at least) rather: "i set a mutex, fire a bun=
ch of instructions (probably only "next XMoveResize") and you rem=
ove 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=C3=BCbking wrote:

Review request for kwin and Martin Gr=C3=A4=C3=9Flin.

By Thomas L=C3=BCbking.

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

Descripti= on

<assume some very angry rant about the "protocol&quo=
t;, 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.

=3D> 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 (s=
o 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 cl=
ient's not done anyway and we cannot send a sync request)
d) move the *request* from "performMoveResize" into handleMoveRes=
ize, replace "syncTimeout" (great name) with "performMoveRes=
ize" 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 wa=
it 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)

=3D=3D> todo:
- I thought about using the compositor to perform a texture scale while we&=
#39;re waiting for the client (but the client/effectwindow would have to ke=
ep "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 i=
t 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 r=
equest. otherwise the entire thing is pointless.
- sending the configure notifications is important. don't ask me why, b=
ut w/o the client will stop sending sync events just like in the above case=

Testing <= /h1>

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

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