<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/116845/">https://git.reviewboard.kde.org/r/116845/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 16th, 2014, 11:10 p.m. EDT, <b>Matthew Dawson</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="https://git.reviewboard.kde.org/r/116845/diff/1/?file=254551#file254551line1055" style="color: black; font-weight: bold; text-decoration: underline;">src/core/kcoreconfigskeleton.h</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">public:</pre></td>
</tr>
</tbody>
<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">1055</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="kt">void</span> <span class="nf">read</span><span class="p">();</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 can't find any instances (lxr.kde.org returns way too many results to thoroughly check) where readConfig() was overridden and I don't know why you would want to override it, but since some users may have anyways I'd prefer if read() stayed a virtual for compatibility, since it replaces readConfig.</pre>
</blockquote>
<p>On March 17th, 2014, 7:15 p.m. EDT, <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;">It doesn't really replace readConfig(). We offer both: readConfig() (soon to be renamed load()) which reloads from disk, and read() which only reads from memory.
I grepped my kde source code along the lines of grep Skeleton `rgrep -l 'void.*readConfig'` and I can't find any reimplementations of readConfig() either. I think it was back in the days of making things virtual "just in case", like in Qt3 (which then got massively cleaned up for Qt4). We might want to do the same. Meanwhile this confirms that read() is fine as I wrote it....
If anyone needs to change behavior there's usrReadConfig() anyway - or the virtual readConfig() in each item. So I see no point in readConfig() or read() being virtual.</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;">Sounds good to me. This can just be dropped.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 16th, 2014, 11:10 p.m. EDT, <b>Matthew Dawson</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="https://git.reviewboard.kde.org/r/116845/diff/1/?file=254552#file254552line989" style="color: black; font-weight: bold; text-decoration: underline;">src/core/kcoreconfigskeleton.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">QVariant KCoreConfigSkeleton::ItemIntList::property() const</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">989</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">d</span><span class="o">-></span><span class="n">mConfig</span> <span class="o">=</span> <span class="n">KSharedConfig</span><span class="o">::</span><span class="n">openConfig</span><span class="p">(</span><span class="n">configname</span><span class="p">,</span> <span class="n">KConfig</span><span class="o">::</span><span class="n">FullConfig</span><span class="hl"> </span><span class="o"><span class="hl">|</span></span><span class="hl"> </span><span class="n"><span class="hl">KConfig</span></span><span class="o"><span class="hl">::</span></span><span class="n"><span class="hl">DelayedParsing</span></span><span class="p">);</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">989</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">d</span><span class="o">-></span><span class="n">mConfig</span> <span class="o">=</span> <span class="n">KSharedConfig</span><span class="o">::</span><span class="n">openConfig</span><span class="p">(</span><span class="n">configname</span><span class="p">,</span> <span class="n">KConfig</span><span class="o">::</span><span class="n">FullConfig</span><span class="p">);</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'd prefer to keep the same behaviour we implemented before. This allows for applications to make use of the optimization if they haven't been ported yet.
It also reduces developer thought in the simple case, as they can just always call readConfig/loadConfig, and be ensured they get the latest values from disk, while avoiding an extra read off disk. That leaves the read() function there as an optimization for the more advanced cases.</pre>
</blockquote>
<p>On March 17th, 2014, 7:15 p.m. EDT, <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;">Well, the apps don't need any porting if they ask kconfig_compiler to generate a singleton for them (which calls read instead of readConfig).
If they don't use a singleton, then, well, that's what I test in "test1", and I just debugged it, and we both missed something:
KCoreConfigSkeleton::addItem calls item->readConfig. So the reading from KConfig happens automatically, without the need for apps to call readConfig at all. The singleton-generating code is just stupid, it all happens automatically without a call to readConfig (except of course if apps wanted to reimplement that...)
So DelayedParsing breaks the Test1 case (constructor, no explicit call to readConfig/read). Yay for unittests.</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;">Ah, so then we don't need to call read/readConfig at all. If that is the case, I rather revert the DelayedParsing addition, as we don't need it. The commit can be added later if necessary, without breaking BC, I believe. Can you revert it before committing this then?</pre>
<br />
<p>- Matthew</p>
<br />
<p>On March 17th, 2014, 7:17 p.m. EDT, David Faure 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 Matthew Dawson.</div>
<div>By David Faure.</div>
<p style="color: grey;"><i>Updated March 17, 2014, 7:17 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kconfig
</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;">Make readConfig() non-virtual anymore, it's not useful.
Apps can reimplement usrReadConfig() or the readConfig() in every item instead.
Remove unnecessary debug output
Add KCoreConfigSkeleton::read() which doesn't call reparseConfiguration.
Call it from generated singletons, since the constructor creates
a KConfig from a filename, which already loads from disk.
This removes the need for using DelayedParsing.</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;">Added two unittests (which is how I discovered I had to remove DelayedParsing, so the tests were useful).
The "bool ok + qWarning" thing is a bit clumsy, maybe we want to port these main() to qtestlib too, even though they are themselves executed by a unittest :-)</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>autotests/kconfig_compiler/test10.cpp.ref <span style="color: grey">(21aea9d87af5fce01e64032257283e0883af2d81)</span></li>
<li>autotests/kconfig_compiler/test1main.cpp <span style="color: grey">(d7dc038d91d2f8babcd281960100d1c6fa264d7c)</span></li>
<li>autotests/kconfig_compiler/test4.cpp.ref <span style="color: grey">(66d0357f114b0aca6148a2091880dd2f62fbf624)</span></li>
<li>autotests/kconfig_compiler/test4main.cpp <span style="color: grey">(8f1c1ec8d41ea123893a59526c52cdbd0b5ee075)</span></li>
<li>autotests/kconfig_compiler/test5.cpp.ref <span style="color: grey">(088cc78f4dc96a719628ece342e149553c1d22aa)</span></li>
<li>autotests/kconfig_compiler/test8b.cpp.ref <span style="color: grey">(dcd61693ff86f02eaeb93b4c4da9e6ed20463469)</span></li>
<li>autotests/kconfig_compiler/test_dpointer.cpp.ref <span style="color: grey">(e50bf8aa29a2d009c4156dabf429c3ffb74eb7ba)</span></li>
<li>autotests/kconfig_compiler/test_signal.cpp.ref <span style="color: grey">(35b5cba2736761753d1ba32baa6f9fc2e7808ba1)</span></li>
<li>autotests/kconfigskeletontest.cpp <span style="color: grey">(31278e767655f7e3d25a4ed9984345e8c4e67d82)</span></li>
<li>src/core/kcoreconfigskeleton.h <span style="color: grey">(a2b828a4ef3f881b551049901d4bc26bb23a014b)</span></li>
<li>src/core/kcoreconfigskeleton.cpp <span style="color: grey">(0c1a96faaa9c511d349a26ad8af4b7b2c9bf313e)</span></li>
<li>src/kconfig_compiler/kconfig_compiler.cpp <span style="color: grey">(7d84cfbc28b1648ca999116726455183d88a7f0a)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/116845/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>