On Friday, 28 June 2013 11:43:32 CEST, Pali Rohár wrote:
Here is git repository (branch master) with my gsoc code: http://quickgit.kde.org/?p=clones/trojita/pali/trojita.git
Hi Pali, wow, looks like you've been busy in the meanwhile -- I'm happy to see that. It would be even cooler if you let us know that you were working; this way I was needlessly worrying why you were slacking off. A couple comments on the code: - Please respect the coding style: do not use "if ( foo )", "qobject_cast <", "QPointer <", put the opening parenthesis of any function on an extra line, etc etc etc -- there's much more than that. - Check that the plugin loading works from within an uninstalled out-of-tree build as well as when installed on the system and that an uninstalled build loads its own plugins, not the systemwide ones. (I have no idea whether it works or not, but it looks fishy to me as it's now.) - No foreach, use Q_FOREACH - What's the point of storing the plugin path in the settings? Check how the localization is loaded to see what I mean with the path discovery. - The signal/slot signatures in connect(...) are not normalized - Could addressbook->requestCompletion ever return 0? You're checking its return values. - Looks like you do a lot of housekeeping before you delete an obsolete completion job. Could you perhaps delegate this to its destructor by any chance? - Not sure whether you need to #include <QPointer>; perhaps a forward declaration is enough (haven't checked, might be wrong) - 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. - What is the NullAddressbookCompletionJob and why is it returning bogous data? - I prefer to split classes into a .h and a .cpp file even when these classes are not "public" and are instead only used as plugins.
I'm not going to push interfaces to trojita master until plugins will be available, because there could be problem with interface.
A check list of stuff to do/have before you push: - an explicit OK from Thomas/Caspar/Kevin/me - no regressions you're aware of - passing builds under both Qt4 and Qt5 (I'm looking at that extra QT4_GENERATE_MOC invocation (which also happens to use caps)) - A clean git history. I'm all for atomic commits changing individual items, but before you push this into trojita's repos, please rewrite the history so that it's manageable and doesn't contain excess churn. An example with no relation to your actual history because I haven't checked it except the total number of commits: an extra commit for adding the plugin loader is OK, but multiple commits which iteratively improve the build system to ensure that the plugins are installed into a correct place are not OK. That said, separate commits which make a particular component play well with the rest of the system are welcome. Cheers, and please keep up the good work Jan -- Trojitá, a fast Qt IMAP e-mail client -- http://trojita.flaska.net/
