<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/115634/">https://git.reviewboard.kde.org/r/115634/</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 17th, 2014, 1:39 a.m. EST, <b>Matthew John 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/115634/diff/1/?file=243130#file243130line153" style="color: black; font-weight: bold; text-decoration: underline;">autotests/kconfig_compiler/kconfigcompiler_test_signals.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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">153</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="kt">void</span> <span class="n">KConfigCompiler_Test_Signals</span><span class="o">::</span><span class="n">testSetters</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;">As this appears to preform the same check as the function below, I rather just have the one set of tests. As the previous test breaks up the different elements under test such that Qt sees the different tests, I prefer for that version to be used.</pre>
</blockquote>
<p>On February 17th, 2014, 10:08 a.m. EST, <b>Alexander Richardson</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;">Doesn't quite do the same thing. This test checks that signals get emitted when using the generated setters. The other test uses the reflective approach with setProperty().
This test actually mostly works before https://git.reviewboard.kde.org/r/115635/ except for the last check with setDefaults().
The second test function only starts working after the patch to kconfig_compiler</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, I see. Can you merge the two sets of tests, so that they both use the data driven system? I just prefer the output Qt generates when that is used.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On February 17th, 2014, 1:39 a.m. EST, <b>Matthew John 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/115634/diff/1/?file=243130#file243130line226" style="color: black; font-weight: bold; text-decoration: underline;">autotests/kconfig_compiler/kconfigcompiler_test_signals.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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">226</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">addSetPropertyRows</span><span class="p">(</span><span class="s">"testStringList"</span><span class="p">,</span> <span class="n">SIGNAL</span><span class="p">(</span><span class="n">testStringListChanged</span><span class="p">(</span><span class="n">QStringList</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;">As the solution for this problem ends up being a wrapper that works for every type that correctly implements the KConfigSkeletonItem contract, I don't think we need to test every possible type. Did you experience any particular issues with the other types? Otherwise I rather just test one type, which should help speed up the test suite.</pre>
</blockquote>
<p>On February 17th, 2014, 10:08 a.m. EST, <b>Alexander Richardson</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;">Do you mean every type of generated KConfigSkeleton (dpointer vs no dpointer and singleton vs no singleton) or every type of stored data (QString vs int, etc).
I actually found a bug in kconfig_compiler due to testing every type: https://projects.kde.org/projects/frameworks/kconfig/repository/revisions/b1487f0c6b4ea64ae9c1ce96348fce8b5f0828df
But in general there should be no difference depending on what gets saved. Maybe there should be another unit test that generates a class that uses config items of every possible type to make sure something like this does not happen and restrict the signals test to a single property. Don't really which solution gets chosen, but I guess I can then finally get rid of that ugly macro in testSetters</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;">Sorry, I meant types of data stored. I think reducing this down to one single data type is best, as it avoids testing too many different ideas in one set of tests. In the future, it definitely would be good to have all the different types tested.
I think it is good to test the different ways KConfigSekeleton is generated, as the type does change how signals are generated.</pre>
<br />
<p>- Matthew John</p>
<br />
<p>On February 10th, 2014, 7:47 p.m. EST, Alexander Richardson 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.</div>
<div>By Alexander Richardson.</div>
<p style="color: grey;"><i>Updated Feb. 10, 2014, 7:47 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;">Add kconfig_compiler autotest that checks whether signals are emitted
Currently this works when using the setters, however when using
setProperty() on the KCoreConfigSkeleton* (as done by KConfigDialog) no
signals are emitted.</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;">Compiles, tests fail until https://git.reviewboard.kde.org/r/115635/ is applied, then they pass.
Rather ugly code IMO, open for suggestions to improve it...</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/CMakeLists.txt <span style="color: grey">(a2ebb9453bacb2c7507bc9477b6753a34bbcd434)</span></li>
<li>autotests/kconfig_compiler/kconfigcompiler_test_signals.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/kconfig_compiler/signals_test.kcfg <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/kconfig_compiler/signals_test_no_singleton.kcfgc <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/kconfig_compiler/signals_test_no_singleton_dpointer.kcfgc <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/kconfig_compiler/signals_test_singleton.kcfgc <span style="color: grey">(PRE-CREATION)</span></li>
<li>autotests/kconfig_compiler/signals_test_singleton_dpointer.kcfgc <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/115634/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>