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

[David Faure]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/104801/#review13648
-----------------------------------------------------------


As documented, the shortcut(types) method allows to retrieve either the current shortcuts (like most other methods), or the *default* shortcuts, i.e. those that were set by the application developer initially, before the user could make changes.

This is done using dynamic properties on the action, so that the configuration dialog can handle both QActions and KActions, IIRC.
So it sounds to me like you'll have to change these dynamic properties, from "defaultPrimaryShortcut"/"defaultSecondaryShortcut" to a single property containing a list of default shortcuts.

I just saw your reply about plans for cleaning this up in kdelibs frameworks -- this would be MORE than welcome.
The only plan right now is http://article.gmane.org/gmane.comp.kde.devel.frameworks/350  but I think you have more details on the shortcut handling itself.

Ideally, I would like the handling of default shortcuts to go into Qt itself, so that we don't need KAction anymore in the simple case of a non-global action, and still have configurable shortcuts. Anyway, let's discuss this further on kde-frameworks-devel.

I agree with Michael Jansen that this patch alone isn't enough, but if you also plan to improve the shortcuts editor to support more than 2 alternates, and then look into Qt5/KF5, then I support this patch, as a first step in the right direction.


kdeui/actions/kaction.h
<http://git.reviewboard.kde.org/r/104801/#comment10829>

    Don't deprecate the method, it's still very much needed, for default shortcuts. So IMHO merge it with shortcut() again, i.e. remove the other method, the KDE_DEPRECATED, and put back the default argument.



kdeui/shortcuts/kshortcut.h
<http://git.reviewboard.kde.org/r/104801/#comment10830>

    Missing @since (same on the next method)


- David Faure


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/20120510/1d443472/attachment.htm>