<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/113815/">http://git.reviewboard.kde.org/r/113815/</a>
</td>
</tr>
</table>
<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="http://git.reviewboard.kde.org/r/113815/diff/1/?file=212620#file212620line38" style="color: black; font-weight: bold; text-decoration: underline;">languages/cpp/codegen/unresolvedincludeassistant.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; ">using namespace Cpp;</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">38</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">Cpp</span><span class="o">::</span><span class="n">AddCustomIncludePathAction</span><span class="o">::</span><span class="n">includeDirectoryDialog</span><span class="p">(</span><span class="n">QWidget</span><span class="o">*</span> <span class="n">textBox</span><span class="p">)</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">That is the wrong location for this code snippet. It should be in the yet-to-be-created CustomIncludePathsWidget class.</pre>
</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="http://git.reviewboard.kde.org/r/113815/diff/1/?file=212620#file212620line84" style="color: black; font-weight: bold; text-decoration: underline;">languages/cpp/codegen/unresolvedincludeassistant.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; ">void Cpp::AddCustomIncludePathAction::execute() {</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">84</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">QSignalMapper</span> <span class="o">*</span><span class="n">signalMapper</span> <span class="o">=</span> <span class="k">new</span> <span class="n">QSignalMapper</span><span class="p">(</span><span class="k">this</span><span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<pre style="margin-left: 2em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">You won't need the signal mapper in case you move the code to the correct location</pre>
</div>
<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;">Okay, there are some issues with this patch. In theory you shouldn't touch the unresolvedincludeassistant.* files at all. You want to modify the behavior of the custom include paths widget, hence you need also need to touch just that.
Proposed change: Make the include path resolver a proper widget subclass, e.g. by providing also a .cpp and .h to the .ui. That's the pattern used when widgets have more complex logic inside. When you have the .cpp and .h file, move the logic you added to unresolvedincludeassistant.* to those newly created files. Signal/slot handling will get much easier when you do it this way.
The .cpp/.h/.ui pattern is a commonly used in Qt, you can see several examples in the kdevelop code base.
Does that make sense to you?</pre>
<p>- Kevin Funk</p>
<br />
<p>On November 12th, 2013, 11:50 a.m. UTC, Meenakshi Khorana 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 KDevelop.</div>
<div>By Meenakshi Khorana.</div>
<p style="color: grey;"><i>Updated Nov. 12, 2013, 11:50 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kdevelop
</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;">Patch for Bug 221792 - integrate folder browser in "Setup Custom Include Paths"
Integrated a folder browser to add custom include paths. User can add custom include paths on a button click and can view the added paths in plain text edit box.</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;">Compiled and Tested successfully on local machine.</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>languages/cpp/codegen/unresolvedincludeassistant.cpp <span style="color: grey">(93a72d3)</span></li>
<li>languages/cpp/codegen/ui/custom_include_paths.ui <span style="color: grey">(1a7c1ed)</span></li>
<li>languages/cpp/codegen/unresolvedincludeassistant.h <span style="color: grey">(b072265)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/113815/diff/" style="margin-left: 3em;">View Diff</a></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">File Attachments </h1>
<ul>
<li><a href="http://git.reviewboard.kde.org/media/uploaded/files/2013/11/12/34ab7f7c-7aca-4c16-ab3f-add513c22f56__folder_browser_1.jpg">Added open file dialog to custom include path</a></li>
<li><a href="http://git.reviewboard.kde.org/media/uploaded/files/2013/11/12/29a48901-93ee-40e7-9455-94acb648025e__folder_browser_2.jpg">Folder browser for selecting include paths</a></li>
<li><a href="http://git.reviewboard.kde.org/media/uploaded/files/2013/11/12/bf9fb89b-8e6e-49f8-b2b5-d865ded209b7__folder_browser_3.jpg">Added include paths will be visible to user in text edit</a></li>
</ul>
</td>
</tr>
</table>
</div>
</body>
</html>