[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-mac
Subject: Re: Review Request 120420: [OS X] a more complete approach to prevent the crash after finishing a co
From: René J.V. Bertin <rjvbertin () gmail ! com>
Date: 2016-12-13 14:47:40
Message-ID: 20161213144740.16852.53734 () mimi ! kde ! org
[Download RAW message or body]
--===============5044594381340043880==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 8bit
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120420/
-----------------------------------------------------------
(Updated Dec. 13, 2016, 3:47 p.m.)
Status
------
This change has been discarded.
Review request for KDE Software on Mac OS X and KDevelop.
Repository: kdevplatform
Description
-------
This RR replaces https://git.reviewboard.kde.org/r/120150/, itself a follow-up to RR \
https://reviewboard.kde.org/r/120081/ . In that RR I proposed an (accepted & \
submitted ) approach to prevent kdevelop from crashing after closing the patch review \
("git/show differences") toolview.
This turned out to be insufficient, the most likely culprit being a nested event \
loop. In my most recent bug hunting I noticed that `closeReview` is called through \
Qt's mouse-eventh handling function when the "Finish Review" button (or "Commit", \
when doing a git->commit) is pressed. The crash I have been experiencing occurs in \
that function (or at least not far down the call stack from that function). I had \
already noticed that the crash never occurred when I closed all document views first \
(the patch file and all modified files), e.g. by using the "Close All" menu action.
So:
- `closeReview` closes all open documents (minus the patchfile) via \
`qDeleteAll(m_highlighters)` or by calling `m_highlighters.erase(iterator)` \
repeatedly. Documents are thus closed by deleting the "content object" that \
represents them, letting that fact filter back to the UI
- closing documents via the UI takes the opposite route, and is more in line with a \
modern GUI framework (at least OS X/Cocoa) where everything happens because of an \
event in the user interface. You receive an event (message) when the user closes a \
document, the framework handles most UI-related stuff for you, and if you have to \
dispose of document-related non-gui data you can do that for example just before the \
widget actually closes (`windowWillClose`, `menuWillClose`, `applicationWillClose` \
etc. messages in Cocoa).
This led to the current patch, which cycles through and closes the `Sublime::View`s \
associated with the window area, and then lets Qt send and process all events that \
are pending and/or result from closing those windows. Only then does it proceed the \
regular course of action (calling `removeHighLighting()` which ought to be no longer \
necessary).
The only change for the user is that the pinkish patch review plugin background (with \
the "back to code" button) is visible briefly before the toolview is closed.
I understand that this crash (or a closely related one) does not only occur on OS X \
but also on Linux, so I did not put the new code in a conditional block.
Diffs
-----
plugins/grepview/grepviewplugin.cpp 7e3d933
plugins/patchreview/patchreview.cpp 18b63db
shell/sessioncontroller.cpp ae4e355
vcs/vcspluginhelper.cpp 26ab57c
vcs/widgets/vcsdiffpatchsources.cpp c3e2dae
vcs/widgets/vcsdiffwidget.cpp 54787b9
Diff: https://git.reviewboard.kde.org/r/120420/diff/
Testing
-------
kdevplatform git/kde4-legacy on KDE/MacPorts OS X 10.6.8 with kdelibs 4.14.1
Thanks,
René J.V. Bertin
--===============5044594381340043880==
MIME-Version: 1.0
Content-Type: text/html; charset="utf-8"
Content-Transfer-Encoding: 8bit
<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/120420/">https://git.reviewboard.kde.org/r/120420/</a>
</td>
</tr>
</table>
<br />
<table bgcolor="#e0e0e0" width="100%" cellpadding="12" style="border: 1px gray solid; \
border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;"> <tr>
<td>
<h1 style="margin: 0; padding: 0; font-size: 10pt;">This change has been \
discarded.</h1> </td>
</tr>
</table>
<br />
<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 KDE Software on Mac OS X and KDevelop.</div>
<div>By René J.V. Bertin.</div>
<p style="color: grey;"><i>Updated Dec. 13, 2016, 3:47 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;">This RR replaces \
https://git.reviewboard.kde.org/r/120150/, itself a follow-up to RR \
https://reviewboard.kde.org/r/120081/ . In that RR I proposed an (accepted & \
submitted ) approach to prevent kdevelop from crashing after closing the patch review \
("git/show differences") toolview.</p> <p style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: inherit;">This turned out to be \
insufficient, the most likely culprit being a nested event loop. In my most recent \
bug hunting I noticed that <code style="text-rendering: inherit;color: \
#4444cc;padding: 0;white-space: normal;margin: 0;line-height: \
inherit;">closeReview</code> is called through Qt's mouse-eventh handling function \
when the "Finish Review" button (or "Commit", when doing a git->commit) is \
pressed. The crash I have been experiencing occurs in that function (or at least not \
far down the call stack from that function). I had already noticed that the crash \
never occurred when I closed all document views first (the patch file and all \
modified files), e.g. by using the "Close All" menu action.</p> <p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">So:
- <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: \
normal;margin: 0;line-height: inherit;">closeReview</code> closes all open documents \
(minus the patchfile) via <code style="text-rendering: inherit;color: \
#4444cc;padding: 0;white-space: normal;margin: 0;line-height: \
inherit;">qDeleteAll(m_highlighters)</code> or by calling <code \
style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: \
0;line-height: inherit;">m_highlighters.erase(iterator)</code> repeatedly. Documents \
are thus closed by deleting the "content object" that represents them, letting that \
fact filter back to the UI
- closing documents via the UI takes the opposite route, and is more in line with a \
modern GUI framework (at least OS X/Cocoa) where everything happens because of an \
event in the user interface. You receive an event (message) when the user closes a \
document, the framework handles most UI-related stuff for you, and if you have to \
dispose of document-related non-gui data you can do that for example just before the \
widget actually closes (<code style="text-rendering: inherit;color: #4444cc;padding: \
0;white-space: normal;margin: 0;line-height: inherit;">windowWillClose</code>, <code \
style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: \
0;line-height: inherit;">menuWillClose</code>, <code style="text-rendering: \
inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: \
inherit;">applicationWillClose</code> etc. messages in Cocoa).</p> <p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This \
led to the current patch, which cycles through and closes the <code \
style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: \
0;line-height: inherit;">Sublime::View</code>s associated with the window area, and \
then lets Qt send and process all events that are pending and/or result from closing \
those windows. Only then does it proceed the regular course of action (calling <code \
style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: \
0;line-height: inherit;">removeHighLighting()</code> which ought to be no longer \
necessary).</p> <p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">The only change for the user is that the pinkish patch \
review plugin background (with the "back to code" button) is visible briefly before \
the toolview is closed.</p> <p style="padding: 0;text-rendering: inherit;margin: \
0;line-height: inherit;white-space: inherit;">I understand that this crash (or a \
closely related one) does not only occur on OS X but also on Linux, so I did not put \
the new code in a conditional block.</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;">kdevplatform git/kde4-legacy on KDE/MacPorts OS X \
10.6.8 with kdelibs 4.14.1</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>plugins/grepview/grepviewplugin.cpp <span style="color: \
grey">(7e3d933)</span></li>
<li>plugins/patchreview/patchreview.cpp <span style="color: \
grey">(18b63db)</span></li>
<li>shell/sessioncontroller.cpp <span style="color: grey">(ae4e355)</span></li>
<li>vcs/vcspluginhelper.cpp <span style="color: grey">(26ab57c)</span></li>
<li>vcs/widgets/vcsdiffpatchsources.cpp <span style="color: \
grey">(c3e2dae)</span></li>
<li>vcs/widgets/vcsdiffwidget.cpp <span style="color: grey">(54787b9)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/120420/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
--===============5044594381340043880==--
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic