Mac: fixing sortedDocumentList menu doesn't change focus (WIP)
ClosedPublic

Authored by rjvbb on Aug 7 2017, 9:12 AM.

Details

Summary

On Mac, selecting a document in the dropdown sorted list of open documents changes the document view but doesn't set the focus and window title accordingly. One needs to click in the editor view to finalise the operation. This can be annoying and hinder productivity. It is not related to the QPA plugin in use (it happens with the Cocoa and the XCB backends).

I've linked this to the use of a QSignalBlocker in Sublime::Container::setCurrentWidget(); removing that instance (on Mac) makes the issue disappear.

https://bugs.kde.org/show_bug.cgi?id=382338

Test Plan

Check if the bug for which the blocker was
(https://cgit.kde.org/kdevplatform.git/commit/?id=e626a9715b3c79dc06ce413cac57203c13868823) still occurs without the block.

Diff Detail

Repository
R33 KDevPlatform
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.Aug 7 2017, 9:12 AM
Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptAug 7 2017, 9:12 AM
rjvbb added a comment.Aug 7 2017, 1:24 PM

So removing the signal blocker indeed leads to MainWindow::activateView() being called twice on Linux (when activating a different view with the sortedDocumentList menu):

void Sublime::Container::setCurrentWidget(QWidget *) widget= KTextEditor::ViewPrivate(0x2ccec10)
void Sublime::MainWindow::activateView(Sublime::View *, bool) view= KDevelop::TextView(0x2c63190) focus= true
void Sublime::Container::setCurrentWidget(QWidget *) widget= KTextEditor::ViewPrivate(0x2ccec10)
void Sublime::MainWindow::activateView(Sublime::View *, bool) view= KDevelop::TextView(0x2c63190) focus= true
void Sublime::Container::setCurrentWidget(QWidget *) widget= KTextEditor::ViewPrivate(0x2ccec10)

So the signal blocker cannot be removed on Linux at least.

I wonder though, isn't there a risk anyway of these functions calling each other in a loop? Couldn't both functions just check if the given widget is already current (or given view is already the active one) and return immediately if that's the case?
Removing the signal blocker would still lead to activeView() being called twice in that case, but at least the 2nd invocation shouldn't do anything in that case.

kfunk added a comment.Aug 7 2017, 1:37 PM
In D7179#133279, @rjvbb wrote:

...
So the signal blocker cannot be removed on Linux at least.

Thanks for confirming.

I wonder though, isn't there a risk anyway of these functions calling each other in a loop? Couldn't both functions just check if the given widget is already current (or given view is already the active one) and return immediately if that's the case?

Makes sense to me. Though it's enough if you make sure activateView(...) isn't emitted twice in this case in Container::widgetActivated only. I think that should work.

Removing the signal blocker would still lead to activeView() being called twice in that case, but at least the 2nd invocation shouldn't do anything in that case.

+1

rjvbb updated this revision to Diff 17826.Aug 7 2017, 1:48 PM

My idea of doing nothing when (re)setting the current widget or re-activating the current view works and doesn't appear to have side-effects. I'm tempted to argue that this is probably a more generically useful change than not emitting the signal twice - unless there are actual cases where setCurrentWidget() and/or activateView() are called with the current widget/view for a reason?

rjvbb updated this revision to Diff 17831.Aug 7 2017, 1:55 PM

oops, uploaded the wrong patch

kfunk accepted this revision.Aug 7 2017, 2:02 PM

To 5.1 branch if possible.

This revision is now accepted and ready to land.Aug 7 2017, 2:02 PM
kfunk added a comment.Aug 7 2017, 2:24 PM

Or, actually, master is fine, too. As this would *only* fix an issue on Mac.

This revision was automatically updated to reflect the committed changes.
rjvbb added a comment.Aug 7 2017, 2:52 PM
In D7179#133355, @kfunk wrote:

Or, actually, master is fine, too. As this would *only* fix an issue on Mac.

Saw that after pushing to the 5.1 branch and realising I'd forgotten to add the auto-closing magic to the commit message ...

@rjvbb Yay for another contribution :) Small note for the future though: if committing to stable branch, don't do a separate commit to master but just merge.
Reason is that this helps seeing which patches have been applied to which branch, and no fixes would have been forgotten to backport if only committed to master.

If afraid of a merge due to some other commits before that had not been merged and which might be conflicting in a merge, wait a few days until someone else did the merge, or ping people on irc to help resolving things.