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

David Faure faure at kde.org
Mon Apr 30 23:19:36 BST 2012


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


I am in favour of the idea, since I was hit by this limitation in the past, too.
However I think the API changes should be more conservative, there's no need to deprecate so many of the existing methods, just because it is *possible* to define more than 2 shortcuts.

Also, Qt's QAction already has void QAction::setShortcuts(const QList<QKeySequence> &shortcuts)... remind me why we are not using that? Because it doesn't know "default shortcut" vs "current shortcut"?



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

    How does this allow multiple shortcuts per action?
    The docu is a bit confusing, for a method that returns only one shortcut.
    
    I think you mean this is the convenience method for returning the primary shortcut, and for other shortcuts, one should use shortcuts().
    
    In that case, just remove the default value from the deprecated method, to fix the ambiguity.



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

    This can't possibly be right.
    You can deprecate the method, but don't break it.
    It should still return the shortcut number 0 or 1, depending on the type argument.



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

    I see no reason to deprecate this constructor.
    It's just convenience for the two-shortcuts case.
    



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

    Ditto.



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

    Ditto.



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

    Even this one could stay.



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

    I don't understand why you want to deprecate this one. The argument is about handling of empty sequences, this is unrelated to "two, or more shortcuts", isn't it?


- David Faure


On April 30, 2012, 8:59 p.m., Mark Gaiser wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/104801/
> -----------------------------------------------------------
> 
> (Updated April 30, 2012, 8:59 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 all of the KShortcut cpp file. The API has remained the same but i would like to deprecate all things that go back to primary/alternate for the KDE 4.x cycle. Remove them for KDE Frameworks.
> 
> I have 2 issues remaining:
> 1. QList<QKeySequence> toList() const;
> 2. KShortcut shortcut() const;
> 
> If i enable those functions the compiler suddenly doesn't know which function to use.. The old ones or the new ones. Some suggestions on how to fix it would be welcome.
> 
> 
> 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/20120430/07231416/attachment.htm>


More information about the kde-core-devel mailing list