Re: [Tracker] Writeback branch review

[Martyn Russell <martyn lanedo com>]

On 22/12/09 15:13, Philip Van Hoof wrote:
> On Tue, 2009-12-22 at 14:53 +0000, Martyn Russell wrote:
> > 01, Why have tracker:writeback for nie:copyright? Are we using that?
>
> It can be written back, so yes.

Ah yea, I see now.

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

Eek, Yea, I think a comment about *why* it is disabled might be useful. 
Thanks for the update, I didn't realise it was causing corruption.

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

Ah yea :/

We might want to add a warning there so we know about it.

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

OK.

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

Thanks.

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

I suspected that.

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

Thanks.

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

Thanks.

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

:/ yea

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

Well, we use the same string "top - left" in tracker-extract, so it 
would be nice to have these in a common place to avoid spelling mistakes 
leading to query issues.

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

Sorry I meant after ;)

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

Ah, bad example :)

Actually, I meant something else here, I just failed epically to 
describe it :)

Scenario: Make = "Canon", Model = "Canon 500D".

We have some hack in tracker-extract to check the make is not not in the 
model string. I wonder if we need this for the writeback support - 
presumably it is written to the database in the correct way in the first 
place so, probably not.

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

A few. git diff master shows all changes and there are a few other 
whitespace issues there.

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

Thanks.

> Cool, thanks

No problem, thanks for the hard work :)

--
Regards,
Martyn