D16927: KTextEditor : disconnect contextmenu from all aboutToXXContextMenu receivers
This revision was automatically updated to reflect the committed changes. Closed by commit R39:3f0c617e22ab: disconnect contextmenu from all aboutToXXContextMenu receivers (authored by rjvbb). CHANGED PRIOR TO COMMIT https://phabricator.kde.org/D16927?vs=45597&id=45664#toc REPOSITORY R39 KTextEditor CHANGES SINCE LAST UPDATE https://phabricator.kde.org/D16927?vs=45597&id=45664 REVISION DETAIL https://phabricator.kde.org/D16927 AFFECTED FILES src/view/kateview.cpp To: rjvbb, #ktexteditor, #frameworks, cullmann Cc: cullmann, kde-frameworks-devel, kwrite-devel, michaelh, ngraham, bruns, demsking, head7, kfunk, sars, dhaumann
D16927: KTextEditor : disconnect contextmenu from all aboutToXXContextMenu receivers
cullmann added a comment. You are right. Please add some comment why one disconnects all things and push it. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D16927 To: rjvbb, #ktexteditor, #frameworks, cullmann Cc: cullmann, kde-frameworks-devel, kwrite-devel, michaelh, ngraham, bruns, demsking, head7, kfunk, sars, dhaumann
D16927: KTextEditor : disconnect contextmenu from all aboutToXXContextMenu receivers
rjvbb added a comment. > Btw., the 2 nullptr in the disconnect can be left out, or? Not to my understanding. From the docs: Disconnect everything connected to a specific signal: disconnect(myObject, SIGNAL(mySignal()), 0, 0); equivalent to the non-static overloaded function myObject->disconnect(SIGNAL(mySignal())); I don't think the 2nd form would match the rest of the code, do you? REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D16927 To: rjvbb, #ktexteditor, #frameworks, cullmann Cc: cullmann, kde-frameworks-devel, kwrite-devel, michaelh, ngraham, bruns, demsking, head7, kfunk, sars, dhaumann
D16927: KTextEditor : disconnect contextmenu from all aboutToXXContextMenu receivers
cullmann added a comment. Btw., the 2 nullptr in the disconnect can be left out, or? + a comment for the future would be nice in the code REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D16927 To: rjvbb, #ktexteditor, #frameworks, cullmann Cc: cullmann, kde-frameworks-devel, kwrite-devel, michaelh, ngraham, bruns, demsking, head7, kfunk, sars, dhaumann
D16927: KTextEditor : disconnect contextmenu from all aboutToXXContextMenu receivers
cullmann accepted this revision. cullmann added a comment. This revision is now accepted and ready to land. Sounds reasonable. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D16927 To: rjvbb, #ktexteditor, #frameworks, cullmann Cc: cullmann, kde-frameworks-devel, kwrite-devel, michaelh, ngraham, bruns, demsking, head7, kfunk, sars, dhaumann
D16927: KTextEditor : disconnect contextmenu from all aboutToXXContextMenu receivers
ngraham edited the summary of this revision. REPOSITORY R39 KTextEditor REVISION DETAIL https://phabricator.kde.org/D16927 To: rjvbb, #ktexteditor, #frameworks Cc: kde-frameworks-devel, kwrite-devel, michaelh, ngraham, bruns, demsking, head7, cullmann, kfunk, sars, dhaumann
D16927: KTextEditor : disconnect contextmenu from all aboutToXXContextMenu receivers
rjvbb created this revision.
rjvbb added reviewers: KTextEditor, Frameworks.
rjvbb added a project: KTextEditor.
Herald added projects: Kate, Frameworks.
Herald added a subscriber: kwrite-devel.
rjvbb requested review of this revision.
REVISION SUMMARY
`ViewPrivate::contextMenu()` has a surprising way of handling the
`ktexteditor_popup` menu aboutToXXX signals: it disconnects them from the
current ViewPrivate instance and then reconnects them to the same instance.
I think the disconnect should be from all receivers that were once connected
to the menu show & hide signals. Doing that resolves the issue where
`ViewPrivate::aboutToShowContextMenu()` is called for all open KTextEditorViews
that once had the context menu open, instead of only for the view that is
currently active.
BUG: https://bugs.kde.org/show_bug.cgi?id=401069
TEST PLAN
Build KTextEditor with a debug trace to
`ViewPrivate::aboutToShowContextMenu()`, e.g.
void KTextEditor::ViewPrivate::aboutToShowContextMenu()
{
QMenu *menu = qobject_cast(sender());
if (menu) {
if (mainWindow()->activeView() == this) {
qWarning() << Q_FUNC_INFO << "emitting contextMenuAboutToShow
for foreground view" << this;
emit contextMenuAboutToShow(this, menu);
} else {
qWarning() << Q_FUNC_INFO << "NOT emitting
contextMenuAboutToShow for background view" << this;
}
}
}
Now open Kate with multiple documents. Right-click in the active document,
notice only that view emits `contextMenuAboutToShow`. Activate other documents
one after the other, right-clicking in them, and notice how the previously
active documents would have emitted the `contextMenuAboutToShow` signal.
This is resolved by applying this patch. Opening the context menu in a
long-running multi-document session keeps feeling snappier during the entire
session too.
REPOSITORY
R39 KTextEditor
REVISION DETAIL
https://phabricator.kde.org/D16927
AFFECTED FILES
src/view/kateview.cpp
To: rjvbb, #ktexteditor, #frameworks
Cc: kde-frameworks-devel, kwrite-devel, michaelh, ngraham, bruns, demsking,
head7, cullmann, kfunk, sars, dhaumann
