Differential D7964

KAcceleratorManager: set icon text on actions to remove CJK markers
ClosedPublic
Actions

Authored by dfaure on Sep 24 2017, 8:44 AM.

Details

Reviewers
mardelle
ilic
sandsmark
Commits
R236:c6191993c7e5: KAcceleratorManager: set icon text on actions to remove CJK markers
Summary

This replaces the KToolBar event filter hack to solve the same issue:
when an action text appears in a menu we want the & accelerator, while in
toolbars wewant that removed. Qt takes care of it, except for the more tricky
case of CJK markers: "<chinese here> (&O)" where &O exists only to get an ascii
accelerator.
Instead of hacking the text at painting time (!) it's much more robust to
remove " (&O)" from action texts and sett hat as the icon text upfront.

With this in, we can remove the KToolBar hack which leads to endless repaints.
BUG: 365050
BUG: 377859

Test Plan

Unittest

Diff Detail

Repository
R236 KWidgetsAddons
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
dfaure created this revision.Sep 24 2017, 8:44 AM
mardelle added a comment.Sep 29 2017, 6:22 AM

Not sure how to test this accelerator thing properly for a review but I would really love to get rid of the ktoolbar hack . Can we get this patch in time for frameworks 5.39 release, or should I commit my workaround D7954 as a temporary fix? Thanks

dfaure added a comment.Oct 7 2017, 4:26 PM

Is there a chance anyone is going to review this, or should I push it on my own?

dfaure added a comment.Oct 7 2017, 4:27 PM

Actually, we need one of the two fixes in 5.39, and I'm more confident in this one than in the old paintEvent hack, even reworked... I'll push this.

Closed by commit R236:c6191993c7e5: KAcceleratorManager: set icon text on actions to remove CJK markers (authored by dfaure). · Explain WhyOct 7 2017, 4:29 PM
This revision was automatically updated to reflect the committed changes.
pino added a subscriber: pino.Oct 7 2017, 5:03 PM

I think you can use the public KLocalizedString::removeAcceleratorMarker(), instead of copying common_helpers...

dfaure added a comment.Oct 7 2017, 5:05 PM
In D7964#153093, @pino wrote:

I think you can use the public KLocalizedString::removeAcceleratorMarker(), instead of copying common_helpers...

KWidgetAddons is tier1, it doesn't depend on ki18n so it can't call KLocalizedString...

Revision Contents
Changeset List

Diff 20451

View Options

autotests/kacceleratormanagertest.cpp

Loading...
View Options

src/CMakeLists.txt

Loading...
View Options

src/common_helpers.cpp

Loading...
View Options

src/common_helpers_p.h

Loading...
View Options

src/kacceleratormanager.cpp

Loading...