- with 1641d714d2ca a reimplementation of PartDocument:close() is not necessary any longer
Details
- Reviewers
kossebau - Group Reviewers
KDevelop - Commits
- R32:2647052da0ef: Remove reimplementation of PartDocument:close() in TextDocument
- close KDevelop with a displayed kmarkdownwebview plugin
Diff Detail
- Repository
- R32 KDevelop
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
kdevplatform/shell/ktexteditorpluginintegration.cpp | ||
---|---|---|
350 ↗ | (On Diff #33456) | Given the viewBarContainer widget is a child of the KDevelop::MainWindow object m_mainWindow and only destructed once that m_mainWindow is destructed, this check seems fragile. Though given that the actual KTextEditor::View objecst are only deleted with deleteLater(), but the KTextEditor::MainWindow object passed to them should be "valid for the complete lifetime of the view", it seems this needs some bigger changes: once to delay the deletion of this object with deleteLater as well and prepare for the actual-mainwindow-already-deleted state. Will see to prepare some patch for this, as I have a debuggin-enriched build already here (needed to find out what I just wrote ;) ) |
kdevplatform/shell/textdocument.cpp | ||
546 | From what I saw so far would agree this seems no longer needed and just a duplication. |
I have been running the change to textdocument.{h,cpp} together with D13905 (edit: since then) and (edit: so far) not seen any (new) issues.
Anyone objecting to have that subpatch to textdocument (removal of TextDocument::close() reimplementation) go in?
- Revert "check for still existing view bars before deletion to prevent crash when quitting"
I reverted the other changes to underline what would be remaining. The title and the revision summary should probably be adapted as well then.
Hm, CI points out that this makes TestShellDocumentOperation::testClosing() now fail. And also does for me now, though when I had been running this method if-def'ed out, it did not fail the last week.
Looking at it, why the test fails is clear, Sublime::View destructor only deletes the widget with deleteLater(), so without any DeferredDelete event handler between the widget is still there, other than what the test expects.
No idea yet, would propose to give this some more look during the next days, why it actually fails now and why not before and how grave that is or if the test needs adaption.
Looking at the history, I could not find any reason for the deferred delete of the widget. . Assuming defensive programming at that time, or perhaps some related issues with ktexteditor.
Running kdevelop now the next days with the actual widget deleted in sync, so far no issues hit.
So currently my favourite. Might also remove the need for the delayed mainwindow wrapper destruction again. Let's see :) Would be good to gain more control about destruction timing by that.
I was just discovering the same. QTest doesn't do DeferredDeletes (QTBUG-12575), so adding
QCoreApplication::sendPostedEvents(0, QEvent::DeferredDelete);
as suggested in the bug report would fix the test.
That bug report though is about running the event handler on exiting the test, no?
While here we are running into the issue that the test case uncovers that before something was by chance running the DeferredDelete event handler as a result of Document::close() being called, but now no longer is:
// Test that both the view and the view widget is deleted when closing // document. { IDocumentController *documentController = Core::self()->documentController(); documentController->openDocumentFromText(QStringLiteral("Test1")); Sublime::Area *area = Core::self()->uiControllerInternal()->activeArea(); QCOMPARE(area->views().count(), 1); QPointer<Sublime::View> the_view = area->views().at(0); QPointer<QWidget> the_widget = the_view->widget(); documentController->openDocuments().at(0)->close(IDocument::Discard); // as result of that ^ call before something was triggering the DeferredDelete handler, but now no longer is QCOMPARE(the_view.data(), (Sublime::View*)nullptr); QCOMPARE(the_widget.data(), (QWidget*)nullptr); // <-- now fails }
By looking at the code I guess this is due to something in the destruction of KTextEditor::Document. Which with the new code only happens in a deferred delete handler as well, as the final destruction of the KDevelop document only is triggered by Sublime::DocumentPrivate::removeView(...) on closing the last view via deleteLater, which is triggered by the closeViews() as called from PartDocument::close(). While the old code did an instant deletion of KTextEditor::Document already in the TextDocument::close() call.
Meh. All the document/view destruction seems partially out of control *insert image of destructed-by-explosion tower falling in wrong direction*. Time to regain control. Will poke around that logic some more and try to come up with some sublime design policies for document & view, like who owns what, in which threads do they live and who is responsible for creation & destruction.