[Ksecretservice-devel] KSecretsService : client API review request

[Valentin Rusu]

Hello Olivier,

Hope you're doing well :-)

Please be advised that the client API is now quite usable and the time
is right for a thourough review, if you will to and have enough time for it.

As we discussed in Randa, this client API is fully asynchrounous and
it's intent is to hide it's DBus internal implementation from the
applications using it. Meanwhile I checked Qt code and found out that
property get/set lead to sync calls, so I introduced the dedicated jobs
ReadPropertyJob and WritePropertyJob.

At the time of writing this mail some code must still be added, but I
intend to push it by tomorrow or the day after. However, I don't expect
you to jump over and go review the code right away :-)

Remember that the service was initially called KSecretService, but in
Randa it's name got an additional "s" to make KSecretsService. The code
reflects that, but the GIT repository was not renamed - no need to do
that as long as it's standing in the playground.

What would be the next steps now? On my side, once I'll push the
remaining code, I'll start a new local branch for kdelibs and go ahead
with KWallet rewrite. This will allow me to test the API inside a live
environment (my computer) and prepare for the move to kdereview. Or
should I request the move to kdereview right away? (the sync tool is far
from ready though, perhaps I should separate my code in several modules
in order to get it integrated in several steps)

Here are some more information:
Code location : git clone git at git.kde.org:ksecretservice
What to be reviewed : ksecretservice/client
How to run:
1. build ksecretservice - this will build the client and the daemon and
the other components,
2. launch the daemon : ksecretsserviced
3. start client/tests/ksecretsservice_client_test

Cheers,
Valentin

[Olivier Goffart]

Hi,

Sorry, i did not get that much time to review the API.
Anyway, i looked at it quickly, and took those notes.
This is just my comments, they are not the letter of god.


Collection::findCollection
collectionName should be the first argument.
maybe the promptParentWindowId should even be the last argument

Collection::PendingFind
rename to Pending

Collection::deleteCollection
documentation says it returns rtue, but it return a KJob

Remember, to ease binary compatibility, the destructor,
default constructor, and operator= must not be inline

Why are the private class not in the namespace?

In general, we use QSharedData/QExplicitlySharedDataPointer for the d.
QSharedPointer is a bit more expensive. But that is an imlementation detail.
What i wonder is why is there so many public constructors that take the
private as argument
--
Olivier
http://woboq.com

Post by Valentin Rusu
Hello Olivier,
Hope you're doing well :-)
Please be advised that the client API is now quite usable and the time
is right for a thourough review, if you will to and have enough time for it.
As we discussed in Randa, this client API is fully asynchrounous and
it's intent is to hide it's DBus internal implementation from the
applications using it. Meanwhile I checked Qt code and found out that
property get/set lead to sync calls, so I introduced the dedicated jobs
ReadPropertyJob and WritePropertyJob.
At the time of writing this mail some code must still be added, but I
intend to push it by tomorrow or the day after. However, I don't expect
you to jump over and go review the code right away :-)
Remember that the service was initially called KSecretService, but in
Randa it's name got an additional "s" to make KSecretsService. The code
reflects that, but the GIT repository was not renamed - no need to do
that as long as it's standing in the playground.
What would be the next steps now? On my side, once I'll push the
remaining code, I'll start a new local branch for kdelibs and go ahead
with KWallet rewrite. This will allow me to test the API inside a live
environment (my computer) and prepare for the move to kdereview. Or
should I request the move to kdereview right away? (the sync tool is far
from ready though, perhaps I should separate my code in several modules
in order to get it integrated in several steps)
Code location : git clone git at git.kde.org:ksecretservice
What to be reviewed : ksecretservice/client
1. build ksecretservice - this will build the client and the daemon and
the other components,
2. launch the daemon : ksecretsserviced
3. start client/tests/ksecretsservice_client_test
Cheers,
Valentin

[Valentin Rusu]

Hello Olivier,

Thanks for the review!

On 08/03/2011 11:10 AM, Olivier Goffart wrote:
[snip] All suggestions taken into account

Post by Olivier Goffart
Why are the private class not in the namespace?

They are private so adding them into the namespace is useless in opinion.

Post by Olivier Goffart
In general, we use QSharedData/QExplicitlySharedDataPointer for the d.
QSharedPointer is a bit more expensive. But that is an imlementation detail.

Some of my Private classes must be QObjects to enable slots on them.
These are internally used by the QDBusPendingCallWatchers.
QSharedDataPointer require QShareData parent, and that's not a QObject.
Hence, I'm forced to use QSharedPointer instead.

Post by Olivier Goffart
What i wonder is why is there so many public constructors that take the
private as argument

Because of the DBus internal stuff. Using private structs in constructor
allow me to hide away things like QDBusPaths.

Cheers,
Valentin

[Valentin Rusu]

Post by Valentin Rusu
They are private so adding them into the namespace is useless in opinion.

Then some other library could use the private name in the global namespace,
and there would be conflict. It is the point of namespace, to separate
everything.

Ok, I moved them.

Post by Valentin Rusu
QSharedDataPointer require QShareData parent, and that's not a QObject.
Hence, I'm forced to use QSharedPointer instead.

You can use double inheritance.

It won't compile, because QObject inheritors must use Q_DISABLE_COPY and
that takes away the copy constructor needed by QSharedData::clone()

[Olivier Goffart]

Post by Valentin Rusu
Hello Olivier,
Thanks for the review!
[snip] All suggestions taken into account

Post by Olivier Goffart
Why are the private class not in the namespace?

They are private so adding them into the namespace is useless in opinion.

Then some other library could use the private name in the global namespace,
and there would be conflict. It is the point of namespace, to separate
everything.

Post by Valentin Rusu

Post by Olivier Goffart
In general, we use QSharedData/QExplicitlySharedDataPointer for the d.
QSharedPointer is a bit more expensive. But that is an imlementation detail.

Some of my Private classes must be QObjects to enable slots on them.
These are internally used by the QDBusPendingCallWatchers.
QSharedDataPointer require QShareData parent, and that's not a QObject.
Hence, I'm forced to use QSharedPointer instead.

You can use double inheritance.

Post by Valentin Rusu

Post by Olivier Goffart
What i wonder is why is there so many public constructors that take the
private as argument

Because of the DBus internal stuff. Using private structs in constructor
allow me to hide away things like QDBusPaths.
Cheers,
Valentin