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

[Not Zed <notzed ximian com>]

On Tue, 2003-12-02 at 15:21, David Moore wrote:
> Updated patch attatched.
>
> > what Michael meant when he said append "(n)" to the filename was not to append the literal "(n)" but to rather append "(" [digit] ")" to the filename, and preferably *before* the .png or .jpg extension.
>
> Oops. I guess that's what I get for coding at 2 AM. :-) Fixed now.
>
> > hmmm, might be better to use open (filename, O_EXCL | O_CREAT | O_WRONLY, 0666) an just loop until you find a version of the filename that works. otherwise there's a race condition - some other app could steal image(1).png before we get a chance to open/save to it.
>
> As has been said, this isn't really too much of a problem, and I think it's probably more effort to fix than it's worth.
>
> > ah, hmmm... as far as my comment above about using open(), perhaps this function should take a flags argument specifying whether or not to:
> >
> > 1) pop up the error dialog (for wallpaper, we don't want this - we should  instead keep incrementing a counter in the filename until one works). in this way, if the file already exists, just return FALSE and have the caller try a different filename.
>
> If we do that, em_utils_save_part_to_file would have to return something other than bool, so that emp_part_popup_set_background would know whether to continue trying (if the file exists) or not (if the file can't be created). I figure that's more complication than there needs to be, as long as it works fine the way it is.

Actually this really isn't a problem, it just returns a pointer (the actual name saved) vs bool.  This implicitly supplies as much info as the bool, plus useful information too, the saved filename.  The only difference is you have to save the return, and g_free it - not a big change.  The harder part is save_part.

---

Index: mail/ChangeLog
===================================================================
RCS file: /cvs/gnome/evolution/mail/ChangeLog,v
retrieving revision 1.2904
diff -u -r1.2904 ChangeLog
--- mail/ChangeLog	1 Dec 2003 04:42:49 -0000	1.2904
+++ mail/ChangeLog	2 Dec 2003 04:15:26 -0000
@@ -1,3 +1,13 @@
+2003-12-01  David Moore  <davmre bellsouth net>
+ 
+ 	* em-popup.c (emp_part_popup_set_background): Implemented; sets an 
+	image attachment as the GNOME wallpaper.
+ 	
+ 	* em-utils.c (emu_save_part_done): Created a prototype at the top
+ 	of the file, so it can be called from anywhere within
+ 	(em_utils_save_part_to_file): Added; save a message part to a 
+ 	specified file on disk, overwriting without prompting.
+
 2003-12-01  Not Zed  <NotZed Ximian com>
 
 	* mail-security.glade: added some padding to the security details
Index: mail/em-popup.c
===================================================================
RCS file: /cvs/gnome/evolution/mail/em-popup.c,v
retrieving revision 1.7
diff -u -r1.7 em-popup.c
--- mail/em-popup.c	12 Nov 2003 21:12:59 -0000	1.7
+++ mail/em-popup.c	2 Dec 2003 04:15:30 -0000
@@ -22,6 +22,13 @@
 #include <camel/camel-folder.h>
 #include <camel/camel-mime-message.h>
 #include <camel/camel-string-utils.h>
+#include <camel/camel-mime-utils.h>
+#include <camel/camel-mime-part.h>
+
+#include <gconf/gconf.h>
+#include <gconf/gconf-client.h>
+
+#include <gal/util/e-util.h>
 
 static void emp_standard_menu_factory(EMPopup *emp, EMPopupTarget *target, void *data);
 
@@ -599,10 +606,73 @@
 }
 
 static void
-emp_part_popup_set_background(GtkWidget *w, EMPopupTarget *t)
+emp_part_popup_set_background (GtkWidget *w, EMPopupTarget *t)

don't add this whitespace stuff.  i preferred the "name(" function calls, like k&r taught me.
>  {
> -	/* set as background ... */
> -	printf("UNIMPLEMENTED: set background, but it would be cool, no?\n");
> +	GConfClient *gconf;
> +	char *str, *path, *fullpath, *filename, *basename, *extension;
> +	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);
> +	}
> +	
> +	e_filename_make_safe (filename);
> +	
> +	path = g_build_filename (g_get_home_dir(), ".gnome2", "wallpapers", NULL);
> +	fullpath = g_build_filename (path, filename, NULL);
> +	
> +	/* if the file exists, cycle through (n) suffixes until it doesn't */
> +	if (g_file_test (fullpath, G_FILE_TEST_EXISTS)) {
> +		
> +		if (!(extension = g_strdup (strrchr (filename, '.'))))

you really can't do this, it assumes you have an extension, but you could have anything given that the value most likely came from the internet.
-> crash if there's no '.' in the name.

> +			extension = "";
> +		
> +		basename = g_strndup (filename, strlen (filename) - strlen (extension));
> +
> +		while (g_file_test (fullpath, G_FILE_TEST_EXISTS)) {
> +			g_free (fullpath);
> +			g_free (filename);
> +			filename = g_strdup_printf ("%s(%d)%s", basename, i++, extension);
> +			fullpath = g_build_filename (path, filename, NULL);
> +		}
> +		
> +		g_free (basename);
> +		g_free (extension);
> +	}
> +	
> +	g_free (filename);
> +	g_free (path);

The above can be done in one loop, without having the encompassing if() {}

i.e.

while (g_file_test(...)) {
   free last name
   create next name
}

actually if you're going to be recreating strings, its cleaner/simpler to use a single g_string and just re-print to it each loop, you don't have to go throuhg alloc/free as often (i.e. for filename above).

> + **/
> +gboolean 
> +em_utils_save_part_to_file (GtkWidget *parent, const char *filename, CamelMimePart *part) 
> +{
> +	int done;
> +	char *dirname;
> +	struct stat st;
> +	
> +	if (filename[0] == 0)
> +		return FALSE;
> +	
> +	dirname = g_path_get_dirname (filename);
> +	if (camel_mkdir (dirname, 0777) == -1) {
> +		e_notice (parent, GTK_MESSAGE_ERROR,
> +				 _("Cannot save to `%s'\n %s"), filename, g_strerror (errno));
> +		return FALSE;
> +	}
> +	g_free(dirname);

You leak dirname if the mkdir fails.