Re: Pushing patch for 342137

[Stephane Delcroix <stephane delcroix org>]

Hi Bengt and all,

here's my patch review for your latest patch.
It's also on b.g.o.

Patch review: patch 69375 for bug 342137

About the code:

a) trivial: don't anti-date the Changelog entry :):)
diff -u -p -r1.1525 ChangeLog
--- ChangeLog   20 Jul 2006 20:23:57 -0000      1.1525
+++ ChangeLog   22 Jul 2006 09:26:11 -0000
@@ -1,3 +1,16 @@
+ 2006-07-27  Bengt Thuree <bengt thuree com> 

b) in ImportCommand.cs: you're doing:

                tray = new FSpot.ScalingIconView (collection);
+               bool SavedDisplayTags;
[...]
+               SavedDisplayTags = tray.DisplayTags;    // So we know
which state we
had.
+               tray.DisplayTags = false;               // Otherwise
deleting just created XMP
tags will crash.
[...]
+                       tray.DisplayTags = SavedDisplayTags;
                        this.Dialog.Destroy ();

It's useless to save the state after the new and restore it just after
the Destroy. Just do:

+               tray.DisplayTags = false;               // Otherwise
deleting just created XMP
tags will crash.

c) XmpTagsImporter.cs: why not integrate all this code in a namespace ?

d) XmpTagsMetadata.cs: idem

e) the style policy for F-Spot ask to add a copyright notice and a short
description for new files (XmpTagsImporter and XmpTagsMetadata)

f) AddTagToPhoto: Hardcoding english string is probably not the good
choice with an international app.

g) why still naming the variable 'place' like in:
+                       // Check if a tag exists for this place
+                       Tag place = tag_store.GetTagByName
(new_tag_name);
now that the code will import all kinds of tags (not only places) ? 

h) is not better to always create the new tags under the 'Last Import'
category, even for Places, Country or Location ? To avoid losing
knowledge about the fact that's a place, maybe create them under
LastImport>Places ...

i)consider removing that #define UseImageFile, it's too C-ish

j)it's really nice to check for a lot of other xmp info than just places
and keyword. hope F-Spot will use them soon

About the UI:
Add a default Icon for Last Import

My best regards,

Stephane

On Sun, 2006-07-23 at 07:55 +0900, Bengt Thuree wrote:
> On Sö, 2006-07-23, 01:36, Stephane Delcroix skrev:
> > Hi,
> 
> Hi
> 
> > guessing that everyone out there will be happy to see things move a bit
> > more in F-Spot.
> 
> Very true :) Especially after the number of months with slow activity due
> to various reasons.
> 
> > Goal: Getting XMP import patch into CVS HEAD.
> > http://bugzilla.gnome.org/show_bug.cgi?id=342137
> 
> Can't wait :)
> 
> >
> > How to do it ?
> > a) making it compliant with latest CVS (done, thx Bengt)
> 
> No Problems
> 
> > b) Larry need to approve the idea of having an 'XMP import capability'
> > in HEAD
> 
> Waiting for Larrys feedback
> 
> > c) Unifying Bengt and Warren patch. Find out the required things from
> > both patches
> 
> Warren: I think my patch covers everything yours do, apart from not
> handling the import from a sidecar to good. This because I use the general
> ImageFile, which looks like always finding XMP data in the file. At least
> those I have tried. If XMP data is found, my patch do not look for a
> sidecar file. (compile option to not use imagefile though).
> 
> > d) reviewing and commenting
> 
> Do break it apart (code review, testing, or whatever) and let me know what
> you find, and how we can improve this patch. It is not a one liner, so the
> more eyes the better... Also, the more that review/test a patch, the
> easier it will be for Larry to review it.
> 
> If do not know where to find this patch:
> http://bugzilla.gnome.org/attachment.cgi?id=69375&action=view
> And leave comments in bugzilla 342137
> http://bugzilla.gnome.org/show_bug.cgi?id=342137
> Comments could be:
> * Works fine
> * Code review, and found .... (or did not find anything)
> * Need to cope with the following...
> * Please add the following...
> 
> > Are you in ?
> 
> Definitely :)
> 
> > in CVS for that day. If nothing has moved at all for the 4th, I'll
> > consider this experiment as failed...
> 
> That would be very bad :(
> 
> >
> > Best Regards,
> > Stephane
> 
> /Bengt
> 
> 
-- 
Stephane Delcroix
stephane delcroix org