<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/129663/">https://git.reviewboard.kde.org/r/129663/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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? :-)</pre>
<br />
<p>- David Faure</p>
<br />
<p>On December 17th, 2016, 12:23 p.m. UTC, Martin Tobias Holmedahl Sandsmark wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for KDE Frameworks, David Faure and Chusslove Illich.</div>
<div>By Martin Tobias Holmedahl Sandsmark.</div>
<p style="color: grey;"><i>Updated Dec. 17, 2016, 12:23 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kxmlgui
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">With this patch not only the control button in Dolphin has an accelerator.</p></pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>src/ktoolbar.cpp <span style="color: grey">(31be9b0)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/129663/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>