On Tue, 2008-12-16 at 12:54 +0000, Martyn Russell wrote:
I will attach a new patch as soon as Ivan fixed issues #13 and #15
> #1, Didn't we decide to depend on Raptor? If so, we don't need the
> AC_DEFINE right?
>
> +# Check for Raptor
> +PKG_CHECK_MODULES(RAPTOR, [raptor], have_raptor=yes, have_raptor=no)
> +AC_SUBST(RAPTOR_CFLAGS)
> +AC_SUBST(RAPTOR_LIBS)
> +
> +if test x$have_raptor == "xyes"; then
> + AC_DEFINE(HAVE_RAPTOR, 1, [Raptor RDF parsers])
> +fi
It's not decided yet if raptor should become a mandatory dependency in
TRUNK just yet.
> #2. This definitely looks like a leak to me, since the "break" will
> leave the while() loop and the volume is freed ONLY in the opposite
> condition. Or am I missing something here?
>
> + while (g_hash_table_iter_next (&iter, &key, &value)) {
> + LibHalVolume *volume;
> + const gchar *udi;
> + const gchar *mp;
> +
> + udi = key;
> +
> + volume = libhal_volume_from_udi (priv->context, udi);
> +
> + if (!volume) {
> + g_message ("HAL device with udi:'%s' has no volume, "
> + "should we delete?",
> + udi);
> + continue;
> + }
> +
> + mp = libhal_volume_get_mount_point (volume);
> +
> + if (g_strcmp0 (mp, path) != 0) {
> + if (g_strrstr (path, mp)) {
> + found = TRUE;
> +
> + if (mount_point)
> + *mount_point = g_strdup (mp);
> +
> + if (available)
> + *available = libhal_volume_is_mounted
> (volume);
> + break;
> + }
> + }
> +
> + libhal_volume_free (volume);
Ah yes I see it now. Fixed.
>
> #3. We are checking the same thing twice here:
>
> field = tracker_ontology_get_field_by_name (field_name);
>
> + if (!field) {
> + g_warning ("Field name '%s' has isn't described in the
> ontology",
> field_name);
> + return;
> + }
> +
> + field = tracker_ontology_get_field_by_name (field_name);
> +
> g_return_if_fail (TRACKER_IS_FIELD (field));
> g_return_if_fail (tracker_field_get_multiple_values (field) == TRUE);
Fixed
> #4. I still don't like the Config and Language being static here. We
> already initialise the data module with them in
> tracker_data_manager_init (), why not just call
> tracker_data_manager_get_config() and
> tracker_data_manager_get_language() respectively?
>
> +typedef struct {
> + TrackerService *service;
> + guint32 iid_value;
> + TrackerLanguage *language;
> + TrackerConfig *config;
> +} ForeachInMetadataInfo;
> +
> +static TrackerConfig *config = NULL;
> +static TrackerLanguage *language = NULL;
> +
Fixed (first time I hear about those methods, but they are indeed
exactly what I needed for this)
> #5. Shouldn't we be adding the Service type == Folder check here before
> doing anything recursive?
>
> + if (service_id != 0) {
> + tracker_data_update_delete_service (service, service_id);
> + tracker_data_update_delete_service_recursively (service, (gchar
> *) path);
> + tracker_data_update_delete_all_metadata (service, service_id);
> + }
Fixed
> #6. Extra spaces here, there are further down in the patch for this file
> too:
>
> + GList *list;
> +
> + for (list = value; list; list = list->next)
> + set_metadata (field, list->data, user_data);
> +
> + }
Fixed
> #7. Can we remove these preprocessor operatives? and/or the debug
> statement itself? If not, we can always improve the debugging to
> actually mean something in the context of all the other logging.
>
> +#ifdef TURTLE_DEBUG
> + g_debug ("Q ERROR: %s\n", error->message);
> +#endif /* TURTLE_DEBUG */
Fixed
> #8. Since we decided to depend on Raptor, we can remove the #ifdefs in
> the tracker-turtle.[ch] I think. There are also a lot of code block
> blank lines which are not necessary.
This is undecided
> #9. Include order should be system, highest dependency -> lowest
> dependency. i.e. these should be the opposite way round in the
> tracker-turtle.h:
>
> +#include <libtracker-data/tracker-data-metadata.h>
> +#include <stdio.h>
>
>
> #10. Coding style, indentation, and again, stmt isn't a good name for a
> public typedef struct, can we call it something sensible like
> TrackerTurtleStatement? We shouldn't need the #idefs there too if we
> depend on Raptor.
>
> +typedef struct {
> + gconstpointer subject;
> + gint subject_type;
> + gconstpointer predicate;
> + gint predicate_type;
> + gconstpointer object;
> + gint object_type;
> + gpointer object_literal_datatype;
> + const guchar *object_literal_language;
> +} stmt;
> +#endif
Renamed to TrackerRaptorStatement
> #11. Can we avoid comments in line like this, it looks ugly, it is
> better to have it above the function or not at all.
>
> +void tracker_turtle_add_metadatas (TurtleFile *turtle,
> + GPtrArray /*
> <TrackerTurtleMetadataItem> */ *metadata_items);
Fixed
> #12. There are some serious coding style issues in
> tracker-removable-device.c. We could probably also use g_strcmp0() in a
> lot of places to reduce the code down a bit and make it easier to read.
> That's really not completely necessary though for this patch to get
> committed.
All strcmp()s are now g_strcmp0()s.
> #13. Is this a leak, looks like only str is freed, not service_type in
> the if (!field):
>
> + gchar *str = NULL;
> + gchar *uri;
> + gchar *service_type;
> +
> + g_value_init (&transform, G_TYPE_STRING);
> +
> + tracker_db_result_set_get (result_set, 0, &uri, -1);
> + tracker_db_result_set_get (result_set, 1, &service_type, -1);
> + tracker_db_result_set_get (result_set, 2, &metadata_id, -1);
> + tracker_db_result_set_get (result_set, 3, &str, -1);
> +
> + field = tracker_ontology_get_field_by_id (metadata_id);
> + if (!field) {
> + g_critical ("Field id %d in database but not in
> tracker-ontology",
> + metadata_id);
> + g_free (str);
> + return;
> + }
Ivan will fix this
> #14. There is no need to cast here in trackerd/tracker-main.c for
> callback_id:
>
>
> + gulong *callback_id = (gulong *)user_data;
> + GError *error;
> + static gint counter = 0;
Fixed
> #15. The || condition should be on the right here, also, this whole code
> block would be nicer in another function I think - which I commented on
> in my previous mail.
>
> + if (flags & TRACKER_DB_MANAGER_FORCE_REINDEX
> + || g_file_test (get_ttl_backup_filename, G_FILE_TEST_EXISTS)) {
>
Ivan will fix this
> Other than that, good work guys. I think we should try to get this
> commit this week!
--
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
_______________________________________________
tracker-list mailing list
[email protected]
http://mail.gnome.org/mailman/listinfo/tracker-list
