D6060: platform-specific document switcher default shortcuts (WIP)

[René J.V. Bertin]

rjvbb created this revision.
rjvbb added a project: KDevelop.
Restricted Application added a subscriber: kdevelop-devel.

REVISION 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 and the Cocoa as well as the XCB QPA plugins.

The patch applies to the 5.1 branch.

REPOSITORY
R33 KDevPlatform

REVISION DETAIL
https://phabricator.kde.org/D6060

AFFECTED FILES
plugins/documentswitcher/documentswitcherplugin.cpp

To: rjvbb, #kdevelop
Cc: kdevelop-devel, kde-mac, geetamc, Pilzschaf, akshaydeo, surgenight, arrowdodger

[Friedrich W. H. Kossebau]

kossebau added a comment.


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 ;)

INLINE COMMENTS

documentswitcherplugin.cpp:69
+ // attribute is set.
+ Qt::Modifier shortcutAccelerator = QCoreApplication::testAttribute(Qt::AA_MacDontSwapCtrlAndMeta) ? Qt::CTRL : Qt::ALT;
+#else

Make both shortcutAccelerator const.

REPOSITORY
R33 KDevPlatform

REVISION DETAIL
https://phabricator.kde.org/D6060

To: rjvbb, #kdevelop
Cc: kossebau, kdevelop-devel, kde-mac, geetamc, Pilzschaf, akshaydeo, surgenight, arrowdodger

[Kevin Funk]

kfunk added a comment.


LGTM, after having fixed Friedrich's remarks

REPOSITORY
R33 KDevPlatform

REVISION DETAIL
https://phabricator.kde.org/D6060

To: rjvbb, #kdevelop
Cc: kfunk, kossebau, kdevelop-devel, kde-mac, geetamc, Pilzschaf, akshaydeo, surgenight, arrowdodger

[René J.V. Bertin]

rjvbb added a comment.

Post by Friedrich W. H. Kossebau
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).

Post by Friedrich W. H. Kossebau
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.

Post by Friedrich W. H. Kossebau
No idea about the Mac shortcuts, the non-Mac code looks fine to me so far ;)

REPOSITORY
R33 KDevPlatform

REVISION DETAIL
https://phabricator.kde.org/D6060

To: rjvbb, #kdevelop
Cc: kfunk, kossebau, kdevelop-devel, kde-mac, geetamc, Pilzschaf, akshaydeo, surgenight, arrowdodger

[René J.V. Bertin]

rjvbb updated this revision to Diff 17888.
rjvbb added a comment.


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

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D6060?vs=15054&id=17888

REVISION DETAIL
https://phabricator.kde.org/D6060

AFFECTED FILES
plugins/documentswitcher/documentswitcherplugin.cpp

To: rjvbb, #kdevelop
Cc: kfunk, kossebau, kdevelop-devel, kde-mac, geetamc, Pilzschaf, akshaydeo, surgenight, arrowdodger

[René J.V. Bertin]

rjvbb set the repository for this revision to R33 KDevPlatform.

REPOSITORY
R33 KDevPlatform

REVISION DETAIL
https://phabricator.kde.org/D6060

To: rjvbb, #kdevelop
Cc: kfunk, kossebau, kdevelop-devel, kde-mac, geetamc, Pilzschaf, akshaydeo, surgenight, arrowdodger

[Friedrich W. H. Kossebau]

kossebau added a comment.


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,

REPOSITORY
R33 KDevPlatform

REVISION DETAIL
https://phabricator.kde.org/D6060

To: rjvbb, #kdevelop
Cc: kfunk, kossebau, kdevelop-devel, kde-mac, antismap, iodelay, vbspam, njensen, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd

[René J.V. Bertin]

rjvbb updated this revision to Diff 36765.
rjvbb added a comment.


refreshed/rebased.

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D6060?vs=17888&id=36765

REVISION DETAIL
https://phabricator.kde.org/D6060

AFFECTED FILES
plugins/documentswitcher/documentswitcherplugin.cpp

To: rjvbb, #kdevelop
Cc: kfunk, kossebau, kdevelop-devel, kde-mac, antismap, iodelay, vbspam, njensen, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd

[René J.V. Bertin]

rjvbb retitled this revision from "platform-specific document switcher default shortcuts (WIP)" to "platform-specific document switcher default shortcuts".
rjvbb edited the test plan for this revision.
rjvbb set the repository for this revision to R32 KDevelop.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D6060

To: rjvbb, #kdevelop
Cc: kfunk, kossebau, kdevelop-devel, kde-mac, antismap, iodelay, vbspam, njensen, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd

[Kevin Funk]

kfunk accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D6060

To: rjvbb, #kdevelop, kfunk
Cc: kfunk, kossebau, kdevelop-devel, kde-mac, antismap, iodelay, vbspam, njensen, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd

[Friedrich W. H. Kossebau]

kossebau added a comment.


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

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D6060

To: rjvbb, #kdevelop, kfunk
Cc: kfunk, kossebau, kdevelop-devel, kde-mac, antismap, iodelay, vbspam, njensen, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd

[René J.V. Bertin]

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.

[Friedrich W. H. Kossebau]

kossebau added a subscriber: brauch.
kossebau added a comment.

Post by René J.V. Bertin
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 :)

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D6060

To: rjvbb, #kdevelop, kfunk
Cc: brauch, kfunk, kossebau, kdevelop-devel, kde-mac, antismap, iodelay, vbspam, njensen, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd

[Friedrich W. H. Kossebau]

kossebau accepted this revision.
kossebau added a comment.


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

REPOSITORY
R32 KDevelop

REVISION DETAIL
https://phabricator.kde.org/D6060

To: rjvbb, #kdevelop, kfunk, kossebau
Cc: brauch, kfunk, kossebau, kdevelop-devel, kde-mac, antismap, iodelay, vbspam, njensen, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd

[René J.V. Bertin]

This revision was automatically updated to reflect the committed changes.
Closed by commit R32:3934fcea209e: Use platform-specific default shortcuts in the document switcher (authored by rjvbb).

REPOSITORY
R32 KDevelop

CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D6060?vs=36765&id=38750

REVISION DETAIL
https://phabricator.kde.org/D6060

AFFECTED FILES
plugins/documentswitcher/documentswitcherplugin.cpp

To: rjvbb, #kdevelop, kfunk, kossebau
Cc: brauch, kfunk, kossebau, kdevelop-devel, kde-mac, antismap, iodelay, vbspam, njensen, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd