New authorization library moved to kdereview

[Nicola Gigante]

On 22/lug/09, at 22:48, Albert Astals Cid wrote:
>
> Hi, some comments from my side:
Thank you, I've been waiting for them :D

> * maybe rename the namespace to KAuth?
Ok. Anybody wants to suggest a better name?

>
> * The code is GPL, we don't accept GPL code in kdelibs.
My fault, I'll change it to LGPL as soon as possible.

> * You have functions that return references (e.g. ActionReply.h), if  
> somewhen
> your internal storage changes you won't be able of returning  
> references ending
> up in a BC problem, you should avoid/remove them
Actually, that's an issue, but the two methods in question,  
Action::arguments() and ActionReply::data(),
are meant to be used to directly access the QVariantMap object. If I  
remove the const, I wouldn't be able to
write something like:

action.arguments()["key"] = value;

which is very confortable if you have to only set one or two keys. If  
I remove the reference (and add a setter method then),
the library user has to write:

QVariantMap args;
args["key"] = value;
action.setArguments(args);

That would be, in my opinion, an ugly API.
There shouldn't be any changes to the internal storage of that  
QVariantMap object, because its only purpose is to be sent directly to  
the helper and back again.

Anybody else has suggestions about this point?

>
> * static ActionReply deserialize(QByteArray data); should probably  
> get a
> const &
Probably. I've not used const & because I directly pass the byte array  
to the QDataStream
constructor that discards the const qualifier. If I add const &, how  
do I solve the issue? QByteArray is implicitly shared anyway..

> * void ActionWatcher::actionPerformedSlot(const QString &action,  
> ActionReply
> reply); should get a const &?
>
> * void ActionWatcher::progressStepSlot(const QString &action,  
> QVariantMap
> data); should get a const &?
>
> * static void HelperSupport::progressStep(QVariantMap data); should  
> get a
> const &?
Yes, I'll add them

> * Having inline code in headers is bad (~AuthBackend())
That destructor has been added in a commit by 'mlaurent', apparently  
to fix a warning. Now the point is that
the header defines an abstract class for the plugin interface. I don't  
have a .cpp file for that class. Is it worth to add
it only to define an empty destructor?

> * this->statusBar()-> is "ugly", kill the this->
Yes that's ugly ;-)

> * AuthStatus executeAsync(QObject *target = NULL, const char *slot =  
> NULL);
> in KDE we usually use 0 and not NULL
I'll change it, then. Where's the link where I can find policies like  
this (and possibly the motivation) ?
>
> * const QString &helperID = "" would be better const QString  
> &helperID =
> QString()
Ok. QString() returns an empty string anyway, right?

>
> * I'm not sure if doing forward declaration of QMap (HelperProxy.h)  
> is a good
> idea
Mh... of course I could simply include the header..

> * You have some  static const ActionReply SuccessReply; i'm not sure  
> if
> that's ok or you have to use some K_STATIC_FOO macro, can someone  
> clarify it?

I don't know how does that macro work. Anyway I didn't use any kdelibs  
stuff because in the early development days I didn't
know where it will be included, but now that the final place seems to  
be kdelibs I could use such a macro without problems.

Thank you very much for the comments :-)