From kwin Fri Dec 30 08:27:26 2011 From: =?utf-8?q?Thomas_L=C3=BCbking?= Date: Fri, 30 Dec 2011 08:27:26 +0000 To: kwin Subject: Re: Review Request: [RFC] Good Bye to virtuals in EffectWindow part 1 Message-Id: <20111230082726.15103.74603 () vidsolbach ! de> X-MARC-Message: https://marc.info/?l=kwin&m=132523379511426 MIME-Version: 1 Content-Type: multipart/mixed; boundary="--===============0117604125930893270==" --===============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.h

On 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 

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 virt=
ual methods. This makes it easier to get a stable ABI. Adding new methods t=
o the class no longer requires to add a pure virtual method.
    =

From a performance point of view this change should not matter. Most Effect=
Window methods are not invoked during the repaint chain. But only in respon=
se to an event like a window got added. There the overhead does not really =
matter as well the previous implementation made strong use of dynamic casts=
 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 data.=

Testing <= /h1> 
tested some effects, properties in general tested through kw=
in scripting

Diffs=

  • kwin/composite.cpp (2fddb33)
  • kwin/effects.h (6530871)
  • kwin/effects.cpp (e388b33)
  • kwin/libkwineffects/kwineffects.h (780cc89= )
  • kwin/libkwineffects/kwineffects.cpp (1ad53= bc)

View Diff
--===============7755149343687808630==-- --===============0117604125930893270== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ kwin mailing list kwin@kde.org https://mail.kde.org/mailman/listinfo/kwin --===============0117604125930893270==--