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...
> 
> Martin Tobias Holmedahl Sandsmark wrote:
>     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?
> 
> Albert Astals Cid wrote:
>     What do accelerators would do in a KToolbar? You can't trigger them at all, no? That's why they are removed afaics
> 
> Martin Tobias Holmedahl Sandsmark wrote:
>     They can be triggered.
>     
>     The case I want to fix, with this patch: https://iskrembilen.com/screenshots/dolphinaccel.png
>     
>     The "Sort by" is not possible to explicitly assign a shortcut, but it gets an accelerator which can be triggered with this patch. Without this patch the accelerator for "Sort by" gets constantly added and removed and is not possible to trigger.
>     
>     FWIW, the accelerator for "Control" does not get stripped by KToolBar, so the current state is a bit inconsistent.
> 
> Albert Astals Cid wrote:
>     > They can be triggered.
>     
>     You mean that pressing Alt+Something will trigger an item with _ in the toolbar? That feels really weird tbh.
> 
> Martin Tobias Holmedahl Sandsmark wrote:
>     > You mean that pressing Alt+Something will trigger an item with _ in the toolbar?
>     
>     Yas.

ping; yay or nay?


- 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/20170129/1b68cd18/attachment.html>