Show assigned shortcut in drawer action delegates
Needs RevisionPublic

Authored by hein on Feb 12 2019, 12:11 AM.



This is a big UI change, I'm not sure if it shouldn't be optional in
some way. Some apps have set tooltips on their actions in lieu of
this, and they will get two shortcuts displays this way.

Diff Detail

R169 Kirigami
No Linters Available
No Unit Test Coverage
Build Status
Buildable 8175
Build 8193: arc lint + arc unit
hein created this revision.Feb 12 2019, 12:11 AM
Restricted Application added a project: Kirigami. · View Herald TranscriptFeb 12 2019, 12:11 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
hein requested review of this revision.Feb 12 2019, 12:11 AM
hein added a comment.Feb 12 2019, 12:13 AM

Screenshot showing ellision:

@mart wanted me to add this to the desktop style's MenuItem as well, but I found no way to access the action data there and QQC2.MenuItem has no way to set a shortcut. Note that using the mnemonic (as @mart wanted on Plasma) would be wrong, that's redundant.

abetts added a subscriber: abetts.Feb 12 2019, 12:14 AM
abetts removed a subscriber: abetts.
hein updated this revision to Diff 51454.Feb 12 2019, 12:26 AM

Fix typo.

apol added a comment.Feb 12 2019, 1:07 AM

Feels a bit in-your-face to me, it can make sense for this application bug for e.g. Discover it would be a bit overwhelming.

I'd prefer keeping it in the tooltip for something generic.


this doesn't anchor to the text, won't the text be going over the shortcut if it's too long?

This makes sense when the drawer is being used as a sort of slide-in context menu. But in Discover for example, we make drawer be always visible (on desktop at least) and abuse it to hold navigation elements.

Optional but on-by-default-for-actions would make sense to me if we can optimize the presentation.

hein added inline comments.Feb 12 2019, 6:25 AM

It's a layout (you don't anchor in them) ... did you see the screenshot and description?

mart requested changes to this revision.Feb 18 2019, 1:42 PM
mart added inline comments.

adding also && root.modal should fix the discover and other always-showing-sidebar apps use case.

Pretty sure the label will break on collapsed mode.
did you try on kirigami gallery to set the sidebar to collapsible with this patch on?

should probably be

visible: modelData.__shortcut.enabled && !Settings.tabletMode && root.modal && !root.collapsible

This revision now requires changes to proceed.Feb 18 2019, 1:42 PM