Review request for KSecretsService components

[Albert Astals Cid]

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 )


> >    * 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

Of course that is a minor thing and won't complain too much if you decide to 
disagree with me ;-)

Albert