On Wednesday 26 June 2013 19:24:52 Jan Kundrát wrote: > On Friday, 21 June 2013 16:05:14 CEST, Pali Rohár wrote: > > I'm sending new password interface. Now instead of type > > enum, plugin should emit one of result signal (with this > > pointer). Also I > > removed private variables which are not needed anymore. > > Thanks. > > A couple comments: > > - I am sceptical about the usefullness of the "PasswordJob > *job" in the job's signals, but I defer to Kevin's experience > when he says that these are useful in practice. OK. > > - I dislike the inlined protected constructor. Why do you need > it? >
In interface are virtual pure functions. So then remove protected
constructor (and compiler will generate one default)?
> - Now that the signals are actually emitted by the PasswordJob
> instances, do you still need the PasswordFactoryInterface and
> PasswordInterface classes? Looks like a single class with
> public non-slotted methods for creating jobs is enough here.
>
I merged both plugin factory interfaces to one (for easier plugin
loading). So now in PasswordInterface.h are only PasswordJob and
PasswordInterface classes.
> - I haven't received your response to my other mail (Date:
> Fri, 21 Jun 2013 14:27:50 +0200, Message-ID:
> <5b92e209-0ac3-44bc-8221-63a949d9e...@flaska.net>).
>
> - Rest of them inline.
>
Now only constructor is inlined. Same as in Addressbook. Delete
or move it to public?
> > enum Error {
> >
> > UnknownError,
> > Stopped,
> > NoPassword
> >
> > };
>
> Missing doxygen. Also, please make the last one a
> NoSuchPassword -- I find the old name a bit confusing.
>
Ok.
> > /**
> >
> > * @short Start job
> > * when finish it will emit just one signal from this
> > class
> >
> > (depends on job type)
> >
> > **/
>
> Could you please make the first line read "/** @short Start
> job", not use the leading *s on the following lines, and
> terminate just by "*/" to match the existing coding style?
Ok.
--
Pali Rohár
pali.ro...@gmail.com
signature.asc
Description: This is a digitally signed message part.
