<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/113197/">http://git.reviewboard.kde.org/r/113197/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On October 14th, 2013, 11 a.m. CEST, <b>Kevin Ottens</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;">Looks good indeed.
Maybe an idea for an improvement: What about having the internal methods use a QScopedPointer on the dialog? It'd avoid the delete before the return, and if someone modifies the file later on adding more such returns it reduces the risk of missing one such delete.
Since those dialogs are exec'd anyway that should do the trick.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Sounded like a good idea, but it does not work because createKMessageBox() deletes the dialog itself. This causes a double-delete when leaving the internal method. Since one can't pass a QScopedPointer as argument to turn createKMessageBox() into a "sink" method, it cannot work.</pre>
<br />
<p>- Aurélien</p>
<br />
<p>On October 11th, 2013, 10:56 a.m. CEST, Aurélien Gâteau 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 KDE Frameworks.</div>
<div>By Aurélien Gâteau.</div>
<p style="color: grey;"><i>Updated Oct. 11, 2013, 10:56 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kdelibs
</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;">Factorize code between regular and *WId functions, reducing the file size by 12%.
This patch adds another level of indirection. For example the sorry() function is changed from:
void sorry(QWidget *parent, ...args)
{
QDialog *dialog = new QDialog(parent);
/* fill dialog */
}
to:
static void sorryInternal(QDialog *dialog, ...args)
{
/* fill dialog */
}
void sorry(QWidget *parent, ...args)
{
sorryInternal(new QDialog(parent), ...args);
}
This makes it possible to turn the sorryWId() function into a forward function rather than a slightly-different copy of the original sorry() function:
void sorryWId(WId parent_id, ...args)
{
sorryInternal(createWIdDialog(parent_id), ...args);
}
createWIdDialog() is a helper function to create a dialog which is a child of a window belonging to another process.
Note: I kept most of the code to the place where it originally was in the file. This keeps the diff small, but readability would be improved by grouping together similar functions. Let me know if you think it is worth doing so.</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;">Builds, kmessageboxwidtest.cpp runs correctly. No other tests are available though.</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>tier1/kwidgetsaddons/src/kmessagebox.cpp <span style="color: grey">(0cfa491)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/113197/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>