Re: scaffold & gnome-build patch

[Gustavo Giráldez <gustavo giraldez gmx net>]

Hi!

On Fri, 2004-01-16 at 15:27, aurelien naldi wrote:
> On ven, 2004-01-16 at 19:15 +0100, aurelien naldi wrote:
> [..]
> > so here is a new version of the gnome-build patch and also a little change to the scaffold one.
> 
> Oops, REALLY join the file now :)

Ok, almost there :-)  The patch applied cleanly, but I got two compiler
warnings (which turned into errors because of the compilation flags):

gbf-am-build.c: In function `gbf_build_run':
gbf-am-build.c:286: `last' might be uninitialized in this function 
Fixed it by initializing it to cur.

gbf-am-project.c: In function `foreach_build_target':
gbf-am-project.c:2434: warning: char format, void arg (arg 2)
Fixed by casting the gpointer key to gchar *.

Besides these two, I have a couple of final requests:

- I should have thought about this before, but it'd be a good thing to
#define the strings for the standard build targets.  The obvious place
to do it would be gbf-am-build.h.  Something like:

#define ALL_BUILD_ID  "ALL"
etc.

This way, should we want to change the strings there's only one place to
touch.

- I see you g_free() the id in gbf_build_run().  The more correct place
to do it is in queue_check() right after the call to gbf_build_run() or
right before g_free(op).

- Magic constants are evil :-)  That's why in gbf_build_run, instead of
adding 5, you should use strlen("USER:") (or better
strlen(USER_BUILD_ID) after you #define the constants).  You don't even
have to worry about overhead of the function call, since GCC will
optimize it for you.

- For the string I told you to mark up for translation, you should use:

g_strdup_printf (_("Build specific target: %s"), node->name);

This way, if for some strange language/locale the location for the name
should be moved, the translator can do so.  See:
http://developer.gnome.org/doc/tutorials/gnome-i18n/developer.html#split-sentences
for a more complete explanation.

After all these are fixed, please commit with a changelog entry.  If you
don't have a CVS account send me the patch and I'll commit it with
pleasure.

Cheers,
Gustavo