<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/120317/">https://git.reviewboard.kde.org/r/120317/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Some more nitpicks, but otherwise this looks good. The only issue I see is the connect to the lambda, afaik that is unsafe the way it stands now.</p></pre>
<br />
<div>
<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/120317/diff/6/?file=318696#file318696line263" style="color: black; font-weight: bold; text-decoration: underline;">interfaces/iplugin.h</a>
<span style="font-weight: normal;">
(Diff revision 6)
</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">261</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">virtual</span> <span class="n">QObject</span><span class="o">*</span> <span class="n">createView</span><span class="p">(</span><span class="n">KTextEditor</span><span class="o">::</span><span class="n">MainWindow</span><span class="o">*</span><span class="p">)</span> <span class="n">override</span> <span class="n">final</span> <span class="p">{</span> <span class="k">return</span> <span class="n">nullptr</span><span class="p">;</span> <span class="p">};</span></pre></td>
</tr>
</tbody>
</table>
<div style="margin-left: 2em;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">don't inline this please</p></pre>
</div>
</div>
<br />
<div>
<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/120317/diff/6/?file=318709#file318709line135" style="color: black; font-weight: bold; text-decoration: underline;">shell/configdialog.cpp</a>
<span style="font-weight: normal;">
(Diff revision 6)
</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">135</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="c1">// qDebug("Removing page %s (%s) (%p) because associated plugin (%p) is unloading.",</span></pre></td>
</tr>
</tbody>
</table>
<div style="margin-left: 2em;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">remove this or comment it out</p></pre>
</div>
</div>
<br />
<div>
<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/120317/diff/6/?file=318721#file318721line218" style="color: black; font-weight: bold; text-decoration: underline;">shell/uicontroller.cpp</a>
<span style="font-weight: normal;">
(Diff revision 6)
</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">214</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="k">class</span> <span class="nc">KTextEditorConfigPageAdapter</span> <span class="o">:</span> <span class="k">public</span> <span class="n">ConfigPage</span></pre></td>
</tr>
</tbody>
</table>
<div style="margin-left: 2em;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'd rather have these two classes in their own .h + .cpp files in shell/</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">the uicontroller is large already after all.</p></pre>
</div>
</div>
<br />
<div>
<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/120317/diff/6/?file=318721#file318721line572" style="color: black; font-weight: bold; text-decoration: underline;">shell/uicontroller.cpp</a>
<span style="font-weight: normal;">
(Diff revision 6)
</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; ">void UiController::addNewToolView(MainWindow *mw, QListWidgetItem* item)</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">563</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">connect</span><span class="p">(</span><span class="n">ICore</span><span class="o">::</span><span class="n">self</span><span class="p">()</span><span class="o">-></span><span class="n">pluginController</span><span class="p">(),</span> <span class="o">&</span><span class="n">IPluginController</span><span class="o">::</span><span class="n">pluginLoaded</span><span class="p">,</span> <span class="n">addPluginPages</span><span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<div style="margin-left: 2em;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Are you sure this is safe? afaik this connect will stay "valid" even when this context is left. so the second time you open the settings dialog and load/unload a plugin, the invalid lambda will be called leading to a crash, no?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Two options:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">a) use the (new? Qt 5.4?) connect syntax for lambdas that also takes a <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">QObject*</code>, which defines the lifetime of the connection. You could use <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">&cfgDlg</code> e.g.
b) make addPluginPages a slot of the ConfigDialog</p></pre>
</div>
</div>
<br />
<p>- Milian Wolff</p>
<br />
<p>On October 14th, 2014, 11:39 p.m. UTC, Alexander Richardson wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for KDevelop.</div>
<div>By Alexander Richardson.</div>
<p style="color: grey;"><i>Updated Oct. 14, 2014, 11:39 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kdevplatform
</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Everything seems to work fine with the converted config pages.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This is a huge diff, should I maybe push my changes to a new branch or to some other repository?</p></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Dialog is shown correctly, apply, okay work correctly.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Not sure why the first title is translated, AFAIK I don't have any KF5 translations installed (but maybe it loads it from my KDE4 installation?)</p></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>project/projectconfigpage.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>project/projectkcmodule.h <span style="color: grey">(fe40e9c3ad05963a5c7986110d1d4cb3e3fa5438)</span></li>
<li>shell/CMakeLists.txt <span style="color: grey">(7847e261f53e4831f7dd662add34a6e6281d5cd4)</span></li>
<li>shell/configdialog.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>shell/configdialog.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>shell/plugincontroller.cpp <span style="color: grey">(9872b4631fdd6c5e586c97ccee598066c0e2deb0)</span></li>
<li>shell/projectcontroller.h <span style="color: grey">(08cb7cbfec32cac918d674ed9a076ce9286ffd30)</span></li>
<li>shell/projectcontroller.cpp <span style="color: grey">(6c1c3844800d05911003d47c6408ff34d24d5afd)</span></li>
<li>shell/settings/CMakeLists.txt <span style="color: grey">(5760ab4cb6a459ec6f1282ae09bb95f5f6e77892)</span></li>
<li>shell/settings/kcm_kdev_pluginsettings.desktop.cmake <span style="color: grey">(10d126ed24adc10d3e6f7183d7b97677c661287e)</span></li>
<li>shell/settings/kcm_kdev_uisettings.desktop.cmake <span style="color: grey">(336e233c2cb1570a2d8523680938f1d094a9a915)</span></li>
<li>shell/settings/pluginpreferences.h <span style="color: grey">(12f9d233d9a06739ff0e460e5c41ff92f9431af3)</span></li>
<li>shell/settings/pluginpreferences.cpp <span style="color: grey">(776df6360acec177aa6338058ede61ba80fc0b15)</span></li>
<li>shell/settings/uipreferences.h <span style="color: grey">(34c46f3902cf06156c7f8279e6672ebb7a06a31e)</span></li>
<li>shell/settings/uipreferences.cpp <span style="color: grey">(c8ba26201516241e29f76db4c6e448de787c4704)</span></li>
<li>shell/uicontroller.h <span style="color: grey">(797fa198ed17608e88f2a9e9a03f8072c5ebf45d)</span></li>
<li>shell/uicontroller.cpp <span style="color: grey">(0eab47ae515f7832ea8b1833252346cec84c8254)</span></li>
<li>plugins/projectfilter/projectfilterkcm.cpp <span style="color: grey">(7eaba23ca09bfedb76fde50a4ff02b28b009c31a)</span></li>
<li>plugins/projectfilter/projectfilterprovider.h <span style="color: grey">(cb1fa978e0c40fcf8cf5e3fcaf73294c1b9d314e)</span></li>
<li>plugins/projectfilter/projectfilterprovider.cpp <span style="color: grey">(e22198349f02675572b1396e7a02bac1b43d2eb8)</span></li>
<li>project/CMakeLists.txt <span style="color: grey">(e484a3c3370db2c5a9f600c3af5f0ef9e0ccb39d)</span></li>
<li>plugins/projectfilter/projectfilterkcm.h <span style="color: grey">(01d143469528117589c86853e5847fad6bc2aaaf)</span></li>
<li>CMakeLists.txt <span style="color: grey">(6a8987db82b5dd589b01718120c7c8021b91f1c4)</span></li>
<li>interfaces/CMakeLists.txt <span style="color: grey">(87916216011e4390ba9b637f9ae22fe54f254e86)</span></li>
<li>interfaces/configpage.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>interfaces/configpage.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>interfaces/iplugin.h <span style="color: grey">(b101ae46fa430101c21a1c8eb8699674cbb1977b)</span></li>
<li>plugins/projectfilter/CMakeLists.txt <span style="color: grey">(7fc4226bc253e033633916d058f0fc32a1f4cb6f)</span></li>
<li>interfaces/iplugin.cpp <span style="color: grey">(1a0677312485ae47747cc3483ce325fe9890af78)</span></li>
<li>plugins/projectfilter/kcm_kdevprojectfilter.desktop.cmake <span style="color: grey">(2d728b3fcb6b0cd8b6976771a214f270995bc2b7)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/120317/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">File Attachments </h1>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2014/09/22/7e219540-1632-4465-9675-ad24a2fde56c__kdev_config_new.png">kdev_config_new.png</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2014/09/22/f4f16e26-e6dd-4c98-9969-588342f85f4c__kdev_config_new1.png">kdev_config_new1.png</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2014/09/25/7130af0f-5e23-45cf-b781-ebe318be8369__kdev_config_new2.png">plugin enabled -> entry exists</a></li>
<li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2014/09/25/dff38c1f-2b3d-4ab7-a97d-adff0a99bb3d__kdev_config_new3.png">plugin unloaded -> entry disappears</a></li>
</ul>
</td>
</tr>
</table>
</div>
</body>
</html>