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

[Jeffrey Stedfast <fejj ximian com>]

a few comments below

On Mon, 2003-12-01 at 01:55, David Moore wrote:
> Updated patch attached. I'm sending in a copyright assignment form tomorrow, so hopefully that'll get to Boston in a few days.
>  
> > Actually, you might also need/want to peek the mime type to create a
> > suitable extension for the image file (.png, etc) if the name isn't
> > set.
> 
> Okay, done. Right now I'm just using the mime subtype as the extension, so it could conceivably end up as untitled_image.x-png or something weird like that, but it generally works as expected (and this is a rare case anyway). 
> 
> > 
> > Since this now saves to a filename based on the email itself, it 
> > needs
> > to:
> >  ...
> >  - do some (automatic) smarts so it doesn't over-write something with
> > the same name (e.g. add a " (n)" to the name if it clashes).
> >      I guess this logic could go into em_utils_save_path_to_file()
> > since
> > its intended for non-interactive, automatic, use.
> 
> I put it in emp_part_popup_set_background(), because it's possible that someone else would use em_utils_save_path_to_file() and would want it to overwrite instead of renaming. Also, doing it in em_utils_save_path_to_file() would require it to return the new path so emp_part_popup_set_background() would know what to set the gconf key to, which is needlessly complex. That okay with you?
> 

sounds reasonable to me.

> 
> ______________________________________________________________________
> 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	1 Dec 2003 06:29:58 -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 to 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.
> +
>  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	1 Dec 2003 06:29:59 -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);
>  
> @@ -601,8 +608,52 @@
>  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 *current_setting, *path;
> +	char *filename;
> +	
> +	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]) {

need to g_free (filename); here or we leak it if it was a empty string.
(NOTE: g_free can handle NULL pointers)

> +		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", filename, NULL);
> +	
> +	while (g_file_test(path, G_FILE_TEST_EXISTS)) {
> +		path = g_realloc(path, strlen(path) + 4);
> +		path = strcat(path, "(n)");
> +	}

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.

for example, if the attachment filename is image.png and image.png
already exists on the file system in the wallpaper directory, then we
want to save as image(1).png

but don't forget to check for image(1).png as well, so you'll need a
loop here.

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.

> +	
> +	if (em_utils_save_part_to_file(w, path, t->data.part.part)) {
> +		gconf = gconf_client_get_default();
> +		
> +		/* if the filename hasn't changed, blank the filename before 
> +		*  setting it so that gconf detects a change and updates it */
> +		if ((current_setting = gconf_client_get_string(gconf, "/desktop/gnome/background/picture_filename", NULL)) != NULL 
> +		     && strcmp(current_setting, path) == 0) {
> +			gconf_client_set_string(gconf, "/desktop/gnome/background/picture_filename", "", NULL);
> +		}

I'd prefer a shorter variable name for current_setting... maybe
something like char *str or something.

> +		
> +		g_free(current_setting);
> +		gconf_client_set_string(gconf, "/desktop/gnome/background/picture_filename", path, NULL);
> +		
> +		/* if GNOME currently doesn't display a picture, set to "wallpaper"
> +		 * display mode, otherwise leave it alone */
> +		if ((current_setting = gconf_client_get_string(gconf, "/desktop/gnome/background/picture_options", NULL)) == NULL 
> +		     || strcmp(current_setting, "none") == 0) {
> +			gconf_client_set_string(gconf, "/desktop/gnome/background/picture_options", "wallpaper", NULL);
> +		}
> +		
> +		g_free(current_setting);
> +		g_object_unref(gconf);
> +	}
> +	
> +	g_free(filename);
> +	g_free(path);
>  }
>  
>  static void
> Index: mail/em-utils.c
> ===================================================================
> RCS file: /cvs/gnome/evolution/mail/em-utils.c,v
> retrieving revision 1.8
> diff -u -r1.8 em-utils.c
> --- mail/em-utils.c	19 Nov 2003 21:22:12 -0000	1.8
> +++ mail/em-utils.c	1 Dec 2003 06:30:02 -0000
> @@ -33,6 +33,7 @@
>  
>  #include <camel/camel-stream-fs.h>
>  #include <camel/camel-url-scanner.h>
> +#include <camel/camel-file-utils.h>
>  
>  #include <filter/filter-editor.h>
>  
> @@ -52,6 +53,7 @@
>  #include "em-format-quote.h"
>  
>  static EAccount *guess_account (CamelMimeMessage *message);
> +static void emu_save_part_done (CamelMimePart *part, char *name, int done, void *data);
>  
>  /**
>   * em_utils_prompt_user:
> @@ -1382,6 +1384,55 @@
>  	gtk_widget_show((GtkWidget *)filesel);
>  }
>  
> +/**
> + * em_utils_save_part_to_file:
> + * @parent: parent window
> + * @filename: filename to save to
> + * @part: part to save
> + * 
> + * Save a part's content to a specific file
> + * Creates all needed directories and overwrites without prompting
> + *
> + * Returns %TRUE if saving succeeded, %FALSE otherwise
> + **/
> +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);
> +
> +	if (access (filename, F_OK) == 0) {
> +		if (access (filename, W_OK) != 0) {
> +			e_notice (parent, GTK_MESSAGE_ERROR,
> +				 _("Cannot save to `%s'\n %s"), filename, g_strerror (errno));
> +			return FALSE;
> +		}
> +	}
> +	
> +	if (stat (filename, &st) != -1 && !S_ISREG (st.st_mode)) {
> +		e_notice (parent, GTK_MESSAGE_ERROR,
> +				 _("Error: '%s' exists and is not a regular file"), filename);
> +		return FALSE;
> +	}
> +	
> +
> +	/* FIXME: This doesn't handle default charsets */
> +	mail_msg_wait(mail_save_part(part, filename, emu_save_part_done, &done));
> +	
> +	return done;
> +}

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.

2) whether O_EXCL should be used in the open() call... except this would
require changes to the mail-ops function. not sure we should bother?

Jeff