On Saturday 29 June 2013 16:02:59 Kevin Krammer wrote: > On Saturday, 2013-06-29, Pali Rohár wrote: > > > - Not sure whether you need to #include <QPointer>; > > > perhaps a forward declaration is enough (haven't checked, > > > might be wrong) > > > > This will not work, because in class is full QPointer object > > - not pointer or reference. > > Could still be possible to forward declare, I think you would > only need to add a non-inline destructor to the class that > has the QPointer member. > > > > - While common in the existing desktop software, I don't > > > find "Default Trojita Addressbook" to be meaningful > > > description. As a user, I'd like to know that it's > > > accessing an abook-formatted file which can be shared > > > with mutt, not that it happens to be used by trojita by > > > default. > > > > Ok. > > Also name() and description() should probably be marked for > translation. >
name() not, because is internal identifier, description() is already marked for translation. > Some other suggestions: > > - Since you are discarding the error value, why not just have > one signal that is emitted regardless of success or error? > In future error value can be used. Now I only written code in settings dialog which discarding error value. > - the start() methods of synchronous jobs should probably not > emit signals, better use QMetaObject::invokeMethod() or a > QTimer on a private slots to trigger the actual job code. > This way start() will always return before any result is > delivered, making their behavior more like the ones really > asynchronous jobs will have. > start() now call QTimer::singleShot(0, this, SLOT(doStart())) and plugins implementing doStart() method. So now jobs are fully async. > - Never ever delete an object in a slot called by the object's > signals. Always use deleteLater() > Fixed. > - AbookAddressbook has lots of public API that is not in > AddressbookInterface (e.g. model). > It is needed in AddressbookInterface (somewhere for Trojita)? Or where is problem? AbookAddressbook is also used by be.contacts standalone application, so that methods must be public for access outside class. -- Pali Rohár pali.ro...@gmail.com
signature.asc
Description: This is a digitally signed message part.
