Review Request 120420: [OS X] a new approach to prevent the crash after finishing a code-difference review or git/commit

[Milian Wolff]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120420/#review68144
-----------------------------------------------------------



plugins/patchreview/patchreview.cpp
<https://git.reviewboard.kde.org/r/120420/#comment47485>

    couldn't you just do setPatch(nullptr)?



plugins/patchreview/patchreview.cpp
<https://git.reviewboard.kde.org/r/120420/#comment47483>

    can't you reuse DocumentController::closeAllDocuments?



plugins/patchreview/patchreview.cpp
<https://git.reviewboard.kde.org/r/120420/#comment47484>

    you could also try to deferr the closeReview implementation, i.e. something like
    
        void PatchReviewPlugin::closeReview()
        {
            QMetaObject::invokeMethod(this, "closeReviewDelayed", Qt::QueuedConnection);
        }
        void PatchReviewPlugin::closeReviewDelayed()
        {
          // old code
        }



plugins/patchreview/patchreview.cpp
<https://git.reviewboard.kde.org/r/120420/#comment47482>

    nested eventloops are very evil, are you sure this helps and does not introduce new issues?


- Milian Wolff


On Oct. 5, 2014, 10:03 p.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. 5, 2014, 10:03 p.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/kdevelop-devel/attachments/20141009/2d93b616/attachment-0001.html>