On Friday 28 June 2013 20:07:55 Jan Kundrát wrote: > 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. >
What I done: * qt plugin interfaces for passwords and addressbook * loading plugins + class for accessing passwords and addressbook * ported current trojita abook to addressbook plugin interface Next I will look at passwords (move current code to plugin) and then on KDE addressbook, kwallet and qt keychain support. > 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. > Ok, is there any document describing trojita coding style? > - 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.) > When loading plugins I need to specify full path to plugin library. Now it loads only plugins from current application directory. For loading plugins from system I need path: <install_prefix>/lib (or other library path). > - No foreach, use Q_FOREACH > Ok. > - 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. > I see that trojita loading localization from PKGDATADIR. But I do not see where it is defined. PKGDATADIR in autoconf/automake systems points somewhere to /usr/share/<program>/ but for storing libraries it is better to use system /usr/lib (or other dynamic linker path like /usr/lib/x86_64-linux-gnu/ on multiarch). Library path can be reused from cmake. > - The signal/slot signatures in connect(...) are not > normalized > Do you mean this? http://marcmutz.wordpress.com/effective-qt/prefer-to-use-normalised-signalslot-signatures > - Could addressbook->requestCompletion ever return 0? You're > checking its return values. > It returns pointer from plugin implementation which can returns also NULL. > - 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? > Ok, I will look at it. > - 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. > - 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. > - What is the NullAddressbookCompletionJob and why is it > returning bogous data? > Plugin for testing if interface, plugin loader and completion in gui working. Nothing for users. > - 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. > Ok. > > 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)) > Now I have problems with cmake. It does not autogenerate two moc files, so I had to add QT4_GENERATE_MOC to force cmake for manuall generation. Another problem is that all plugins not compiling without adding line "#include "file.moc" at end of plugin cpp file. I spend more time with cmake and I looks like I'm not able to properly fix it. Now it compiling fine but with above hacks. Can somebody can look at cmakelists.txt and show me how to fix it properly? Another problem could be qt5. My linux distribution does not have qt5 yet. What do you use for compiling with qt5? I can use virtualized system in qemu, but need to know which distribution have qt5 out-of-box and working fine. > - 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. > I will rewrite git history and squash commits to have clean git history before sending merge requests. I think that reviewboard is not good for bigger merge requests (with more commits which depends on each other) and classic git format-patch or git request-pull (used for linux kernel) isnt good too. When my branch will be ready for merge I will send info (with commit links) here to ML for comments, so we can discuss about it. > Cheers, and please keep up the good work > Jan -- Pali Rohár pali.ro...@gmail.com
signature.asc
Description: This is a digitally signed message part.
