Differential D21944

Show shortcut in menu item when specified
ClosedPublic
Actions

Authored by astippich on Jun 20 2019, 5:23 PM.

Details

Reviewers
mart
ngraham
apol
Commits
R858:721c348d1a1e: Show shortcut in menu item when specified
Summary

Show the shortcut as text for menu items when
they are given via an action

BUG: 405541

Diff Detail

Repository
R858 Qt Quick Controls 2: Desktop Style
Branch
menu_item
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 13139
Build 13157: arc lint + arc unit
astippich created this revision.Jun 20 2019, 5:23 PM
Restricted Application added a project: Plasma. · View Herald TranscriptJun 20 2019, 5:23 PM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
astippich requested review of this revision.Jun 20 2019, 5:23 PM
astippich added a comment.Jun 20 2019, 5:27 PM

This is required to port the menu of Elisa to qqc2 (D21943)

qqc1 version

qqc2 version without this patch

qqc2 with this patch

astippich edited the summary of this revision. (Show Details)Jun 20 2019, 5:38 PM
ngraham accepted this revision.Jun 20 2019, 9:58 PM

Excellent. Looks like a sane way to do it.

This revision is now accepted and ready to land.Jun 20 2019, 9:58 PM
apol added a subscriber: apol.Jun 20 2019, 11:35 PM
apol added inline comments.
org.kde.desktop/MenuItem.qml
98

label.color?

103

Use Layout.rightMargin on the shortcut object.
https://doc.qt.io/qt-5/qml-qtquick-layouts-layout.html#rightMargin-attached-prop

apol requested changes to this revision.Jun 20 2019, 11:35 PM
This revision now requires changes to proceed.Jun 20 2019, 11:35 PM
astippich updated this revision to Diff 60300.Jun 22 2019, 10:03 AM
  • simplify
astippich added a comment.Jun 22 2019, 10:04 AM

The menu separator now does not look correct:


I don't know why

ngraham added inline comments.Jun 22 2019, 10:06 AM
org.kde.desktop/MenuItem.qml
103

I think the original approach to use an Item was correct, both because now it looks wrong, but also because using an Item makes the layout automatically RTL compatible, while setting the right padding property requires conditional handling for the reversed case.

astippich updated this revision to Diff 60332.Jun 22 2019, 11:36 AM
  • use item for margin again
astippich added a comment.Jun 22 2019, 11:38 AM

Unfortunately, even with this reverted change I know get a wrong menu separator. I don't know what was different before...

Since it was also there without this patch (see screenshot), it is probably a different issue which needs investigation

astippich marked 2 inline comments as done.Jun 24 2019, 3:26 PM
ngraham added a comment.Jun 28 2019, 2:30 PM

@apol, is this good to go now?

apol accepted this revision.Jun 28 2019, 5:53 PM
This revision is now accepted and ready to land.Jun 28 2019, 5:53 PM
Closed by commit R858:721c348d1a1e: Show shortcut in menu item when specified (authored by astippich). · Explain WhyJun 29 2019, 9:49 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Morg.kde.desktop/MenuItem.qml (14 lines)

Diff 60332

View Options

org.kde.desktop/MenuItem.qml

Loading...