<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/115459/">https://git.reviewboard.kde.org/r/115459/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On February 4th, 2014, 5:19 p.m. CET, <b>Aurélien Gâteau</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;">Any particular reason for not using an abstract class? The D macro makes the code a bit surprising to read.</pre>
</blockquote>
<p>On February 5th, 2014, 4:43 p.m. CET, <b>Martin Gräßlin</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;">see discussion of https://git.reviewboard.kde.org/r/115225/ (comment thread started by Aaron) - I wanted to keep the same pattern.</pre>
</blockquote>
<p>On February 5th, 2014, 4:58 p.m. CET, <b>Aurélien Gâteau</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;">Sounds like premature optimization to me, but then, you are the maintainer, so it's your call. Naming the macro DELEGATE like in the other patch would make it easier to read, and a good idea if you want to follow the same pattern.</pre>
</blockquote>
<p>On February 5th, 2014, 5:13 p.m. CET, <b>Martin Gräßlin</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;">Yes it kind of sounds like premature optimization, but I like another aspect of it: each backend needs to provide an implementation for all methods and cannot rely on the dummy implementation. Thus the maintainer of the backend has to think about the proper default value. Also a change like introducing a new method will compile fail on other platforms.
Renaming the macro is a good suggestion, though.</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;">You would get the compilation failures with a pure abstract class as well.</pre>
<br />
<p>- Aurélien</p>
<br />
<p>On February 5th, 2014, 5:55 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, kdewin and Alexander Richardson.</div>
<div>By Martin Gräßlin.</div>
<p style="color: grey;"><i>Updated Feb. 5, 2014, 5:55 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;">Introduce runtime platform support in KWindowSystem
This is a change similar to the one in KWindowInfo, but with variation
to the pattern due to the static container.
There is now a generic implementation of KWindowSystem which is
completely windowing system platform independent. This implementation
delegates all methods into a KWindowSystemPrivate class.
Each windowing system platform implementation needs to provide a
subclass (e.g. KWindowSystemPrivateX11) and provide all the methods
which are delegated. Note that there are no virtual methods defined,
instead the d-pointer gets casted into the proper type. Thus if a
method is not provided it will end in a compile error.
To make use of a platform implementation it needs to be included in
the ctor of KWindowSystemStaticContainer and the PlatformImplementation
enum needs to be extended by a value for the platform. This is used in
the D macro to cast and delegate correctly.
There is a dummy implementation for all not supported windowing system
platforms.
This change also includes some API changes:
* KWindowSystem::windows() returns a copy instead of const-ref
* All methods are provided, there is no longer X11 specific methods
* private methods and enums are removed
NOTE: This change breaks the implementation for Windows and Mac OS!
They are currently excluded from build.</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 tests still succeed for X11, but they are not complete, though the most important aspects are tested.</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">(23133d581944a8373b9b753b300d97054b7d6f18)</span></li>
<li>src/kwindowinfo.cpp <span style="color: grey">(c706b29b306b65c992a178d490819b76e1aeca84)</span></li>
<li>src/kwindowsystem.h <span style="color: grey">(d288e1ab7c49f68482c94285c2aab695f08f3524)</span></li>
<li>src/kwindowsystem.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/kwindowsystem_p.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/kwindowsystem_p_x11.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>src/kwindowsystem_x11.cpp <span style="color: grey">(8d841cbcec41dc2d4df381338803902badf3f35e)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/115459/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>