New authorization library moved to kdereview

[Albert Astals Cid]

A Dijous, 23 de juliol de 2009, Nicola Gigante va escriure:
> 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.

Is all the code yours? Otherwise you need to ask others that contributed code.

>
> > * 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?

See Aurélien's mail

>
> > * 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..

See Aurélien's mail

> > * 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. 

Right

> 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?

The problem is that in the future nothing guarantees you'll make this 
"interface" a base class with some shared functionality and them maybe you 
need adding something to the destructor, and then you're in trouble. So yeah 
add a .cpp file, nothing's lost.

>
> > * 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) ?

My bad, i was informed on IRC that NULL vs 0 is not a "done" thing so it's 
left to coder to decide.

>
> > * const QString &helperID = "" would be better const QString
> > &helperID =
> > QString()
>
> Ok. QString() returns an empty string anyway, right?

Yes, the problem with "" is that it involves the constructor from char * that 
involves lots of things you really want to avoid.

>
> > * 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.

As said i'm not sure about that, i'd like some input from anyone else.

Albert

>
> Thank you very much for the comments :-)