KTextEditor : disconnect contextmenu from all aboutToXXContextMenu receivers
ClosedPublic

Authored by rjvbb on Nov 16 2018, 4:24 PM.

Details

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

Test Plan

Build KTextEditor with a debug trace to ViewPrivate::aboutToShowContextMenu(), e.g.

void KTextEditor::ViewPrivate::aboutToShowContextMenu()
{
    QMenu *menu = qobject_cast<QMenu *>(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.

Diff Detail

Repository
R39 KTextEditor
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
rjvbb created this revision.Nov 16 2018, 4:24 PM
Restricted Application added projects: Kate, Frameworks. · View Herald TranscriptNov 16 2018, 4:24 PM
Restricted Application added a subscriber: kwrite-devel. · View Herald Transcript
rjvbb requested review of this revision.Nov 16 2018, 4:24 PM
ngraham edited the summary of this revision. (Show Details)Nov 16 2018, 4:26 PM
cullmann accepted this revision.Nov 16 2018, 9:15 PM
cullmann added a subscriber: cullmann.

Sounds reasonable.

This revision is now accepted and ready to land.Nov 16 2018, 9:15 PM

Btw., the 2 nullptr in the disconnect can be left out, or?
+ a comment for the future would be nice in the code

rjvbb added a comment.Nov 16 2018, 9:39 PM
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?

You are right. Please add some comment why one disconnects all things and push it.

This revision was automatically updated to reflect the committed changes.