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 <phi...@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
_______________________________________________
tracker-list mailing list
tracker-list@gnome.org
http://mail.gnome.org/mailman/listinfo/tracker-list