<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/104802/">http://git.reviewboard.kde.org/r/104802/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 1st, 2012, 6:15 p.m., <b>Lamarque Vieira Souza</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/104802/diff/1/?file=61677#file61677line1484" style="color: black; font-weight: bold; text-decoration: underline;">kdeui/colors/kcolordialog.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 KColorDialog::setColor(const QColor &col)</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">1484</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">KColorDialo<span class="hl">g</span></span><span class="hl"> </span><span class="n"><span class="hl">dl</span>g</span><span class="p">(</span><span class="n">parent</span><span class="p">,</span> <span class="kc">true</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">1484</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="hl"> </span><span class="n"><span class="hl">QPointer</span></span><span class="o"><span class="hl"><</span></span><span class="n"><span class="hl">KColorDialog</span></span><span class="o"><span class="hl">></span></span><span class="hl"> </span><span class="n"><span class="hl">dlg</span></span><span class="p"><span class="hl">(</span></span><span class="k"><span class="hl">new</span></span> <span class="n">KColorDialog</span><span class="p">(</span><span class="n">parent</span><span class="p">,</span> <span class="kc">true</span><span class="p">)<span class="hl">)</span>;</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;">You should use QWeakPointer instead of QPointer.</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;">There really is no point in doing that here except to introduce changes in more lines of code. These are not performance critical code paths and as such I fail to see what is gained by using QWeakPointer which requires more work (read: more lines to change).</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 1st, 2012, 6:15 p.m., <b>Lamarque Vieira Souza</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/104802/diff/1/?file=61692#file61692line561" style="color: black; font-weight: bold; text-decoration: underline;">kio/kfile/kfiledialog.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; ">QStringList KFileDialogPrivate::getOpenFileNames(const KUrl& startDir,</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">561</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">if</span><span class="p">(</span><span class="n">selectedFilter</span><span class="p">)</span> <span class="o">*</span><span class="n">selectedFilter</span> <span class="o">=</span> <span class="n">dlg</span><span class="o">-></span><span class="n">currentMimeFilter</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;">stick to the code style please.</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;">huh ? You do know that is exactly how it was written before right ? Can we please review the changes on their merits ? I do not care about styling issues that already existed in the document being modified. It is up to the original author to change those or for kdelibs to enforce it using some mechanism upon commit. If I write new code from scratch I will follow whatever guideline is set, otherwise butchering the style used in existing code only serves to make it more difficult to read the code.</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 1st, 2012, 6:15 p.m., <b>Lamarque Vieira Souza</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/104802/diff/1/?file=61695#file61695line133" style="color: black; font-weight: bold; text-decoration: underline;">kio/kfile/kurlrequesterdialog.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; ">KUrl KUrlRequesterDialog::selectedUrl() const</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">132</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k"><span class="hl">const</span></span><span class="hl"> </span><span class="n"><span class="hl">KUrl</span></span><span class="o"><span class="hl">&</span></span> <span class="n">url</span> <span class="o">=</span> <span class="n">dlg</span><span class="p"><span class="hl">.</span></span><span class="n">selectedUrl</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">133</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="hl"> </span> <span class="n">url</span> <span class="o">=</span> <span class="n">dlg</span><span class="o"><span class="hl">-></span></span><span class="n">selectedUrl</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;">replace tab to spaces.</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;">For whomever wants to review these patches. Please review on the merit of the code. I do not care about styling issues that already existed in the document that was modified. It is a pointless waste of time and effort to review those unless they are completely outrageous. Thank you.</pre>
<br />
<p>- Dawit</p>
<br />
<p>On May 1st, 2012, 4:37 p.m., Dawit Alemayehu wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for kdelibs.</div>
<div>By Dawit Alemayehu.</div>
<p style="color: grey;"><i>Updated May 1, 2012, 4:37 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;">This patch attempts to mitigate the unintended crashes that might result from using QDialog::exec in kdelibs. Since nested event loops are potential sources of inadvertent crashes, this patch attempts to prevent that by changing how dialogs are created in kdelibs. All blocking dialog calls, i.e. those that invoke QDialog.exec(), are wrapped with QPointer and the QPointer is checked once QDialog.exec returns. See http://www.kdedevelopers.org/node/3919 for more details.
Note that I am aware of other classes that create nested event loops (e.g. QProcess), but this fix is only applicable to QDialog usage.</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>kdeui/colors/kcolordialog.cpp <span style="color: grey">(95bb7f5)</span></li>
<li>kdeui/dialogs/kedittoolbar.cpp <span style="color: grey">(bb80952)</span></li>
<li>kdeui/dialogs/kinputdialog.cpp <span style="color: grey">(2801c00)</span></li>
<li>kdeui/dialogs/kpixmapregionselectordialog.cpp <span style="color: grey">(11d964b)</span></li>
<li>kdeui/dialogs/kshortcutsdialog.cpp <span style="color: grey">(a73f8f2)</span></li>
<li>kdeui/dialogs/kshortcutseditor.cpp <span style="color: grey">(5984a9d)</span></li>
<li>kdeui/findreplace/kfinddialog.cpp <span style="color: grey">(de2dd90)</span></li>
<li>kdeui/fonts/kfontdialog.cpp <span style="color: grey">(9bea490)</span></li>
<li>kdeui/widgets/ktextedit.cpp <span style="color: grey">(1e58706)</span></li>
<li>kdeui/xmlgui/kmenumenuhandler_p.cpp <span style="color: grey">(d8c82b6)</span></li>
<li>kfile/kdiroperator.cpp <span style="color: grey">(18ffc34)</span></li>
<li>kfile/kdirselectdialog.cpp <span style="color: grey">(e0dcafa)</span></li>
<li>kfile/kfileplaceeditdialog.cpp <span style="color: grey">(5537551)</span></li>
<li>kio/kfile/kacleditwidget.cpp <span style="color: grey">(d89429f)</span></li>
<li>kio/kfile/kencodingfiledialog.cpp <span style="color: grey">(4686065)</span></li>
<li>kio/kfile/kfiledialog.cpp <span style="color: grey">(d121e4d)</span></li>
<li>kio/kfile/kicondialog.cpp <span style="color: grey">(b7d646f)</span></li>
<li>kio/kfile/kpropertiesdialog.cpp <span style="color: grey">(feb0c9e)</span></li>
<li>kio/kfile/kurlrequesterdialog.cpp <span style="color: grey">(8ee29e1)</span></li>
<li>kio/kio/jobuidelegate.cpp <span style="color: grey">(85679c2)</span></li>
<li>kio/kio/kbuildsycocaprogressdialog.cpp <span style="color: grey">(fba30ec)</span></li>
<li>kio/kio/passworddialog.cpp <span style="color: grey">(faf0c77)</span></li>
<li>kio/kio/paste.cpp <span style="color: grey">(ca451fb)</span></li>
<li>kio/kssl/kcm/cacertificatespage.cpp <span style="color: grey">(0a269a3)</span></li>
<li>knewstuff/knewstuff2/ui/downloaddialog.cpp <span style="color: grey">(b4d2dcd)</span></li>
<li>knewstuff/knewstuff2/ui/kdxsbutton.cpp <span style="color: grey">(e8f8c83)</span></li>
<li>knewstuff/knewstuff3/knewstuffbutton.cpp <span style="color: grey">(9c14e99)</span></li>
<li>kparts/browserrun.cpp <span style="color: grey">(c89829d)</span></li>
<li>kutils/kpluginselector.cpp <span style="color: grey">(505e53f)</span></li>
<li>nepomuk/ui/tagwidget.cpp <span style="color: grey">(7c59922)</span></li>
<li>nepomuk/utils/searchwidget.cpp <span style="color: grey">(f46e72a)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/104802/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>