<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="http://git.reviewboard.kde.org/r/111908/">http://git.reviewboard.kde.org/r/111908/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On August 9th, 2013, 9:27 p.m. CEST, <b>David Faure</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/111908/diff/4/?file=177609#file177609line99" style="color: black; font-weight: bold; text-decoration: underline;">tier1/kconfig/src/gui/kconfigloader.h</a>
<span style="font-weight: normal;">
(Diff revision 4)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">99</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="n">class</span> <span class="n">KCONFIGGUI_EXPORT</span> <span class="n">KConfigLoader</span> <span class="o">:</span> <span class="n">public</span> <span class="n">KConfigSkeleton</span></pre></td>
</tr>
</tbody>
</table>
<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 wonder if this should really derive from KConfigSkeleton, rather than encapsulate it.
For instance it means you can't get a "core only" version of it (see KCoreConfigSkeleton).
It doesn't reimplement any virtual methods, so this inheritance seems unnecessary. On the other hand I'm not sure how it would work by composition anyway; maybe the caller would have to create it themselves (which would be a bit cumbersome?).
The use of QColor in the code makes core/gui separation difficult anyway, it would need to be provided by a runtime hook.
Just an idea.
Don't know how much core/gui separation is useful for this class.</pre>
</blockquote>
<p>On September 2nd, 2013, 3:03 p.m. CEST, <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;">anything else I should do here or can I merge the patch in?</pre>
</blockquote>
<p>On September 3rd, 2013, 12:53 p.m. CEST, <b>David Faure</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, can you investigate if this could use KConfigSkeleton as a member instead of a base class?</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I think it's not possible as it needs to implement the protected method KCoreConfigSkeleton::usrWriteConfig(). Only solution would be to have a private extended class and use that as a member. But I think that this wouldn't be worth the effort.</pre>
<br />
<p>- Martin</p>
<br />
<p>On August 9th, 2013, 2:38 p.m. CEST, Martin Gräßlin wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://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, Plasma and Aaron J. Seigo.</div>
<div>By Martin Gräßlin.</div>
<p style="color: grey;"><i>Updated Aug. 9, 2013, 2:38 p.m.</i></p>
<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 KConfigLoader from Plasma Framework to KConfigGui
The ConfigLoader is way to awesome to not be directly in KConfig.</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>tier1/kconfig/autotests/CMakeLists.txt <span style="color: grey">(c913da3)</span></li>
<li>tier1/kconfig/autotests/kconfigloadertest.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>tier1/kconfig/autotests/kconfigloadertest.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>tier1/kconfig/autotests/kconfigloadertest.xml <span style="color: grey">(PRE-CREATION)</span></li>
<li>tier1/kconfig/src/gui/CMakeLists.txt <span style="color: grey">(0913349)</span></li>
<li>tier1/kconfig/src/gui/kconfigloader.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>tier1/kconfig/src/gui/kconfigloader.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>tier1/kconfig/src/gui/kconfigloader_p.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>tier1/kconfig/src/gui/kconfigloaderhandler_p.h <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/111908/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>