D22424: TextDocument: remove actions from contextmenu on hide already

[Friedrich W. H. Kossebau]

kossebau marked 4 inline comments as done.
kossebau added a comment.


  Thanks for first review & testing.
  
  In D22424#494980 <https://phabricator.kde.org/D22424#494980>, @rjvbb wrote:
  
  > It seems that it achieves its primary goal (on Linux, haven't tested OS X but context menus don't use platform menu APIs so should work the same).
  
  
  Sounds good so far.
  
  > However, the patch has a side-effect: I'm getting messages like this each time I switch or open documents:
  > 
  >   kdevplatform.shell: Broken text-document:  QUrl("file:///opt/local/var/tmp/kdevelop-tmp-505-%7Bf793a513-f14e-47b9-8448-06ca72900c04%7D/kdevelop_Q27955.patch")
  > 
  > 
  > This example is for a file that indeed doesn't exist, but that I also do not have open (so WTH?!) but I also get it for files that open perfectly.
  > 
  > I fail to see an immediate reason for this, so I'm back to running my own patch.
  
  Interesting. Not seen here, but giving this some stress testing now. The kdevelop_xxxxx.patch is coming from the diff review system creating text document objects for displaying the diffs, but I also see no direct relation ship to this patch here. Will poke around a bit,

INLINE COMMENTS

> rjvbb wrote in textdocument.cpp:244-249
> As above:
> 
> `// The addedContextMenu owns the actions` (it doesn't have any other actions, right?)
> `// and thus deletes them on destruction` ("as well" doesn't really add anything here).
> 
> `// Some actions could potentially be connected` [...] `does so currently)`
> `// Deleting them here would delete the connection` [...]
> `// so we delay their destruction to the next eventloop by default.` (I don't see any other instances that are deleteLater'ed "as well").

No, to confuse everyone it does have other actions which it does not own: some plugins also have persistent QAction objects, e.g. used for the app main menu, which they also reuse for the context menu.

Trying to reword to make this more clear here.

> anthonyfieroni wrote in textdocument.cpp:253
> Why not deleteLater in any case?

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 :)

> rjvbb wrote in textdocument.cpp:760
> Why take the risk of hard to understand behaviour in production builds? IMO just make this a runtime `if` if you cannot prove right here and now that the ASSERT is redundant. The gain of skipping the if is purely academic (and failing the test doesn't justify causing an abort in debug builds either; a qWarning() would do just fine).

Left over from debug/experiment setup, removed now as it serves no real purpose, as this should just work and for the case of two menus at the same time there would be the check in populateContextMenu() to catch this.

> rjvbb wrote in textdocument.cpp:771
> Idem, just call unpopulateContextMenu().

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.

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/20190715/22fff58f/attachment-0001.html>