Review request for KSecretsService components

[Valentin Rusu]

On 11/10/2011 12:48 AM, Albert Astals Cid wrote:
> A Dijous, 10 de novembre de 2011, Valentin Rusu vàreu escriure:
>> Hello Albert,
>>
>> Thanks for the thourough review.
>>
>> On 11/09/2011 03:26 PM, Albert Astals Cid wrote:
>>> 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.
> Are you saying you don't have anything as simple as this in the new API?
>
> QString walletName = KWallet::Wallet::NetworkWallet();
> KWallet::Wallet *wallet = KWallet::Wallet::openWallet(walletName, parentwid);
> if ( !wallet->hasFolder( "KPdf" ) )
>    wallet->createFolder( "KPdf" );
> wallet->setFolder( "KPdf" );
> wallet->readPassword( walletKey, retrievedPass )
Yes, your code isn't asynchrounous ! (boo)
Well, I don't fully agree with this point of view, but in Randa they 
managed to convince me about the benefits.
Take a look inside kwallet.cpp class to see how this new API should be used.
Another example is the ksecrets API inside kdereview/ksecrets repository.
>
>
>>>     * 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.
> That is the problem, that a boolean is Yes/No, and then you end up with a call
> that says
>   foo->createItem(label, attributes, mySecret, false);
> And it seems you do not want to create the item :D while
>   foo->createItem(label, attributes, mySecret, DoNotReplace);
> is much more obvious.
> More on my side at
> http://wiki.qt-project.org/API_Design_Principles#The_Boolean_Parameter_Trap
> and
> http://ariya.ofilabs.com/2011/08/hall-of-api-shame-boolean-trap.html
Ok,  I see. I'll change that.
> Of course that is a minor thing and won't complain too much if you decide to
> disagree with me ;-)
>
> Albert


-- 
Valentin Rusu (IRC valir, KDE vrusu)
KSecretsService (former KSecretService, KWallet replacement)