New authorization library moved to kdereview
Albert Astals Cid
aacid at kde.org
Thu Jul 23 21:36:12 BST 2009
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;
> 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.
> 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.
> Thank you very much for the comments :-)
More information about the kde-core-devel