Re: [Tracker] Review of the new Evolution/Generic E-mail metadata support for Tracker
On Thu, 2009-01-15 at 11:20 +, Martyn Russell wrote: > On 14/01/09 18:04, Philip Van Hoof wrote: > OK, from my point of view, we can commit this patch. Carlos are you > happy with this too or do you have anything further to add? Carlos's final review happened on the bug. Committed as 2794 and closed the bug as fixed. Thanks for all the reviews! -- 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 ___ tracker-list mailing list tracker-list@gnome.org http://mail.gnome.org/mailman/listinfo/tracker-list
Re: [Tracker] Review of the new Evolution/Generic E-mail metadata support for Tracker
On 14/01/09 18:04, Philip Van Hoof wrote:
On Wed, 2009-01-14 at 15:39 +, Martyn Russell wrote:
Thanks Philip.
OK, so I have finished commenting on the rest of the patch. I haven't
been able to build it yet because I am still building the evolution
development environment. So any further comments will be appended here
later.
Just want to say, that this is a huge patch and amount of work and you
have really done well to coordinate with the Evolution developers, the
rest of the Tracker developers and keep things up to date in Bugzilla
and on the mailing lists. So kudos to you Philip and thanks for doing
the work!
Thanks a lot for all those nice words
;)
#5, Just wondering why you did this?
+ tracker_module_config_init ();
+
tracker_turtle_init ();
tracker_thumbnailer_init (config);
- tracker_module_config_init ();
-
Ah, tracker_thumbnailer_init is getting a config passed that ain't
initialized.
You know that TrackerConfig isn't the same as TrackerModuleConfig right?
I don't think the module config needs to be intiated first.
#10, About these #defines, there are ways to specify the namespace used
by DBus from the xml files and this could avoid the need for these
defines. Did you know/try this?
I did, problem is that the XML is used twice. Once where the namespace
matches the C API, and one where it doesn't. This is for the one where
it doesn't.
I figured that this was more easy and more nice than having two XML
files.
Ah I see.
#19, I am a bit worried about the type mismatch here. I wonder if it
makes sense to have gint with the number of days not a time - it depends
on the granularity you want here I suppose.
+static guint
+get_stored_last_checkout (void)
+{
+ return (guint) tracker_data_manager_get_db_option_int
("EvolutionLastCheckout");
+}
No, in-seconds is the required granularity.
OK.
#27 I think we should collect all crack like this
(tracker-evolution-plugin.c) and keep them together. Also, I checked,
the DBus API is in my documentation. It looks like we just aren't
including the dbus-glib includes we need :)
+int e_plugin_lib_enable (EPluginLib *ep, int enable);
+/* Not in the public headers of DBus? */
+gchar* dbus_g_method_get_sender (DBusGMethodInvocation *context);
I had this fixed already. For future reference, the correct fix was:
#include
OK.
#30, Again, you have a lot of while (copy) { ...; next_in_list; } in
this code (tracker-evolution-plugin.c) - which would be better as a for
loop so the conditions are all clear at the top.
Leaving as is. At least for now.
#31, Can we avoid setting a local variable and testing the condition all
in one place like this:
+ if (!account->enabled ||!(uri = account->source->url))
+ return;
This is Evolution's structures. I have to check for both for
correctness. It's also Evolution who changes the values of those
instance's struct members, not just us.
Remember that this code runs in Evolution's process.
OK, as far as I could see, it was just being set to a variable in the
local scope (called uri) and used in that function intermittently as was
account->source->url - which is a bit confusing. But it is no big issue.
It isn't very clear. It makes more sense to me to just use
account->source->url the whole time, it is rarely used as url. Also
there are some spacing issues in that line above of course ;)
#32, There are a couple of char* cases which should be gchar* in this file.
+ char *uri = reg_info->uri;
Fixed that one, but note that some are Camel API matching. For example
Camel doesn't use gpointer but void*. Although the two are exactly the
same I prefer to reflect what the library's .h wants.
I agree. I suspected that might be the case.
#34, I see you use:
+ guint latest = smallest;
But guint can be 32 bits depending on architecture. It makes sense to
make this guint64 like the smallest variable is defined:
+ guint64 smallest = (guint64) time (NULL);
Agree, just forgot about that one while I was converting all those to
guint64. Thanks for spotting this, it would have been a horrible bug.
NP ;)
#35, Does it make more sense here to reference the connection
(tracker-evolution-registrar.c):
+ priv->connection = connection; /* weak */
DBusGConnection is not a GObject afaik.
We can use dbus_g_connection_ref() and dbus_g_connection_unref().
--
OK, from my point of view, we can commit this patch. Carlos are you
happy with this too or do you have anything further to add?
--
Regards,
Martyn
___
tracker-list mailing list
tracker-list@gnome.org
http://mail.gnome.org/mailman/listinfo/tracker-list
Re: [Tracker] Review of the new Evolution/Generic E-mail metadata support for Tracker
On Wed, 2009-01-14 at 15:39 +, Martyn Russell wrote:
> Thanks Philip.
>
> OK, so I have finished commenting on the rest of the patch. I haven't
> been able to build it yet because I am still building the evolution
> development environment. So any further comments will be appended here
> later.
>
> Just want to say, that this is a huge patch and amount of work and you
> have really done well to coordinate with the Evolution developers, the
> rest of the Tracker developers and keep things up to date in Bugzilla
> and on the mailing lists. So kudos to you Philip and thanks for doing
> the work!
Thanks a lot for all those nice words
> The same preface applies here to my last email. A lot of these comments
> are pedantic, there aren't many big issues at all that I could see :)
Okay.
New patch is attached btw. I have replied a little bit inline.
> #1, I think the normal thing here is to just not set anything instead
> of using /dev/null here. I would need to check where it is used. Also,
> we should be calling that variable EVOLUTION_PLUGIN_DIR really.
>
> Is this Evolution plugin optional at all or required?
>
> +if test x$have_evoplug == "xyes"; then
> +EVOPLUG_INSTALL_DIR=`$PKG_CONFIG evolution-plugin --variable=plugindir`
> +else
> +EVOPLUG_INSTALL_DIR=/dev/null
> +fi
Fixed
> #2, Isn't this exactly the same as Email:Body
> (data/services/email.metadata)?
>
> +[Email:Text]
> +DisplayName=Text
> +Description=The body of the E-mail
> +DataType=Indexable
> +Weight=1
You are right, fixed.
> #3, This seems a little over the top to me. I think it is clearer and
> easier if we just have a config.h definition instead of adding it to the
> CFLAGS in the Makefile.am. Also, we should possibly just compile support
> in for this regardless since we already have #ifdefs around creating a
> new object here anyway.
>
> +INCLUDES += -DNO_EVOLUTION_PLUGIN
> +
> +libtracker_module_evolution_la_SOURCES +=\
> + evolution-imap-db.h \
> + evolution-imap-db.c
>
>
> #4, I would change the name of this too because in a lot of cases you
> will find your comparing a double negative. This is quite taxing for the
> brain generally, compared to just #ifdef HAVE_EVOLUTION_PLUGIN:
>
> +#ifndef NO_EVOLUTION_PLUGIN
>
Fixed (both #3 and #4 as they are related)
> #5, This code block should be done in a different order I think
> (tracker-indexer/tracker-main.c)
>
> +#ifndef NO_EVOLUTION_PLUGIN
> + tracker_evolution_idx_init (config, indexer);
> +#endif
>
> + tracker_data_manager_shutdown ();
> + g_object_unref (language);
> +
>
> I would group the unref and shutdown with the others. The unref would be
> before the config unref and the shutdown can occur with the other
> shutdown calls (since the data_manager should have a reference on the
> language anyway). The way the startup/shutdown calls were done is by
> FILO but this is not a requirement. The other reason for doing this, is
> that the timeout is then cleaned up before we really start to clean up
> other subsystems. The same should probably be done for the turtle
> subsystem, but that's another patch.
Fixed
> #6, Again, I would remove the definition from the trackerd/Makefile.am
> here and just do this in one place in the config.h.
>
> +if !HAVE_EVOLUTION_PLUGIN
> +INCLUDES += -DNO_EVOLUTION_PLUGIN
> +endif
> +
Fixed
>
> #5, Just wondering why you did this?
>
> + tracker_module_config_init ();
> +
> tracker_turtle_init ();
> tracker_thumbnailer_init (config);
>
> - tracker_module_config_init ();
> -
Ah, tracker_thumbnailer_init is getting a config passed that ain't
initialized.
> #7, Again, I would group all shutdown calls together (in
> trackerd/tracker-main.c):
>
> +#ifndef NO_EVOLUTION_PLUGIN
> + tracker_evolution_shutdown ();
> +#endif
Fixed
>
> #8, The initialisation here should be higher I think, with the other
> initialisations. I don't know how important this is yet. For a lot of
> subsystems these have to be in a particular order. All of them are done
> typically before DBus objects are registered on the session bus (but
> after the DBus name is acquired of course):
>
> +#ifndef NO_EVOLUTION_PLUGIN
> + tracker_evolution_init (config);
> +#endif
Leaving as is, I want this to happen first:
if (!tracker_dbus_register_objects (config,
language,
file_index,
email_index,
private->processor)) {
return EXIT_FAILURE;
}
> #9, The test Makefiles for the indexer and daemon just explicitly
> declare NO_EVOLUTION_PLUGIN. Shouldn't this depend on if we build it
> (i.e. like every other Makefile.am does or from config.h which I
> personally prefer).
>
> - $(GLIB2_CFLAGS)
> + $(GLIB2_CFLAGS)
Re: [Tracker] Review of the new Evolution/Generic E-mail metadata support for Tracker
On 12/01/09 16:07, Philip Van Hoof wrote: New patch: Thanks Philip. OK, so I have finished commenting on the rest of the patch. I haven't been able to build it yet because I am still building the evolution development environment. So any further comments will be appended here later. Just want to say, that this is a huge patch and amount of work and you have really done well to coordinate with the Evolution developers, the rest of the Tracker developers and keep things up to date in Bugzilla and on the mailing lists. So kudos to you Philip and thanks for doing the work! The same preface applies here to my last email. A lot of these comments are pedantic, there aren't many big issues at all that I could see :) -- #1, I think the normal thing here is to just not set anything instead of using /dev/null here. I would need to check where it is used. Also, we should be calling that variable EVOLUTION_PLUGIN_DIR really. Is this Evolution plugin optional at all or required? +if test x$have_evoplug == "xyes"; then +EVOPLUG_INSTALL_DIR=`$PKG_CONFIG evolution-plugin --variable=plugindir` +else +EVOPLUG_INSTALL_DIR=/dev/null +fi #2, Isn't this exactly the same as Email:Body (data/services/email.metadata)? +[Email:Text] +DisplayName=Text +Description=The body of the E-mail +DataType=Indexable +Weight=1 #3, This seems a little over the top to me. I think it is clearer and easier if we just have a config.h definition instead of adding it to the CFLAGS in the Makefile.am. Also, we should possibly just compile support in for this regardless since we already have #ifdefs around creating a new object here anyway. +INCLUDES += -DNO_EVOLUTION_PLUGIN + +libtracker_module_evolution_la_SOURCES += \ + evolution-imap-db.h \ + evolution-imap-db.c #4, I would change the name of this too because in a lot of cases you will find your comparing a double negative. This is quite taxing for the brain generally, compared to just #ifdef HAVE_EVOLUTION_PLUGIN: +#ifndef NO_EVOLUTION_PLUGIN #5, This code block should be done in a different order I think (tracker-indexer/tracker-main.c) +#ifndef NO_EVOLUTION_PLUGIN + tracker_evolution_idx_init (config, indexer); +#endif + tracker_data_manager_shutdown (); + g_object_unref (language); + I would group the unref and shutdown with the others. The unref would be before the config unref and the shutdown can occur with the other shutdown calls (since the data_manager should have a reference on the language anyway). The way the startup/shutdown calls were done is by FILO but this is not a requirement. The other reason for doing this, is that the timeout is then cleaned up before we really start to clean up other subsystems. The same should probably be done for the turtle subsystem, but that's another patch. #6, Again, I would remove the definition from the trackerd/Makefile.am here and just do this in one place in the config.h. +if !HAVE_EVOLUTION_PLUGIN +INCLUDES += -DNO_EVOLUTION_PLUGIN +endif + #5, Just wondering why you did this? + tracker_module_config_init (); + tracker_turtle_init (); tracker_thumbnailer_init (config); - tracker_module_config_init (); - #7, Again, I would group all shutdown calls together (in trackerd/tracker-main.c): +#ifndef NO_EVOLUTION_PLUGIN + tracker_evolution_shutdown (); +#endif #8, The initialisation here should be higher I think, with the other initialisations. I don't know how important this is yet. For a lot of subsystems these have to be in a particular order. All of them are done typically before DBus objects are registered on the session bus (but after the DBus name is acquired of course): +#ifndef NO_EVOLUTION_PLUGIN + tracker_evolution_init (config); +#endif #9, The test Makefiles for the indexer and daemon just explicitly declare NO_EVOLUTION_PLUGIN. Shouldn't this depend on if we build it (i.e. like every other Makefile.am does or from config.h which I personally prefer). - $(GLIB2_CFLAGS) + $(GLIB2_CFLAGS) \ + -DNO_EVOLUTION_PLUGIN #10, About these #defines, there are ways to specify the namespace used by DBus from the xml files and this could avoid the need for these defines. Did you know/try this? +/* These defines/renames are necessary for -glue.h */ +#define tracker_evolution_registrar_set tracker_evolution_indexer_set +#define tracker_evolution_registrar_set_many tracker_evolution_indexer_set_many +#define tracker_evolution_registrar_unset_many tracker_evolution_indexer_unset_many +#define tracker_evolution_registrar_unset tracker_evolution_indexer_unset +#define tracker_evolution_registrar_cleanup tracker_evolution_indexer_cleanup +#define dbus_glib_tracker_evolution_indexer_object_info dbus_glib_tracker_evolution_registrar_object_info #11, Unnecessary spaces here
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 +, 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 > > +#include > > 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 > > 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: > > pvanh...@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 > pvanh...@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-ev
Re: [Tracker] Review of the new Evolution/Generic E-mail metadata support for Tracker
On Mon, 2009-01-12 at 12:36 +, 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 > +#include 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 > 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: pvanh...@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 pvanh...@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 pvanh...@tinc:~/repos/gnome/tracker/eplug$ svn status | grep ^C pvanh...@tinc:~/repos/gnome/tracker/eplug$ svn status | grep ^D pvanh...@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 tracker-evo-plugin.diff.gz Descriptio
Re: [Tracker] Review of the new Evolution/Generic E-mail metadata support for Tracker
On 12/01/09 12:36, Martyn Russell wrote: 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 :) I should add, this is about 1/3 of the total review looking at the scrollbar for the patch :) I should be able to find time to finish this tomorrow. -- Regards, Martyn ___ tracker-list mailing list tracker-list@gnome.org http://mail.gnome.org/mailman/listinfo/tracker-list
Re: [Tracker] Review of the new Evolution/Generic E-mail metadata support for Tracker
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 #4, I don't think we need to include glib.h AND the dbus-glib bindings here. Just the later one should suffice: +#include +#include #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 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
Re: [Tracker] Review of the new Evolution/Generic E-mail metadata support for Tracker
As promised.
Please review
I also attached the two remaining bugfixes for Evolution and Evolution's
Data Server as patches.
This patch depends on those bugfixes:
http://bugzilla.gnome.org/show_bug.cgi?id=565082
http://bugzilla.gnome.org/show_bug.cgi?id=565681
This one is solved, reviewed and committed to EDS's trunk:
http://bugzilla.gnome.org/show_bug.cgi?id=566279
Note that this patch's own bug is:
http://bugzilla.gnome.org/show_bug.cgi?id=565091
More information about this work can be found here (please do read this
before reviewing):
http://live.gnome.org/Evolution/Metadata
Have fun reviewing (and don't forget to read the wiki page).
On Wed, 2009-01-07 at 13:57 +0100, Philip Van Hoof wrote:
> This Friday I will post a patch that will need a review.
>
> The patch implements this proposal. The people who are interested in
> reviewing it should read it first:
>
> http://live.gnome.org/Evolution/Metadata
>
> Short version:
>
> "Several applications on our desktop computers and mobile devices want
> to know about the metadata that E-mail clients have about the messages
> they know about. This document is an attempt at specifying a D-Bus API
> for retrieving this metadata from the clients in a way that the
> components will see themselves being updated by pushing (Evolution will
> send new data actively, as it's a push mechanism, not a pull
> mechanism)."
>
> I have been posting intermediate patches, so if you can't wait with
> reviewing you can already go look at the current state of the patch at
>
> http://bugzilla.gnome.org/show_bug.cgi?id=565091
>
>
> But life ain't awesome just yet. The work depends on bugs and patches
> for Evolution Data Server. More information about its dependencies can
> be found here:
>
> http://bugzilla.gnome.org/show_bug.cgi?id=565082
> http://bugzilla.gnome.org/show_bug.cgi?id=565681
> http://bugzilla.gnome.org/show_bug.cgi?id=566279
>
> The reviewer for those patches is Srinivasa Ragavan (Evolution
> maintainer for Novell).
>
> Reviewers that I have in mind for the Tracker parts at this moment are:
>
> - Martyn Russel: contributed to the current support and various
>infrastructure related to this
> - Carlos Garnacho: developed the current support
>
> Other people should feel free to review alongside if they want to do
> this.
>
>
--
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
tracker-evo-plugin.diff.gz
Description: GNU Zip compressed data
Index: camel/camel-db.c
===
--- camel/camel-db.c (revision 9848)
+++ camel/camel-db.c (working copy)
@@ -769,18 +769,17 @@
return ((camel_db_command (cdb, query, ex)));
}
-int
-camel_db_prepare_message_info_table (CamelDB *cdb, const char *folder_name, CamelException *ex)
+
+
+static int
+camel_db_create_message_info_table (CamelDB *cdb, const char *folder_name, CamelException *ex)
{
int ret;
char *table_creation_query, *safe_index;
/* README: It is possible to compress all system flags into a single column and use just as userflags but that makes querying for other applications difficult an d bloats the parsing code. Instead, it is better to bloat the tables. Sqlite should have some optimizations for sparse columns etc. */
-
- table_creation_query = sqlite3_mprintf ("CREATE TABLE IF NOT EXISTS %Q ( uid TEXT PRIMARY KEY , flags INTEGER , msg_type INTEGER , read INTEGER , deleted INTEGER , replied INTEGER , important INTEGER , junk INTEGER , attachment INTEGER , msg_security INTEGER , size INTEGER , dsent NUMERIC , dreceived NUMERIC , subject TEXT , mail_from TEXT , mail_to TEXT , mail_cc TEXT , mlist TEXT , followup_flag TEXT , followup_completed_on TEXT , followup_due_by TEXT , part TEXT , labels TEXT , usertags TEXT , cinfo TEXT , bdata TEXT )", folder_name);
-
+ table_creation_query = sqlite3_mprintf ("CREATE TABLE IF NOT EXISTS %Q ( uid TEXT PRIMARY KEY , flags INTEGER , msg_type INTEGER , read INTEGER , deleted INTEGER , replied INTEGER , important INTEGER , junk INTEGER , attachment INTEGER , msg_security INTEGER , size INTEGER , dsent NUMERIC , dreceived NUMERIC , subject TEXT , mail_from TEXT , mail_to TEXT , mail_cc TEXT , mlist TEXT , followup_flag TEXT , followup_completed_on TEXT , followup_due_by TEXT , part TEXT , labels TEXT , usertags TEXT , cinfo TEXT , bdata TEXT, created TEXT, modified TEXT )", folder_name);
ret = camel_db_add_to_transaction (cdb, table_creation_query, ex);
-
sqlite3_free (table_creation_query);
/* FIXME: sqlize folder_name before you create the index */
@@ -810,18 +809,149 @@
ret = camel_db_add_to_transaction (cdb, table_creation_query, ex);
g_free (safe_index);
sqlite3_free (table_creation_query);
-
+
return ret;
}
+static int
+camel_db_migrate_folder_prepare (CamelDB *cdb, const char *folder_name, gint version, CamelException *ex)
+{
+ int ret = 0;
+ char *table_cr
