> On Jan. 22, 2014, 3:22 p.m., Aaron J. Seigo wrote:
> > Looks quite nice, other than the missing win/mac support which you can do
> > little about at this point.
> >
> > Use of the explicitly shared pointer approach is a simple and nice
> > performance booster when passing these objects around (as these tend to).
> > +1 for that.
> >
> > I took a moment to ponder if there is enough duplication of window info
> > objects for the same window ID to make it worthwhile to have a global hash
> > of winid to dptrs for re-use between separately created instances (and not
> > just copies of the same instance). In the desktop shell there is at least
> > one per window for the taskbar and the pager (assuming the pager is active,
> > anyways); the window runner usually isn't loaded in the shell directly but
> > were it to be that'd be a third instance. So the highest duplication likely
> > to be seen is probably 3 and so it isn't worth it.
> >
> > It's a bit of a shame that the runtime detection requires a dptr full of
> > virtuals; this is probably only required on Linux/UNIX where there are
> > multiple window info protocols (xcb, wayland, ..) so sucks for the other
> > platforms, but I also suspect that this will never actually be used in
> > practice even on Linux as one is either going to be in a Wayland or X11 (or
> > whatever) session and switching between them requires logging in/out. It's
> > private API so it can be changed later if this were ever to actually show
> > up on in runtime performance which I seriously doubt it will. (I can
> > imagine sth horrible like a struct/union which has a pointer to an xcb and
> > a wayland implementation, both of which have the same literal API for
> > consistency but no base class and then a macro that calls whichever one is
> > actually instantiated: "#define callimpl(method) if (d->xcb) {
> > d->xcb->method(); } else { d->wayland->method(); }" win/mac/etc would have
> > a rather simpler callimpl macro. yes, ugly as
#($* but it would get rid of the virtuals and allow win/mac/android/etc to be
simple compile-time classes without unnecessary runtime detection features ..
but probably not worth the uglification w/out good justification, which
currently doesn't exist.
> >
> > I haven't done anything more than add it to the compile, so I can't mark it
> > as ShipIt with a clean conscience (as I haven't tested it at runtime), but
> > the code looks good and the design is reasonable imho.
... or a template class instead of the #define, though that adds a layer of
function call indirection I bet it could be 100% inlined functions and be both
fast and non-#define-hackery.
- Aaron J.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/115225/#review48044
-----------------------------------------------------------
On Jan. 22, 2014, 2:03 p.m., Martin Gräßlin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/115225/
> -----------------------------------------------------------
>
> (Updated Jan. 22, 2014, 2:03 p.m.)
>
>
> Review request for KDE Frameworks and kdewin.
>
>
> Repository: kwindowsystem
>
>
> Description
> -------
>
> Add runtime platform support to KWindowInfo
>
> Main idea of this change is to only pick the X11 implementation in case
> that the application is running on the X11 platform. So far it was a
> compile time switch which meant that if compiled with X11 support but
> not running on the X11 platform it would have caused runtime errors.
>
> To make this possible a KWindowInfoPrivate class with a dummy
> implementation is provided. This is used as d-ptr for KWindowInfo.
> Thus there is one generic implementation and the implementation of
> KWindowInfo is no longer ifdefed for the supported platforms.
>
> The platform specific code can inherit from KWindowInfoPrivate and
> overwrite the dummy method implementation. KWindowInfoPrivate provides
> a factory method where the platform specific implementation can be
> hooked into. There we can have both compile time and runtime checking.
> If there is no platfom specific implementation available the dummy
> implementation is used.
>
> NOTE: THIS CHANGE BREAKS THE WINDOWS AND MAC BACKEND!
>
> Windows and Mac is excluded from build. At the moment they get the
> dummy implementation. Unfortunately I don't have the possibility to
> compile the changes and thus don't dare to touch the code. Fixes from
> the teams are highly appreciated.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt e32a1150a2c190f23ad456ca8218b012c5d71507
> src/kwindowinfo.h 171f441ff329a5356ccf560341272199e20c837a
> src/kwindowinfo.cpp PRE-CREATION
> src/kwindowinfo_p.h PRE-CREATION
> src/kwindowinfo_p_x11.h PRE-CREATION
> src/kwindowinfo_x11.cpp 865d8bed085e987f97f479ea8aa0e6de8567586f
>
> Diff: https://git.reviewboard.kde.org/r/115225/diff/
>
>
> Testing
> -------
>
> Unit test from https://git.reviewboard.kde.org/r/115190/ is still working.
> Now you can guess why I wrote that test ;-)
>
>
> Thanks,
>
> Martin Gräßlin
>
>
_______________________________________________
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
