Add new standard KDE tab switching shortcuts (ctrl+pgup/pgdn)
ClosedPublic

Authored by ngraham on Aug 16 2017, 5:41 PM.

Details

Summary

BUG: 383603

Add the new KDE standard tab switching shortcuts that were introduced in https://phabricator.kde.org/R237:d85a57645f94d2d087711d7c608a0c5a46ed7ede.

Test Plan

Built and tested with current git master in up-to-date KDE Neon. Expected behavior is seen (default shortcuts have become shift+left/right and cmd+pgup/pgdn).

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
ngraham created this revision.Aug 16 2017, 5:41 PM
Restricted Application added a project: Konsole. · View Herald TranscriptAug 16 2017, 5:41 PM
Restricted Application added a subscriber: Konsole. · View Herald Transcript
aacid added a subscriber: aacid.Aug 16 2017, 8:47 PM

-1 i don't see why you should change shortcuts that people are already used to.

ngraham added a comment.EditedAug 16 2017, 8:52 PM

I understand your concern, Albert. But it seems that the decision to change the tab switching shortcuts throughout all KDE apps has already been made (not by me), in https://phabricator.kde.org/R237:d85a57645f94d2d087711d7c608a0c5a46ed7ede.

All this does is make Konsole consistent with the new shortcut, because Konsole doesn't implement KStandardShortcut::tabPrev() and KStandardShortcut::tabNext(). If it did, then it would have gotten up the change automatically, no new commits required.

Anyway these shortcuts are all user-changeable, so it's not like in the GTK world where what we decide to do may irritate some users forever with no recourse.

aacid added a comment.Aug 16 2017, 9:14 PM

I already complained there, and will complain here too.

Anyhow there's magic that allows for two shortcuts, if you really need Ctrl based shortcuts, you can add them as secondary shortcuts and not break people that are used to Shift.

Good point. It looks like setDefaultShortcut() can take a QList of shortcuts instead of just a single one, which will add the new shortcut without blowing away the current one. This works since Konsole currently has no secondary shortcut, so we wouldn't be breaking anybody's secondary shortcut muscle memory.

ngraham updated this revision to Diff 18258.Aug 16 2017, 10:08 PM

Appended the new shortcut rather than replacing the existing one

ngraham updated this revision to Diff 18411.Aug 19 2017, 8:53 PM
ngraham edited the test plan for this revision. (Show Details)

use correct method (setDefaultShortcuts)

ngraham retitled this revision from Use standard KDE ctrl+pgup/pgdn shortcuts for switching tabs to Add new standard KDE tab switching shortcuts (ctrl+pgup/pgdn).
ngraham edited the test plan for this revision. (Show Details)
ngraham edited the summary of this revision. (Show Details)Aug 20 2017, 9:39 PM
ngraham edited the test plan for this revision. (Show Details)

Konsole is a special case when it comes to shortcuts.

Terminal apps use ctrl+whatever themselves and obviously konsole can't use the same.

For example if copy was control+c it would conflict with being able to exit an app.

That's true, but ctrl-pgup/pgdn don't emit any special signals, do they? I proposed this change only because it doesn't seem to conflict with anything (I wouldn't have done it if we decided on ctrl-c or ctrl-x for tab switching :) ).

dfaure added a subscriber: dfaure.Aug 23 2017, 9:58 PM

+1 from me, the change as it is now is pretty harmless (I mean, not dangerous), since it doesn't remove any previously known shortcut, and I find it extremely unlikely that a text console application would be using Ctrl+PgUp/PgDown itself.

src/ViewManager.cpp
253–254

could be written in a much shorter way:

const QList<QKeySequence> previousViewActionKeys{Qt::SHIFT + Qt::Key_Left, Qt::CTRL + Qt::KeyPageUp};
hindenburg edited edge metadata.Aug 26 2017, 11:21 PM

I don't have any strong objection - I didn't realize browsers use this shortcut - please use David's shorter code

ngraham updated this revision to Diff 18852.Aug 27 2017, 4:30 PM

Updated the diff to use more concise syntax.

dfaure accepted this revision.Aug 27 2017, 7:01 PM
This revision is now accepted and ready to land.Aug 27 2017, 7:01 PM
ngraham marked an inline comment as done.Aug 27 2017, 7:03 PM

Thanks David! Do I need do do anything else, or will someone else commit this? I don't have commit access.

aacid closed this revision.Aug 27 2017, 11:01 PM

Thanks Albert! Much appreciated.