On Thursday 20 June 2013 10:47:27 Jan Kundrát wrote:
> On Wednesday, 19 June 2013 10:28:09 CEST, Pali Rohár wrote:
> > AddressbookInterface(QObject * parent) : QObject(parent)
> > {} virtual ~AddressbookInterface() {}
>
> I wouldn't make these inline, but in the .cpp file. Perhaps
> it's not needed for QObject, but the destructor is typically
> the first virtual function being defined, and it is often
> used for RTTI purposes (dynamic_cast etc). This is probably a
> huge oversimplification, but the lesson I've learned is that
> virtual destructors shall never be inlined.
> Moving function outside *.h file could be problem, because this is interface for plugins... I look at qt plugins howto and there is also used inlined virtual destructor: https://qt-project.org/doc/qt-4.8/plugins-howto.html > > * - Matches are case INsensitive > > * - The match aligns to either Name or [email protected], > > ie. > > > > "ja" or "jkt" match > > > > * "Jan Kundrát <[email protected]>" but "gentoo" does > > not * - Strings in the ignore list are NOT included in > > the > > > > return, despite they may match. > > > > * - The return can be empty. > > Oops, these comments don't really match reality now. For > [email protected], the completion matches against > "jan", "kundrat", "foo" and "example" IIRC. I'm not sure that > these details belong to .h file describing the interface, > though -- perhaps leaving the completion to be > implementation-defined is the way to go to accomodate e.g. > fast LDAP searches which might only work on a subset of > fields, or plugins which can somehow look at other fields of > the contact info they manage (a cmopany with domain > "lbbw.com", but a contact with "Organization: LandesWhatEver > Bank" should probably match on "Landes" even if it's not > present in the e-mail address,...) etc. > Ok, I will remove this comment from interface. It will be up to plugin how completion implement. > > virtual void startComplete(const QString &input, const > > > > QStringList &ignores = QStringList(), int max = -1) = 0; > > I got confused when reading this -- to me, the name suggests > that it's a signal saying that "the starting phase has > completed". Perhaps "requestCompletion"? > > > virtual void startPrettyNamesForAddress(const QString > > &email) = 0; > > s/start/request/ again for the same reason? > I rename all signal and slots. > > * empty email and name strings means opening window for > > > > adding new contact > > This is a hack, IMHO. I'd prefer to split these into: > > openAddContactWindow(const QString &mailAddress, const QString > &prettyName) > > openEditContactWindow(const QString &mailAddress) > There could be problem, because Trojita does not know if email address or contact name is already in addressbook or not. I think that plugin should decide if creating new contact or editing existing. So then plugin api needs only one function with "hints" for email address and display name. > > private: > > /** > > > > * @short Return name and email in format "[Name > > > > <][email protected][>]" needed by method startComplete() > > > > */ > > > > static inline QString formatAddress(const QString &name, > > > > const QString &email) { > > > > if (name.isEmpty() || name == email) return email; > > else return name + " <" + email + '>'; > > > > } > > I don't see a point in having a *private* static method > available in the public header of a plugin interface. Please > remove this from the plugin API -- perhaps into some utility > class. > Kevin suggested that this method can be usefull for plugins. So then it is needed to be stored somewhere from plugins can call it (plugins do not have pointer to some window/util objects). But I think that this method is too small and plugins can copy paste it if they really need it. > > void completeResult(const QString &input, const > > QStringList &list); > > completionAvailable? OK, you prefer ...Result, so what about > completionResult? > > > virtual AddressbookInterface * create(QObject * parent) > > = 0; > > Nitpicking: "QObject *parent" to match the coding style of > trojita. > Ok. > Cheers, > Jan -- Pali Rohár [email protected]
signature.asc
Description: This is a digitally signed message part.
