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 g...@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 _______________________________________________ Ksecretservice-devel mailing list Ksecretservice-devel@kde.org https://mail.kde.org/mailman/listinfo/ksecretservice-devel
