<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/122500/">https://git.reviewboard.kde.org/r/122500/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On February 9th, 2015, 5:34 p.m. UTC, <b>Hugo Pereira Da Costa</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;">Although I have no objection against the change, I must admit I don't understand what's wrong with the current code, nor the actual description of the patch.
"registerWidget may take an existing widget as a parameter. If so, we
don't want to delete it"
if I understand my own code right, <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">widget is not the argument passed as the registerWidget method. It is an internal member, created parentless, at first accepted call to registerWidget. So as such it is not 'explicitly' owned by anyone, and implicitly owned by us.
And then, what is wrong with deleting it in our destructor ?
is it because, though parentless it might get deleted elsewhere ?
or because of a thread issue ?
What do I miss ?
(PS: the reason behind this interal _widget member, is that you can not track palettechanged events on a widget, via event filter, once you set it your own 'altered' palette: it won't recive these events anymore.
So: eventFilter must be installed on either qApp (which is then getting the event for _every</em> widget, for which we did not alter the palette, which is quite a lot), or on a widget for which we are sure the pallette is not altered. Hence: our own).</p></pre>
</blockquote>
<p>On February 9th, 2015, 9:13 p.m. UTC, <b>David Edmundson</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;">oh, you're absolutely right...</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I just had my valgrind traces and in my haste didn't see the difference between _widget and widget.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The crash was in QApplication trying to update the palette on _widget after you change styles in the style KCM.
https://paste.kde.org/pvhhfielh</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">According to valgrind, the widget it was trying to update was very much the one deleted in the PaletteHelper destructor.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'll try replacing just delete _widget with _widget->deleteLater() and see if that crash still happens; it might be the more relevant part of the fix. It's generally a bad idea to directly delete a QObject in anything that might be called from a slot.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Thanks.</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;">Clear enough. I agree with the delete -> deleteLater change. Keep me posted !<br style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
Does the crash you report above happen every time ? (and is there a bug report ?), At least here i could not reproduce so far.</p></pre>
<br />
<p>- Hugo</p>
<br />
<p>On February 9th, 2015, 2:33 p.m. UTC, David Edmundson 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 Plasma and Hugo Pereira Da Costa.</div>
<div>By David Edmundson.</div>
<p style="color: grey;"><i>Updated Feb. 9, 2015, 2:33 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
breeze
</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;">Don't delete widgets we don't own when changing styles</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">registerWidget may take an existing widget as a parameter. If so, we
don't want to delete it when our paletteHelper is deleted for example if
we change style.</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>kstyle/breezepalettehelper.cpp <span style="color: grey">(31c32c3)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/122500/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>