[Ksecretservice-devel] KSecretsService : client API review request

Olivier Goffart ogoffart at kde.org
Wed Aug 3 09:10:12 UTC 2011


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.

  collectionName should be the first argument.
  maybe the promptParentWindowId should even be the last argument
   rename to Pending

   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


On Monday 01 August 2011 00:48:24 Valentin Rusu wrote:
> 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

More information about the Ksecretservice-devel mailing list