D16882: [KDevelop/Shell] prevent duplicate added contextmenu actions

[René J.V. Bertin]

rjvbb added a subscriber: egospodinova.
rjvbb added a comment.


  >   I would see the flaw also in that there is no specification in the KTextEditor API how the context menu is shared/reused.
  
  Did you see the reaction to my proposed KTextEditor fix? My argument "sounds reasonable". To me that suggests no one ever thought about whether there should only be a single signal, the one for the active view.
  
  >   `KTextEditor::View::contextMenu()` talks about "the xmlgui menu" or custom set "context menu object", but no hint/promise whether there is any instance sharing done, e.g. between views for the same document or even across all views in the same process(?).
  
  No, but from the code it's clear that there's only a single instance that is shared among all views. With KXMLGUI that's even unavoidable (and incidentally a source of problems on Mac but that's a different can of worms).
  
  >   That might be a way, yes.
  
  It's the simplest approach and the most robust, the poor man's equivalent of moving the whole context menu logic to KDevelop::MainWindow where the question of what view is active shouldn't arise (I didn't check if this is a feasible alternative). I'd call it elegantly simple if it weren't for the fact it involves a global static variable.
  
  As to the other alternative,
  
  >   Another option might be to link up to the menu being closed and clean up then, as the comment on the `addedContextMenu` member claimed. (personally preferred to clean up right after use). 
  
  my first reflex was to start implementing that, then I saw this could get complex and possibly ugly when I went over the various things the code would have to take into account. I didn't write them down (sorry) but I do remember being suspicious of Qt's aboutToHide signal, apparently with reason (http://labs.trolltech.com/blogs/2010/02/23/unpredictable-exec/).
  And then I realised only a tiny change was required ...
  
  A truly proper implementation would probably have a dedicated context menu class that has the addedContextMenu QMenu with KDevelop actions, changing that only when required instead of rebuilding it every time, etc. And that sounds more like a junior job than as a bugfix to me, esp. since it would apparently require thorough checking of the current `aboutToHide` reliability.
  
  A middle ground is possible: a dedicated context menu class that for now keeps the same working principle (JIT removal).
  
  >   Given the current implementation kdevelop-side I would not be surprised if KTextEditor changed implementation here, but needs to be explored.
  
  Don't hesitate to double-check, but I didn't see any recent changes in the code involved. The strange disconnect/reconnect trick (now fixed) has been there for years, in particular.
  
  >   The proposed fix relies on the current implementation, which is a bit fragile.
  
  ? It relies on the fact that the context menu is always the same instance, and that's a given as long as it's a kxmlgui menu, no? Removing non-existent menu actions doesn't (currently) generate errors, so there's no problem there. And we could cache the target QMenu address in order to raise a warning if ever we get a different one.
  
  Hah! Actually, the JIT implementation could remove d->addedContextMenu from the cached QMenu instance. That should make the approach work whether the menu is always the same or whether it changes all the time, because there will still ever be only a single context menu active at a time.

REPOSITORY
  R32 KDevelop

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

To: rjvbb, #kdevelop, kossebau
Cc: egospodinova, kossebau, kde-frameworks-devel, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20181118/4f53e485/attachment.html>