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

Wed Feb 20 22:33:11 UTC 2013

> On Feb. 19, 2013, 7:20 a.m., Kevin Ottens wrote:
> > kdeui/actions/kaction.cpp, line 204
> > <http://git.reviewboard.kde.org/r/109019/diff/1/?file=114349#file114349line204>
> >
> >     Aren't those checks already done in KGlobalAccel somehow? Just wondering if that makes sense to keep them here, especially the call to doRegister.

Good point, as setShortcut and setDefaultShortcut got the doRegister call I overlooked here. Fixed. I also realised that these two methods should return a boolean to give the client a clue about the issue of the call. As such, this method's logic became simpler and cleaner.

> On Feb. 19, 2013, 7:20 a.m., Kevin Ottens wrote:
> > kdeui/shortcuts/kglobalaccel.h, line 167
> > <http://git.reviewboard.kde.org/r/109019/diff/1/?file=114351#file114351line167>
> >
> >     Return a KShortcut here, not a const KShortcut.

Why not return const KShortcut here? The QMap holding these, from where the return value came straight through, reuturns a const itself! What's wrong with this const qualifier?

> On Feb. 19, 2013, 7:20 a.m., Kevin Ottens wrote:
> > kdeui/shortcuts/kglobalaccel.h, line 169
> > <http://git.reviewboard.kde.org/r/109019/diff/1/?file=114351#file114351line169>
> >
> >     resetShortcuts()?

I prefer removeAllShortcuts, the same as removeAllGestures (remember?) for consistency's sake.

> On Feb. 19, 2013, 7:20 a.m., Kevin Ottens wrote:
> > kdeui/shortcuts/kglobalaccel.h, line 170
> > <http://git.reviewboard.kde.org/r/109019/diff/1/?file=114351#file114351line170>
> >
> >     hasShortcut()?

Why not? :)

> On Feb. 19, 2013, 7:20 a.m., Kevin Ottens wrote:
> > kdeui/shortcuts/kglobalaccel.h, line 165
> > <http://git.reviewboard.kde.org/r/109019/diff/1/?file=114351#file114351line165>
> >
> >     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.

I preferred moving GlobalShortcutLoading to KGlobalAccel instead of copying it. It belongs there and future confusions will be avoided.

> On Feb. 19, 2013, 7:20 a.m., Kevin Ottens wrote:
> > kdeui/shortcuts/kglobalaccel.cpp, line 302
> > <http://git.reviewboard.kde.org/r/109019/diff/1/?file=114352#file114352line302>
> >
> >     Why the const KAction here? Doesn't make sense to me here, and forces you to const_cast later on.

Sorry, but const KAction comes from the original code. Should I understand that we should drop this ?

> On Feb. 19, 2013, 7:20 a.m., Kevin Ottens wrote:
> > kdeui/shortcuts/kglobalaccel.cpp, line 357
> > <http://git.reviewboard.kde.org/r/109019/diff/1/?file=114352#file114352line357>
> >
> >     Should probably become a signal on KGlobalAccel itself.

Moving the signal to KGlobalAccel solved also the const cast problem :-)

> On Feb. 19, 2013, 7:20 a.m., Kevin Ottens wrote:
> > kdeui/shortcuts/kglobalaccel.cpp, line 462
> > <http://git.reviewboard.kde.org/r/109019/diff/1/?file=114352#file114352line462>
> >
> >     Doesn't need the const KAction here, only forces the const_cast for no gain.

The const_cast is no longer needed by moving globalShortcut signal to KGlobalAccel so the const semantics may be kept.

> On Feb. 19, 2013, 7:20 a.m., Kevin Ottens wrote:
> > kdeui/shortcuts/kglobalaccel_p.h, line 57
> > <http://git.reviewboard.kde.org/r/109019/diff/1/?file=114353#file114353line57>
> >
> >     No need for that change, can be reverted too. Not the bool return type though, AFAICT you use it in KAction.

doRegister is no longer used in KAction, but the bool should still be kept as it gives good indication about this method call failure.

> On Feb. 19, 2013, 7:20 a.m., Kevin Ottens wrote:
> > kdeui/shortcuts/kglobalaccel.cpp, line 426
> > <http://git.reviewboard.kde.org/r/109019/diff/1/?file=114352#file114352line426>
> >
> >     Why const here? It forces you to const_cast later on.

The const_cast is no longer needed by moving globalShortcut signal to KGlobalAccel so the const semantics may be kept.

- Valentin

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

On Feb. 20, 2013, 10:32 p.m., Valentin Rusu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109019/
> -----------------------------------------------------------
> 
> (Updated Feb. 20, 2013, 10:32 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/tests/kglobalshortcuttest.cpp 49fb8ec 
>   kdeui/actions/kaction.h ddaa4de 
>   kdeui/actions/kaction.cpp ec63a72 
>   kdeui/actions/kaction_p.h b50e678 
>   kdeui/actions/kactioncollection.cpp 555b204 
>   kdeui/dialogs/kshortcutseditoritem.cpp 5aa8437 
>   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/20130220/04154022/attachment-0001.html>