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

[René J.V. Bertin]

rjvbb created this revision.
rjvbb added a reviewer: KDevelop.
rjvbb added a project: KDevelop.
rjvbb requested review of this revision.

REVISION SUMMARY
`TextDocument::populateContextMenu()` is called when the user opens the contextmenu but this can happen for more than just the view currently active. When that happens, each view will add its own copy of the list of items to be added to the contextmenu, resulting in duplicate entries.

This patch prevents the consequence of calling `populateContextMenu()` multiple times by checking if the target view is indeed the one currently active in the mainWindow that it belongs to, *after* removing the items from `d->addedContextMenu` from the target `menu`.

Preventing the multiple calls might also be possible but would probably introduce more complexity (connecting and disconnecting `populateContextMenu()` in reaction to `focusIn` and `focusOut` signals?).

This issue can be triggered by loading the Kate CTags plugin (after applying D16779 <https://phabricator.kde.org/D16779>). 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?

TEST PLAN
1- apply D16779 <https://phabricator.kde.org/D16779> to Kate, rebuild and reinstall the kate-ctags plugin
2- load that plugin in KDevelop; observe contextmenu action duplication (once for each document that has opened a contextmenu)
3- apply this patch, rebuild and reinstall libKDevPlatformShell
4- repeat 2, contextmenu is free of duplicates

REPOSITORY
R32 KDevelop

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

AFFECTED FILES
kdevplatform/shell/textdocument.cpp

To: rjvbb, #kdevelop
Cc: kde-frameworks-devel, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd

[Friedrich W. H. Kossebau]

kossebau added a comment.

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?

REPOSITORY
R32 KDevelop

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

To: rjvbb, #kdevelop
Cc: kossebau, kde-frameworks-devel, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd

[Friedrich W. H. Kossebau]

kossebau requested changes to this revision.
kossebau added a comment.
This revision now requires changes to proceed.


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.

REPOSITORY
R32 KDevelop

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

To: rjvbb, #kdevelop, kossebau
Cc: kossebau, kde-frameworks-devel, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd

[René J.V. Bertin]

rjvbb added a comment.

Post by Friedrich W. H. Kossebau
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).

Post by Friedrich W. H. Kossebau
(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?

REPOSITORY
R32 KDevelop

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

To: rjvbb, #kdevelop, kossebau
Cc: kossebau, kde-frameworks-devel, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd

[René J.V. Bertin]

rjvbb added a comment.


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

REPOSITORY
R32 KDevelop

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

To: rjvbb, #kdevelop, kossebau
Cc: kossebau, kde-frameworks-devel, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd

[Friedrich W. H. Kossebau]

kossebau added a comment.

Post by René J.V. Bertin

Post by Friedrich W. H. Kossebau
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)

Post by René J.V. Bertin
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.

Post by René J.V. Bertin
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.

Post by René J.V. Bertin

Post by Friedrich W. H. Kossebau
(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 ."

REPOSITORY
R32 KDevelop

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

To: rjvbb, #kdevelop, kossebau
Cc: kossebau, kde-frameworks-devel, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd

[René J.V. Bertin]

rjvbb added a comment.

Post by Friedrich W. H. Kossebau
_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).

Post by Friedrich W. H. Kossebau
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.

Post by Friedrich W. H. Kossebau

Post by Friedrich W. H. Kossebau
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.

Post by Friedrich W. H. Kossebau
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).

REPOSITORY
R32 KDevelop

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

To: rjvbb, #kdevelop, kossebau
Cc: kossebau, kde-frameworks-devel, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd

[Friedrich W. H. Kossebau]

kossebau added a comment.

Post by René J.V. Bertin

Post by Friedrich W. H. Kossebau
_If_ it is found that the root bug is in KTextEditor, sure.

<snip>

Post by René J.V. Bertin
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.

Post by René J.V. Bertin
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.

Post by René J.V. Bertin

Post by Friedrich W. H. Kossebau
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 ;) ).

Post by René J.V. Bertin

Post by Friedrich W. H. Kossebau

Post by Friedrich W. H. Kossebau
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?

Post by René J.V. Bertin

Post by Friedrich W. H. Kossebau
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.

REPOSITORY
R32 KDevelop

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

To: rjvbb, #kdevelop, kossebau
Cc: kossebau, kde-frameworks-devel, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd

[René J.V. Bertin]

rjvbb added a comment.


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

Post by Friedrich W. H. Kossebau
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.

Post by Friedrich W. H. Kossebau
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.

Post by Friedrich W. H. Kossebau
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.

Post by Friedrich W. H. Kossebau
What do you mean by platform dependent?

UI events are, by definition.

Post by Friedrich W. H. Kossebau
This bug became now also your bug

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

REPOSITORY
R32 KDevelop

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

To: rjvbb, #kdevelop, kossebau
Cc: kossebau, kde-frameworks-devel, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd

[René J.V. Bertin]

rjvbb abandoned this revision.

REPOSITORY
R32 KDevelop

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

To: rjvbb, #kdevelop, kossebau
Cc: kossebau, kde-frameworks-devel, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd

[René J.V. Bertin]

rjvbb reclaimed this revision.
rjvbb added a comment.
This revision now requires changes to proceed.


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.

REPOSITORY
R32 KDevelop

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

To: rjvbb, #kdevelop, kossebau
Cc: kossebau, kde-frameworks-devel, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd

[René J.V. Bertin]

rjvbb updated this revision to Diff 45642.
rjvbb added a comment.


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

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D16882?vs=45471&id=45642

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

AFFECTED FILES
kdevplatform/shell/textdocument.cpp

To: rjvbb, #kdevelop, kossebau
Cc: kossebau, kde-frameworks-devel, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd

[René J.V. Bertin]

rjvbb edited the summary of this revision.
rjvbb edited the test plan for this revision.
rjvbb set the repository for this revision to R32 KDevelop.

REPOSITORY
R32 KDevelop

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

To: rjvbb, #kdevelop, kossebau
Cc: kossebau, kde-frameworks-devel, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd

[René J.V. Bertin]

rjvbb added a comment.


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

REPOSITORY
R32 KDevelop

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

To: rjvbb, #kdevelop, kossebau
Cc: kossebau, kde-frameworks-devel, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd

[Friedrich W. H. Kossebau]

kossebau added a comment.

Post by René J.V. Bertin
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.

Post by René J.V. Bertin
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(?).

Post by René J.V. Bertin
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.

REPOSITORY
R32 KDevelop

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

To: rjvbb, #kdevelop, kossebau
Cc: kossebau, kde-frameworks-devel, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd

[Friedrich W. H. Kossebau]

kossebau added a comment.


For the record, related commits are:
Adding the principal clean-up logic: R32:b837392d5f05394794a813afb7ca94e54650fcff <https://phabricator.kde.org/R32:b837392d5f05394794a813afb7ca94e54650fcff>
Changing it from clean-on-hide to clean-on-about-to-show-again: R32:4d63d74c7d189479b752a11df70afab77859a457 <https://phabricator.kde.org/R32:4d63d74c7d189479b752a11df70afab77859a457>

REPOSITORY
R32 KDevelop

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

To: rjvbb, #kdevelop, kossebau
Cc: kossebau, kde-frameworks-devel, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd

[René J.V. Bertin]

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

Post by Friedrich W. H. Kossebau
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.

Post by Friedrich W. H. Kossebau
`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).

Post by Friedrich W. H. Kossebau
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,

Post by Friedrich W. H. Kossebau
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).

Post by Friedrich W. H. Kossebau
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.

Post by Friedrich W. H. Kossebau
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

[Friedrich W. H. Kossebau]

kossebau added a comment.

Post by René J.V. Bertin

Post by Friedrich W. H. Kossebau
`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)

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

[René J.V. Bertin]

rjvbb updated this revision to Diff 45759.
rjvbb added a comment.


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

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D16882?vs=45642&id=45759

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

AFFECTED FILES
kdevplatform/shell/textdocument.cpp
kdevplatform/shell/textdocument.h

To: rjvbb, #kdevelop, kossebau
Cc: egospodinova, kossebau, kde-frameworks-devel, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd

[René J.V. Bertin]

rjvbb set the repository for this revision to R32 KDevelop.

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

[René J.V. Bertin]

rjvbb updated this revision to Diff 45761.
rjvbb added a comment.


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

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D16882?vs=45759&id=45761

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

AFFECTED FILES
kdevplatform/shell/textdocument.cpp
kdevplatform/shell/textdocument.h

To: rjvbb, #kdevelop, kossebau
Cc: egospodinova, kossebau, kde-frameworks-devel, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd

[René J.V. Bertin]

rjvbb updated this revision to Diff 45782.
rjvbb added a comment.


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.

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D16882?vs=45761&id=45782

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

AFFECTED FILES
kdevplatform/shell/textdocument.cpp
kdevplatform/shell/textdocument.h

To: rjvbb, #kdevelop, kossebau
Cc: egospodinova, kossebau, kde-frameworks-devel, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd

[René J.V. Bertin]

rjvbb set the repository for this revision to R32 KDevelop.

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

[René J.V. Bertin]

rjvbb added a comment.

Post by Friedrich W. H. Kossebau
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?

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

[René J.V. Bertin]

rjvbb updated this revision to Diff 45797.
rjvbb added a comment.


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.

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D16882?vs=45782&id=45797

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

AFFECTED FILES
kdevplatform/shell/textdocument.cpp
kdevplatform/shell/textdocument.h

To: rjvbb, #kdevelop, kossebau
Cc: egospodinova, kossebau, kde-frameworks-devel, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd

[René J.V. Bertin]

rjvbb set the repository for this revision to R32 KDevelop.

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

[René J.V. Bertin]

rjvbb added a comment.


bump?

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