Review Request 109019: Move global shortcut facilities from KAction to KGlobalAccel

[Kevin Ottens]

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


First round of review.


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

    Aren't those checks already done in KGlobalAccel somehow? Just wondering if that makes sense to keep them here, especially the call to doRegister.



kdeui/shortcuts/kglobalaccel.h
<http://git.reviewboard.kde.org/r/109019/#comment20756>

    For the & and * the space should be before not after per the kdelibs coding standard.
    
    Don't use a boolean for the last parameter, copy over the corresponding enum from KAction. Also set the default value for this parameter to Autoloading like in KAction.



kdeui/shortcuts/kglobalaccel.h
<http://git.reviewboard.kde.org/r/109019/#comment20759>

    Same comment than the line above.



kdeui/shortcuts/kglobalaccel.h
<http://git.reviewboard.kde.org/r/109019/#comment20757>

    Return a KShortcut here, not a const KShortcut.



kdeui/shortcuts/kglobalaccel.h
<http://git.reviewboard.kde.org/r/109019/#comment20758>

    Return a KShortcut here not a const KShortcut.



kdeui/shortcuts/kglobalaccel.h
<http://git.reviewboard.kde.org/r/109019/#comment20760>

    resetShortcuts()?



kdeui/shortcuts/kglobalaccel.h
<http://git.reviewboard.kde.org/r/109019/#comment20755>

    hasShortcut()?



kdeui/shortcuts/kglobalaccel.cpp
<http://git.reviewboard.kde.org/r/109019/#comment20761>

    Good point, the cleaner would be to split that out in two parameters with proper type instead of packing everything in a single uint.



kdeui/shortcuts/kglobalaccel.cpp
<http://git.reviewboard.kde.org/r/109019/#comment20765>

    Why the const KAction here? Doesn't make sense to me here, and forces you to const_cast later on.



kdeui/shortcuts/kglobalaccel.cpp
<http://git.reviewboard.kde.org/r/109019/#comment20762>

    Should probably become a signal on KGlobalAccel itself.



kdeui/shortcuts/kglobalaccel.cpp
<http://git.reviewboard.kde.org/r/109019/#comment20763>

    Why const here? It forces you to const_cast later on.



kdeui/shortcuts/kglobalaccel.cpp
<http://git.reviewboard.kde.org/r/109019/#comment20766>

    Doesn't need the const KAction here, only forces the const_cast for no gain.



kdeui/shortcuts/kglobalaccel.cpp
<http://git.reviewboard.kde.org/r/109019/#comment20767>

    Again, drop the gratuitous const.



kdeui/shortcuts/kglobalaccel_p.h
<http://git.reviewboard.kde.org/r/109019/#comment20768>

    No need for that change, can be reverted.



kdeui/shortcuts/kglobalaccel_p.h
<http://git.reviewboard.kde.org/r/109019/#comment20769>

    No need for that change, can be reverted too. Not the bool return type though, AFAICT you use it in KAction.



kdeui/shortcuts/kglobalaccel_p.h
<http://git.reviewboard.kde.org/r/109019/#comment20770>

    You can revert that change.



kdeui/shortcuts/kglobalaccel_p.h
<http://git.reviewboard.kde.org/r/109019/#comment20771>

    That one too.



kdeui/shortcuts/kglobalaccel_p.h
<http://git.reviewboard.kde.org/r/109019/#comment20772>

    Don't make them const KAction *.



kdeui/shortcuts/kglobalaccel_p.h
<http://git.reviewboard.kde.org/r/109019/#comment20773>

    Don't make them const KAction *.


- Kevin Ottens


On Feb. 18, 2013, 11:08 p.m., Valentin Rusu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109019/
> -----------------------------------------------------------
> 
> (Updated Feb. 18, 2013, 11:08 p.m.)
> 
> 
> Review request for KDE Frameworks, David Faure and Kevin Ottens.
> 
> 
> Description
> -------
> 
> KDELibs cleanup task: move global shortcut facilities from KAction to KGlobalAccel. The second step, registering QActions instead of KActions, will be done after completing this review.
> 
> 
> Diffs
> -----
> 
>   kdeui/actions/kaction.cpp ec63a72 
>   kdeui/actions/kaction_p.h b50e678 
>   kdeui/shortcuts/kglobalaccel.h ed68bba 
>   kdeui/shortcuts/kglobalaccel.cpp 36dbab7 
>   kdeui/shortcuts/kglobalaccel_p.h 04feaba 
> 
> Diff: http://git.reviewboard.kde.org/r/109019/diff/
> 
> 
> Testing
> -------
> 
> Code compiles
> 
> 
> Thanks,
> 
> Valentin Rusu
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20130219/6ff7b445/attachment-0001.html>