Re: [Tracker] Refactored the src/plugins & (new) Evolution support

Philip Van Hoof wrote:
This is a new patch but with the indentation right for the two new files
this time

Hi Philip,

Some initial comments. I have not had a chance to try the new code yet.

#1, I don't have src/trackerd/tracker-push.[ch] to patch, so the patch fails to apply. This also means I have some more reviewing to do once I have those files.

#2, Before we go on, can you explain ALL the libraries and what they do. From the names I don't really have any concept of what they are supposed to be doing. As far as I can see, we have:

 - (now renamed)
 - ...

#3, The naming convention here should be a bit more thought out I think. Something like libtracker-push-evolution-{daemon|index}.la. Either that or we follow the convention we have for the indexer, i.e. libtracker-module-evolution-{daemon|indexer}.la. They will be in this directory anyway: $libdir/tracker/push-modules/.

+pushd_modules_LTLIBRARIES =
+pushi_modules_LTLIBRARIES =

#4, Will tracker-push.[ch] apply to JUST evolution or all modules which push data to Tracker? If this is just for evolution I would rather that was reflected in the file name.

#5, Can we keep names consistent here please, i.e. keep to _init() - we use that everywhere in the Tracker source. If we don't we use _new().

-tracker_evolution_storer_init (TrackerConfig *config,
-                              TrackerIndexer *indexer)
+tracker_push_module_create (TrackerConfig *config,
+                           TrackerIndexer *indexer)

#6, Why did you remove this?

-G_DEFINE_TYPE (TrackerEvolutionRegistrar, tracker_evolution_registrar, G_TYPE_OBJECT)

#7, Do we really have to use goto statements here? Can't we just propagate the error up and return?

+       /* Creation of the registrar */
+       if (!org_freedesktop_DBus_request_name (dbus_proxy,
+                                               TRACKER_EVOLUTION_REGISTRAR_SERVICE,
+                                               DBUS_NAME_FLAG_DO_NOT_QUEUE,
+                                               &result, &nerror)) {
+               g_critical ("Could not setup DBus, %s in use\n",
+                           TRACKER_EVOLUTION_REGISTRAR_SERVICE);
+               goto handle_error;
+       }
+       if (nerror)
+               goto handle_error;

#8, Remember to use void in functions with no argument signature:

+tracker_push_shutdown ()

#9, I presume this is where Carlos suggested we use a GInterface:

+typedef struct {
+       TrackerPushRegistrar* (*create) (void);
+       void (*shutdown) (TrackerPushRegistrar *registrar);
+       TrackerPushRegistrar *registrar;
+       GModule *module;
+} PushModule;

Since we do this in both tracker-push.[ch] for the trackerd and tracker-indexer. It feels like we are duplicating a lot of stuff here to the other tracker-push.c file. Also, with the above code block, it looks like you are defining the functions again in the struct, doesn't it make more sense to define that struct in a header and be used by both trackerd and tracker-indexer? We should listen to Carlos, he is our resident GModule and GTypeModule expert :)

#10, Why do you wait until the end of the load_modules() function (tracker-indexer/tracker-push.c) to handle the error? Isn't it just easier to print the message, handle the error then return there instead of adding a goto?


        if (error) {
                g_debug ("%s\n", error->message);
                g_error_free (error);

#11, Can we not use while() for list iterations. It is less maintainable and error prone:

+       while (copy && !found) {
+               PushModule *p_module = copy->data;
+               TrackerPushRegistrar *registrar = p_module->registrar;

It means, if someone puts in a continue; in there, the whole block is broken because they forget to add:

  copy = g_list_next (copy);

It is easier for people to understand the rules of the iteration if they are all listed at the top of the code block:

  for (; copy && !found; copy = g_list_next (copy)) {

#12, I see you have more places where you use goto on an error just to print something out. To avoid jumping around the code to find out what is actually happening, it is more maintainable for places with errors to just be handled there and then and for return to be called (unless there are a lot of places in the function that would have to do the same operation at the end - like cleaning up memory). Can we do that and avoid goto unless we need to use it. It makes it harder to understand the flow of the functions.

#13, For the cases where we reference another object here, we need to make sure we reference the new object first. We do this to protect ourselves from unreferencing the same object we are about to reference, (in the unlikely event that we set the object to the same object we already have in priv->object):

+tracker_push_registrar_set_object (TrackerPushRegistrar *registrar,
+                                  GObject              *object)
+       TrackerPushRegistrarPrivate *priv;
+       priv = TRACKER_PUSH_REGISTRAR_GET_PRIVATE (registrar);
+       if (priv->object)
+               g_object_unref (priv->object);
+       if (object)
+               priv->object = g_object_ref (object);
+       else
+               priv->object = NULL;

#14, For all the tracker_push_registrar_() calls which are properties, we need to add g_object_notify() calls when there is a change.

#15, Spacing:

+       if (TRACKER_PUSH_REGISTRAR_GET_CLASS (registrar)->enable) {
+               TRACKER_PUSH_REGISTRAR_GET_CLASS (registrar)->enable (registrar,

#16, Just wondering if this makes more sense to have a boolean in the ->enable method and no ->disable method.

+struct TrackerPushRegistrarClass {
+       GObjectClass parent_class;
+       void (*enable) (TrackerPushRegistrar *registrar,
+                       DBusGConnection      *connection,
+                       DBusGProxy           *dbus_proxy,
+                       GError              **error);
+       void (*disable) (TrackerPushRegistrar *registrar);

#17, Since we no longer use this:


Do we actually have an option to build the push modules at this point? Currently they are disabled by default. We should do the same here for a while until things stabilise.


Other than that, thanks for the patch and sorry for the delayed response to reviewing it. I would like to get this committed some time this week if possible, does that sound plausible to you Philip? :)


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]