Re: [Tracker] Writeback branch review

[Philip Van Hoof <spam pvanhoof be>]

On Tue, 2009-12-22 at 14:53 +0000, Martyn Russell wrote:
> Hi Philip,
>
> I have reviewed the branch and here are my comments with the diff 
> between writeback branch and master:
>
> 01, Why have tracker:writeback for nie:copyright? Are we using that?

It can be written back, so yes.

> 02, Nice fix on the nfo:entryCounter maxCardinality (and others) ;)

Np

> 03, Why did you disable the mp3 extractor in the Makefile.am? Same with 
> totem-plparser?

The MP3 extractor uses id3lib which according to reports on my blog's
comments can corrupt files. The people writing this suggested using
another library for writing ID3 tags (like taglib, but not id3lib).

We're also only writing back just one field (the title). So I think it's
not worth the risk of file corruption at this moment.

> 04, Was this needed then? (For loop please)
> +               if (module) {
> +                       g_hash_table_insert (priv->modules, g_strdup 
> (path), module);
> +               }

If the module failed to load, module is NULL at that point. If a NULL is
added to the hashtable, a crash happens at a later point.

> 05, I wonder if we are missing some content-types in the list we have 
> for play lists?

The playlist writeback ain't finished yet. Carlos is completing this.
It's also why one of the commits disables it for now.

> 06, Coding style, function params, one per line please (rewrite_playlist).

Carlos is working on this file at this moment. I asked him to fix this
for me =)

> 07, In func rewrite_playlist(), path is unused.

Same as #06, I'll ask Carlos to deal with this

> 08, Why the #if 0s in that func?

Because the API ain't finished yet. And because I planned to let Carlos
deal with this (I think he is at this moment).

> 09, I would not use if(!error) then if(array...), since it is likely 
> that both are non null so you are checking twice for the normal 
> operation, I would simply have the array check and use g_clear_error() 
> (which checks for NULL GErrors anyway). Also the check for array being 
> non-null is done twice for the free at the end, I would just use if 
> (array) { ... if (array->len > 0) { ...} /* do free here */ }

Fixed

> 10, Same trick with the writeback_xmp_update_file_metadata() and GErrors.

Fixed

> 11, Why do we need to use xmp_set_property() after 
> xmp_delete_property(), can't we just set?

Yes, if you don't delete the existing property first then exempi
implicitly creates a list of two items. Bit silly API, but oh well.

> 12, For the orientation, is it right that "orientation-bottom" is 
> "bottom - right", because "orientation-top" is "top - left".

Sharp question ;), I'll 

> 13, I think the orientation and metering should be #defines, also who 
> defines these? Nepomuk?

Nepomuk, yes. They are only used once. Not sure what use a #define is
here?

> 14, Extra spaces before: xmp_delete_property (xmp, NS_EXIF, 
> "WhiteBalance"); and for "flash".

I'm seeing three tabs, which is the right indentation level underneath
the if block. Anything I missed?

> 15, The camera make/mode code looks like it could be wrong... what if 
> the model is "Canon 500D" ?

Make will be "Canon", Model will be "500D" (it splits on the first
space).

What do you suggest here?

> 16, White space issues in line if (g_strcmp0 (row[1], TRACKER_MLO_PREFIX 
> "location") == 0 ||

I removed a trailing whitespace on that line. Was there anything else?

> 17, We should probably use tracker_is_blank_string() not 
> tracker_is_empty_string() here.

Fixed

> There are other white space issues I found too, so we should really use 
> git diff --check before committing here.

> I can fix some of these if you would prefer.

I committed a first batch of fixes in 'writeback', feel free to address
the other whitespace issues you found there as commits.

> Other than that, it looks like a good start, thanks.

Cool, thanks


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