<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/115225/">https://git.reviewboard.kde.org/r/115225/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On January 22nd, 2014, 4:22 p.m. CET, <b>Aaron J. Seigo</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
<p>On January 22nd, 2014, 4:30 p.m. CET, <b>Aaron J. Seigo</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">... 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.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">> 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.</pre>
<br />
<p>- Martin</p>
<br />
<p>On January 22nd, 2014, 3:03 p.m. CET, Martin Gräßlin wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KDE Frameworks and kdewin.</div>
<div>By Martin Gräßlin.</div>
<p style="color: grey;"><i>Updated Jan. 22, 2014, 3:03 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kwindowsystem
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Unit test from https://git.reviewboard.kde.org/r/115190/ is still working. Now you can guess why I wrote that test ;-)</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>src/CMakeLists.txt <span style="color: grey">(e32a1150a2c190f23ad456ca8218b012c5d71507)</span></li>
<li>src/kwindowinfo.h <span style="color: grey">(171f441ff329a5356ccf560341272199e20c837a)</span></li>
<li>src/kwindowinfo.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/kwindowinfo_p.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/kwindowinfo_p_x11.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/kwindowinfo_x11.cpp <span style="color: grey">(865d8bed085e987f97f479ea8aa0e6de8567586f)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/115225/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>