Review Request 129663: Don't break accelerators in KToolBar

[Martin Tobias Holmedahl Sandsmark]

> On Dec. 17, 2016, 11:24 p.m., David Faure wrote:
> > I agree that doing this in Show is far too late - and that KAcceleratorManager needs to be told, to avoid the infinite loop.
> > 
> > However the reason for this code still holds I think, so it seems to me that it needs to be improved instead of removed.
> > 
> > => this should be done in Polish (the event, not the language :-) ) and KAcceleratorManager::setNoAccel(tb) should prevent the recursion.
> > 
> > (... and the no-op case (nothing to remove) shouldn't trigger anything... How come tb->setText(tb->text()) does anything? Or are you seeing this bug with CJK languages only?)
> 
> Martin Tobias Holmedahl Sandsmark wrote:
>     but what is the actual reason for stripping accelerators? some stuff in toolbars still get accelerators, just inconsistently.
> 
> David Faure wrote:
>     The reason is explained by the comment in the code...

The comment only explains why it removes the parenthesis construct thing, not why it removes the accelerators in the first place, unless I'm missing something?


- Martin Tobias Holmedahl


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


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/20170114/eebaf677/attachment.html>