On Mon, 2009-01-12 at 12:36 +0000, Martyn Russell wrote:
Superb wiki page, superb patch(es) and really really good work here! Most of my comments are really pedantic, so bare that in mind - there isn't much else to pick at :)
Thanks! New patch attached
#1, I am not sure "tracker-evolution-plugin" is the right name for this directory and if it is in the right place in the source tree too. I am just thinking that if we have another plugin for another application which has a push mechanism for Tracker, we might want another plugin here. So maybe a src/plugins/... is better?
Moved to src/plugins/evolution
#2, Can we avoid shortening names for things like build_evoplug and HAVE_EVOPLUG here. We should be consistent with names for everything. It also isn't helpful to people learning how Tracker works. +if HAVE_EVOPLUG +build_evoplug = tracker-evolution-plugin +endif +
Fixed, using HAVE_EVOLUTION_PLUGIN now and NO_EVOLUTION_PLUGIN for the define being passed at INCLUDES.
#3, Putting your name here is interesting, is this something that Nokia approve of?
Asked to the project manager at Nokia. Awaiting his comment on this.
#4, I don't think we need to include glib.h AND the dbus-glib bindings here. Just the later one should suffice: +#include <glib.h> +#include <dbus/dbus-glib-bindings.h>
Fixed
#5, These arguments should be on separate lines for tracker-evolution-indexer.h: +void tracker_evolution_idx_init (TrackerConfig *config, TrackerIndexer *indexer);
Fixed
#6, Every header/source function with a DBusGMethodInvocation should also have a GError parameter after the context.
Fixed
#7, The tracker-evolution-common.h header has some duplication (namely dbus_async_return_if_fail and possibly the error domain) from libtracker-common's DBus headers. Can we not just link/use those? Also, not sure if it makes sense for the error domain to be different, if it does, we should be using a proper namespace other than DBUS_ERROR_DOMAIN and DBUS_ERROR).
This was done to avoid having to link the EPlugin with Tracker's shared object files. The EPlugin runs in Evolution's process, not in Tracker's. Linking with Tracker's so files should in my opinion be avoided (besides, the only symbol I needed was that one, which was a macro).
#8, In src/tracker-evolution-plugin/tracker-evolution-plugin.c, you include gtk.h - is that necessary? I did a quick scan and didn't see anything related. Also, we should make sure the include blocks are grouped properly - which they are except for the #include <camel/camel-session.h> which is in the middle of the mail/* includes, is that required?
Removed both the gtk.h and the gconf.h includes. The camel-session.h include is indeed needed but I have moved it to its block. Fixed
#9, If you don't have tracker-evolution-plugin requirements in configure then trackerd/Makefile.am tries to find the .la file for a library that never gets built. Some conditionals are required in there I think.
Fixed
As you can tell, I tried to build it at point #9 to get the diffs and code syntax highlighting in emacs, so that's all I have for now. If you could fix those things, I can re-review from this point forward tomorrow.
Ok New patch: pvanhoof tinc:~/repos/gnome/tracker/eplug$ svn status | grep ^M M src/tracker-indexer/tracker-main.c M src/tracker-indexer/modules/evolution.c M src/tracker-indexer/modules/Makefile.am M src/tracker-indexer/Makefile.am M src/trackerd/tracker-main.c M src/trackerd/Makefile.am M src/Makefile.am M tests/trackerd/Makefile.am M tests/tracker-indexer/Makefile.am M configure.ac M data/services/email.metadata M data/db/sqlite-tracker.sql pvanhoof tinc:~/repos/gnome/tracker/eplug$ svn status | grep ^A A src/plugins A src/plugins/evolution A src/plugins/evolution/tracker-evolution-indexer.h A src/plugins/evolution/tracker-evolution-plugin.h A src/plugins/evolution/org-freedesktop-Tracker-evolution-plugin.eplug.xml A src/plugins/evolution/tracker-evolution.h A src/plugins/evolution/tracker-evolution-registrar.h A src/plugins/evolution/tracker-evolution-plugin.xml A src/plugins/evolution/tracker-evolution-common.h A src/plugins/evolution/Makefile.am A src/plugins/evolution/tracker-evolution-plugin.c A src/plugins/evolution/tracker-evolution-indexer.c A src/plugins/evolution/tracker-evolution-registrar.xml A src/plugins/evolution/tracker-evolution.c A src/plugins/evolution/tracker-evolution-registrar.c pvanhoof tinc:~/repos/gnome/tracker/eplug$ svn status | grep ^C pvanhoof tinc:~/repos/gnome/tracker/eplug$ svn status | grep ^D pvanhoof tinc:~/repos/gnome/tracker/eplug$ -- Philip Van Hoof, freelance software developer home: me at pvanhoof dot be gnome: pvanhoof at gnome dot org http://pvanhoof.be/blog http://codeminded.be
Attachment:
tracker-evo-plugin.diff.gz
Description: GNU Zip compressed data