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

[Mark Gaiser]

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

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?


> On May 10, 2012, 8:38 a.m., David Faure wrote:
> > kdeui/shortcuts/kshortcut.h, line 231
> > <http://git.reviewboard.kde.org/r/104801/diff/2/?file=62031#file62031line231>
> >
> >     Missing @since (same on the next method)

Will do.


> On May 10, 2012, 8:38 a.m., David Faure wrote:
> > kdeui/actions/kaction.h, line 316
> > <http://git.reviewboard.kde.org/r/104801/diff/2/?file=62029#file62029line316>
> >
> >     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.

True, deprecating it is not needed anymore.


- Mark


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