<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 2nd, 2012, 3:20 p.m., <b>David Faure</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=61694#file61694line332" style="color: black; font-weight: bold; text-decoration: underline;">kio/kfile/kpropertiesdialog.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; ">bool KPropertiesDialog::showDialog(const KFileItemList& _items, QWidget* parent,</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">330</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">KPropertiesDialog</span><span class="o"><span class="hl">*</span></span> <span class="n">dlg</span> <span class="o"><span class="hl">=</span></span><span class="hl"> </span><span class="k">new</span> <span class="n">KPropertiesDialog</span><span class="p">(</span><span class="n">_items</span><span class="p">,</span> <span class="n">parent</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">332</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n"><span class="hl">QPointer</span></span><span class="o"><span class="hl"><</span></span><span class="n">KPropertiesDialog</span><span class="o"><span class="hl">></span></span> <span class="n">dlg</span> <span class="p"><span class="hl">(</span></span><span class="k">new</span> <span class="n">KPropertiesDialog</span><span class="p">(</span><span class="n">_items</span><span class="p">,</span> <span class="n">parent</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;">Nice one, deleting the dialog immediately after show() !
See, this is exactly why I'm against such massive changes. It's just too easy to slip in an untested change, which introduces a regression.
So instead of a fix for a bug that did not actually happen, we have a regression that does happen. Not good.
I'm veto'ing this for 4.8.
At most, review it again to ensure it *only* changes code paths that call exec(), *test it*, and put it in the frameworks branch only.
Yes it's hard to test so many code paths at once, so let's only change code paths one by one, after testing them.</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;">Now this is exactly why posting patches to reviewboard is always a good idea. You are correct on both counts. And all this time I was wondering why the properties dialog quit showing up in Konqueror and Dolphin for me. *sigh*. Anyways, I withdraw the patch. Not only do I now completely agree that such changes should be done one at a time, but also they should be done only when the situation warrants it. That is the offending code must responsible for a reproducible crash.</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>