Re: Add some conversion checks to GtkFileSelection (bug 50298)

[Owen Taylor <otaylor redhat com>]

Alexander Larsson <alla lysator liu se> writes:

> This patch adds conversion checks to all "user" operations in the
> GtkFileSelection dialog. It also changes the error dialog to use
> GtkMessageDialog.

Well, certainly adding checks to the create-dir/rename/delete buttons
is a step forward.

It isn't really enough - at a minimum, code like 
(from gtk_file_selection_get_filename).

===
      sys_filename = g_filename_from_utf8 (cmpl_completion_fullname (text, filesel->cmpl_state), -1, NULL, NULL, NULL);
      strncpy (something, sys_filename, sizeof (something));
===

Will _segfault_ if the user entered invalid characters in the filename.
So, at least a check for NULL is needed.

In most of the internal checks, this should probably be sufficient - a
file with unrepresentable strings is not different from a file that
isn't there for other reasons.

gtk_file_selection_get_filename() is somewhat problematical. The user
deserves to get a decent error message if they type in an invalid
name and press enter.

On the other hand, having gtk_file_selection_get_filename() pop up
a dialog is, at the very, least, unexpected. Especially if the
file selector is hidden at that point. 

I think the a better possibility would be to filter on ::insert_text
and call g_filename_from_utf8 on the resulting string, and refuse to
insert the text at that point if it fails. This isn't perfect, but
it at least gives the user some immediate feedback.

>    /* If file dialog is grabbed, make this dialog modal too */
>    /* When error dialog is closed, file dialog will be grabbed again */
>    if (GTK_WINDOW (fs)->modal)
>        gtk_window_set_modal (GTK_WINDOW (dialog), TRUE);

Should we just always make the error dialogs modal? Is there
a good reason to have non-modal error dialogs in this case?

> @@ -892,6 +869,7 @@ gtk_file_selection_create_dir_confirmed 
>    gchar *full_path;
>    gchar *sys_full_path;
>    gchar *buf;
> +  GError *error = NULL;
>    CompletionState *cmpl_state;
>    
>    g_return_if_fail (fs != NULL);
> @@ -902,10 +880,17 @@ gtk_file_selection_create_dir_confirmed 
>    path = cmpl_reference_position (cmpl_state);
>    
>    full_path = g_strconcat (path, G_DIR_SEPARATOR_S, dirname, NULL);
> -  sys_full_path = g_filename_from_utf8 (full_path, -1, NULL, NULL, NULL);
> -  if (mkdir (sys_full_path, 0755) < 0) 
> +  sys_full_path = g_filename_from_utf8 (full_path, -1, NULL, NULL, &error);
> +  if (error)
> +    {
> +      buf = g_strconcat (_("Error creating directory")," \"", dirname, "\":  ", 
> +			 error->message, "\n", _("You probably used symbols not allowed in filenames."), NULL);

Good iternationalization practice is to always use sprintf for thigns
like this instead of assembling from pieces:

 buf = g_strdup_printf (_("Error creating directory \"%s\": %s\n"
                          "You probably used symbols not allowed in filenames."));

However, I don't think:
 
 Error creating directory "FOO": Invalid byte sequence in conversion input.
 You probably used symbols not allowed in filenames.

This error seems a bit unecessarily cryptic. In the case of 
g_error_matches (error, G_CONVERT_ERROR, G_CONVERT_ERROR_ILLEGAL_SEQUENCE), we
probably can say:

 "The directory name "FOO" contains symbols that are not allowed in filenames"

If a different error is returned, something is screwed up, and just reporting
the error from g_convert() is probably fine.

> @@ -1121,19 +1115,35 @@ gtk_file_selection_rename_file_confirmed
>    new_filename = g_strconcat (path, G_DIR_SEPARATOR_S, file, NULL);
>    old_filename = g_strconcat (path, G_DIR_SEPARATOR_S, fs->fileop_file, NULL);
>  
> -  sys_new_filename = g_filename_from_utf8 (new_filename, -1, NULL, NULL, NULL);
> -  sys_old_filename = g_filename_from_utf8 (old_filename, -1, NULL, NULL, NULL);
> -
> -  if (rename (sys_old_filename, sys_new_filename) < 0) 
> +  sys_new_filename = g_filename_from_utf8 (new_filename, -1, NULL, NULL, &error);
> +  if (error)
>      {
> -      buf = g_strconcat ("Error renaming file \"", file, "\":  ", 
> -			 g_strerror (errno), NULL);
> +      buf = g_strconcat (_("Error renaming file to")," \"", new_filename, "\":  ", 
> +			 error->message, "\n", _("You probably used symbols not allowed in filenames."), NULL);
>        gtk_file_selection_fileop_error (fs, buf);
> +      g_error_free (error);
> +    }
> +  else
> +    {
> +      sys_old_filename = g_filename_from_utf8 (old_filename, -1, NULL, NULL, &error);
> +      if (error)
> +	{
> +	  buf = g_strconcat (_("Error renaming file")," \"", old_filename, "\":  ", 
> +			     error->message, NULL);

You seem to be missing the "You probably used symbols..." here?

> +	  gtk_file_selection_fileop_error (fs, buf);
> +	  g_error_free (error);
> +	}
> +      else if (rename (sys_old_filename, sys_new_filename) < 0) 
> +	{
> +	  buf = g_strconcat ("Error renaming file \"", file, "\":  ", 
> +			     g_strerror (errno), NULL);
> +	  gtk_file_selection_fileop_error (fs, buf);
> +	}
> +      g_free (sys_old_filename);
>      }
>    g_free (new_filename);
>    g_free (old_filename);
>    g_free (sys_new_filename);
> -  g_free (sys_old_filename);
>    
>    gtk_widget_destroy (fs->fileop_dialog);
>    gtk_file_selection_populate (fs, "", FALSE);

You can take or leave the suggestion, but I'm a great fan of well-structured
gotos for error handling instead of deeply nested if statements:

====
  gchar *sys_old_filename = NULL;
  gchar *sys_new_filename = NULL;
  gchar *error_message = NULL;
  
  new_filename = g_strconcat (path, G_DIR_SEPARATOR_S, file, NULL);
  old_filename = g_strconcat (path, G_DIR_SEPARATOR_S, fs->fileop_file, NULL);

  sys_new_filename = g_filename_from_utf8 (new_filename, -1, NULL, NULL, &error);
  if (error)
    {
      error_message = g_strconcat (_("Error renaming file to")," \"", new_filename, "\":  ", 
				   error->message, "\n", _("You probably used symbols not allowed in filenames."), NULL);
      g_error_free (error);

      goto out;
    }

  sys_old_filename = g_filename_from_utf8 (old_filename, -1, NULL, NULL, &error);
  if (error)
    {
      error_message = g_strconcat (_("Error renaming file")," \"", old_filename, "\":  ", 
			 error->message, NULL);
      g_error_free (error);

      goto out;
    }

  if (rename (sys_old_filename, sys_new_filename) < 0)
    {
      error_message = g_strconcat ("Error renaming file \"", file, "\":  ", 
			 g_strerror (errno), NULL);
      gtk_file_selection_fileop_error (fs, buf);
    }

 out:

  if (error_message)
    gtk_file_selection_fileop_error (fs, error_message);
  
  g_free (new_filename);
  g_free (old_filename);
  g_free (sys_old_filename);
  g_free (sys_new_filename);
====

Look like basically the right patch, in any case. I think it just needs
to be somewhat more comprehensive in what it checks, and perhaps the
error messages can be improved a bit.

Regards,
                                        Owen