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 timeHi 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.
Attached You can also just use git on this (branch is called push): http://git.codethink.co.uk/?p=tracker;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: - libtracker_evolution_indexer.la (now renamed) - libtracker-evolution-daemon-module.la - libtracker-evolution-indexer-module.la - ...
*-indexer-module.la is an 'indexer' module which runs in tracker-indexer. It will pump the data actually into the database. *-daemon-module.la 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 *-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
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)
Renamed
#6, Why did you remove this?
-G_DEFINE_TYPE (TrackerEvolutionRegistrar, tracker_evolution_registrar, G_TYPE_OBJECT)
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",
+ TRACKER_EVOLUTION_REGISTRAR_SERVICE);
+
+ goto handle_error;
+ }
+
+ if (nerror)
+ goto handle_error;
Fixed
#8, Remember to use void in functions with no argument signature: +void +tracker_push_shutdown ()
Fixed
#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?
handle_error:
if (error) {
g_debug ("%s\n", error->message);
g_error_free (error);
}
Fixed
#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)) {
...
}
Fixed
#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):
+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;
+}
+
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.
Done
#15, Spacing:
+ if (TRACKER_PUSH_REGISTRAR_GET_CLASS (registrar)->enable) {
+
+ TRACKER_PUSH_REGISTRAR_GET_CLASS (registrar)->enable (registrar,
+
Fixed
#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: -#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.
Added: --enable-evolution-push-module --enable-rss-push-module --enable-kmail-push-module
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 http://git.codethink.co.uk/?p=tracker;a=shortlog;h=push -- 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-push.c
Description: Text Data
Attachment:
tracker-push.h
Description: Text Data