Review request for KSecretsService components

[Valentin Rusu]

Hello Albert,

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.
>>
>> Thanks,
>>
>> On 10/31/2011 11:48 PM, Valentin Rusu wrote:
>>> Hello,
>>>
>>> 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
>>> kde-runtime/ksecretsserviced
>>> It needs testing but it's quite usable.
>>>
>>> *2. kdelibs branch ksecretsservice*
>>> kdelibs/kdeui/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
> kwalletmanager?
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 
appropriate documentation.
>    * 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. 
Note added.
>    * 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
> change?
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
> myself?
This class was marked as @internal and corresponding comment was added.
>    * SecretItemJob does not have a d-pointer
Oops! Added.
>    * void finished( ItemJobError, const QString&  msg =""); should probably be
> void finished( ItemJobError, const QString&  msg = QString());
Right!
>
>   - ksecretsservice/ksecretsservicesecret.h
>     Is installed and has no documentation
Documentation added.
>
> Albert
>
>>> kdelibs/kdeui/util/kwallet.cpp
>>> 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)