On Thursday 20 June 2013 11:15:16 Jan Kundrát wrote: > Hi Pali, > a couple of comments: > > - You're using a plain QString as a key, but do not define how > this key is going to be derived from the > (type-of-account-IMAP-SMTP-etc, username). That's not bad at > the plugin level; the plugin perhaps doesn't need to know, > and it's possible that the existing key stores only work with > this flat indexing after all. But it has to be decided > anyway. >
If Trojita supports multiple imap and smtp servers, then instead of key we can use pair of account id (something internal for Trojita) and password type (e.g. imap, smtp)... > - It would be cool if there was a plugin which would store the > password in the configuration file. It should be a reasonably > trivial proof-of-concept implementation. > It is in the plan (at least reusing current password storage implementation). > - Some of the comments I had for the abook interface apply > here as well (inline ctor/dtor, spaces around "*"). > > > virtual void startReadPassword(const QString &key) = 0; > > It shall either be "startReadingPassword", or something like > "requestPassword". I prefer the latter one. > > > virtual void startWritePassword(const QString &key, > > const > > > > QString &password) = 0; > > savePassword? > > > void gotPassword(const QString &key, const QString > > &password); > > > > void writtenPassword(const QString &key); > > passwordAvailable, passwordStored? The name "writtenPassword" > seems weird to me, "gotPassword" is fine, but I changed them > both for consistency. > Ok, I rename signal and slots. > > void error(const QString &str); > > It isn't clear what this signal means and the receiver has no > way of knowing to which request it is related. Remember, > multiple requests for reading/writing passwords for multiple > accounts could be in flight at any given time. > > Also, a signal notifying the caller that "I have no password > for such key" is missing. It's a different condition that an > "error"; error means that something unexpected has happened > (kwalletd/dbus/whatever died horribly, etc). > What about adding int/enum variable to all signals which will contains status of request (success, no password, other error)? > The interface is also missing a slot for deleting a password > for a particular key (along with the slots for reporting > results). (I don't think that support for deleting stuff > would be needed for the abook interface, though.) > Ok, I add it. > It would also be cool to have some per-plugin functions for > specifying details like whether the plugin supports encrypted > storage and/or possibly to enforce some kind of encryption > when saving a password. This could be hard to get right -- > should it be an async method as well? Ideas welcome. > It could be good in future. Now I'm going to add functions name() and description() which will return some info about plugin. It will be shown in GUI and here can be written also that plugin support only plain text storage. > Cheers, > Jan -- Pali Rohár pali.ro...@gmail.com
signature.asc
Description: This is a digitally signed message part.
