[Tracker] ontology-cope-for-master branch review

[Martyn Russell <martyn lanedo com>]

Hi all,

Philip recently worked on the ontology-cope-for-master branch to add 
support for real time ontology updates (usually by 3rd party apps that 
are installed for example). This is the review of that branch:

  http://git.gnome.org/browse/tracker/log/?h=ontology-cope-for-master

Warning, pedandic comments follow :)

--

1. Can we avoid using "Priv" and use "Private" for TrackerOntologyPriv 
et al, we use "Private" everywhere else. If we don't in some other place 
then that place is wrong too.


2. tracker_ontology_get_last_modified() returns a time_t but you cast to 
a gint64 before returning the value, shouldn't we cast to the return 
type OR change the return type?


3. Just a note, in get_ontology_from_file() you do this:

+ TrackerTurtleReader *reader;
+ GError              *error = NULL;
+ GHashTable          *ontology_uris;
+ TrackerOntology     *ret = NULL;

Variable alignments are not necessary any more.


4. You reference the ontology then unreference it in the next statement 
in get_ontology_from_file():

+ g_hash_table_insert (ontology_uris,
+                      g_strdup (subject),
+                      g_object_ref (ontology));
+
+ g_object_unref (ontology);

Why not save the extra CPU cycles and pass ontology without ref/unref there?


5. Just a question really, but I wonder if we should have some debugging 
in get_ontology_from_file() so we know what file we are parsing and what 
we are doing during that phase?


6. In get_ontology_from_file() I notice we return a referenced 
TrackerOntology, it would probably be more efficient to return the 
pointer in the hash table instead to save the additional ref/unref.


7. In get_ontologies() it is faster to sort the list AFTER prepending 
the items than each time:

+ sorted = g_list_insert_sorted (sorted,
+                                g_strdup (conf_file),
+                                (GCompareFunc) strcmp);

Probably not noticeable here though and I notice it is just moved code 
that was there already anyway :)


8. We don't generally use \t in debugging because the indentation is too 
much, 2 spaces is normally what's used, also, please use a parameter per 
line, it is clearer to see what is going on when you have a bunch of 
string manipulation going on.

+ g_debug ("%sAltering database for class '%s' property '%s': single 
value (%s)",
+          in_alter ? "" : "\t", service_name, field_name,
+          in_alter ? "alter" : "create");


9. Does this mean we only support NEW ontologies, not UPDATES?

+ /* TODO: copy with tracker_property_get_is_new together with a
+ * tracker_property_get_multiple_values (ALTER TABLE situation) */


10. Don't use tracker_ namespace for internal functions, coding style:

+ static gint
+ tracker_data_update_get_new_service_id (void)

There are others in that file too of course, but they should be fixed 
another time.

--

Really good work here Philip, this is not an easy bit of work to review 
or implement so kudos to you for doing this!

--
Regards,
Martyn