Review request for KSecretsService components

[Albert Astals Cid]

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? Or is this API for more complex apps like 
kwalletmanager?

My comments on the kdelibs API:
 - ksecretsservice/ksecretsservicecodec.h
  * Seems to be installed but has no documentation at all
  * Uses references for output parameters when KDE/Qt usually uses pointers
  * Does not use d-pointers thus maintaining ABI is going to be difficult

 - 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?
  * createItem falls in the "let's use a bool instead of an enum because it 
just have two values" trap for the replace parameter
  * There are a few of spacing "glitches", e.g.
       const QVariantMap collectionProperties = QVariantMap(),
       const WId &promptParentWindowId =0 );
  * There are a few not implemented signals

 - ksecretsservice/ksecretsservicedbustypes.h
   Contains a plain struct and some inline functions, what if those have to 
change?

 - ksecretsservice/ksecretsserviceitem.h
  * Same question about ownership of jobs

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

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)