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

On Wed, 2009-02-11 at 17:55 +0000, Martyn Russell wrote:
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.


You can also just use git on this (branch is called push):;a=shortlog;h=push

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

* is an 'indexer' module which runs in
tracker-indexer. It will pump the data actually into the database.

* is a 'deamon' module which runs in trackerd. It will
receive the data from the external application (presumably over DBus)
and it will 'pass it on' to the *

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

Added -module to the filename.

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

tracker-push.[ch] will apply to all modules which push data to Tracker.

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

It's not removed, it's at line 68 in tracker-evolution-registrar.c

#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",
+             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? 

No it doesn't, the structures are not the same, they don't have the same
purpose. And the init code is also different for the daemon and the
indexer modules.

So these two can't be shared.

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

Sure. For errors I often use the concept of 'try, except, finally', but
with C not having support for exceptions and such try-except blocks I
often simulate it by using a goto.

Which is probably one of the few situations where goto isn't necessarily
a bad idea.

But I'll replace them where possible.

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

So, what is the problem here exactly?

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

I think it's better that way because the module developer's enable and
disable are really distinctive kinds of functions. These are btw called
when the DBus service disappears and appears.

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

Yeah, I can commit this today.

I think it's best if you just use the git at;a=shortlog;h=push

Philip Van Hoof, freelance software developer
home: me at pvanhoof dot be 
gnome: pvanhoof at gnome dot org

Attachment: tracker-push.c
Description: Text Data

Attachment: tracker-push.h
Description: Text Data

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