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

[Martyn Russell <martyn imendio com>]

On 14/01/09 18:04, Philip Van Hoof wrote:
> On Wed, 2009-01-14 at 15:39 +0000, 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<dbus/dbus-glib-lowlevel.h>

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