Review Request: Fix KShortcut to really allow the usage of multiple shortcuts

[Michael Jansen]

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20120509/a74669dc/attachment.htm>