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
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) \ + -DNO_EVOLUTION_PLUGIN
Fixed
#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.
+/* 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;
Fixed
#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.
Done
#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().
Fixed, replaced
#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?
Added proper assertions for a lot of those functions.
#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().
Will leave as is. There were a few other reasons for some of those while loops.
#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.
Fixed, improved the names of those two functions.
#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).
This seems to have already been fixed. But I had made some changes since I sent the last patch.
#18, Why do we need this (tracker-evolution-registrar.h)? + extern const DBusGObjectInfo dbus_glib_tracker_evolution_registrar_object_info;
Because of trackter-evolution-registrar.c and tracker-evolution.c both needing it and tracker-evolution-registrar-glue.h not using the ... #ifndef __LIBTRACKER_EVOLUTION_REGISTRAR_GLUE H__ #define __LIBTRACKER_EVOLUTION_REGISTRAR_GLUE_H__ #endif ... trick. I also noticed that tracker-evolution-registrar-glue.h declares it static so I changed it to have a symbol that I can share between tracker-evolution.c and tracker-evolution-registrar.c and and an assignment to the symbol in the latter. Fixed that.
#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. I think the cast is fine as long as we know about it (if we see a negative value in the DB, it means a very large value). I also don't think that there's a sqlite3_bind_uint anyway.
#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 ();
+ }
Fixed
#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;
Fixed ...
#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++;
+ }
Leaving it that way, I don't see why it would make any code more elegant.
#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;
Simplified :)
#24, Spelling error here:
+ g_message ("Evolutiopn support service %s",
+ private->is_enabled ? "enabled" : "disabled");
Fixed
#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;
Fixed
#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);
Fixed. There was no leak because the unref was done in shutdown, but you are right that it's better to define a free-function at the level of the private key itself.
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);
Correct, fixed too
#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>
#28, Doesn't it make sense to lock before creating the hash table here - if we need to lock at all?
Moved up
+ 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);
Ah yes, bad habits. Fixed
#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. The spacing issues have been fixed.
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. In case of Camel a gchar is often a normal char, a gint is often a normal int and a gpointer is usually a void*. Although the sole reason for that is that Camel was done before our beloved glib had all these fancy toys and typedefs and, also, because the (former) developers of Camel hated the typedefs. I have no opinion myself but I prefer to follow the library's .h file.
#33, We should avoid C++ comments:
ninfo->signal = 0; // known
Fixed
#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.
#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. http://dbus.freedesktop.org/doc/dbus-glib/dbus-glib-DBusGConnection.html#DBusGConnection
#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
Fixed. Habit
#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?
Fixed -- 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