Re: XMP import patch - Best version yet :)

[Bengt Thuree <bengt thuree com>]

On Wed, 2006-07-26 at 18:18 +0530, Cosme Sevestre wrote:

> Thanks,  I think it is taking a pretty good shape too.  I tested the
> latest patch and imported successfully all my pictures (jpeg, a few
> hundred of them having tags with non-ASCII characters).
Great :)

> If you can just add the rating tag I discussed about in a previous
> email, then F-Spot would retrieve both Urgency and Rating data and
> would be ready for bug 326561 [1] :-).  Attached a small patch to
> apply after yours - I tested it with a picture rated using Lightroom.
Added, but not verified. 
Tried the photo you send to me, but I can not read anything from it.
CVS F-Spot can not read anything either.

I get the following error when importing this file
=====
> open uri = file:///home/bengt/test-pictures/slask29-Rating/test_02.jpg
> read = 623
> approximate quality = 0
> open uri = file:///home/bengt/test-pictures/slask29-Rating/test_02.jpg
> read = 623
> approximate quality = 0
> System.NullReferenceException: Object reference not set to an instance of an object
> in <0x0002c> FSpot.JpegFile:get_Description ()
> open uri = file:///home/bengt/Photos/2006/7/26/test_02.jpg
> read = 623
> approximate quality = 0
> System.NullReferenceException: Object reference not set to an instance of an object
> in <0x0003f> FSpot.JpegFile:get_ExifData ()
> in <0x0001c> FSpot.JpegFile:GetEmbeddedThumbnail ()
> in <0x00152> PhotoStore:GenerateThumbnail (System.Uri uri)
> open uri = file:///home/bengt/Photos/2006/7/26/test_02.jpg
=====

> 
> >  - have some fundamental coding problems
> 
> It is a more generic feedback and just my humble opinion (so feel free
> to ignore it) but I think F-Spot would benefit in a more structured
> file organization (now 121 .cs files in src/) and more comments in the
> code.
I tend to agree with you... F-Spot would definitely benefit with a bit
more sub directories, and as you stated more comments in the code. With
a proper header as well describing the file.

> 
> Here is a few propositions specific to the XMP patch:
> XmpTagsMetadata.cs
>  - I am not sure how the variables (creator to captionwriter/rating)
> are sorted.  Maybe capturing using comments where those variables come
> from (exif, all the xmp stuff with dublin core, xap, raw, photoshop,
> etc.) would help.  For example I wasn't sure were to add the rating
> stuff (part of the XMP Basic Schema) so just added it to the end...
> since there are a lot of possible metadata tags, it might be worth
> doing.
Done

> - Maybe capture the rational behind the GetSingleRowData and
> ProcessItemsList methods.  I think someone new to the code would
> wonder why some tags are fetched using the first and others using the
> second as the methods name are not very clear.
Tried to add more comments... Since myself is a bit unclear on bags and
xmp and such I am not sure the comments are good though....

> 
> >  - white space problems
> >  - missing whatever
> 
> Thanks for your hard work :-)
No worries. and the more comments/feedback the better :)
> 
> Cheers,
> 
> Cosme
/Bengt