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

[Martyn Russell <martyn imendio com>]

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 and the break is not indented correctly. 
Also, isn't a break required here for default (to prevent compiler 
warnings). At this point, I just realised I am not compiling this code 
:) (this is for tracker-evolution-indexer.c)

+       switch (prop_id) {
+
+       case PROP_INDEXER:
+               priv->indexer = g_value_dup_object (value);
+       break;


#12, We really should have a "Plugins" section for the configure summary 
and include the evolution build state there I think - so people know if 
it is built or not.


#13,  Looks like the permissions are not important here. We already have 
a function to do this (this is for tracker-evolution-indexer.c):

+#if defined(__linux__)
+                       fd = g_open (path, O_RDONLY | O_NOATIME, S_IRUSR | S_IWUSR);
+#else
+                       fd = g_open (path, O_RDONLY , S_IRUSR | S_IWUSR);
+#endif

The function we already have is tracker_file_open().


#14, Since you do a while loop in perform_set() and expect predicates 
and values to be the same length, I would add a check in 
tracker_evolution_indexer_set():

+void
+tracker_evolution_indexer_set (TrackerEvolutionIndexer *object,
+                              const gchar *subject,
+                              const GStrv predicates,
+                              const GStrv values,
+                              DBusGMethodInvocation *context,
+                              GError *derror)
+{
+       dbus_async_return_if_fail (subject != NULL, context);
+
+       perform_set (object, subject, predicates, values);
+
+       dbus_g_method_return (context);
+}

Something like: dbus_async_return_if_fail (g_strv_length (predicates) == 
g_strv_length (values)); Also, this depends on what you do if both are 
NULL? Right now it looks like NULL just sets an empty hash table for 
EvolutionEmails. This is different to perform_unset(). Perhaps a check 
for both predicates/values being NULL should call unset?


#15, You use while() loops a lot for the same functionality you would 
get from a for() loop like this:

+       while (subjects[i] != NULL) {
+               GStrv preds = g_ptr_array_index (predicates, i);
+               GStrv vals = g_ptr_array_index (values, i);
+
+               perform_set (object, subjects[i], preds, vals);
+
+               i++;
+       }

For clarity, it makes more sense to use a for loop with the iterative 
action defined at the top with the conditions, this also avoids a 
scenario where the code changes and someone forgets to i++ when adding a 
call to continue; for example. This is more prevalent in perform_set() 
where you have to goto a position at the end of the loop just to call 
i++. Instead a continue; would normally be fine if this was a for loop. 
As a result, it makes it harder to maintain. Same in 
tracker_evolution_indexer_{un}set_many().


#16, I am guessing the _idx_ stands for "index" ? I am not really sure 
what it means if it does? It looks like it should be _indexer_ but 
conflicts with some other name space? If that's the case we should have 
a much better namespace for this whole file to make things clearer.


#17, The tracker-evolution-{plugin|registrar}.h needs function variables 
aligned. Also, there is the inclusion of glib.h before the dbus-glib 
bindings again (in the *plugin.h). We only need the later. The GError is 
missing from this API call (in the *plugin.h).


#18, Why do we need this (tracker-evolution-registrar.h)?

+ extern const DBusGObjectInfo 
dbus_glib_tracker_evolution_registrar_object_info;


#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");
+}


#20, For code blocks like this, it might make more sense to use 
tracker_is_empty_string(), it would be clearer at least.

+       if (g_strcmp0 (name, TRACKER_EVOLUTION_MANAGER_SERVICE) == 0) {
+		if ((!new_owner || strlen (new_owner) == 0) && (old_owner && strlen 
(old_owner) > 0))
+                       deactivate_registrar ();
+		if ((!old_owner || strlen (old_owner) == 0) && (new_owner && strlen 
(new_owner) > 0))
+                       activate_registrar ();
+       }


#21, There should be braces here and setting the object to NULL should 
be done inside that block, there is no point in checking if an object is 
NULL and then setting it anyway if it is (tracker-evolution.c). This is 
done in quite a few places actually.

+       if (private->object)
+               g_object_unref (private->object);
+       private->object = NULL;


#22, Again this should be a for loop:

+       while (names[i] != NULL) {
+               /* If the manager's service is found, start the registrar */
+               if (g_strcmp0 (names[i], TRACKER_EVOLUTION_MANAGER_SERVICE) == 0) {
+                       activate_registrar ();
+                       break;
+               }
+               i++;
+       }


#23, I think this function could be simplified:

+static gboolean
+is_enabled (TrackerConfig *config)
+{
+       gboolean enabled = TRUE;
+       GSList *disabled = tracker_config_get_disabled_modules (config);

+       /* If none of the disabled modules include the Evolution module,
+        * we assume we are enabled in the configuration. */
+
+       while (disabled && enabled) {
+               if (g_strcmp0 (disabled->data, "Evolution") == 0)
+                       enabled = FALSE;
+               disabled = g_slist_next (disabled);
+       }
+
+       return enabled;
+}

To something like (perhaps using g_strcmp0()):

        return g_slist_find_custom (tracker_config_get_disabled_modules 
(config),
                                                    "Evolution",
                                                    (GCompareFunc) strcmp)) != NULL;


#24, Spelling error here:

+       g_message ("Evolutiopn support service %s",
+                  private->is_enabled ? "enabled" : "disabled");


#25, I would probably try to get the bus first and then set up private - 
since it all hinges on that:

+       private->deactivating = FALSE;
+       private->config = g_object_ref (config);
+       private->is_enabled = is_enabled (config);
+       private->connection = dbus_g_bus_get (DBUS_BUS_SESSION, &error);
+
+       if (error)
+               goto error_handler;


#26, A proper clean up function should be used here. This way we don't 
leave anything to chance to not be freed:

+       g_static_private_set (&private_key,
+                             private,
+                             (GDestroyNotify) g_free);

It also means we don't have to go around cleaning up the private members 
in the shutdown function either:

+       if (private->is_enabled)
+               deactivate_dbus_client ();
+
+       g_object_unref (private->config);


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


#28, Doesn't it make sense to lock before creating the hash table here - 
if we need to lock at all?

+       priv->mutex = g_slice_new0 (GStaticRecMutex);
+       g_static_rec_mutex_init (priv->mutex);
+       priv->registrars = g_hash_table_new_full (g_str_hash, g_str_equal,
+                                                 (GDestroyNotify) g_free,
+                                                 (GDestroyNotify) g_object_unref);
+
+       g_static_rec_mutex_lock (priv->mutex);


#29, We can use the GList functions here:

+       copy = priv->registered_clients;
+       while (copy) {
+               ClientRegistry *creg = copy->data;
+               client_registry_info_free (creg);
+               copy = g_list_next (copy);
+       }
+       g_list_free (priv->registered_clients);

I.e.:

  g_list_foreach (priv->registered_clients,
                          (GFunc) client_registry_info_free,
                          NULL);
  g_list_free (priv->registered_clients);


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


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

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;


#33, We should avoid C++ comments:

        ninfo->signal = 0; // known

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


#35, Does it make more sense here to reference the connection 
(tracker-evolution-registrar.c):

+       priv->connection = connection; /* weak */


#36, This is not necessary, GObject initialises the private data for us:

+static void
+tracker_evolution_registrar_init (TrackerEvolutionRegistrar *object)
+{
+	TrackerEvolutionRegistrarPrivate *priv = 
TRACKER_EVOLUTION_REGISTRAR_GET_PRIVATE (object);
+
+       priv->idx_proxy = NULL;
+
+}tracker_evolution_registrar_set


#37, Again, perhaps we should check the predicates and values before 
calling the DBus proxy with it in 
tracker_evolution_registrar_set{_many}()? That and the length of the 
string arrays are equal. This depends on the policy we have here - can 
we use empty GStrv values?

--

Thanks again for the patch Philip ;)

--
Regards,
Martyn