[Ksecretservice-devel] KSecretsService : client API review request

[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


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