Review request for KSecretsService components
kde at rusu.info
Wed Nov 9 23:27:20 GMT 2011
Thanks for the thourough review.
On 11/09/2011 03:26 PM, Albert Astals Cid wrote:
> A Dimarts, 8 de novembre de 2011, Valentin Rusu vàreu escriure:
>> Hello Again,
>> The freeze will come in less thant two days now and I'd like to know if
>> anyone reviewed these components.
>> On 10/31/2011 11:48 PM, Valentin Rusu wrote:
>>> Please be advised three repostories need review before integration
>>> into the next release:
>>> 1. /kdereview/ksecretservice
>>> 2. kdelibs branch ksecretsservice
>>> 3. /kdereview/ksecrets
>>> *1. /kdereview/ksecretservice*
>>> The first one has several subdirectories. The only relevant one is the
>>> daemon subdirectory. Other directories contents was already moved to
>>> other repositories (see below).
>>> This daemon directory contains the sources of the future
>>> It needs testing but it's quite usable.
>>> *2. kdelibs branch ksecretsservice*
>>> This is the API KDE applications will be supposed to use instead of
>>> KWallet class. Tools listed below already use this API.
> This is scary, last time i used kwallet, i had to add a single line, and now
> there are like a billion of classes?
Welcome to the asynchronous jobs world. I had the same opinion at start.
And, to be honest, that's why it takes so long to implement, as I'm not
working on it only during my spare time.
> Or is this API for more complex apps like
Well, the secrets service infrastructure, specified with GNOME's Stef
Walter is intended to do more things thant KWallet was.
For instance, the KWallet API won't let me do the synchronization tool I
wanted without significant improvement.
> My comments on the kdelibs API:
> - ksecretsservice/ksecretsservicecodec.h
> * Seems to be installed but has no documentation at all
It's indeed installed but it's an @internal class - I added the
> * Uses references for output parameters when KDE/Qt usually uses pointers
> * Does not use d-pointers thus maintaining ABI is going to be difficult
Once again, it's an internal class, no breakage risk here as I'll
maintain the API and the daemon together.
> - ksecretsservice/ksecretsservicecollection.h
> * I miss a note saying who belongs the ownsership of all those job that come
> as result of the getters. Do I have to delete them?
Oh, I thought that'll not be needed. Usually jobs autodelete themselves.
> * createItem falls in the "let's use a bool instead of an enum because it
> just have two values" trap for the replace parameter
Well, I don't agree. When presenting a new item to the collection, you
should specify to replace existing item or not. Yes/No is boolean for me.
> * There are a few of spacing "glitches", e.g.
> const QVariantMap collectionProperties = QVariantMap(),
> const WId&promptParentWindowId =0 );
Added a space before the 0.
> * There are a few not implemented signals
Yes, but they'll not be done before tommorrow, unfortunately.
> - ksecretsservice/ksecretsservicedbustypes.h
> Contains a plain struct and some inline functions, what if those have to
Marked as @internal
> - ksecretsservice/ksecretsserviceitem.h
> * Same question about ownership of jobs
Same note added.
> - ksecretsservice/ksecretsserviceitemjobs.h
> * Without having at all a clue on what it is used for, i miss a note saying
> to who belongs the ownership of the SecretItem passed to the constructor
> and returned in the secretItem() function, i.e. do i have to delete it
This class was marked as @internal and corresponding comment was added.
> * SecretItemJob does not have a d-pointer
> * void finished( ItemJobError, const QString& msg =""); should probably be
> void finished( ItemJobError, const QString& msg = QString());
> - ksecretsservice/ksecretsservicesecret.h
> Is installed and has no documentation
>>> Contains code depending on a configuration flag that directs calls to
>>> ksecretsserviced instead of kwalletd, via the new API.
>>> *3. /kdereview/ksecrets*
>>> Contains several tools in a less or more mature state:
>>> a. kwl2kss - tool to import kwallet files to ksecretsservice,
>>> b. ksecrets - tool to list the contents of a ksecretsservice
>>> collection (e.g. wallet),
>>> c. kio - KIO slave in a just started state, intended to show
>>> collections in konqueror or dolphin,
>>> d. secretsync - this was the tool I initially wanted to do for
>>> KWallet, but drought me into ksecretsservice :-)
>>> It's half way implemented.
>>> The mandatory components for next release would be 1, 2, 3 (a, b), the
>>> others may wait, but releasing them may cause no harm if communication
>>> is done right (I'll take care of that).
>>> Thanks for your comments (any comments),
>> Valentin Rusu (IRC valir, KDE vrusu)
>> KSecretsService (former KSecretService, KWallet replacement)
Valentin Rusu (IRC valir, KDE vrusu)
KSecretsService (former KSecretService, KWallet replacement)
More information about the kde-core-devel