Re: XMP Import patch : Now importing Sidecar + XMP tags. Test please.

[Warren Baird <photogeekmtl gmail com>]

Bengt Thuree wrote:
> Hi Warren
>
> I have just uploaded a new version of the patch that does the following:
>
> 1) Read a possible Sidecar's XMP tags first
> 2) Read the embedded tags in the photo (EXIF, XMP etc)
>
> This means that the picture will always have priority, if the same tag
> exists there.

Hi Bengt,

thanks!   I tried the patch and it seems to work fine...


> Also, the way F-Spot works is that as soon as the tags have been read,
> f-spot commits the information, which means that the tags are written to
> the photos. 
>
> If you import the photos with Link option (not copy), then even if you
> cancel the import the photo will have been modified and the tags added.

This worries me a little bit...   For my use-case, it's not an issue, 
but I can imagine some people getting annoyed if their photos are 
modified "behind their backs".   It's probably enough to make sure it's 
documented somewhere...

> If the patch discovers an Sidecar file which has XMP tags in it, then a
> new F-Spot tag will be created and attached to this photo. I know, not
> good, but temporarily the best and easiest way to indicate which photo
> have a sidecar file associated with it. 

This isn't really necessary - but it's a nice touch.    In my use-case I 
don't think the user cares which photos had sidecars attached. 
However, it's really easy to delete the tag if you don't want it..

> The sidecar is not copied with
> the photo though, nor is the actual path to the sidecar stored anywhere.
> So the patch is not really 100% yet, due to other limitations in F-Spot.

For my use-case, you don't want to copy the sidecar with the photo - you 
just want to read it on import and then include it with the photo.

I guess there might be situations when importing a raw file or something 
where you might need that - but for jpgs, definitely not.

> So far all the feedbacks have been incorporated, so do keep on giving
> feedbacks :)

I took a more detailed look at the patch.   One suggestion that you 
might want to consider.  It looks to me like there is a long list of 
labels you read, and a lot of code that looks kinda like this

private string label1 ; public string Label1 { get ....}
private string label2 ; public string Label2 { get ....}

[...]
if (tag == "Label1") {label1 = value}
if (tag == "Label2") {label2 = value}

Did you consider just storing a hashmap?  the access code would be:

public string getValue(string key) {return hash[key]}

and the setting code would be:

hash[tag] = value

It looks to me like this would cut about 50 lines of code, and make it 
easier to add support for new keywords later --- currently it looks like 
you need to make changes in 3 or 4 places to add support for a new keyword

Also - line 314 of XmpTagsMetadata.cs:   Isn't the "goto case" redundant?

Aside from that, it looks pretty good...


Warren