GNOME.org
 

Re: [Tracker] Merging Turtle branch to Trunk

Hi there,

Here's a new patch

svn merge -r 2696:2709 ../branches/turtle

svn diff ain't adding the created files this time (for some reason), so
I added them manually to this mail


On Tue, 2008-12-16 at 14:37 +0100, Philip Van Hoof wrote:

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

Attachment: merge-turtle-to-trunk.diff
Description: Text Data

Attachment: tracker-removable-device.c
Description: Text Data

Attachment: tracker-removable-device.h
Description: Text Data

Attachment: tracker-turtle.c
Description: Text Data

Attachment: tracker-turtle.h
Description: Text Data

Attachment: tracker-backup.c
Description: Text Data

Attachment: tracker-backup.h
Description: Text Data

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