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

[David Faure]

> On May 10, 2012, 8:38 a.m., David Faure wrote:
> > 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.
> 
> Mark Gaiser wrote:
>     Yes, i am going to improve the shortcut editor for this. The shortcut editor should then simply list all shortcuts instead of primary/alternate. Just "Shortcuts". I'm not sure yet how i'm going to display that.
>     
>     Lets for a second assume that i'm going to clean this up for frameworks (quite likely). Would we deprecate the primary/alternate stuff? If so, where would we deprecate it? In KDE 4.9? 4.10?

Frameworks is 5.0.

I think we wouldn't deprecate individual methods. Ideally we would deprecate all of KShortcut, if we can ensure that QShortcut (or rather QAction and its list of QKeySequences, it seems?) has everything we need. Again, let's discuss that on k-f-d rather than here ;)


- David


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


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/a45c4f6e/attachment.htm>