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

[Philip Van Hoof <spam pvanhoof be>]

On Mon, 2010-02-22 at 13:52 +0000, Martyn Russell wrote:
> 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.


Fixed

> 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?


Fixed, no casting was needed at all here

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

Okay

> 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?

Fixed

> 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?

No, this is done for each ontology file at startup, yet the file isn't
processed (notice the break): it's only to get the nao:lastModified date
for comparison later. 

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

ok (fine for me)

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

Yes, I prefer not to change this code because it alters existing code
while this code moving was done for reusing said code. Such a change
(sorting after prepending) wouldn't have anything to do with the feature
being implemented here.

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

Fixed

> 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) */

This comment is removed in a later commit

> 10. Don't use tracker_ namespace for internal functions, coding style:
>
> + static gint
> + tracker_data_update_get_new_service_id (void)

This change was reverted in a later commit, and thus not related to the
branch as a whole.

> 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!


No problem :)


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