Review Request 115225: Add runtime platform support to KWindowInfo

Wed Jan 22 18:48:11 UTC 2014

> On Jan. 22, 2014, 4: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.
> 
> Aaron J. Seigo wrote:
>     ... 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.

> 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

Probably won't work as KWindowInfo doesn't update if the window is changed. One of the next things I want to do is significantly improve the API documentation of that class.

> or a template class

that sounds like fun (yes I have a strange understanding of what is fun), I'll think about it.

- Martin

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/115225/#review48044
-----------------------------------------------------------

On Jan. 22, 2014, 3: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, 3: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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-windows/attachments/20140122/f6cbff9b/attachment-0001.html>