Re: [evolution-patches] Dont' overwrite folders on rename (shell, #45982)

[Mike Kestner <mkestner ximian com>]

This function is really a thing of beauty.  :)  I know I'm commenting on
a lot of existing code below, but...

On Tue, 2003-07-08 at 11:42, Ettore Perazzoli wrote:
> 
> ______________________________________________________________________
> Index: e-shell-folder-commands.c
> ===================================================================
> RCS file: /cvs/gnome/evolution/shell/e-shell-folder-commands.c,v
> retrieving revision 1.58
> diff -u -p -r1.58 e-shell-folder-commands.c
> --- e-shell-folder-commands.c	7 May 2003 19:38:18 -0000	1.58
> +++ e-shell-folder-commands.c	8 Jul 2003 16:39:48 -0000
> @@ -517,9 +517,10 @@ e_shell_command_rename_folder (EShell *s
>  	RenameCallbackData *callback_data;
>  	const char *old_name;
>  	char *prompt;
> -	char *new_name;
> -	char *old_base_path;
> -	char *new_path;
> +	char *new_name = NULL;
> +	char *old_base_path = NULL;
> +	char *new_path = NULL;
> +	gboolean name_ok;
>  
>  	g_return_if_fail (shell != NULL);
>  	g_return_if_fail (E_IS_SHELL (shell));

Existing code, but the (shell != NULL) check is redundant now.  The Gtk2
type checking macros have it built in.  We should be removing these all
over.

> @@ -537,38 +538,47 @@ e_shell_command_rename_folder (EShell *s
>  	old_name = e_folder_get_name (folder);
>  	prompt = g_strdup_printf (_("Rename the \"%s\" folder to:"), old_name);
>  
> -	while (1) {
> +	name_ok = FALSE;

Might as well initialize name_ok on the declaration line and kill this
line.

> +	while (! name_ok) {
>  		const char *reason;
>  
> +		g_free (new_name);
>  		new_name = e_request_string (shell_view != NULL ? GTK_WINDOW (shell_view) : NULL,
>  					     _("Rename Folder"), prompt, old_name);

shell_view is preconditioned != null above, no need to check it here.
>  
>  		if (new_name == NULL)
> -			return;
> -
> -		if (e_shell_folder_name_is_valid (new_name, &reason))
> -			break;
> -
> -		e_notice (shell_view, GTK_MESSAGE_ERROR,
> -			  _("The specified folder name is not valid: %s"), reason);
> -	}
> +			goto done;
>  
> -	g_free (prompt);
> -
> -	if (strcmp (old_name, new_name) == 0) {
> -		g_free (new_name);
> -		return;
> +		if (! e_shell_folder_name_is_valid (new_name, &reason)) {

"if not valid, else."  Love that negative logic.

> +			e_notice (shell_view, GTK_MESSAGE_ERROR,
> +				  _("The specified folder name is not valid: %s"), reason);
> +		} else {
> +			g_free (old_base_path);

old_base_path is local to this block.  You could declare it here, and
free it after the g_build_filename call below instead of this pre-free
hack.

> +			g_free (new_path);
> +			old_base_path = g_path_get_dirname (folder_path);
> +			new_path = g_build_filename (old_base_path, new_name, NULL);
> +
> +			if (e_storage_set_get_folder (storage_set, new_path) == NULL) {

Do we own the EFolder ref that's returned?  Is this a leak?  It's hard
to tell since that's returned by a class method down in e-storage.c.

> +				name_ok = TRUE;
> +			} else {
> +				e_notice (shell_view, GTK_MESSAGE_ERROR,
> +					  _("A folder named \"%s\" already exists. Please use a different name."),
> +					  new_name);
> +			}
> +		}
>  	}
>  
> -	old_base_path = g_path_get_dirname (folder_path);
> -	new_path = g_build_filename (old_base_path, new_name, NULL);
> +	if (strcmp (old_name, new_name) == 0)
> +		goto done;
>  
>  	callback_data = rename_callback_data_new (shell_view, new_path);
>  	e_storage_set_async_xfer_folder (storage_set, folder_path, new_path, TRUE, rename_cb, callback_data);
>  
> + done:

Seems like it shouldn't be hard to do this without these goto done
hacks.  The first goto done could remain a break, and the second one
removed by something like:

if (name_ok && strcmp (old_name, new_name)) {
        callback_data = rename_callback_data_new (shell_view, new_path);
        e_storage_set_async_xfer_folder (storage_set, folder_path, new_path, TRUE, rename_cb, callback_data);
}

>  	g_free (old_base_path);
>  	g_free (new_path);
>  	g_free (new_name);
> +	g_free (prompt);
>  }
>  
>