> 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 > >
