<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/128016/">https://git.reviewboard.kde.org/r/128016/</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 26th, 2016, 1:19 a.m. CEST, <b>Aleix Pol Gonzalez</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Can't you use a smart pointer (i.e. QScopedPointer)? code filled with deletes is scary.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Will we need such a patch for every QMenu in the code?</p></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Heh, I don't have that issue with pointer management, I almost don't know any better being mainly a C user since longer than I care to remember :)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I guess QScopedPointer would be fine as long as we can be sure it uses <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">deleteLater</code> or an equivalent method. I'll try to figure that out, from the looks of it there's a custom cleanup handler that should have that effect (<code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">QScopedPointerDeleteLater</code>, for <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">pointers to QObject's that are actively participating in a QEventLoop</em>).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">From what I remember this particular instance was rather easy to find and correct as crashing was sufficiently reproducible. I haven't continued to test without the patch for each new Qt or KDevelop version, though.
Anyway, I think that in theory every location that uses a temporary QMenu object is a potential candidate for this kind of patch, if there is no guarantee that all underlying objects can be released safely when the QMenu object goes out of scope. This is related to the changes I've had to make to the patch review plugin, of course.
The Qt documentation makes it clear (but not abundantly so) that it's a good idea to use <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">deleteLater</code> on QObjects that participate in an event loop. That doesn't mean that using <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">delete</code> will lead to disaster systematically, unfortunately. The presence of pending events or not may even depend on CPU speed or load, so we can probably get away with making this kind of change on a case-by-case basis, tackling issues as they present themselves.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">There may also be other workarounds, like doing a forced event handler sync (<code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">sendPostedEvents()</code>, <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">processEvents()</code>). I'm not really sure if that's really preferable ...</p></pre>
<br />
<p>- René J.V.</p>
<br />
<p>On May 25th, 2016, 10:54 p.m. CEST, René J.V. Bertin 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 René J.V. Bertin.</div>
<p style="color: grey;"><i>Updated May 25, 2016, 10:54 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;">OS X can be capricious when instances corresponding to a widget are deleted, if the class in question uses "native" ObjC SDKs behind the scenes. Pending events can in that case be (generated and) delivered to objects that were already deleted.
According to the documentation, one should prefer to use <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">QObject::deleteLater()</code> rather than the regular, direct <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">delete</code> whether it be explicit or implicit.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I've long used a local patch that uses this approach in order to prevent a recurring crash after using the context menu of the "ideal dock widget". Somehow I never put it up for review here, apparently.</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;">Builds and permits reliable behaviour on both OS X and Linux.</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>sublime/idealdockwidget.cpp <span style="color: grey">(dae0ea2)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/128016/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>