Re: Re: [evolution-patches] Bounty Hunt patch: Set wallpaper from mailer

[Not Zed <notzed ximian com>]

> >  struct _EMPopupFactory {
> > @@ -602,8 +609,62 @@
> >  static void
> >  emp_part_popup_set_background(GtkWidget *w, EMPopupTarget *t)
> >  {
> > -	/* set as background ... */
> > -	printf("UNIMPLEMENTED: set background, but it would be cool, no?\n");
> > +	GConfClient *gconf;
> > +	char *str, *filename, *path, *extension;
> > +	unsigned int i=1;
> > +	
> > +	filename = g_strdup(camel_mime_part_get_filename(t->data.part.part));
> > +	   
> > +	/* if filename is blank, create a default filename based on MIME type */
> > +	if (!filename || !filename[0])
> > +		filename = g_strconcat(_("untitled_image."), camel_mime_part_get_content_type(t->data.part.part)->subtype, NULL);
> > +
>
> I'd prefer you do something more like:
>
> CamelContentType *ct;
>
> if (!filename || !filename[0]) {
>    ct = camel_mime_part_get_content_type(t->data.part.part);
>    g_free (filename);
>    filename = g_strdup_printf (_("untitled_image.%s"), ct->subtype);
> }
>
> it makes the code a bit clearer... also, you need to g_free (filename)
> or it gets leaked if filename[0] == 0 :-)
Wastes a local variable though, ct should at least be in the if-block, i don't think it matters either way.
(stylistically we rarely do anything like that, so if only for consistency reasons, use another variable).

> > +	e_filename_make_safe(filename);
> > +	
> > +	path = g_build_filename(g_get_home_dir(), ".gnome2", "wallpapers", filename, NULL);
> > +	
> > +	extension = strrchr(filename, '.');
> > +	if (extension)
> > +		*extension++ = 0;
> > +	
> > +	/* if file exists, stick a (number) on the end */
> > +	while (g_file_test(path, G_FILE_TEST_EXISTS)) {
> > +		char *name;
> > +		name = g_strdup_printf(extension?"%s (%d).%s":"%s (%d)", filename, i++, extension);
>
> if extension is NULL, you have too many args here.
That was my suggestion Jeff :)

Its perfectly legal c, afaict and avoids some otherwise pointless bulky logic.  Ahh if only we had an argument-stack-model language which exposed the stack.

Anyway looks ok, even as is.  When Boston gets the paperwork we can proceed.

Cheers,
 Michael