Hi!
On mié, 2010-02-03 at 12:08 +0000, Martyn Russell wrote:
> Hi all,
>
> Carlos and Philip recently worked on the anonymous-file-nodes branch to
> fix a regression with the way resources are stored in the database which
> was changed recently. This is the review of that branch:
>
> http://git.gnome.org/browse/tracker/log/?h=anonymous-file-nodes
>
>
> 1. We should use G_DIR_SEPARATOR_S here instead:
>
> + slash_uri = g_strconcat (source_uri, "/", NULL);
Hmm, we're building an uri here, I don't think we want uris with '\' if
tracker is ported (again?) to Windows :)
>
>
> 2. This looks like some left over crack that needs fixing :)
>
> - tracker_sparql_builder_object_iri (sparql, uri);
> + tracker_sparql_builder_object (sparql, "_:foo");
I guess _:foo can be renamed to something more descriptive, but that's
precisely the way to create autogenerated urns.
>
>
> 3. In the search-bar you have these changes, but surely urn should be
> changed for uri in ALL cases here, otherwise we have no ranking? I could
> be wrong though.
>
> + " ?uri ?title ?tooltip ?urn fts:rank(?urn) " \
I think it only changed for queries that return files (as of now at
least), I guess all queries could change to provide both urn and uri
>
>
> 4. Don't we need quotes round ?f (in add_tag_for_urns and again in
> remove_tag_for_urns)?
>
> - " ?urn nie:isStoredAs ?f ."
> + " ?urn nie:url ?f ."
not AFAIK, if we used a literal there such as \"%s\" we'd need these.
>
>
> 5. I would improve the this so the query is more readable personally:
>
> stmt = tracker_db_interface_create_statement (iface, "SELECT ID FROM
> \"nie:DataObject\" WHERE \"nie:DataObject\".\"nie:url\" = ?")
hmm? has that changed in this branch?
>
> It also looks erroneous to me. Why do we have this addition "." before
> \"nie:url\" ?
>
> Shouldn't that be \"nie:DataObject\" ?do . \"nie:url\" ...
>
>
> 6. I think we should add proper error handling here:
>
> + /* For fallback to the old behaviour, we could do this here:
> + * resource_id = tracker_data_query_resource_id (url); */
> + return;
hmm, I don't remember this changing in the branch. There should be only
changes to extractors, tracker-miner-fs and libtracker-common.
>
>
> 7. Hmm, not sure why this change was added, usually if result is set and
> > 0 in length then there was no error, checking that here seems
> pointless to me as we would still check the array and its length even if
> there was no error so? This is in
> src/tracker-writeback/tracker-writeback-consumer.c in sparql_query_cb().
>
> - if (result && result->len > 0) {
> + if (!error && result && result->len > 0) {
>
>
> 8. Again, I would make queries like this easier to read for maintainability:
>
> + query = g_strdup_printf ("SELECT ?url '%s' ?predicate ?object {"
> + " <%s> ?predicate ?object ; nie:url ?url ."
> + " ?predicate tracker:writeback true "
> + "}", data->subject, data->subject);
>
> Also, I presume this is valid SPARQL to put '%s' after ?url, looks to me
> like it will just be included as one of the columns in the results -
> isn't that a little inefficient (depending on how many results are
> returned).
>
>
> 9. I didn't know this could be used as an OPTIONAL replacement:
>
> - query = g_strdup_printf ("SELECT ?city ?state ?address ?country "
> - "WHERE { <%s> mlo:location ?location . "
> - "OPTIONAL { ?location mlo:address ?address } . "
> - "OPTIONAL { ?location mlo:city ?city } . "
> - "OPTIONAL { ?location mlo:country ?country } . "
> - "OPTIONAL { ?location mlo:state ?state} "
> - "}", row[0]);
> + query = g_strdup_printf ("SELECT mlo:city (?city) mlo:state (?state) "
> + " mlo:address (?address) "
> + " mlo:country (?country) "
> + "WHERE { <%s> mlo:location ?location }",
>
> If this works then it is a nice reduction and simplification in the
> SPARQL :)
>
>
> 10. Please don't add spaces after conditional statements (in extract_mp3
> and extract_vorbis) :)
>
> if (md.track_count > 0) {
> +
> + tracker_sparql_builder_insert_close (preupdate);
>
>
> 11. Another case of "_foo" in miner_files_add_to_datasource() and
> process_file_cb(), should use something more meaningful. The "_foo" is
> used to generate the id you know :P and it looks unprofessional :)
>
>
> 12. The documentation here needs a small fix:
>
> + * through tracker_sparql_builder_new_embedded_insert(), The subject
>
> No need to capitalise "The" there. Unless the comma should be a ".".
>
> + * function should just provide predicate/object(s) pairs. the data
>
> Here there is no capital "T".
>
>
> 13. Hmm, is this correct in extract_html(), extract_imagemagick(), etc:
>
> - tracker_sparql_builder_subject_iri (metadata, uri);
Yes it is, metadata here is now an embedded TrackerSparqlBuilder, and
the subject must be set by tracker-extract.c so it's the same than the
one used in tracker-miner-fs, so extractors now must not specify
subjects for the embedded metadata.
>
>
> 14. The get_file_metadata() function in tracker-extract.c needs fixing
> more I think for failure conditions. More specifically, I think the _out
> variables should be set to NULL initially so they don't have to be set
> in every place where we return FALSE. The libstreamanalyser part doesn't
> set them at all, which would break and likely crash things. We also
> don't unref the preupdate variable when we return FALSE on line 267.
Yeah, the LSA part could need some double checking, I don't think it's
going to provide us the sparql we actually expect.
>
>
> 15. No need for braces here round preinserts_str:
>
> + dbus_g_method_return (context,
> + (preinserts_str) ? preinserts_str : "",
> + tracker_sparql_builder_get_result (sparql));
>
>
>
> Other than that the rest looks good. Let's see if we can get these fixed
> so we can merge to master for tomorrow's release :)
>
> Thanks guys,
>
_______________________________________________
tracker-list mailing list
tracker-list@gnome.org
http://mail.gnome.org/mailman/listinfo/tracker-list
