Re: Pushing patch for 342137
- From: Stephane Delcroix <stephane delcroix org>
- To: f-spot-list gnome org
- Subject: Re: Pushing patch for 342137
- Date: Sun, 23 Jul 2006 10:14:48 +0200
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
[
Date Prev][
Date Next] [
Thread Prev][
Thread Next]
[
Thread Index]
[
Date Index]
[
Author Index]