D22424: TextDocument: remove actions from contextmenu on hide already

[René J.V. Bertin]

rjvbb added inline comments.

INLINE COMMENTS

> kossebau wrote in textdocument.cpp:253
> Originall I wanted to use delete here only, and added the deleteLater as (temporary) work-around for the one known and any potentially 3rd-party action which has a Qt::QueuedConnection connect. The latter I see as questionable thing, as with context menus there is no promise t will exist any longer once the menu is closed, and the actions here are added to the context menu also passing ownership to it. Something I plan to fix afterwards, but did not want to mess with additionally, and also not change behaviour in the 5.3 branch.
> 
> In general I also personally disfavour using deleteLater, as it makes it harder to track as human the life-time  management of objects and the order in which they are deleted, making debugging more complicated.
> 
> A deleteLater also runs into troubles on shutdown, if all document objects & other stuff is deleted, where there might be no more event loop to reach afterwards. Think especially unit tests. Which is why in case of TextDocument destructor already now I propose to use delete.
> 
> But given current unit tests are now running into issues with the need for extra eventloop hit for clean shutdown, and runtime also showed no issues, changed now to just always deleteLater. Private note made to revisit this once some patch to fix the SwitchToBuddy actions is done and accepted :)

Yes, `deleteLater()` introduces a kind of object management as found in ObjC and Cocoa (refcounting and autorelease pools). What's missing in C++ is something that allows an object to cancel its own deleting when it detects that it is still in use (to avoid issues with deletion order).

I haven't checked the code, but if deleteLater() causes the target object to be deleted when the current eventloop exits I would expect that this also happens when the last eventloop exits (and gets deleted). It's less clear what happens if you call the method after the main eventloop stopped:

  If deleteLater() is called after the main event loop has stopped, the object will not be deleted.
  Since Qt 4.8, if deleteLater() is called on an object that lives in a thread with no running event loop, the object will be destroyed when the thread finishes.

You'd say that those 2 statements contradict each other.

I did find something quite clever in the QSingleShotTimer ctor: `connect(QCoreApplication::instance(), &QCoreApplication::aboutToQuit, this, &QObject::deleteLater);`.

Anyway, I have run into multiple issues with window (and menu) objects being deleted directly on Mac, because of pending events which then got delivered to a stale (C++ or private ObjC) object. Hence the official Qt advice to use deleteLater() on UI element instances. Here it might be safe not to since the object(s) in question shouldn't be displayed at this point (I think?).

> kossebau wrote in textdocument.cpp:771
> Indeed, qCWarning is better in here as long-term solution, where the Q_ASSERT only served me to quickly get kicked while drafting the code.

Thanks for confirming what I already expected about the presence of certain of those statements :)

REPOSITORY
  R32 KDevelop

REVISION DETAIL
  https://phabricator.kde.org/D22424

To: kossebau, #kdevelop
Cc: anthonyfieroni, rjvbb, kdevelop-devel, hmitonneau, christiant, glebaccon, domson, antismap, iodelay, alexeymin, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20190716/ce7d05a7/attachment.html>