Review Request 129663: Don't break accelerators in KToolBar

[David Faure]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/129663/#review102317
-----------------------------------------------------------



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? :-)

- David Faure


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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20170129/2e285351/attachment-0001.html>