> On Jan. 29, 2017, 7:32 p.m., David Faure wrote:
> > Actually, Alt+letter for actions in toolbars doesn't work anymore. I
> > *think* it worked long ago, when e.g. konqueror's toolbars had
> > &Location: [URL LineEdit]
> >
> > But re-reading the comment, that is exactly the point: no accel feature in
> > toolbars so Qt *strips* '&' from action texts in toolbars, that's the
> > "default removal of ampersand" the comment is talking about. The same
> > action might have a working accelerator for the case where it's used in a
> > menu, and that accelerator needs to be stripped.
> > The (broken) code extends that to CJK style accelerators.
> >
> > Testcase:
> > m_paPrint->setText("Print... (&P)");
> > in konqmainwindow.cpp:3467
> >
> > Err, interesting, this feature (the CJK accel removal) is broken (maybe
> > since the KAction=>QAction port), because act->iconText() calls
> > qt_strippedText(text()) internally which strips '&' before
> > KLocalizedString::removeAcceleratorMarker can see it.
> >
> > With QAction defaulting to iconText in text and defaulting to text in
> > iconText, it seems to be pretty hard to inject our own accelerator-removal
> > code...
> >
> > I tried to improve this code to look at both text and iconText and find out
> > if iconText was generated, but even getting this right doesn't help. The
> > code works the first time, then as you say KAcceleratorManager comes in an
> > sets a '&' elsewhere, breaking the (&P) detection logic.
> > KAcceleratorManager::setNoAccel(tb) does not help because it finds the
> > action from the menu and updates the action text and then this affects the
> > QToolButton via QEvent::ActionChanged. We could filter that out but what
> > about when the application really wants to change the text...
> >
> > This is all quite complicated. Ideally qt_strippedText would be taught
> > about the "(&P)" case and then none of this would be necessary.
> >
> > The second best solution I can think of is for KAcceleratorManager to take
> > care of this, that way it won't conflict with itself.
> > KWidgetAddons is a tier 1 though so I can't call KLocalizedString from
> > there, I have to duplicate the whole function.
> > Apart from that, it appears to work.
> > http://www.davidfaure.fr/2017/0001-KAcceleratorManager-set-icon-text-on-actions-to-remo.patch
> > (in addition to the patch in this review, obviously).
> > Thoughts?
> >
> > (why do I let myself get into full debugging and fixing of the issue when I
> > just wanted to post a comment initially? :-)
> But re-reading the comment, that is exactly the point: no accel feature in
> toolbars so Qt *strips* '&' from action texts in toolbars,
But my point is that Qt doesn't do this, and accelerators work just fine in
toolbars here with this patch. I like accelerators in toolbars, especially in
Dolphin where the Control button in the toolbar already has an accelerator so
without this patch it is just very inconsistent.
- Martin Tobias Holmedahl
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129663/#review102317
-----------------------------------------------------------
On Dec. 17, 2016, 12:23 p.m., Martin Tobias Holmedahl Sandsmark wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129663/
> -----------------------------------------------------------
>
> (Updated Dec. 17, 2016, 12:23 p.m.)
>
>
> Review request for KDE Frameworks, David Faure and Chusslove Illich.
>
>
> Repository: kxmlgui
>
>
> Description
> -------
>
> Don't try to strip out accelerators in the KToolBar event handler. It makes
> no sense to me, potentially creates an endless repaint loop and fights with
> KAcceleratorManager which will constantly re-add accelerators.
>
>
> Diffs
> -----
>
> src/ktoolbar.cpp 31be9b0
>
> Diff: https://git.reviewboard.kde.org/r/129663/diff/
>
>
> Testing
> -------
>
> With this patch not only the control button in Dolphin has an accelerator.
>
>
> Thanks,
>
> Martin Tobias Holmedahl Sandsmark
>
>
