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

[Mark Gaiser]

> On May 9, 2012, 8:53 p.m., Michael Jansen wrote:
> > 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

Well.. that sucks!

Anyway, this patch is the first part. It doesn't break any backwards compatibility and simply allows apps that want to use more shortcuts to freely use them.
The second patch would be against Dolphin to allow some more shortcuts. I already have the dolphin patch ready.

After that the next patch would be to get the Shortcut Editor to support this as well. I don't have a patch for that, yet!

>From a quick look at KShortcut in frameworks it seems to be just the same as the current 4.8 branch. Just a bunch of TODO items for the constructors to use QShortcut. KShortcut doesn't seem to be going away.

I would like to push this one and fix dolphin and the shortcut editor if i'm allowed to.


If that's not oke, then please do tell me how to fix this the proper way. This is what i would like to start doing if it's not oke to push:
Step 1. KAction to inherit QAction and add the global shortcut stuff. Perhaps some more stuff.
Step 2. Rewrite KShortcut to inherit QShortcut and ONLY have the additional option for global shortcuts.
Step 3. Adjust the Shortcut Editor to use the new structure.
Step 4. Deprecate most of KAction in the 4.8 kdelibs branch.
Step 5. Deprecate all of KShortcut except the global related stuff

Your input would be welcome.

-- little braindump --
It would be very nice to have a KDE "KInputShortcut" class in which you can mix shortcuts from various input devices. For example: KInputShortcut(Qt::CTRL + Qt::LeftButton) .. That would really add something useful!


- Mark


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


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