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



Here's a patch that also addresses Carlos's remarks which he posted in
comments on the bug that I opened for this:

http://bugzilla.gnome.org/show_bug.cgi?id=565091#c16

I also attached this patch with some replies to Carlos's remarks:

http://bugzilla.gnome.org/show_bug.cgi?id=565091#c20

This patch also addresses martyn's first remarks (it's a continuation on
the last patch that I just sent, martyn).

Hf

On Mon, 2009-01-12 at 15:49 +0100, Philip Van Hoof wrote:
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



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]