[KDevelop/Shell] prevent duplicate added contextmenu actions
Needs RevisionPublic

Authored by rjvbb on Nov 14 2018, 6:36 PM.

Details

Reviewers
kossebau
mwolff
Group Reviewers
KDevelop
Summary

TextDocument::populateContextMenu() is called when the user opens the contextmenu and intends to remove the menu items added in a previous invocation before adding an updated list of actions.

That list of actions to remove has to be menu-specific; it can be a static member variable since in practice there is only a single QMenu instance for the context menu.

Test Plan

1- apply D16779 to Kate, rebuild and reinstall the kate-ctags plugin
2- apply D16927 to KTextEditor, rebuild, reinstall
3- load that plugin in KDevelop; observe contextmenu action duplication (once for each document that has opened a contextmenu)
4- apply this patch, rebuild and reinstall libKDevPlatformShell
5- repeat 2, contextmenu is free of duplicates

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 5108
Build 5126: arc lint + arc unit
rjvbb requested review of this revision.Nov 14 2018, 6:36 PM
rjvbb created this revision.

TextDocument::populateContextMenu() is called when the user opens the contextmenu but this can happen for more than just the view currently active.

What does "more than just the view currently active" mean? How can that happen? From what I see KTextEditor::View::contextMenuAboutToShow signal triggers the method. Why would there be multiple signals?

kossebau requested changes to this revision.Nov 15 2018, 12:29 AM

Oh, I missed the latter "For an as-yet unknown reason populateContextMenu() will be called with every view in which the contextmenu has been opened when the CTags plugin is loaded. A bug in KTextEditor maybe?"

Please, let's find the root causes and fix things at the base instead of adding such (even uncommented=unexplained=surprising) work-arounds for symptoms. Saves future code readers/maintainers from wanting to toast the original developers of such confusing bug/misbehaviour masking code.

This revision now requires changes to proceed.Nov 15 2018, 12:29 AM
rjvbb added a comment.Nov 15 2018, 1:49 AM
Please, let's find the root causes and fix things at the base instead of adding such

I suck at debugging event-driven code, unfortunately. But even if "we" find a fixable bug in some framework there's still no guarantee that no one will ever run KDevelop against a non-fixed version of that framework.

I'll see if a breakpoint in and backtrace from my added if teaches me anything but I have my doubts (and no other ideas).

(even uncommented=unexplained=surprising)

That's easy to fix. See the "todo" comment elsewhere in the same file.
TBH, I didn't add a comment yet because in itself I see nothing wrong in checking if you're dealing with the active view before you start adding things to a contextmenu.

Ultimately the problem lies in having a single contextmenu (the menu argument) and a d->addedContextMenu per document (view). As I said on the ML, moving populateContextMenu() and addedContextMenu to a class like KDevelop::MainWindow would solve the issue too without having to rewrite populateContextMenu(). There can only be a single contextmenu active at any time, so it would make sense to populate it in a singleton class, no?

rjvbb added a comment.Nov 15 2018, 2:36 PM

So multiple contextMenu signals arrive in Kate too except they don't have any visible consequence.
Let's see what the KTextEditor devs have to say about this. I'd rather stay away from getting too familiar with that framework, KXMLGUI even more.

https://bugs.kde.org/show_bug.cgi?id=401069

Please, let's find the root causes and fix things at the base instead of adding such

"we"

(tried to not make this a "you" vs. "others" thing in the language, but yes, your itch only so far here, you have to scratch as a matter of normal software developer life)

find a fixable bug in some framework there's still no guarantee that no one will ever run KDevelop against a non-fixed version of that framework.

_If_ it is found that the root bug is in KTextEditor, sure.

For now the root is unknown, and there is some chance that it is actually the ctag plugin code which does something hacky to trigger the reported unexpected multiple parallel emissions of the menuAboutToShow signal.

I'll see if a breakpoint in and backtrace from my added if teaches me anything but I have my doubts (and no other ideas).

I can only urge you to invest into learning how to do debugging the stuff that you work on. It's basic developer tooling. Like a serioius electrical engineer is expected to know how to use a multimeter or oscilloscope for the circuits they are fiddling with.

(even uncommented=unexplained=surprising)

That's easy to fix. See the "todo" comment elsewhere in the same file.
TBH, I didn't add a comment yet because in itself I see nothing wrong in checking if you're dealing with the active view before you start adding things to a contextmenu.

There are lots of things to check usually. But ones does not as one relies on callees fulfilling the explicit and implicit API contracts. And here the API seems to be the emission of the KTextEditor::View::contextMenuAboutToShow signal, that one should only be done once and for the given view where the menu is shown on: "Signal which is emitted immediately prior to showing the current context menu".
Adding guard code (thus complexity and stuff to maintain) to protect against contract breakages is only done as last resort.
KTextEditor and kate ctags plugin are OpenSource. They can (and should) be fixed. Everything else is just adding debts for future code maintainers. Surely many humans are fine with having their fun & easy life now on the costs of future generation.s, but seeing myself still part of the near future kdevelop generation, here my banner script: "Stop creating code to work around bug symptoms ."

rjvbb added a comment.Nov 15 2018, 3:48 PM
_If_ it is found that the root bug is in KTextEditor, sure.

Each KTextEditor::ViewPrivate has a KateViewInternal instance that inherits QWidget and overrides the contextMenuEvent() method. In that override it obains a QMenu from ViewPrivate::contextMenu() which is where KXMLGUI comes into play and where some disconnect/connect voodoo happens (reconnecting 2 signals from the same emitter to the same receiver, including the aboutToShowContextMenu slot). It then calls that menu's popup() method which triggers ViewPrivate::aboutToShowContextMenu() which in turn emits aboutToShowContextMenu.

The question is thus probably if (and why) KateViewInternal::contextMenuEvent() is called for invisible QWidgets. If that doesn't happen the disconnect/connect voodoo in ViewPrivate::contextMenu() is probably to blame. I remember scratching my head about that bit in the past, shouldn't the disconnect be from *all* receivers?.
See https://bugs.kde.org/show_bug.cgi?id=401069#c2

I don't want to delve any deeper than that into code that isn't mine and I'm not planning to work otherwise. This goes beyond using the CTags plugin in KDevelop (which is someone else's idea) and as far as that use is relevant to me I'm perfectly happy with a workaround (here or in KTextEditor).

I can only urge you to invest into learning how to do debugging the stuff that you work on. It's basic developer tooling.

Oh, I'm pretty confident I've logged more hours in more different debuggers than you, more than enough to know my strengths and weaknesses.

>   And here the API seems to be the emission of the KTextEditor::View::contextMenuAboutToShow signal, that one should only be done once and for the given view where the menu is shown on: "Signal which is emitted immediately prior to showing the current context menu".

That doesn't say explicitly that there will only be 1 such signal for the active view so this aspect could even be platform dependent.

generation.s, but seeing myself still part of the near future kdevelop generation, here my banner script: "Stop creating code to work around bug symptoms ."

Another banner would "nobody is paid to fix someone else's bugs" (except for a few poor sods whom I hope get paid really well for it).

_If_ it is found that the root bug is in KTextEditor, sure.

<snip>

See https://bugs.kde.org/show_bug.cgi?id=401069#c2

That looks like good work onto finding the culprit code, Please concentrate the related discussion there.

I don't want to delve any deeper than that into code that isn't mine and I'm not planning to work otherwise. This goes beyond using the CTags plugin in KDevelop (which is someone else's idea) and as far as that use is relevant to me I'm perfectly happy with a workaround (here or in KTextEditor).

Please check what you wrote here. To me it tells that you are in the wrong place. KDevelop, KTextEditor & Co are not for mainly having fun tinkering with code of an IDE, but to create a usable product, which can be easily maintained in snippets of free time and which is independent from one single company's interest.

Dumping hacked-together-until-it-works solutions one does not plan to work on further right from the start or really care for would be quickly the death of this.

Please tell, are you not serious about using the CTags plugin and maintaining the integration? I need to know why I should spent my time on reviewing this patch and the related issues.

I can only urge you to invest into learning how to do debugging the stuff that you work on. It's basic developer tooling.

Oh, I'm pretty confident I've logged more hours in more different debuggers than you, more than enough to know my strengths and weaknesses.

Well, I just picked up your "I suck at debugging event-driven code". If that is your weakness, exercise on it to improve it. It's a required skill here, given that lots of stuff is event-driven in kdevelop. I do not make assumptions about what you have done and what not (and I also resist, almost, to hint to quantity vs, quality ;) ).

>   And here the API seems to be the emission of the KTextEditor::View::contextMenuAboutToShow signal, that one should only be done once and for the given view where the menu is shown on: "Signal which is emitted immediately prior to showing the current context menu".

That doesn't say explicitly that there will only be 1 such signal for the active view so this aspect could even be platform dependent.

Would you agree that it is surprising to have more than 1 signal per case? So would we agree that this is implicitly given? What do you mean by platform dependent?

generation.s, but seeing myself still part of the near future kdevelop generation, here my banner script: "Stop creating code to work around bug symptoms ."

Another banner would "nobody is paid to fix someone else's bugs" (except for a few poor sods whom I hope get paid really well for it).

This bug became now also your bug as it is inhibiting your desire to enable the ctags plugin. Bad luck, but also normal developer life.

Your work-around would become also my work-around as kdevelop co-contributor, and I do not like work-arounds when they are toll code for being lazy now. Even more if it is someone else being lazy.

rjvbb added a comment.Nov 15 2018, 6:59 PM

Friedrich W. H. Kossebau wrote on 20181115::17:41:58 re: "D16882: [KDevelop/Shell] prevent duplicate added contextmenu actions"

Please tell, are you not serious about using the CTags plugin and maintaining the integration?

I cannot tell for sure but it's not something I'll be using every other minute. I am NOT planning to bring it into KDevelop, though; ideally those Kate plugins that make sense in KDevelop would work without having to patch anything outside of those plugins. I'll maintain the code in Kate as long as that remains doable.

I need to know why I should spent my time on reviewing this patch and the related issues.

Now that I have identified the upstream issue I will be abandoning this patch.

Would you agree that it is surprising to have more than 1 signal per case? So would we agree that this is implicitly given?

Yes, and probably, and I don't see a use-case for doing things another way but that doesn't exclude the possibility I'm overlooking one.

What do you mean by platform dependent?

UI events are, by definition.

This bug became now also your bug

No. My problem, maybe, but not my bug.

rjvbb abandoned this revision.Nov 15 2018, 7:17 PM
rjvbb reclaimed this revision.Nov 17 2018, 10:58 AM

Re-opening because I found an actual flaw in KDevelop after noticing that context menu duplication still occurred when only the active view receives the aboutToShowContextMenu signal.

The addedContextMenu member exists because "we want to remove the added stuff when the menu hides". This should of course read "when the menu reopens in again, possibly in a different view".
The flaw here is that the design forgets that the context menu instance is shared among views. It expects d->addedContextMenu to exist and contain the QMenu added by a previous view, but this cannot be the case in the current implementation where the variable is only allocated when the menu is first opened in a given view.
If the addedContextMenu is to be removed in JIT-fashion before reopening the context menu, it should be a static variable.

Fix upcoming.

This revision now requires changes to proceed.Nov 17 2018, 10:58 AM
rjvbb updated this revision to Diff 45642.Nov 17 2018, 11:04 AM

New patch, same purpose, active principle as outlined in the reopening comment.

rjvbb edited the summary of this revision. (Show Details)Nov 17 2018, 11:08 AM
rjvbb edited the test plan for this revision. (Show Details)
rjvbb set the repository for this revision to R32 KDevelop.

In case anyone wonders why this has gone undetected: I think because of an undocumented feature, the fact aboutToShowContextMenu was called for all views. Indeed, with the KTextEditor fix in place the duplication issue occurs also without loading the CTags plugin (= with stock KDevelop code).

Re-opening because I found an actual flaw in KDevelop after noticing that context menu duplication still occurred when only the active view receives the aboutToShowContextMenu signal.

The addedContextMenu member exists because "we want to remove the added stuff when the menu hides". This should of course read "when the menu reopens in again, possibly in a different view".

Good find, that seems indeed broken.

The flaw here is that the design forgets that the context menu instance is shared among views. It expects d->addedContextMenu to exist and contain the QMenu added by a previous view, but this cannot be the case in the current implementation where the variable is only allocated when the menu is first opened in a given view.

I would see the flaw also in that there is no specification in the KTextEditor API how the context menu is shared/reused. 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(?).

If the addedContextMenu is to be removed in JIT-fashion before reopening the context menu, it should be a static variable.

That might be a way, yes.

Before though I would like to have it first sorted out with the KTextEditor people what to expect here and whether the API dox could resolve that undefinedness. Given the current implementation kdevelop-side I would not be surprised if KTextEditor changed implementation here, but needs to be explored. The proposed fix relies on the current implementation, which is a bit fragile.

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).
Also needs to be explored if there once was such an implementation and why it has been removed. Might do in the next days.

kossebau added a comment.EditedNov 18 2018, 9:08 PM

No time for more thoughts yet, but dropping at least the found related commits here:
Adding the principal clean-up logic: R32:b837392d5f05394794a813afb7ca94e54650fcff
Changing it from clean-on-hide to clean-on-about-to-show-again: R32:4d63d74c7d189479b752a11df70afab77859a457

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.

`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).

What I meant is: in a perfect world from KDevelop side when writing code against KTextEditor API we would only need to look as far as the API and its documentation. Anything which is not specified in the API dox is implementation details, and the KTextEditor developers would be free to change things as they need, unless they break a promise given in the API dox.

Thus it would be nice to have this detail specified in the API dox of KTextEditor, about what to expect about the lifetime of the QMenu instance and if it is shared and if so, between what.
That would help both sides, the developers of KTextEditor to know what they are bound to, as well as users of the API to know what to prepare for/deal with.

E.g. I would have expected before looking at things that each view has their own separate context menu instance, possibly even created on the fly per display :)

So, "from the code it's clear" ideally would be only interesting when trying to find bugs. When writing code, the API should be what we look at, and not further. Anything else is a bug/missing feature in the API.

This is orthogonal to your actually proposed bug fix, which might be the final fix. But right now it would be relying on internal implementation, not what is documented in the API (by what I quickly read).
(sorry, myself not enough time left now, more the next week if still needed)

rjvbb updated this revision to Diff 45759.Nov 18 2018, 10:34 PM

updated as suggested:

  • moves the context menu added stuff logic into a dedicated class (as an upbeat to possible future improvement)
  • caches the QMenu* to which actions were last added, and removes them from that menu when the next context menu is shown. This should be equivalent to removing the items on contextMenuAboutToHide.
rjvbb set the repository for this revision to R32 KDevelop.Nov 18 2018, 10:34 PM
rjvbb updated this revision to Diff 45761.Nov 18 2018, 10:41 PM

Apologies, the tear-off bit shouldn't have been included of course.

rjvbb updated this revision to Diff 45782.Nov 19 2018, 9:03 AM

Another fix: use the active MainWindow as the parent of the contextMenuData instance and do NOT delete it in the TextDocumentPrivate dtor.

Also, do not assume there will ever only be a single MainWindow in a KDevelop session: the least we can do is reset TextDocumentPrivate::contextMenuData when that instance is deleted.

This should round up any issues with the new approach; I don't see anything that cries for immediate improval. Instead it'd be nice if this could make the 5.3.1 release, being a bugfix.

rjvbb set the repository for this revision to R32 KDevelop.Nov 19 2018, 9:03 AM
rjvbb added a comment.Nov 19 2018, 9:14 AM
E.g. I would have expected before looking at things that each view has their own separate context menu instance, possibly even created on the fly per display :)

I think that is what I would have expected too (maybe not per display :)), and that's one reason I didn't feel like delving into KXMLGUI. That kind of dynamic approach would surely have made things much more complex in that framework. Instead, it looks like that GUI "skeleton" is created once and reused, and that must of course be a lot simpler.
This also implies that it must be evident for anyone knowning KXMLGUI that the context menu instance for aboutToShowContextMenu signals is shared among views. As to the using a custom context menu feature: I suppose that devs using that are expected to know what they're doing...

FWIW, this is exactly why I've subscribed the frameworks ML to this diff. Do you think we need to make a more targeted effort to draw KTextEditor developer attention to these last few comments?

rjvbb updated this revision to Diff 45797.Nov 19 2018, 12:00 PM

Well, apparently the contextmenu CAN change during a session (at least on Mac and when I open it in different opened-at-session-load documents when the initial project load and parsing activity is still in progress).

Using a QPointer<QMenu> fixes that but I left in debugging traces for good measure.

rjvbb set the repository for this revision to R32 KDevelop.Nov 19 2018, 12:00 PM
mwolff requested changes to this revision.Jan 12 2019, 12:34 PM
mwolff added a subscriber: mwolff.
mwolff added inline comments.
kdevplatform/shell/textdocument.cpp
691

can't you just add a slot here that removes the actions we added once the menu is closed? that would fix this issue with way less code

This revision now requires changes to proceed.Jan 12 2019, 12:34 PM
mwolff added inline comments.Jan 12 2019, 12:35 PM
kdevplatform/shell/textdocument.cpp
691

and with slot I mean an local lambda that takes a copy of the actions list

rjvbb added a comment.Jan 12 2019, 1:30 PM

Milian Wolff wrote on 20190112::12:35:11 re: "D16882: [KDevelop/Shell] prevent duplicate added contextmenu actions"

mwolff wrote in textdocument.cpp:691
can't you just add a slot here that removes the actions we added once the menu is closed? that would fix this issue with way less code

and with slot I mean an local lambda that takes a copy of the actions list

I agree that would be more elegant, and there was code (yours, IIRC) which aimed to do this. The problem is that for some reason the aboutToClose signal is not reliable.

While trying to understand the problem, I found that perhaps we could still revert to using the aboutToHide signal, with proper boundaroes.
So alternative solution proposal up as D22424

rjvbb added a comment.Jul 12 2019, 3:32 PM

Please check the earlier discussion; IIRC there is a reliability problem with that signal, and I did try reverting to its use before coming up with the current solution.

Please check the earlier discussion; IIRC there is a reliability problem with that signal, and I did try reverting to its use before coming up with the current solution.

I read indeed the very discussion before, a few times even (see also that I fixed the link to the blog post in the commit message of mine) :)
Initially I hoped we could just go with your patch and be done, but I wanted to understand the problem in details first, to not have to rely on IIRC and vague commit messages and historic situations .And when doing so I found nothing which actually prevent to use that signal, but rather some issue in the original commit which tried to use it. So please take some time to read the text about that patch to understand why I rather went to propose that as altenativenow.

rjvbb added a comment.Jul 13 2019, 9:51 AM

I could try your solution, of course, but what annoys me is that it comes months after I worked on mine. I currently have too many other things going on in my life to dive in and figure out what on earth was going on again. I do think I outlined it well enough above in the initial description and/or exchange; I should find a moment this weekend to sit down and re-read it with a fresh mind.
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"). No disrespect intended, but your description in D22424 isn't that easy to read either (it felt like reading German, for some inexplicable reason ;) ).

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'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.
Maybe the KTextEditor framework should simply provide a method to get a pointer to the current context menu?!

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 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 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.