TextDocument: remove actions from contextmenu on hide already
ClosedPublic

Authored by kossebau on Jul 12 2019, 3:10 PM.

Details

Summary

Fixes a bug of context menu entries being duplicated each time the context
menu is shown with another document in same cases.
This happened, as each document instance tracks what actions it adds to a
context menu, but only removes the actions once the menu is shown again.
Which fails if the menu is shown with another document than before, but the
same context menu object is reused again, thus still having the actions from
the other document.

Small recap, as the situation is a bit complex:

A context menu in KDevelop text documents is triggered by the method
KateViewInternal::contextMenuEvent(), where KateViewInternal is an internal
QWidget class of KTextEditor::ViewPrivate, which is the
actual KTextEditor::View implementation. That KateViewInternal method asks its
container KTextEditor::ViewPrivate for the contextMenu() and calls popup() on it.

KTextEditor::ViewPrivate::contextMenu() itself either returns a user-set
context menu, or queries the toplevel KXMLGUIClient of the XMLGUI structure
it belongs to for a "menu" container by the name "ktexteditor_popup".
This container is defined by katepart5ui.rc
<Menu name="ktexteditor_popup" noMerge="0">, and KTE plugins can extend this
with a respecive <Menu name="ktexteditor_popup" noMerge="1"> in their ui.rc
files.

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.
More, even per view one gets a new QMenu object every time the view becomes the
active one again (and thus merging into the app XMLGUI structure is done).
Due to this the duplicated entries in the context menu are currently only
happening if a plugin also provides a "ktexteditor_popup" menu to merge in,
like the CTags one does.

In a perfect world we would have a symmetric signal
KTextEditor::View::contextMenuAboutToHide for the
KTextEditor::View::contextMenuAboutToShow signal, so we could unplug our
per-view instance custom menu additions. As fallback (and assuming the QMenu
is always show as popup) there is the QMenu::aboutToHide signal for use
instead. This was once was used here with by commit
b837392d5f05394794a813afb7ca94e54650fcff. Where though the connection was
made to the contextmenu object at the time of the creation of the view.
Which, at least these days, as pointed out above, runs risk to miss the
change of the context menu object.
At least it was found at the time already that approach was not working out,
commit 4d63d74c7d189479b752a11df70afab77859a457 use a new approach instead
to clear the menu from the old actions only right before the/a menu was
shown again. Sadly the commit message does not explain where exactly
things failed, and just described the symptom: "don't trust Qt's aboutToHide
signal as there seemed to be many cases where it didn't fire as I thought
it would be.". And points to
https://blog.qt.io/blog/2010/02/23/unpredictable-exec/ where though any
relation is not immediately obvious, as there should be little chance that
the context menu is triggered to be shown again from any actions in the
menu. And if the menu object was instead deleted without emitting any
aboutToHide signal, some shutdown would be going on which should be caught
by other means.

This patch reverts to relying to the aboutToHide signal again, but making
sure this is done for the respective menu instance currently shown. As well
as also catches any intermediate shutdown of the menu instance as well as
the document itself.

Test Plan

Used with & without CTags plugin, opening and using context menus on
different views incl. multiple views on same document with splitted view
areas.

Diff Detail

Repository
R32 KDevelop
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
kossebau created this revision.Jul 12 2019, 3:10 PM
Restricted Application added a project: KDevelop. · View Herald TranscriptJul 12 2019, 3:10 PM
Restricted Application added a subscriber: kdevelop-devel. · View Herald Transcript
kossebau requested review of this revision.Jul 12 2019, 3:10 PM

Reasons why currently I favour this over D16882:

  • slightly less complex, as it does more what one expects, adding & removing actions on show & hide
  • no global static
rjvbb added a subscriber: rjvbb.Jul 13 2019, 9:51 AM

Just a few remarks on the comments that should make them easier to understand (a priori comments should illustrate code and not require lots of different code the understand their meaning ;))

kdevplatform/shell/textdocument.cpp
89–94

This can be made easier to understand:

`Handle the case we are being deleted while the context is not yet hidden.
We want to` [...]
Move the but owned by the plugins to before the explanation in parentheses.

245–250

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

anthonyfieroni added inline comments.
kdevplatform/shell/textdocument.cpp
254

Why not deleteLater in any case?

rjvbb added inline comments.Jul 13 2019, 1:11 PM
kdevplatform/shell/textdocument.cpp
707

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

716

Idem, just call unpopulateContextMenu().

rjvbb added a comment.Jul 13 2019, 3:36 PM

I've tested this in the 5.3 branch now, which required applying hunk #3 manually to texteditor.cpp .

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

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.

kossebau marked 4 inline comments as done.Jul 15 2019, 9:33 PM

Thanks for first review & testing.

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,

kdevplatform/shell/textdocument.cpp
245–250

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.

254

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

707

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.

716

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.

kossebau updated this revision to Diff 61833.Jul 15 2019, 9:33 PM
kossebau marked 3 inline comments as done.

update to first review feedback

rjvbb added inline comments.Jul 16 2019, 8:33 AM
kdevplatform/shell/textdocument.cpp
254

Yes, deleteLater() introduces a kind of object management as found in ObjC and Cocoa (refcounting and autorelease pools). What's missing in C++ is something that allows an object to cancel its own deleting when it detects that it is still in use (to avoid issues with deletion order).

I haven't checked the code, but if deleteLater() causes the target object to be deleted when the current eventloop exits I would expect that this also happens when the last eventloop exits (and gets deleted). It's less clear what happens if you call the method after the main eventloop stopped:

If deleteLater() is called after the main event loop has stopped, the object will not be deleted.
Since Qt 4.8, if deleteLater() is called on an object that lives in a thread with no running event loop, the object will be destroyed when the thread finishes.

You'd say that those 2 statements contradict each other.

I did find something quite clever in the QSingleShotTimer ctor: connect(QCoreApplication::instance(), &QCoreApplication::aboutToQuit, this, &QObject::deleteLater);.

Anyway, I have run into multiple issues with window (and menu) objects being deleted directly on Mac, because of pending events which then got delivered to a stale (C++ or private ObjC) object. Hence the official Qt advice to use deleteLater() on UI element instances. Here it might be safe not to since the object(s) in question shouldn't be displayed at this point (I think?).

716

Thanks for confirming what I already expected about the presence of certain of those statements :)

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,

Playing around I finally managed to reproduce this as well. Though also for 5.3 and without this patch: seems something to do with workingsets code being eager to create TextDocument objects for any urls they have in the sessionrc file, Dumped my findings as https://bugs.kde.org/show_bug.cgi?id=409858

kossebau edited the summary of this revision. (Show Details)Jul 17 2019, 12:00 AM
This revision was not accepted when it landed; it landed in state Needs Review.Jul 17 2019, 12:12 AM
This revision was automatically updated to reflect the committed changes.
rjvbb added a comment.Jul 18 2019, 9:01 AM

Friedrich W. H. Kossebau wrote on 20190717::00:12:44 re: "D22424: TextDocument: remove actions from contextmenu on hide already"

This revision was not accepted when it landed; it landed in state "Needs Review".

Hmm, since when is this acceptable?

I see the same change was backported to the 5.3 branch, understandable in a way but what about the glitch I mentioned and you confirmed? Have you solved that somehow?

FWIW I got to look at the KTE implementation of the context menu mechanism that is used here. It indeed uses and reuses a single QMenu instance (there's even a comment in the code about that).

Friedrich W. H. Kossebau wrote on 20190717::00:12:44 re: "D22424: TextDocument: remove actions from contextmenu on hide already"

This revision was not accepted when it landed; it landed in state "Needs Review".

Hmm, since when is this acceptable?

It is acceptable when the person doing it is semi-maintainer (by their role of release manager). The other option would have been to release 5.3.3 without a fix for ctags plugin users, rendering it unusable for people relying on packaged kdevelop. See also https://mail.kde.org/pipermail/kdevelop-devel/2019-July/063157.html

I see the same change was backported to the 5.3 branch, understandable in a way but what about the glitch I mentioned and you confirmed? Have you solved that somehow?

If by glitch you mean the "Broken text-document:" log output appearing, please see my comment D22424#496348 above, repeating here: "Playing around I finally managed to reproduce this as well. Though also for 5.3 and without this patch: seems something to do with workingsets code being eager to create TextDocument objects for any urls they have in the sessionrc file, Dumped my findings as https://bugs.kde.org/show_bug.cgi?id=409858"

But you might mean some other glitch, which I have no idea right now?

FWIW I got to look at the KTE implementation of the context menu mechanism that is used here. It indeed uses and reuses a single QMenu instance (there's even a comment in the code about that).

Please give links into the code, as I am lost what you exactly refer to here.

The other option would have been to release 5.3.3 without a fix for ctags plugin users, rendering it unusable for people relying on packaged kdevelop.

Erm, aren't you forgetting about my patch which could have been applied to the (EOL!) 5.3 branch? That does the job just as well and would have left time to find a fix for #409858 in time for the next release.

>  But you might mean some other glitch, which I have no idea right now?

No, that glitch, which for me is just as unacceptable as the glitch your patch fixes. I get it systematically.

> FWIW I got to look at the KTE implementation of the context menu mechanism that is used here. It indeed uses and reuses a single QMenu instance (there's even a comment in the code about that).

Please give links into the code, as I am lost what you exactly refer to here.

see the source of KTextEditor::ViewPrivate::contextMenu(). The ctx menu is in fact managed by KXMLGUI.

The other option would have been to release 5.3.3 without a fix for ctags plugin users, rendering it unusable for people relying on packaged kdevelop.

Erm, aren't you forgetting about my patch which could have been applied to the (EOL!) 5.3 branch? That does the job just as well and would have left time to find a fix for #409858 in time for the next release.

Please read the email behind the link I gave in my last comment (https://mail.kde.org/pipermail/kdevelop-devel/2019-July/063157.html), I did not forget about it and explicitly considered it.

>  But you might mean some other glitch, which I have no idea right now?

No, that glitch, which for me is just as unacceptable as the glitch your patch fixes. I get it systematically.

As my comment should have made clear, but perhaps was not, that very glitch is unrelated to this very patch. It can be observed without the patch applied, and the origin of the logs appearing have been roughly described in the bug created and linked.

> FWIW I got to look at the KTE implementation of the context menu mechanism that is used here. It indeed uses and reuses a single QMenu instance (there's even a comment in the code about that).

Please give links into the code, as I am lost what you exactly refer to here.

see the source of KTextEditor::ViewPrivate::contextMenu(). The ctx menu is in fact managed by KXMLGUI.

The context menu is queried every time from KXMLGUI that method is called. So if KXMLGUI internally decides to recreate the context menu, you get another object the next time. Please see again the description of this very bug.

>>   > FWIW I got to look at the KTE implementation of the context menu mechanism that is used here. It indeed uses and reuses a single QMenu instance (there's even a comment in the code about that).
>>   
>>   Please give links into the code, as I am lost what you exactly refer to here.
> 
> see the source of `KTextEditor::ViewPrivate::contextMenu()`. The ctx menu is in fact managed by KXMLGUI.

The context menu is queried every time from KXMLGUI that method is called.  So  if KXMLGUI internally decides to recreate the context menu, you get another object the next time. Please see again the description of this very bug.

Since we're dealing with "please read/see again": idem for my comment :P which only states that a single QMenu instance is being (re)used. I've never seen it change but I without trying to understand the KXMLGUI internals (or docs) I will have to assume that this is possible. That's moot though, I've been only been saying that the ctx menu instance should probably be cached in a more central location.

kossebau added a comment.EditedJul 18 2019, 12:37 PM

Since we're dealing with "please read/see again": idem for my comment :P which only states that a single QMenu instance is being (re)used. I've never seen it change

Please add a line

qDebug() << "Showing context menu" << menu;

to KDevelop::TextDocument::populateContextMenu.

Rebuild & install, then run kdevelop, disable the CTags plugin, have 2 text documents open:

  1. invoke context menu a few times on view of document A -> same menu instance CM_A
  2. switch to view of document B, invoke context menu a few times -> same menu instance CM_B (!= CM_A)
  3. switch back to view of document A, invoke context menu a few times -> same menu instance CM_A2 (!=, CM_A, != CM_B)

This is behaviour I pbserved here consistently with KF 5.60.

When enabling the CTags plugin again, suddenly it is always the same menu instance.

If we consider that every time a document is switched, that this internally triggers a remerge of all the menus defined by the kxmlguiclients, due to the xmlgui menu data from the old view being removed and the xmlgui menu data from the new view being added, it is actually not that surprising (well, once one has thought a lot about it, it only appeared to myself now while writing this :P ) that the context menu object is recreated. Because without any other kxmlgui client injecting a menu of id "ktexteditor_popup" into the system (like the ctags plugin does), when a texteditor view is removed from the shell kxmlgui, nothing else demands/defines a menu with that id, so it gets deleted. And then recreated, because the new view getting active and adding its kxmlgui data has again such a menu defined with that id.

rjvbb added a comment.Jul 18 2019, 1:12 PM
Please add a line

  qDebug() << "Showing context menu" << menu;

to KDevelop::TextDocument::populateContextMenu.

How do you think I debugged the situation? I did the same thing in Kate too.

If memory serves me well but I did not see what you are seeing but that can have been in Kate, which I tend to consider as the reference of how all things KTE should work. I recall quite clearly thinking that KXMLGUI caches UI elements for a reason and that it's not "normal" if it recreates them.
I really don't have time right now to revisit this but there is no longer a hurry. I'll activate the debug trace on change of my lastShownMenu though; the fact it's there does suggest that I too have seen menu instance changes under certain conditions.

How do you think I debugged the situation? I did the same thing in Kate too.

The idea was to show you how I did my testing exactly, by which code and test steps, so you could do the very same, so we are on the same page and can compare our observations.
If you read something else into this, don't.

rjvbb added a comment.Jul 18 2019, 1:36 PM

In any case there isn't much else you can do; stepping through the code with a debugger is near impossible (the menu about-to-be-opened will already have grabbed mouse and keyboard focus so you'd need to display remotely).

rjvbb added a comment.Thu, Jul 18, 3:26 PM

I'll activate the debug trace on change of my lastShownMenu though

I'm doing this (in my patch from D16882 which still applies after reverting D22424):

+        if (menu != lastShownMenu.data()) {
+            if (lastShownMenu) {
+                qCWarning(SHELL) << "Added items to new contextmenu" << menu;
+            }
+            lastShownMenu = menu;
+        }

under the assumption that this will print a warning (enabled in qtlogging.ini) when lastShownMenu will be updated but not the 1st time a ctx menu is opened. Is that assumption wrong, because I'm not seeing any warnings after disabling the CTags menu (and restarting KDevelop for good measure).
Wouldn't it be great if something in my patch prevented the re-creation of the ctx menu instance? O:-)

I'll activate the debug trace on change of my lastShownMenu though

I'm doing this (in my patch from D16882 which still applies after reverting D22424):

+        if (menu != lastShownMenu.data()) {
+            if (lastShownMenu) {
+                qCWarning(SHELL) << "Added items to new contextmenu" << menu;
+            }
+            lastShownMenu = menu;
+        }

under the assumption that this will print a warning (enabled in qtlogging.ini) when lastShownMenu will be updated but not the 1st time a ctx menu is opened. Is that assumption wrong, because I'm not seeing any warnings after disabling the CTags menu (and restarting KDevelop for good measure).

There is another condition here: if (lastShownMenu). So if the QMenu object instance got deleted (like there is chance this happens when kxmlgui data is removed first on view switch and nothing else has id of that context menu at that very moment), this condition will not be met and no log will be created. I would propose to add debug log also for the "not lastShownMenu" case to make this visible in the log.

Wouldn't it be great if something in my patch prevented the re-creation of the ctx menu instance? O:-)

Fear "Something" not closer defined otherwise in software has a small chace of getting the "great" attribute ;)

rjvbb added a comment.Sat, Jul 20, 9:27 AM
There is another condition here: `if (lastShownMenu)`. So if the QMenu object instance got deleted

You mean I should do something like this?

+        if (menu != lastShownMenu.data()) {
+            if (lastShownMenuSet) {
+                // don't warn about setting the cache for the 1st time
+                qCWarning(SHELL) << "Added items to new contextmenu" << menu;
+            }
+            lastShownMenu = menu;
+            lastShownMenuSet = true;
+        }

Same effect for now, no debug output.

> Wouldn't it be great if something in my patch prevented the re-creation of the ctx menu instance? O:-)

Fear "Something" not closer defined otherwise in software has a small chace of getting the "great" attribute ;)

Which is why I said "it", not referring to my patch.
I find that a perfectly analysed fix rarely qualifies for great either: with hindsight most things you understand always become trivial (and the things they fix, the opposite of great) O:-)