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)
