----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/104801/#review13624 -----------------------------------------------------------
Hate to break the news to you after all that work. But the exact same thing you have achieved with this patch could be achieved by using a QAction for the missing Shortcut. Or a KAction for that matter as long as it is not part of the KActionCollection. There is only one reason to use KAction. The Shortcuts Editor. And KXMLwhatever. And global shortcuts. So make that there are only three reasons to use KAction. None of them know how to handle a KAction with three Shortcuts set. So your patch has not (yet?) achieved anything you could not gain by just using a hidden unconfigurable second Q(K)Action. So i would say it does not make sense to apply it in its current form. Unless you are willing to go all the way which you only should do after finding out what the frameworks branchs does to kaction. So you effort is not thrown away in the near / middle future. Mike - Michael Jansen On May 9, 2012, 6:21 p.m., Mark Gaiser wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/104801/ > ----------------------------------------------------------- > > (Updated May 9, 2012, 6:21 p.m.) > > > Review request for kdelibs. > > > Description > ------- > > So i was trying to fix this bug: https://bugs.kde.org/show_bug.cgi?id=181531 > That only asked for one more shortcut. That issue seems to be a little more > complicated than it looks. Till this point KActions could only have a > "Primary" and a "Alternate" shortcut. 2 in total which is - in some > situations - not enough. > > I fixed this by roughly restructuring nearly all of the KShortcut cpp file. > > The only thing i'm not quite sure about is how "KShortcut > KAction::shortcut(ShortcutTypes type) const" looks right now.. If anyone has > some clarification on that one..? > > > Diffs > ----- > > kdeui/actions/kaction.h d877554 > kdeui/actions/kaction.cpp 309cf82 > kdeui/shortcuts/kshortcut.h c720830 > kdeui/shortcuts/kshortcut.cpp e307ab0 > > Diff: http://git.reviewboard.kde.org/r/104801/diff/ > > > Testing > ------- > > I tested this by adding the missing bindings for Dolphin's back/forward and > it seems to be working just fine. I can use all available shortcuts. > > > Thanks, > > Mark Gaiser > >
