After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 341622 - DVD burning fails in presence of invalid encoded file(s)
DVD burning fails in presence of invalid encoded file(s)
Status: RESOLVED FIXED
Product: nautilus-cd-burner
Classification: Deprecated
Component: cd-burner
2.16.x
Other Linux
: Normal major
: ---
Assigned To: Nautilus CD Burner Maintainers
Nautilus CD Burner Maintainers
: 341621 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2006-05-13 11:38 UTC by Fabio Bonelli
Modified: 2006-12-04 17:21 UTC
See Also:
GNOME target: ---
GNOME version: 2.15/2.16


Attachments
Patch (26.10 KB, patch)
2006-07-08 18:08 UTC, Fabio Bonelli
none Details | Review
Right patch (26.29 KB, patch)
2006-07-09 14:58 UTC, Fabio Bonelli
needs-work Details | Review
Program (244 bytes, text/x-csrc)
2006-10-25 18:46 UTC, Fabio Bonelli
  Details
New patch (27.34 KB, patch)
2006-11-19 13:01 UTC, Fabio Bonelli
committed Details | Review
patch to use glade (15.29 KB, patch)
2006-12-04 17:20 UTC, William Jon McCann
committed Details | Review

Description Fabio Bonelli 2006-05-13 11:38:53 UTC
When you write DVDs on the fly, the presence of an invalid encoded filename makes  the process fail.

The disc is written until that file, but n-c-b doesn't signal the error so, you can burn your discs without some data and don't notice that at all.
Comment 1 Fabio Bonelli 2006-07-08 18:08:51 UTC
Created attachment 68632 [details] [review]
Patch

Preliminary patch that adds a user friendly rename dialog, the message shown needs some love...

Now fixing bug 323506 should be really easy as well.
Comment 2 Fabio Bonelli 2006-07-09 14:58:06 UTC
Created attachment 68672 [details] [review]
Right patch
Comment 3 Fabio Bonelli 2006-09-15 10:10:47 UTC
Jon?
Comment 4 William Jon McCann 2006-09-29 18:27:55 UTC
Hi :)

Sorry for the delay.  This can only go in after branching (due to ui/feature changes) so I'll take a good look at this as soon as we do... which should be in the next week or two.

Thanks.
Comment 5 William Jon McCann 2006-10-25 17:30:47 UTC
Hey Fabio.  What's the best way to create test data that will trigger this?  Can you attach a small test set as a tarball or something?  Thanks.
Comment 6 Fabio Bonelli 2006-10-25 18:46:22 UTC
Created attachment 75395 [details]
Program

Hi Jon,

this program creates a file with invalid encoding in its name.
Comment 7 William Jon McCann 2006-11-06 18:45:01 UTC
Comment on attachment 68672 [details] [review]
Right patch

Looks very good.  A few minors comments.

>+ *
>+ * Authors: Fabio Bonelli <fabiobonelli@libero.it> */

Put the end of the comment on a new line.  Besides consistency one of the reasons for this is
it allows for modification of the line or adding new authors without messing with CVS history.

>+#include "config.h"
>+
>+#include <string.h>
>+
>+#include <gdk/gdk.h>
>+
>+#include <glib.h>
>+#include <glib/gi18n.h>
>+#include <gtk/gtk.h>

We try to keep the includes in order from low in the stack to high.  So, gdk should come after glib.  But in this case gtk already includes gdk so we don't need it explicitly.

>+#include <glade/glade.h>
>+#include <libgnomevfs/gnome-vfs.h>
>+#include <libgnomeui/libgnomeui.h>

I see that this pulls in libgnomeui for the mime-type stuff.  I'd rather not do this.  Is there another way?

>+#include "ncb-rename-dialog.h"
>+
>+static void     ncb_rename_dialog_class_init (NcbRenameDialogClass	*klass);
>+static void     ncb_rename_dialog_init       (NcbRenameDialog		*dialog);
>+static void     ncb_rename_dialog_finalize   (GObject			*object);
>+
>+#define NCB_RENAME_DIALOG_GET_PRIVATE(o) (G_TYPE_INSTANCE_GET_PRIVATE ((o), NCB_TYPE_RENAME_DIALOG, NcbRenameDialogPrivate))
>+
>+struct NcbRenameDialogInvalidFile {
>+	char	*mime_type;
>+	char	*filename;
>+};
>+typedef struct NcbRenameDialogInvalidFile NcbRenameDialogInvalidFile;
>+
>+struct NcbRenameDialogPrivate
>+{
>+	/* View and model for the list
>+	 * of files */
>+	GtkTreeView	*view;
>+	GtkListStore	*store;
>+
>+	/* Contextual popup menu */
>+	GtkWidget	*menu;
>+	GtkWidget	*rename_item;
>+
>+	/* List of NcbRenameDialogInvalidFile invalid filenames */
>+	GSList		*invalid_files;
>+
>+	/* Root path, used by gnome_vfs_directory_visit() */
>+	char		*root_path;
>+};
>+
>+/* List store columns */
>+enum {
>+	NCB_TREESTORE_PIXBUF,
>+	NCB_TREESTORE_OLDNAME,
>+	NCB_TREESTORE_NEWNAME
>+};
>+
>+G_DEFINE_TYPE (NcbRenameDialog, ncb_rename_dialog, GTK_TYPE_MESSAGE_DIALOG)
>+
>+static void
>+ncb_rename_dialog_class_init (NcbRenameDialogClass *klass)
>+{
>+        GObjectClass   *object_class = G_OBJECT_CLASS (klass);
>+
>+        object_class->finalize     = ncb_rename_dialog_finalize;
>+
>+        g_type_class_add_private (klass, sizeof (NcbRenameDialogPrivate));
>+}
>+
>+/* treemodel_file_move_cb:
>+ *
>+ * Callback for gtk_tree_model_foreach ().
>+ * Takes the row referred by @iter in the invalid files' model and
>+ * moves the file in burn:// associated with it to its valid name. */
>+static gboolean
>+treemodel_file_move_cb (GtkTreeModel	*model,
>+			GtkTreePath	*tree_path,
>+			GtkTreeIter	*iter,
>+			gpointer	 data)
>+{
>+	char		*source_uri = NULL;
>+	char		*dest_uri;
>+	char		*path_uri;
>+	char		*shortname  = NULL;
>+	NcbRenameDialog	*dialog;
>+	GnomeVFSResult	 res;
>+
>+	dialog = (NcbRenameDialog *) data;
>+
>+	gtk_tree_model_get (GTK_TREE_MODEL (dialog->priv->store), iter,
>+			    NCB_TREESTORE_OLDNAME, &source_uri,
>+			    NCB_TREESTORE_NEWNAME, &shortname, -1);
>+
>+	/* Build @dest_uri with a full path */
>+	path_uri = g_path_get_dirname (source_uri);
>+	dest_uri = g_strdup_printf ("%s/%s", path_uri, shortname);
>+	g_free (shortname);
>+	g_free (path_uri);
>+
>+	res = gnome_vfs_move (source_uri, dest_uri, FALSE);
>+	if (res != GNOME_VFS_OK) {
>+		g_warning ("Cannot move `%s' to `%s': %s", source_uri, dest_uri,
>+			   gnome_vfs_result_to_string (res));
>+	}
>+
>+	g_free (source_uri);
>+	g_free (dest_uri);
>+
>+	/* Done, but call me again for other rows.*/
>+	return FALSE;
>+}
>+
>+
>+/* ncb_rename_dialog_response_cb:
>+ *
>+ * Convenience response callback for #NcbRenameDialog.
>+ * Once called it renames the files in burn:// if the response
>+ * is a GTK_RESPONSE_OK. */
>+void
>+ncb_rename_dialog_response_cb (NcbRenameDialog *dialog,
>+			       int		id,
>+			       gpointer		user_data)
>+{
>+	g_return_if_fail (dialog != NULL);
>+	g_return_if_fail (NCB_IS_RENAME_DIALOG (dialog));
>+
>+	if (id == GTK_RESPONSE_OK) {
>+		gtk_tree_model_foreach (GTK_TREE_MODEL (dialog->priv->store),
>+					treemodel_file_move_cb, dialog);
>+	}
>+}
>+
>+
>+/* cell_editing_done_cb:
>+ *
>+ * Callback for the "edited" signal emitted by the text
>+ * renderer.
>+ * It updates the name in the treemodel with the name typed in. */
>+static void
>+cell_editing_done_cb (GtkCellRendererText	*renderer,
>+		      gchar			*tree_path,
>+		      gchar			*new_string,
>+		      gpointer			 data)
>+{
>+	NcbRenameDialog *dialog;
>+	GtkTreeIter	 iter;
>+	int		 r;
>+
>+	g_return_if_fail (strlen (new_string) != 0);
>+
>+	dialog = (NcbRenameDialog *) data;
>+
>+	r = gtk_tree_model_get_iter_from_string (GTK_TREE_MODEL (dialog->priv->store),
>+						 &iter, tree_path);
>+	g_return_if_fail (r);
>+
>+	gtk_list_store_set (dialog->priv->store, &iter,
>+			    NCB_TREESTORE_NEWNAME, new_string, -1);
>+}
>+
>+/* rename_menu_item_cb:
>+ *
>+ * Callback for the "activate" signal emitted by the rename popup menu item.
>+ * Updates the desired new name for the files represented by
>+ * the selected row. We disable that menu item when multiple rows are
>+ * selected. */
>+static void
>+rename_menu_item_cb (GtkWidget *widget,
>+		     gpointer   user_data)
>+{
>+	NcbRenameDialog		*dialog;
>+	GtkTreeSelection	*selection;
>+	GtkListStore		*store;
>+	GtkTreeViewColumn	*column;
>+	GtkTreePath		*path;
>+	GList			*list;
>+
>+	dialog = (NcbRenameDialog *) user_data;
>+
>+	store = GTK_LIST_STORE (dialog->priv->store);
>+
>+	selection = gtk_tree_view_get_selection (dialog->priv->view);
>+
>+	list = gtk_tree_selection_get_selected_rows (selection, NULL);
>+	if (g_list_length (list) != 1) {
>+
>+		/* This shouldn't happen. Rename makes no sense if more than
>+		 * one item is selected. */
>+		g_warning ("More than one row selected during a rename?");
>+	}
>+
>+	path = (GtkTreePath *) list->data;
>+
>+	column = gtk_tree_view_get_column (dialog->priv->view, 0);
>+	g_return_if_fail (column != NULL);

Generally you only use g_return_if_fail to check input into a public function.  If this is just error checking then you should use "if (FALSE) { g_warning(); return; }" if it only occurs when there is a programming error then use g_assert().

>+	/* Start editing the cell */
>+	gtk_tree_view_set_cursor (dialog->priv->view,
>+				  path, column, TRUE);
>+
>+	g_list_foreach (list, (GFunc)gtk_tree_path_free, NULL);
>+	g_list_free (list);
>+}
>+
>+/* delete_menu_item_cb:
>+ *
>+ * Callback for the "activate" signal emitted by the delete popup menu item.
>+ * Removes the file represented by the selected row(s) from burn://. */
>+static void
>+delete_menu_item_cb (GtkWidget *widget,
>+		     gpointer   user_data)
>+{
>+	NcbRenameDialog		*dialog;
>+	GtkTreeSelection	*selection;
>+	GtkTreeModel		*model;
>+	GList			*list;
>+	GList			*row_references = NULL;
>+	GList			*tmp;
>+
>+	dialog = (NcbRenameDialog *) user_data;
>+
>+	model = GTK_TREE_MODEL (dialog->priv->store);
>+
>+	selection = gtk_tree_view_get_selection (dialog->priv->view);
>+
>+	list = gtk_tree_selection_get_selected_rows (selection, NULL);
>+
>+	/* Build a GtkTreeRowReference list out of the GtkTreePath list 'cause
>+	 * we are going to modify the tree */
>+	for (tmp = list; tmp; tmp = g_list_next (tmp)) {
>+		GtkTreeRowReference *reference;
>+
>+		reference = gtk_tree_row_reference_new (model, (GtkTreePath *)tmp->data);
>+
>+		row_references = g_list_append (row_references, reference);
>+	}
>+	g_list_foreach (list, (GFunc)gtk_tree_path_free, NULL);
>+	g_list_free (list);
>+
>+
>+	/* Remove the selected rows and relative files */
>+	for (tmp = row_references; tmp; tmp = g_list_next (tmp)) {
>+		GtkTreePath	*path;
>+		GtkTreeIter	 iter;
>+
>+		path = gtk_tree_row_reference_get_path ((GtkTreeRowReference *)tmp->data);
>+
>+		if (gtk_tree_model_get_iter (model, &iter, path)) {
>+			char		*name = NULL;
>+			GnomeVFSResult	 res;
>+
>+			gtk_tree_model_get (model, &iter,
>+					    NCB_TREESTORE_OLDNAME, &name, -1);

Once the line is split somewhat arbitrarily I like to split it completely with an arg per line.

>+			res = gnome_vfs_unlink (name);
>+			if (res == GNOME_VFS_OK) {
>+				gtk_list_store_remove (GTK_LIST_STORE (model), &iter);
>+			} else {
>+				g_warning ("Cannot remove %s (%s)", name,
>+					   gnome_vfs_result_to_string (res));
>+			}
>+			g_free (name);
>+		}
>+	}
>+	g_list_foreach (row_references, (GFunc)gtk_tree_row_reference_free, NULL);
>+	g_list_free (row_references);
>+}
>+
>+/* show_popup_menu:
>+ *
>+ * @dialog: the #NcbRenameDialog.
>+ * @event:  #GdkEventButton that triggered the dialog,
>+ *          it can be NULL.
>+ * @show_rename: wheter to show the rename menu item.
>+ *
>+ * Shows the contextual menu. */
>+static void
>+show_popup_menu (NcbRenameDialog *dialog,
>+		 GdkEventButton  *event,
>+		 gboolean	  show_rename)
>+{
>+	GtkWidget *menu;
>+	GtkWidget *rename_item;
>+
>+	menu = dialog->priv->menu;
>+	rename_item = dialog->priv->rename_item;
>+
>+	gtk_widget_set_sensitive (rename_item, show_rename);
>+
>+	gtk_widget_show_all (menu);
>+
>+	gtk_menu_popup (GTK_MENU (menu), NULL, NULL, NULL, NULL,
>+			(event != NULL) ? event->button : 0,
>+			gdk_event_get_time ((GdkEvent *)event));
>+}
>+
>+/* treeview_button_pressed_cb:
>+ *
>+ * Callback for the "button-press-event" signal emitted by the treeview.
>+ * On right click, it selects the row under the cursor if no one is
>+ * selected and pops up the contextual menu. */
>+static gboolean
>+treeview_button_pressed_cb (GtkWidget		*treeview,
>+			    GdkEventButton	*event,
>+			    gpointer		 user_data)
>+{
>+        GtkTreeSelection *selection;
>+	NcbRenameDialog  *dialog;

Spacing issue here.

>+	if (event->type == GDK_BUTTON_PRESS && event->button == 3) {
>+		int rows;
>+
>+		selection = gtk_tree_view_get_selection (GTK_TREE_VIEW (treeview));
>+
>+		/* Select the row under the cursor if there is no other row selected
>+		 * or if there is another one selected */
>+		rows = gtk_tree_selection_count_selected_rows (selection);
>+		if (rows <= 1) {
>+			GtkTreePath *path;
>+
>+			/* Get tree path for row that was clicked */
>+			if (gtk_tree_view_get_path_at_pos (GTK_TREE_VIEW (treeview),
>+							  (gint) event->x,
>+							  (gint) event->y,
>+							  &path, NULL, NULL, NULL)) {
>+
>+				gtk_tree_selection_unselect_all (selection);
>+				gtk_tree_selection_select_path (selection, path);
>+				gtk_tree_path_free (path);
>+
>+				rows = 1;
>+			}
>+		}
>+
>+		dialog = (NcbRenameDialog *) user_data;
>+
>+		/* Show the popup menu and show the "rename" menu item only
>+		 * if we selected just a single row */
>+		show_popup_menu (dialog, event, (rows == 1));
>+
>+		return TRUE;
>+	}
>+
>+	return FALSE;
>+}
>+
>+/* treeview_popup_menu_cb:
>+ *
>+ * Callback for the "popup-menu" signal.
>+ * Pops up the contextual menu. */
>+static void
>+treeview_popup_menu_cb (GtkWidget	*widget,
>+			gpointer	 user_data)
>+{
>+	NcbRenameDialog *dialog;
>+
>+	dialog = (NcbRenameDialog *) user_data;
>+
>+	show_popup_menu (dialog, NULL, TRUE);
>+}
>+
>+static void
>+ncb_rename_dialog_init (NcbRenameDialog *dialog)
>+{
>+	GtkListStore		*store;
>+	GtkWidget		*view;
>+	GtkWidget		*scrolled;
>+	GtkTreeSelection	*selection;
>+	GtkCellRenderer		*renderer;
>+	GtkCellRenderer		*pixbuf_renderer;
>+	GtkTreeViewColumn	*newname;
>+	GtkWidget		*menu;
>+	GtkWidget		*rename_item;
>+	GtkWidget		*delete_item;
>+	char			*primary_text;
>+	char			*primary_markup;
>+
>+        dialog->priv = NCB_RENAME_DIALOG_GET_PRIVATE (dialog);

Spacing.

>+	gtk_window_set_resizable (GTK_WINDOW (dialog), TRUE);
>+
>+	/** Set primary and secondary labels **/
>+	primary_text = _("Some files have incorrect filenames");

Since this text is const it is probably ok to just put it as an arg to printf (on a separate line) or alternatively the variable should be marked as const.

Since you are adding strings for translation the file should be included in po/POTFILES.in

I think it should say "invalid" rather than "incorrect".

I think it is better to put this all in a glade file instead of having it all in the code.  Can you give that a try?

>+	primary_markup = g_strdup_printf ("<span size='larger' weight='bold'>%s</span>", primary_text);
>+	gtk_message_dialog_set_markup (GTK_MESSAGE_DIALOG (dialog),
>+				       primary_markup);
>+	g_free (primary_markup);
>+
>+	gtk_message_dialog_format_secondary_text (GTK_MESSAGE_DIALOG (dialog),
>+						  _("The following filenames can't be used, click to "
>+						    "rename them:"));
>+
>+	/** Set message type and buttons **/
>+	g_object_set (G_OBJECT (dialog),
>+		      "message-type", GTK_MESSAGE_WARNING,
>+		      "buttons", GTK_BUTTONS_OK_CANCEL,
>+		       NULL);
>+	gtk_dialog_set_default_response (GTK_DIALOG (dialog), GTK_BUTTONS_CANCEL);
>+
>+	/** Set list store and tree view **/
>+	store = gtk_list_store_new (3, G_TYPE_OBJECT, G_TYPE_STRING, G_TYPE_STRING);
>+	view = gtk_tree_view_new_with_model (GTK_TREE_MODEL (store));
>+	g_signal_connect (G_OBJECT (view), "button-press-event",
>+			  (GCallback) treeview_button_pressed_cb, dialog);
>+	g_signal_connect (G_OBJECT (view), "popup-menu",
>+			  (GCallback) treeview_popup_menu_cb, dialog);
>+
>+	/* Setup selection mode */
>+	selection = gtk_tree_view_get_selection (GTK_TREE_VIEW (view));
>+	gtk_tree_selection_set_mode (selection, GTK_SELECTION_MULTIPLE);
>+
>+	/* Icon renderer */
>+	pixbuf_renderer = gtk_cell_renderer_pixbuf_new ();
>+
>+	/* Filename renderer */
>+        renderer = gtk_cell_renderer_text_new ();
>+	g_object_set (G_OBJECT (renderer), "editable", TRUE,
>+		      NULL);

Spacing.

>+	g_signal_connect (G_OBJECT (renderer), "edited",
>+			  (GCallback) cell_editing_done_cb, dialog);
>+
>+	/* Column */
>+	newname = gtk_tree_view_column_new ();
>+	gtk_tree_view_column_set_title (newname, _("Filename"));
>+
>+	gtk_tree_view_column_pack_start (newname, pixbuf_renderer, FALSE);
>+	gtk_tree_view_column_set_attributes (newname, pixbuf_renderer,
>+					     "pixbuf", NCB_TREESTORE_PIXBUF, NULL);
>+	gtk_tree_view_column_pack_start (newname, renderer, FALSE);
>+	gtk_tree_view_column_set_attributes (newname, renderer,
>+					     "text", NCB_TREESTORE_NEWNAME, NULL);
>+
>+	/* Append column to the view */
>+        gtk_tree_view_append_column (GTK_TREE_VIEW (view), newname);

Spacing.

>+	/** Scrolled window **/
>+	scrolled = gtk_scrolled_window_new (NULL, NULL);
>+	gtk_scrolled_window_set_policy (GTK_SCROLLED_WINDOW (scrolled),
>+					GTK_POLICY_AUTOMATIC,
>+					GTK_POLICY_AUTOMATIC);
>+	gtk_container_add (GTK_CONTAINER (scrolled), view);
>+
>+	/* Set scrolled window's size */
>+	gtk_widget_set_size_request (scrolled, -1, 150);
>+
>+	gtk_box_pack_start (GTK_BOX (GTK_DIALOG (dialog)->vbox),
>+			    scrolled,
>+			    TRUE, TRUE, 0);

A small HIG thing: the box that the list is in should be aligned with the right side of the buttons at the bottom.

>+	/** Set popup menu **/
>+	menu = gtk_menu_new ();
>+
>+	rename_item = gtk_menu_item_new_with_label (_("Rename file"));
>+	g_signal_connect (rename_item, "activate",
>+			  (GCallback) rename_menu_item_cb, dialog);
>+
>+	delete_item = gtk_image_menu_item_new_from_stock (GTK_STOCK_REMOVE,
>+							  gtk_accel_group_new ());
>+	g_signal_connect (delete_item, "activate",
>+			  (GCallback) delete_menu_item_cb, dialog);
>+
>+	gtk_menu_shell_append (GTK_MENU_SHELL (menu), rename_item);
>+	gtk_menu_shell_append (GTK_MENU_SHELL (menu), delete_item);
>+
>+	dialog->priv->store = store;
>+	dialog->priv->view = GTK_TREE_VIEW (view);
>+	dialog->priv->menu = menu;
>+	dialog->priv->rename_item = rename_item;
>+
>+	gtk_widget_show_all (GTK_WIDGET (GTK_DIALOG (dialog)->vbox));
>+}
>+
>+/* make_valid_filename:
>+ *
>+ * @name: the string containing an invalid filename.
>+ *
>+ * Return a filename that can be written to a UTF-8 ISO9660
>+ * filesystem.
>+ * Adapted from gnome_vfs_make_valid_utf8().
>+ *
>+ * Return value: a newly allocated string with a valid
>+ *		 filename based on @name. Free with g_free (). */
>+static char *
>+make_valid_filename (const char *name)
>+{
>+        GString *string;
>+        const char *remainder, *invalid;
>+        int remaining_bytes, valid_bytes;

Put all declarations on separate lines and align them.

>+        string = NULL;
>+        remainder = name;
>+        remaining_bytes = strlen (name);
>+
>+        while (remaining_bytes != 0) {
>+                if (g_utf8_validate (remainder, remaining_bytes, &invalid))
>+                        break;

These days, I like to use braces in all cases.

>+                valid_bytes = invalid - remainder;
>+
>+                if (string == NULL) {
>+                        string = g_string_sized_new (remaining_bytes);
>+		}

Spacing on the brace.

>+                g_string_append_len (string, remainder, valid_bytes);
>+		/* Use the Unicode replacement character (<?>) as substitute
>+		 * for invalid chars. */
>+                g_string_append_unichar (string, 0xFFFD);

Spacing on the comment.

>+                remaining_bytes -= valid_bytes + 1;
>+                remainder = invalid + 1;
>+        }
>+
>+        if (string == NULL) {
>+                return g_strdup (name);
>+	}

Spacing.

>+        string = g_string_append (string, remainder);
>+        g_assert (g_utf8_validate (string->str, -1, NULL));
>+
>+        return g_string_free (string, FALSE);
>+}
>+
>+/* check_filename_cb:
>+ *
>+ * Callback for gnome_vfs_directory_visit().
>+ * Checks each file and adds it to the @dialog->priv->invalid_files list
>+ * if the filename is not encoded properly. */
>+static gboolean
>+check_filename_cb (const gchar		*rel_path,
>+		   GnomeVFSFileInfo	*info,
>+		   gboolean		 will_loop,
>+		   gpointer		 data,
>+		   gboolean		*recurse)

Just use char and not gchar.  Possible spacing issue.

>+{
>+	NcbRenameDialog	*dialog;
>+
>+	dialog = (NcbRenameDialog *) data;
>+
>+	if (! g_utf8_validate (info->name, -1, NULL)) {
>+		NcbRenameDialogInvalidFile *invalid_file;
>+
>+		invalid_file = g_new0 (NcbRenameDialogInvalidFile, 1);
>+		invalid_file->filename = g_strdup_printf ("%s/%s",
>+							  dialog->priv->root_path, rel_path);
>+		invalid_file->mime_type = g_strdup (info->mime_type);
>+
>+		dialog->priv->invalid_files = g_slist_prepend (dialog->priv->invalid_files,
>+							       invalid_file);
>+	}
>+
>+	*recurse = (info->type == GNOME_VFS_FILE_TYPE_DIRECTORY && ! will_loop);
>+
>+	/* Call me again. */
>+	return TRUE;
>+}
>+
>+/* pixmap_from_mime_type:
>+ *
>+ * @mime_type: the MIME type
>+ *
>+ * Get the icon representing a certain MIME type.
>+ *
>+ * Return value: #GdkPixbuf icon or NULL. Unref it with g_object_unref(). */

End the comment on a new line.

>+static const GdkPixbuf *
>+pixmap_from_mime_type (const char *mime_type)
>+{
>+        GtkIconTheme    *theme;
>+	GdkPixbuf	*pixbuf;
>+        char            *icon;

Spacing.

>+        g_return_val_if_fail (mime_type != NULL, NULL);
>+
>+        theme = gtk_icon_theme_get_default ();
>+
>+        icon = gnome_icon_lookup (theme, NULL, NULL, NULL, NULL,
>+                                  mime_type,
>+                                  GNOME_ICON_LOOKUP_FLAGS_NONE, NULL);
>+
>+        pixbuf = gtk_icon_theme_load_icon (theme, icon,
>+					   GTK_ICON_SIZE_SMALL_TOOLBAR, 0, NULL);
>+
>+	g_free (icon);
>+
>+	return pixbuf;
>+}
>+

Spacing.

>+/* add_to_liststore:
>+ *
>+ * @data: gpointer to a #NcbRenameDialogInvalidFile.
>+ * @user_data: gpointer to the #NcbRenameDialog.
>+ *
>+ * Adds a #NcbRenameDialogInvalidFile to the #NcbRenameDialog's
>+ * list store. */
>+static void
>+add_to_liststore (gpointer data,
>+		  gpointer user_data)
>+{
>+	GtkTreeIter			 iter;
>+	NcbRenameDialogInvalidFile	*invalid_file;
>+	char				*invalid_basename;
>+	NcbRenameDialog			*dialog;
>+	const GdkPixbuf			*pixbuf;
>+	char				*newname;
>+
>+	invalid_file = (NcbRenameDialogInvalidFile *) data;
>+	dialog = (NcbRenameDialog *) user_data;
>+
>+	/* Use just the basename for the suggested filenames list */
>+	invalid_basename = g_path_get_basename (invalid_file->filename);
>+	newname = make_valid_filename (invalid_basename);
>+	g_free (invalid_basename);
>+
>+	pixbuf = pixmap_from_mime_type (invalid_file->mime_type);
>+
>+	gtk_list_store_append (dialog->priv->store, &iter);
>+	gtk_list_store_set (dialog->priv->store, &iter,
>+			    NCB_TREESTORE_PIXBUF, pixbuf,
>+			    NCB_TREESTORE_OLDNAME, invalid_file->filename,
>+			    NCB_TREESTORE_NEWNAME, newname, -1);
>+
>+	g_free (newname);
>+}
>+
>+/* ncb_rename_dialog_list_free:
>+ *
>+ * dialog: the #NcbRenameDialog.
>+ *
>+ * Free the @dialog list of invalid files. */

Don't really need docs on the private functions.  But when you do try to follow the gtk-doc style that we use in the other files (say nautilus-burn-drive.c).

>+static void
>+ncb_rename_dialog_list_free (NcbRenameDialog *dialog)
>+{
>+	GSList *list;
>+
>+	list = dialog->priv->invalid_files;
>+	for (; list; list = g_slist_next (list)) {

Probably good to initialize list in the for statement for symmetry.  Can appreviate list as "l" if you want to save space (since it is temporary).

>+		NcbRenameDialogInvalidFile *invalid;
>+
>+		invalid = (NcbRenameDialogInvalidFile *) list->data;
>+
>+		g_free (invalid->mime_type);
>+		g_free (invalid->filename);
>+		g_free (invalid);
>+	}
>+	g_slist_free (dialog->priv->invalid_files);
>+
>+	dialog->priv->invalid_files = NULL;
>+}
>+
>+/* burn_folder_get_invalid_filenames:
>+ *
>+ * Scan the burn folder for invalid filenames (incorrectly encoded, name too long, etc.)
>+ *
>+ * Returns: a #GHashTable of invalid filenames in the burn folder where
>+ *	       keys are invalid filenames and
>+ *	       values are suggested valid filenames
>+ *	    or NULL if there aren't invalid filenames */

Use the doc style that we use in other places.

>+gboolean
>+ncb_rename_dialog_set_invalid_filenames (NcbRenameDialog *dialog,
>+					 const char	 *uri)
>+{
>+	GnomeVFSResult		 res;
>+	gboolean		 found;
>+
>+	g_return_val_if_fail (dialog != NULL, FALSE);
>+	g_return_val_if_fail (NCB_IS_RENAME_DIALOG (dialog), FALSE);
>+	g_return_val_if_fail (uri != NULL, FALSE);
>+
>+	gtk_list_store_clear (dialog->priv->store);
>+
>+	dialog->priv->root_path = g_strdup (uri);
>+
>+	res = gnome_vfs_directory_visit (uri,
>+					 GNOME_VFS_FILE_INFO_GET_MIME_TYPE|GNOME_VFS_FILE_INFO_FORCE_FAST_MIME_TYPE,
>+					 GNOME_VFS_DIRECTORY_VISIT_LOOPCHECK,
>+					 check_filename_cb, dialog);

Need spaces around the "|" and should probably be broken onto separate lines.

>+	g_free (dialog->priv->root_path);
>+
>+	if (res != GNOME_VFS_OK) {
>+		g_warning ("gnome_vfs_directory_visit () failed for %s",
>+			   uri);
>+
>+		return FALSE;
>+	}
>+
>+	/* Add invalid files to the liststore */
>+	g_slist_foreach (dialog->priv->invalid_files, add_to_liststore, dialog);
>+	found = (dialog->priv->invalid_files != NULL);
>+
>+	ncb_rename_dialog_list_free (dialog);
>+
>+	return found;
>+}
>+
>+/* ncb_rename_dialog_new:
>+ *
>+ * Create a new #NcbRenameDialog.
>+ *
>+ * Return value: a new #NcbRenameDialog or NULL. Free with gtk_widget_destroy() */

Doc style.

>+NcbRenameDialog *
>+ncb_rename_dialog_new (void)
>+{
>+        GObject *object;
>+
>+        object = g_object_new (NCB_TYPE_RENAME_DIALOG, NULL);
>+	g_return_val_if_fail (object != NULL, NULL);

Spacing.  And shouldn't be using g_return_* here but an assert if it is needed at all.

>+	gtk_window_set_title (GTK_WINDOW (object), "");
>+	gtk_window_set_icon_name (GTK_WINDOW (object), "nautilus-cd-burner");

Spacing.  These functions should probably go into the _init() instead.

>+        return NCB_RENAME_DIALOG (object);
>+}
>+
>+static void
>+ncb_rename_dialog_finalize (GObject *object)
>+{
>+        NcbRenameDialog *dialog;
>+
>+        g_return_if_fail (object != NULL);
>+        g_return_if_fail (NCB_IS_RENAME_DIALOG (object));
>+
>+        dialog = NCB_RENAME_DIALOG (object);
>+        g_return_if_fail (dialog->priv != NULL);
>+
>+	gtk_list_store_clear (dialog->priv->store);
>+
>+	G_OBJECT_CLASS (ncb_rename_dialog_parent_class)->finalize (object);
>+}

Spacing.

>--- /dev/null	2006-06-19 18:46:33.000000000 +0200
>+++ src/ncb-rename-dialog.h	2006-07-09 15:38:33.000000000 +0200
>@@ -0,0 +1,66 @@
>+/* -*- Mode: C; indent-tabs-mode: t; c-basic-offset: 8; tab-width: 8 -*-
>+ *
>+ * Copyright (C) 2006 Fabio Bonelli <fabiobonelli@libero.it>
>+ *
>+ *  ncb-rename-dialog: Provide a dialog to rename files when their
>+ *  name is invalid (eg. not encoded properly, too long, etc.).
>+ *
>+ * This program is free software; you can redistribute it and/or
>+ * modify it under the terms of the GNU General Public License as
>+ * published by the Free Software Foundation; either version 2 of the
>+ * License, or (at your option) any later version.
>+ *
>+ * This program is distributed in the hope that it will be useful,
>+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
>+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>+ * General Public License for more details.
>+ *
>+ * You should have received a copy of the GNU General Public
>+ * License along with this program; if not, write to the
>+ * Free Software Foundation, Inc., 59 Temple Place - Suite 330,
>+ * Boston, MA 02111-1307, USA.
>+ *
>+ * Authors: Fabio Bonelli <fabiobonelli@libero.it> */
>+
>+#ifndef NCB_RENAME_DIALOG_H
>+#define NCB_RENAME_DIALOG_H
>+
>+#include <glib-object.h>
>+#include <gtk/gtk.h>
>+
>+G_BEGIN_DECLS
>+
>+#define NCB_TYPE_RENAME_DIALOG            (ncb_rename_dialog_get_type ())
>+#define NCB_RENAME_DIALOG(obj)            (G_TYPE_CHECK_INSTANCE_CAST ((obj), NCB_TYPE_RENAME_DIALOG,	\
>+					   NcbRenameDialog))
>+#define NCB_RENAME_DIALOG_CLASS(klass)    (G_TYPE_CHECK_CLASS_CAST ((klass), NCB_TYPE_RENAME_DIALOG,	\
>+					   NcbRenameDialogClass))
>+#define NCB_IS_RENAME_DIALOG(obj)         (G_TYPE_CHECK_INSTANCE_TYPE ((obj),  NCB_TYPE_RENAME_DIALOG))
>+#define NCB_IS_RENAME_DIALOG_CLASS(klass) (G_TYPE_INSTANCE_GET_CLASS ((klass), NCB_TYPE_RENAME_DIALOG))
>+
>+typedef struct NcbRenameDialog                   NcbRenameDialog;
>+typedef struct NcbRenameDialogClass              NcbRenameDialogClass;
>+typedef struct NcbRenameDialogPrivate            NcbRenameDialogPrivate;
>+
>+struct NcbRenameDialog {
>+	GtkMessageDialog	 parent;
>+
>+	NcbRenameDialogPrivate  *priv;
>+};
>+
>+struct NcbRenameDialogClass {
>+	GtkMessageDialogClass parent_class;
>+};
>+
>+GType			ncb_rename_dialog_get_type		(void);
>+
>+NcbRenameDialog       *	ncb_rename_dialog_new			(void);

Spacing.

>+gboolean		ncb_rename_dialog_set_invalid_filenames	(NcbRenameDialog	*dialog,
>+								 const char		*uri);
>+void			ncb_rename_dialog_response_cb		(NcbRenameDialog	*dialog,
>+								 int			 id,
>+							         gpointer		 user_data);
>+
>+G_END_DECLS
>+
>+#endif /* ! NCB_RENAME_DIALOG_H */
Comment 8 Fabio Bonelli 2006-11-19 13:00:38 UTC
>>+#include <glade/glade.h>
>>+#include <libgnomevfs/gnome-vfs.h>
>>+#include <libgnomeui/libgnomeui.h>

>I see that this pulls in libgnomeui for the mime-type stuff.  I'd rather not do
>this.  Is there another way?

We need a replacement for gnome_icon_lookup().

>I think it is better to put this all in a glade file instead of having it all
>in the code.  Can you give that a try?

The documentation doesn't provide any hint on how to integrate libglade and GObjects. I must admit that sometimes GNOME documentation can be frustrating even for basic procedures.
Comment 9 Fabio Bonelli 2006-11-19 13:01:45 UTC
Created attachment 76840 [details] [review]
New patch
Comment 10 Fabio Bonelli 2006-12-04 14:13:35 UTC
*** Bug 341621 has been marked as a duplicate of this bug. ***
Comment 11 William Jon McCann 2006-12-04 17:20:30 UTC
Created attachment 77658 [details] [review]
patch to use glade

This patch is after the above patch was committed to CVS HEAD.  It converts it to use glade.
Comment 12 William Jon McCann 2006-12-04 17:21:43 UTC
Looks good Fabio!  I've committed this to HEAD.  Thanks.