> 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.
The reason is explained by the comment in the code... - David ----------------------------------------------------------- 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 > >
