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

[Martyn Russell <martyn imendio com>]

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:

 - libtracker_evolution_indexer.la (now renamed)
 - libtracker-evolution-daemon-module.la
 - libtracker-evolution-indexer-module.la
 - ...


#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 = libtracker-evolution-daemon-module.la
+pushi_modules_LTLIBRARIES = libtracker-evolution-indexer-module.la


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

+void
+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?

        handle_error:

        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):

+void
+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:

-#ifdef HAVE_EVOLUTION_PLUGIN

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? :)

--
Regards,
Martyn