Differential D6060

platform-specific document switcher default shortcuts
ClosedPublic
Actions

Authored by rjvbb on Jun 1 2017, 3:45 PM.

Details

Reviewers
kfunk
kossebau
Group Reviewers
KDevelop
Commits
R32:3934fcea209e: Use platform-specific default shortcuts in the document switcher
Summary

The document switcher plugin defines its default shortcuts as Ctrl+Tab and Ctrl+Shift+Tab. On Mac this translates to Command[+Shift]+Tab, which are intercepted by the host and trigger the application switcher. I *think* that something similar will happen on MSWindows.

To make things more complicated, Qt/Mac has an attribute that tells the keyboard event functions NOT to swap the Control and Meta (Command) keys, and an as-yet unidentified bug (probably) in KF5 code causes Control+Tab shortcuts to be discarded.

The patch introduces a platform-specific shortcutAccelerator variable that is set to Qt::CTRL except when running on Mac and the DontSwapCtrlAndMeta attribute is set. In the latter case it is set to Qt::ALT (instead of Qt::META). If the Qt::ALT modifier is defined on MSWindows it could be used there too.

Test Plan

Works as intended on Linux and Mac, with Qt 5.8.0 - 5.9.6 and the Cocoa as well as the XCB QPA plugins.

The patch applies to the 5.2 branch.

I'm taking off the WIP label because I haven't felt a need to change anything in this patch since quite a while now.

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.
rjvbb created this revision.Jun 1 2017, 3:45 PM
Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptJun 1 2017, 3:45 PM
kossebau added a subscriber: kossebau.Aug 6 2017, 2:52 PM

From the description it seems you are planning more work on this (i.e. finding out why Control+Tab shortcuts are eaten somewhere)?
Could you please tag this as "Changes planned" then? The informal [WIP] is good, using the system understood flag better :)

No idea about the Mac shortcuts, the non-Mac code looks fine to me so far ;)

plugins/documentswitcher/documentswitcherplugin.cpp
68

Make both shortcutAccelerator const.

kfunk added a subscriber: kfunk.Aug 7 2017, 8:13 AM

LGTM, after having fixed Friedrich's remarks

rjvbb added a comment.Aug 8 2017, 4:32 PM
In D6060#132935, @kossebau wrote:

From the description it seems you are planning more work on this (i.e. finding out why Control+Tab shortcuts are eaten somewhere)?

It's a bit misty exactly why I labelled this a WIP (going on vacations tends to have that effect on me :-O). I think it was mostly related to the fact that MSWin might be affected similarly. I had another look at the KF5 framework sources and don't see any code that eats Control+Tab or Meta+Tab key presses. There are a number of places where Shift+Tab is handled but I don't think they're involved (they shouldn't just cause problems on Mac if they were).

Could you please tag this as "Changes planned" then? The informal [WIP] is good, using the system understood flag better :)

I tried but must misunderstand something. I don't get to see such a tag, and apparently cannot use undefined tags either.

No idea about the Mac shortcuts, the non-Mac code looks fine to me so far ;)

rjvbb updated this revision to Diff 17888.Aug 8 2017, 4:37 PM

updated as requested. Should I commit and if so, close the review or leave it open and set the tag?

rjvbb set the repository for this revision to R33 KDevPlatform.Aug 8 2017, 4:38 PM
kossebau added a comment.Jun 27 2018, 11:36 AM

ping? @rjvbb Do you consider this patch done and ready for final review/acception? Please remove the "(WIP)" from the task title then, otherwise no-one will have a look again,

rjvbb updated this revision to Diff 36765.Jun 27 2018, 1:53 PM

refreshed/rebased.

rjvbb retitled this revision from platform-specific document switcher default shortcuts (WIP) to platform-specific document switcher default shortcuts.Jun 27 2018, 1:55 PM
rjvbb edited the test plan for this revision. (Show Details)
rjvbb set the repository for this revision to R32 KDevelop.
kfunk accepted this revision.Jul 2 2018, 8:30 AM
This revision is now accepted and ready to land.Jul 2 2018, 8:30 AM
kossebau added a comment.Jul 22 2018, 3:38 PM

@rjvbb Are you able to push this soon, or want someone to help out?

rjvbb added a comment.Jul 22 2018, 4:05 PM

I'm just waiting to know what branch to push to. I'm using it in 5.2.x so technically it could go there.

kossebau added a subscriber: brauch.Jul 25 2018, 3:09 PM
In D6060#295873, @rjvbb wrote:

I'm just waiting to know what branch to push to. I'm using it in 5.2.x so technically it could go there.

From what I understood this fixes a bug on macOS and makes the shprtcut option work there (i.e. change them to some which are not eaten by the system for something else).

So bug fix -> 5.2 branch. Otherwise master.

But I look at @kfunk @brauch for saying the final word :)

kossebau accepted this revision.Jul 29 2018, 10:55 PM

So let's have this patch as bug fix in the 5.2 branch.. Please push, I take the blame :)

Closed by commit R32:3934fcea209e: Use platform-specific default shortcuts in the document switcher (authored by rjvbb). · Explain WhyJul 30 2018, 8:00 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 38750

View Options

plugins/documentswitcher/documentswitcherplugin.cpp

Loading...