Review Request 129663: Don't break accelerators in KToolBar

[Martin Tobias Holmedahl Sandsmark]

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20170211/1dbee96f/attachment.html>