Commit d940a88e authored by Friedrich W. H. Kossebau's avatar Friedrich W. H. Kossebau
Browse files

TextDocument: remove actions from contextmenu on hide already

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
b837392d. 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  4d63d74c 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.

Reviewers: #kdevelop

Subscribers: anthonyfieroni, rjvbb, kdevelop-devel

Tags: #kdevelop

Differential Revision: https://phabricator.kde.org/D22424
parent 7f636275
......@@ -86,8 +86,11 @@ public:
~TextDocumentPrivate()
{
delete addedContextMenu;
addedContextMenu = nullptr;
// Handle the case we are being deleted while the context menu is not yet hidden.
// We want to remove all actions we added to it, especially those not owned by the document
// but by the plugins (i.e. created on-the-fly during ContextMenuExtension::populateMenu
// with ownership set to our addedContextMenu)
cleanContextMenu();
saveSessionConfig();
delete document;
......@@ -225,6 +228,31 @@ public:
setStatus(document, dirty);
}
void cleanContextMenu()
{
if (!addedContextMenu) {
return;
}
if (currentContextMenu) {
const auto actions = addedContextMenu->actions();
for (QAction* action : actions) {
currentContextMenu->removeAction(action);
}
currentContextMenu.clear();
}
// The addedContextMenu owns those actions created on-the-fly for the context menu
// (other than those actions only shared for the context menu, but also used elsewhere)
// and thuse deletes then on its own destruction.
// Some actions potentially could be connected to triggered-signal handlers
// using Qt::QueuedConnection (at least SwitchToBuddyPlugin does so currently).
// Deleting them here also would also delete the connection before the handler is triggered.
// So we delay the menu's and thus their destruction to the next eventloop by default.
addedContextMenu->deleteLater();
addedContextMenu = nullptr;
}
TextDocument * const q;
QPointer<KTextEditor::Document> document;
......@@ -233,6 +261,7 @@ public:
bool loaded = false;
// we want to remove the added stuff when the menu hides
QMenu* addedContextMenu = nullptr;
QPointer<QMenu> currentContextMenu;
};
class TextViewPrivate
......@@ -670,15 +699,25 @@ void KDevelop::TextDocument::textChanged(KTextEditor::Document *document)
notifyContentChanged();
}
void KDevelop::TextDocument::unpopulateContextMenu()
{
auto* menu = qobject_cast<QMenu*>(sender());
disconnect(menu, &QMenu::aboutToHide, this, &TextDocument::unpopulateContextMenu);
d->cleanContextMenu();
}
void KDevelop::TextDocument::populateContextMenu( KTextEditor::View* v, QMenu* menu )
{
if (d->addedContextMenu) {
foreach ( QAction* action, d->addedContextMenu->actions() ) {
menu->removeAction(action);
}
delete d->addedContextMenu;
qCWarning(SHELL) << "populateContextMenu() called while we still handled another menu.";
d->cleanContextMenu();
}
d->currentContextMenu = menu;
connect(menu, &QMenu::aboutToHide, this, &TextDocument::unpopulateContextMenu);
d->addedContextMenu = new QMenu();
EditorContext c(v, v->cursorPosition());
......
......@@ -86,6 +86,7 @@ private:
void newDocumentStatus(KTextEditor::Document*);
void populateContextMenu(KTextEditor::View*, QMenu*);
void unpopulateContextMenu();
void textChanged(KTextEditor::Document*);
void documentUrlChanged(KTextEditor::Document*);
void slotDocumentLoaded();
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment