Add actions for switching to a specific tab
ClosedPublic

Authored by alexmi on Oct 1 2019, 10:48 PM.

Details

Summary

Add actions to switch to each of the first 10 tabs. By default these
have no assigned shortcuts, they can be configured from the usual
Configure Shortcuts dialog.

This feature makes it much easier to quickly switch between tabs just
like you normally would be able to when using a web browser or other
applications.

Test Plan

Compile, assign shortcuts via the Configure Shortcuts dialog, verify
tab switching works as expected.

Diff Detail

Repository
R318 Dolphin
Branch
feature-tab-shortcuts (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 17372
Build 17390: arc lint + arc unit
alexmi created this revision.Oct 1 2019, 10:48 PM
Restricted Application added a project: Dolphin. · View Herald TranscriptOct 1 2019, 10:48 PM
Restricted Application added a subscriber: kfm-devel. · View Herald Transcript
alexmi requested review of this revision.Oct 1 2019, 10:48 PM
This revision is now accepted and ready to land.Oct 2 2019, 5:10 PM

Why not assign a default shortcut? Alt + i seems to be the standard pattern for this (tried with Firefox and Chrome).

Why not assign a default shortcut? Alt + i seems to be the standard pattern for this (tried with Firefox and Chrome).

+1

alexmi updated this revision to Diff 67230.Oct 2 2019, 10:38 PM
  • Add default shortcuts for the first 10 tabs
alexmi updated this revision to Diff 67231.Oct 2 2019, 11:05 PM

GIT_SILENT Make numberForShortcut const

meven added a subscriber: meven.Oct 3 2019, 9:10 AM
meven added inline comments.
src/dolphinmainwindow.cpp
1513

Simplier written with one check instead of two (i is < MaxActivateTabShortcuts) :

int numberForShortcut;
if (i == 9) {
     // tenth
     numberForShortcut = 0;
} else {
    numberForShortcut = i + 1;
}
alexmi added inline comments.Oct 3 2019, 1:02 PM
src/dolphinmainwindow.cpp
1513

The issue with doing that is that if MaxActivateTabShortcuts ever gets changed to anything higher than 10, then there would be extra calls to try to set shortcuts like Alt + 11.
I would say it's unlikely it ever gets changed to something higher than 10 but still.

Even when changing that else to an else if, you would still have to add another check somehow to make sure you're not calling setDefaultShortcut when i > 10.

I don't foresee ever adding shortcuts for more than 10 tabs. Even web browsers--which not infrequently host dozens of tabs--do that.

meven added inline comments.Oct 3 2019, 2:26 PM
src/dolphinmainwindow.cpp
1513

Fair enough I didn't get MaxActivateTabShortcuts could be changed.

elvisangelaccio requested changes to this revision.Oct 3 2019, 8:32 PM

This is what I get in the shortcuts dialog:

Possibly related to this warning:

KLocalizedStringPrivate::substituteSimple(): "0 instead of 1 arguments to message {Activate Tab %1} supplied before conversion
This revision now requires changes to proceed.Oct 3 2019, 8:32 PM
alexmi updated this revision to Diff 67295.Oct 4 2019, 1:38 AM
  • Fix bad argument substitution in a i18nc() call
alexmi added a comment.Oct 4 2019, 1:41 AM

This is what I get in the shortcuts dialog:

Possibly related to this warning:

KLocalizedStringPrivate::substituteSimple(): "0 instead of 1 arguments to message {Activate Tab %1} supplied before conversion

My bad, I think I know what happened. i18nc returns a QString and I passed an argument to that instead of directly passing it to the i18nc function.

Oddly enough I could never manage to reproduce the bug after trying a few different things. So let me know if it's fixed now.

ngraham accepted this revision.Oct 5 2019, 10:56 PM

Yep, looks fixed now!

Yep, the i18n issue if fixed now.

One last thing: neither Firefox nor Chrome use Alt + 0 for the tenth tab, so I think we should stop at Alt + 9.

Bonus point if you manage to open the last open tab at at Alt + 9 keypress, like both Firefox and Chrome do ;)

alexmi added a comment.Oct 6 2019, 7:21 PM

As far as I know "Switch to tab n" shortcuts are not identical between applications and vary quite a bit. Some applications use 0-9, some use 1-9. Some have a "Switch to last tab" shortcut assigned to 0 or 9. Some use Ctrl, some use Alt instead or even both.
In the case of web browsers I would assume they don't use 0 because Ctrl + 0 resets the zoom level, and Firefox on Windows uses Ctrl instead of Alt, Chrome uses both Ctrl and Alt I think but I don't use Chrome.

I'd rather leave things as they are right now, but I'm not against those changes either. IMO most alternatives are reasonable in this case.
@ngraham Thoughts?

I think Alt+0 for the tenth tab is fine, but maybe we should have it switch to the last tab instead. I doubt it's that common for people to have ten tabs open in Dolphin!

alexmi updated this revision to Diff 67397.Oct 6 2019, 10:48 PM
  • Add action to switch to the last tab
ngraham accepted this revision.Oct 6 2019, 11:37 PM
elvisangelaccio accepted this revision.Oct 13 2019, 2:43 PM
This revision is now accepted and ready to land.Oct 13 2019, 2:43 PM
This revision was automatically updated to reflect the committed changes.