--===============0117604125930893270== Content-Type: multipart/alternative; boundary="===============7755149343687808630==" --===============7755149343687808630== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable > On Dec. 29, 2011, 2:40 p.m., Thomas L=C3=BCbking wrote: > > I was about to issue that you must prevent adding dynamic properties...= but that's covered ;-) > > Otherwise i don't have major concerns (you spare the vtable lookup and = the dynamic casts and trade in the hashing, hash lookup and the dynamic typ= ing (QVariant conversion)) > > I just wonder whether it would make sense to depreciate the helper func= tions and add > > QVariant qtProperty(const char *name) const; // windowProperty could be= misleading in our context?! > > bool setQtProperty(const char *name, const QVariant &value); > > making the dynamic typing more visible to ppl. will rather cache result= s for hot loop usage. > > = > > PS: > > either you forgot to diff it or to add it or i'm blind or QObject has s= ome magic secret autoproperty feature, but i somehow miss the Q_PROPERTY st= uff in toplevel.h > = > Martin Gr=C3=A4=C3=9Flin wrote: > > either you forgot to diff it or to add it or i'm blind or QObject h= as some magic secret autoproperty feature, but i somehow miss the Q_PROPERT= Y stuff in toplevel.h > it needs my other review request as a parent diff. I thought I added = it... ahhh... there it is. i had somehow sensed this was -partially- qml related ;-) - Thomas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103567/#review9348 ----------------------------------------------------------- On Dec. 29, 2011, 9:47 a.m., Martin Gr=C3=A4=C3=9Flin wrote: > = > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/103567/ > ----------------------------------------------------------- > = > (Updated Dec. 29, 2011, 9:47 a.m.) > = > = > Review request for kwin. > = > = > Description > ------- > = > EffectWindow becomes a QObject and make use of properties in EffectWindow= where possible instead of inheritance > = > If possible use the property to Toplevel or Client instead of the pure vi= rtual methods. This makes it easier to get a stable ABI. Adding new methods= to the class no longer requires to add a pure virtual method. > = > From a performance point of view this change should not matter. Most Effe= ctWindow methods are not invoked during the repaint chain. But only in resp= onse to an event like a window got added. There the overhead does not reall= y matter as well the previous implementation made strong use of dynamic cas= ts which are also not knwon for being very fast. > = > Some virtuals are still there and should be removed. E.g. isManaged needs= a new method and property in Toplevel. Some other virtuals will stay - e.g= . addRepaint. Some virtuals should go into a d-pointer: e.g setData and dat= a. > = > = > Diffs > ----- > = > kwin/composite.cpp 2fddb33 = > kwin/effects.h 6530871 = > kwin/effects.cpp e388b33 = > kwin/libkwineffects/kwineffects.h 780cc89 = > kwin/libkwineffects/kwineffects.cpp 1ad53bc = > = > Diff: http://git.reviewboard.kde.org/r/103567/diff/diff > = > = > Testing > ------- > = > tested some effects, properties in general tested through kwin scripting > = > = > Thanks, > = > Martin Gr=C3=A4=C3=9Flin > = > --===============7755149343687808630== Content-Type: text/html; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable
This is an automatically generated e-mail. To reply, visit: http://git.revie= wboard.kde.org/r/103567/ |
On December 29th, 2011, 2:40 p.m., Thomas L= =C3=BCbking wrote:
I was abo= ut to issue that you must prevent adding dynamic properties... but that'= ;s covered ;-) Otherwise i don't have major concerns (you spare the vtable lookup and = the dynamic casts and trade in the hashing, hash lookup and the dynamic typ= ing (QVariant conversion)) I just wonder whether it would make sense to depreciate the helper function= s and add QVariant qtProperty(const char *name) const; // windowProperty could be mis= leading in our context?! bool setQtProperty(const char *name, const QVariant &value); making the dynamic typing more visible to ppl. will rather cache results fo= r hot loop usage. PS: either you forgot to diff it or to add it or i'm blind or QObject has s= ome magic secret autoproperty feature, but i somehow miss the Q_PROPERTY st= uff in toplevel.hOn December 30th, 2011, 7:46 a.m., Martin Gr=C3=A4=C3=9Flin wrot= e:
> eith= er you forgot to diff it or to add it or i'm blind or QObject has some = magic secret autoproperty feature, but i somehow miss the Q_PROPERTY stuff = in toplevel.h it needs my other review request as a parent diff. I thought I added it...<= /pre>
ahhh... the= re it is. i had somehow sensed this was -partially- qml related ;-)
- Thomas
On December 29th, 2011, 9:47 a.m., Martin Gr=C3=A4=C3=9Flin wrote:
Review request for kwin.
By Martin Gr=C3=A4=C3=9Flin.
Updated Dec. 29, 2011, 9:47 a.m. Descripti= on
Testing <= /h1>
Diffs=
|