Re-arrange selection tool order and shortcuts to reflect new arrangement
Needs RevisionPublic

Authored by ngraham on Jun 6 2019, 7:27 PM.

Details

Reviewers
ndavis
aacid
Group Reviewers
Okular
VDG
Summary

D21624 re-arranged the ordering of the selection tools in the toolbar menu. However
this made it inconsistent with the order in the menu, and now their keyboard
shortcuts no longer nicely increase.

This patch fixes that. Note that it does involve and necessitate a shortcut switch:
Text Selection is now Ctrl+3, and Area Selection is now Ctrl+4.

Test Plan

rm ~/.config/okularpartrc

In toolbar button:

In Menu:

Diff Detail

Repository
R223 Okular
Branch
re-arrange-shortcuts-and-menu-items-accordingly (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 12498
Build 12516: arc lint + arc unit
ngraham created this revision.Jun 6 2019, 7:27 PM
Restricted Application added a project: Okular. · View Herald TranscriptJun 6 2019, 7:27 PM
Restricted Application added a subscriber: okular-devel. · View Herald Transcript
ngraham requested review of this revision.Jun 6 2019, 7:27 PM
ndavis accepted this revision.Jun 6 2019, 7:32 PM
This revision is now accepted and ready to land.Jun 6 2019, 7:32 PM

I am a little hesitant to do this since changing shortcuts is always a bit controversial and we'd be resetting people's muscle memory for these tools. :/

But I don't see any way around it unless we revert D21624 and find a different way to make the toolbar button show the Text Selection tool by default that doesn't involve re-arranging its order in the pop-up.

aacid requested changes to this revision.Jun 6 2019, 8:45 PM
aacid added a subscriber: aacid.

*Do not change the shortcuts*

This revision now requires changes to proceed.Jun 6 2019, 8:45 PM

*Do not change the shortcuts*

In that case perhaps we should revert D21624 and then find another way to make the toolbar selection menu button show Text Selection by default, which was the original goal. Do you have any ideas about that?

aacid added a comment.Jun 6 2019, 10:30 PM

But we're setting the menu here no?

ac->setDefaultShortcut(d->aMouseTextSelect, Qt::CTRL + Qt::Key_3);

Can it just be still be a 4?

But we're setting the menu here no?

ac->setDefaultShortcut(d->aMouseTextSelect, Qt::CTRL + Qt::Key_3);

Can it just be still be a 4?

The current keyboard shortcuts aren't a problem if we don't re-arrange the items in the Selection Mode toolbar button. I only re-arranged them so that Text Selection could be the default item shown in the button when you first open Okular. If there is an alternative method to do that, then we don't have to re-arrange the items, and we don't have to change any shortcuts. Does that make sense?

The current keyboard shortcuts aren't a problem if we don't re-arrange the items in the Selection Mode toolbar button.

Is it important to have a 3-4-5 order in that button menu? The other 3 shortcuts are not shown in the toolbar, so to learn the shortcuts it is neccessary to look into the menu. And there, the 1-2-3-4-5-6 order is a big aid to remember them.

Additionally, a 3-4-5 order does not start at 1, which makes it harder to recognize as order.

So, I like D21624, but can live without this.

aacid added a comment.Wed, Aug 7, 9:10 PM

Personally I don't really about the button order, what i care is about the shortcuts i've been using for 10 years not changing