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

[Kevin Ottens]

On Wednesday 20 February 2013 22:33:11 Valentin Rusu wrote:
> > 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#file114351l
> > > ine167>> > 
> > >     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?

It's not dangerous per say, it's just that we return a copy anyway (so no 
encapsulation is broken) and in such a case the custom is to return it non 
const. It's merely a principle of least surprise for whoever will use the API 
later on.

> > 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#file114351l
> > > ine169>> > 
> > >     resetShortcuts()?
> 
> I prefer removeAllShortcuts, the same as removeAllGestures (remember?) for
> consistency's sake.

Good point.

> > 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#file114351l
> > > ine170>> > 
> > >     hasShortcut()?
> 
> Why not? :)

Otherwise I read it like the passed action instance *is* a shortcut, while 
what we want is to check it has one.

> > 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#file114351l
> > > ine165>> > 
> > >     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.

Please not, it really should be copied because we try to maintain as much 
source compatibility as possible! As for confusion it won't really be a 
problem since in the end KAction will move in kde4support and be deprecated 
while KGlobalAccel will be around, so newcomers will look at KGlobalAccel 
only, we're doing the copy to help the old code porting.

> > 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#file114352l
> > > ine302>> > 
> > >     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 ?

I checked again your previous patch, it was not coming from the original code, 
you changed the type of the preexisting QMultiHash and QSet, that's what 
forced your hand to use const everywhere else even where it wasn't making 
sense.

Personally I would not use const in the hash, set and map storage as that 
limits the use of those KAction (as the presence of the const_cast indicates) 
later on. Keeping it in the methods where we indeed doesn't modify the 
instance at all is OK though (e.g. not _k_invokeAction).

> > 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#file114352l
> > > ine357>> > 
> > >     Should probably become a signal on KGlobalAccel itself.
> 
> Moving the signal to KGlobalAccel solved also the const cast problem :-)

Not really, you still had one lingering on.
 
> > 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#file114353l
> > > ine57>> > 
> > >     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.

Indeed, it's overall an improvement.

Regards.
-- 
Kévin Ottens, http://ervin.ipsquad.net

KDAB - proud supporter of KDE, http://www.kdab.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20130221/19b7c737/attachment.sig>