Review Request 129663: Don't break accelerators in KToolBar

[David Faure]

> On Feb. 12, 2017, 8:13 a.m., David Faure wrote:
> > "Qt doesn't do this" = Qt doesn't strip '&' from action texts in toolbars? I can't confirm that.
> > 
> > With this patch and my kxmlgui patch reverted, I get "Open File (F)" and "Print (P)" in the konqueror toolbar, and Alt+F opens the file menu and Alt+P does nothing. Accelerators do not work.
> > 
> > I see what you mean with the Control button in dolphin, but that is a bit different, it's a QToolButton created by code directly, not a QAction.
> > 
> > So I stand by what I said: Qt strips '&' from *action* texts in toolbars (and it happens in QAction::iconText).
> 
> Martin Tobias Holmedahl Sandsmark wrote:
>     I'm very confused. Why does the accelerator for "Sort By" show up and work here? https://iskrembilen.com/screenshots/dolphinaccel.png
>     
>     And even with my patch here, no accelerators show up for anything in Konqueror for me. Which patch by you did you revert?

I'm just as confused ;)  I edited toolbars in dolphin to add "Sort by" to the toolbar, and even when pressing Alt, there is no accelerator shown in the toolbar.

Qt 5.8 from git, dolphin 16.12.2 from current Applications/16.12 branch, breeze widget style from Plasma/5.9 branch.

qt_strippedText is in qaction.cpp since Qt 5.0 so surely we both have it. How is it possible that in your case QAction::iconText doesn't call qt_strippedText ? How about we compare debug output ?

After applying this patch to Qt http://www.davidfaure.fr/2017/qaction.cpp.diff
if I run `dolphin |& grep sort` I get

QAction::iconText: ICON "sort" stripping "Sort By" => "Sort By"
QAction::iconText: ICON "sort" stripping "&Sort By" => "Sort By"

Do you get the same?

(this happens twice because KAcceleratorManager adds the '&' at runtime; but it gets stripped anyhow in iconText(), clearly)

QToolButton::setDefaultAction is what calls iconText() in order to set the text on the toolbutton.
qtbase commit a09b41b changed QToolButton::setDefaultAction, but that is in 5.5.1 and later, surely you have this commit too?

The revert I mentionned wasn't in kxmlgui (oops) but my suggested fix in kwidgetaddons.


- David


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


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/20170226/32e48b6e/attachment.html>