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

Wed Jul 17 00:19:08 BST 2019

kossebau added a comment.

  In D16882#494867 <https://phabricator.kde.org/D16882#494867>, @rjvbb wrote:

  > I could try your solution, of course, but what annoys me is that it comes months after I worked on mine.

  I think we all share the feeling that this should have already been handled the first time completely to end and now be a thing of the past, given we all once spent time on it before and would prefer not to have spent more :)

  > It would help if you had a specific critique on my solution other than "it doesn't use this or that signal" (or, what I kind of sense, "it comes from you").

  I made some respective comment in D22424#494708 <https://phabricator.kde.org/D22424#494708> while you asked.

  To emphasize once more, I was about to give your proposed patch a go as it worked by what I tested, but trying to understand the problem fixed with this patch with proper depth to give you a proper review, I played around and then found that the approach one would have done initially (as everyone seems to have tried) still might work out, if one simply connects to the abouttohide signal of the currently passed context menu, instead of assuming a static one per view. The current clean-only-on-next-show code always has felt rather messy to me. And so I simply proposed what arose by accident from my experiments around the phenomenon as an alternative solution, to compare & perhaps inspire for more elegant solution.

  > No disrespect intended, but your description in D22424 <https://phabricator.kde.org/D22424> isn't that easy to read either (it felt like reading German, for some inexplicable reason ;) ).

  Yes, rereading I see how this was rather a memory dump trying to still get something written after having spent some hours on it, before falling into the bed. Will see to update a bit.

  > You claim that
  > 
  >> For some yet to explore internal reason, if there is a plugin with a
  >>  "ktexteditor_popup" menu to merge in, then the QMenu object instance is the
  >>  same across all KTextEditor::View instances, otherwise each view has its
  >>  own.
  > 
  > but I have never seen any proof that the context QMenu instance is ever NOT the same. On the contrary, it makes perfect sense that it is always the same because there can only be a single context menu.

  I saw this from all the debug output I had added, comparing pointers of the context menu object handed in. Ideally indeed I would have also spent more time in kxmlgui code to find the code culprit for this to reason why this happens, but my test log showed consistent results for the different settings (ctags plugin enabled, not enabled)., so I stayed with just the observed behaviour from that black box kxmlgui, as it showed what cases one has to deal with in released versions of kxmlgui.

  > I'm not saying we shouldn't rely on the aboutToHide signal if this signal can indeed be relied on. But it would be good to do things properly and get to the bottom of the shared-or-not context QMenu instance question. It looks like your new `QPointer<QMenu>` is an acknowledgement to the fact that it can at least be a unique instance; if it always is it could (and maybe should) be moved to a central, shared structure and accessed from there.

  The idea behind the `QPointer<QMenu>` member is rather to be able to have access to any last added-to context menu to remove any persistent actions from it (those not created on-the-fly but owned by plugins and only shared for the context menu), in the very case any action triggered from the context menu results in the deletion of the textdocument object while the menu is still open

  > Maybe the KTextEditor framework should simply provide a method to get a pointer to the current context menu?!

  The problem is at least that the API of KTextEditor::View allows to set a custom menu per view instance. If we go with the assumption that only one context menu can be active globally at the same time (which might be true in case of popup menus at least), KTextEditor could perhaps help and track this internally and expose access.
  For our use-case though reliable "abouttoshow" & "abouttohide" signals (with ktexteditor ensuring reliability by all means) might be more helpful, so we do not have to do all the extra tracking work on our side.

REPOSITORY
  R32 KDevelop

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

To: rjvbb, #kdevelop, kossebau, mwolff
Cc: mwolff, egospodinova, kossebau, kde-frameworks-devel, 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/kde-frameworks-devel/attachments/20190716/ee600e19/attachment-0001.html>