Re: [Tracker] Review of the new Evolution/Generic E-mail metadata support for Tracker

[Martyn Russell <martyn imendio com>]

On 09/01/09 13:48, Philip Van Hoof wrote:
> As promised.
>
> Please review

Hi Philip,

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 :)

--

#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?


#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
+


#3, Putting your name here is interesting, is this something that Nokia 
approve of? Since they have some issues with our addresses in the 
ChangeLog too. In my experience adding it to the source is more relevant 
to them. We should check this. Also, no one else is doing this. We 
should do one thing or the other.

+ * Authors:
+ *  Philip Van Hoof <philip codeminded be>


#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>


#5, These arguments should be on separate lines for 
tracker-evolution-indexer.h:

+void  tracker_evolution_idx_init            (TrackerConfig *config, 
TrackerIndexer *indexer);


#6, Every header/source function with a DBusGMethodInvocation should 
also have a GError parameter after the context.


#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).


#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?


#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.


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.

--
Regards,
Martyn