[KDE/Mac] Review Request 120420: [OS X] a new approach to prevent the crash after finishing a code-difference review or git/commit
René J.V. Bertin
rjvbertin at gmail.com
Thu Oct 9 12:54:45 UTC 2014
> On Oct. 9, 2014, 2:19 p.m., Milian Wolff wrote:
> > plugins/patchreview/patchreview.cpp, line 309
> > <https://git.reviewboard.kde.org/r/120420/diff/3/?file=316725#file316725line309>
> >
> > couldn't you just do setPatch(nullptr)?
Possibly, might even be better (in case we enter the dtor without having gone through closeReview first).
- René J.V.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120420/#review68144
-----------------------------------------------------------
On Oct. 6, 2014, 12:03 a.m., René J.V. Bertin wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/120420/
> -----------------------------------------------------------
>
> (Updated Oct. 6, 2014, 12:03 a.m.)
>
>
> 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/patchreview/patchreview.cpp 18b63db
>
> 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-mac/attachments/20141009/480995de/attachment-0001.html>
More information about the kde-mac
mailing list