<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="https://git.reviewboard.kde.org/r/118839/">https://git.reviewboard.kde.org/r/118839/</a>
</td>
</tr>
</table>
<br />
<p>On June 19th, 2014, 5:33 p.m. CEST, <b>Kurt Hindenburg</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;">yes go ahead - you are committing to frameworks branch? Do you think this might help in master/KDE4?</pre>
</blockquote>
<p>On June 20th, 2014, 1:52 a.m. CEST, <b>Alexander Richardson</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;">I doubt this is useful for KDE4 because the actual reason for this crash seems to be is due to a Qt5 behaviour change in QPointer:
Note that Qt 5 introduces a slight change in behavior when using QPointer.
When using QPointer on a QWidget (or a subclass of QWidget), previously the QPointer would be cleared by the QWidget destructor. Now, the QPointer is cleared by the QObject destructor (since this is when QWeakPointers are cleared). Any QPointers tracking a widget will NOT be cleared before the QWidget destructor destroys the children for the widget being tracked.
</pre>
</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;">@Kurt: Konsole master does not suffer from this issue, so I'd rather not fix what aren't broken
@Alexander: Oh I wasn't aware about this change. Good to know, it is indeed likely the reason for the crash.
</pre>
<br />
<p>- Aurélien</p>
<br />
<p>On June 20th, 2014, 2:04 p.m. CEST, Aurélien Gâteau wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://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 Konsole.</div>
<div>By Aurélien Gâteau.</div>
<p style="color: grey;"><i>Updated June 20, 2014, 2:04 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>
<a href="http://bugs.kde.org/show_bug.cgi?id=331724">331724</a>
</div>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
konsole
</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;">Move code responsible for 'forgetting' a view outside of code responding to the
TerminalDisplay deletion.
This avoids a loop like this:
~MainWindow
=> ~QStackedWidget
=> ~TerminalDisplay
=> QObject::destroyed
=> ViewContainer::viewDestroyed
=> ViewContainer::removeViewWidget
- internal cleanup
- try to remove TerminalDisplay from QStackedWidget which is being deleted and
crash
Instead the code now does:
~MainWindow
=> ~QStackedWidget
=> ~TerminalDisplay
=> QObject::destroyed
=> ViewContainer::viewDestroyed
=> ViewContainer::forgetView (does the internal clean up)
And if one tries to explicitly remove a view, sequence is:
ViewContainer::removeView
=> ViewContainer::forgetView
=> ViewContainer::removeViewWidget
The patch also removes ViewManager::focusActiveView() because it causes a crash
when closing a TerminalDisplay as it tries to put the focus on the deleted
TerminalDisplay. I initially called it through a queued connection, but realized
it is actually not needed for focus to be passed to the correct view, so just
removed it.
</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;">Started and closed Konsole, created and deleted tabs, created and deleted split views. No crash.</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>src/ViewContainer.h <span style="color: grey">(60d2bd9)</span></li>
<li>src/ViewContainer.cpp <span style="color: grey">(79c24d5)</span></li>
<li>src/ViewManager.h <span style="color: grey">(047b7ee)</span></li>
<li>src/ViewManager.cpp <span style="color: grey">(75473e9)</span></li>
<li>src/ViewSplitter.cpp <span style="color: grey">(bfc727e)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/118839/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>