How useful is this compared to using nautilus to copy the file?

This would be form more practises to copy the file, in not need to open the destination folder.

i'd like to have this too, as i rather often used it in thunar and i missed it ever since i switched to gnome and nautilus.

I also think this is very useful, I use it a lot in winxp, it's a typical situation that someone comes to your desk with a memory usb and says "could you copy here the xyz file", and I just have to find that file in my pc and right-click and select "send-to -> usb device".

It's just a shortcut to do some repetitive task quicker, I call that usability :) .

just changing title to remove USB, as perhaps we want to support Firewire and e-sata :)

See patch in bug 543965 comment 1.

Apart from not applying cleanly anymore (files moved around in SVN), the patch has:

+		GFile *source = g_file_new_for_uri (iter->data);
+		gchar *file_name = g_file_get_basename (source);
+		gchar *dest_uri = g_strconcat (vol_uri, "/", file_name, NULL);
+		GFile *dest = g_file_new_for_uri (dest_uri);

This is wrong, and you should be using g_file_get_child () instead.

+		g_file_copy (source, dest, G_FILE_COPY_NONE, NULL, NULL, NULL, NULL);

Just like for the nautilus-burn plugin, this won't work for directories.

The code should be shared between the 2 plugins, which I can do if you write the code for recursively copying directories properly.

Created attachment 126238 [details] [review]
corrected patch

Comment on attachment 126238 [details] [review]
corrected patch

>Index: configure.in
>===================================================================
>--- configure.in	(revision 385)
>+++ configure.in	(working copy)
>@@ -36,6 +36,7 @@
> GUPNP_AV_REQUIRED=0.2.1
> EMPATHY_REQUIRED=2.23.91
> EMPATHY_GTK_REQUIRED=2.25.2
>+GIO_REQUIRED=2.18.0

Why 2.18?

> 
> AC_SUBST(GLIB_REQUIRED)
> AC_SUBST(GTK_REQUIRED)
>@@ -56,7 +57,8 @@
> 	 gmodule-2.0 >= $GLIB_REQUIRED		  \
> 	 gtk+-2.0    >= $GTK_REQUIRED             \
> 	 libglade-2.0 >= $GLADE_REQUIRED            \
>-	 gconf-2.0 >= $GCONF_REQUIRED)
>+	 gconf-2.0 >= $GCONF_REQUIRED               \
>+	 gio-2.0 >= $GIO_REQUIRED)

That should filed in a separate bug.

> # The full list of plugins
>-allowed_plugins="balsa bluetooth empathy evolution gaim gajim nautilus-burn pidgin sylpheed-claws thunderbird upnp"
>+allowed_plugins="balsa bluetooth empathy evolution gaim gajim nautilus-burn pidgin share sylpheed-claws thunderbird upnp"
> 
> plugin_error_or_ignore()
> {
>@@ -226,6 +228,9 @@
> 				add_plugin="0"
> 			fi
> 		;;
>+		share)
>+			add_plugin="1"
>+		;;

Please call the plugin something other than share. Call it "removable-devices" or something, share is too broad a term.

<snip>
>+GVolumeMonitor* vol_monitor = NULL;
>+GList *vol;

Please call it "volumes", or "volumes_list". "vol" makes me think there's only one item, not a list.

<snip>
>+static
>+GtkWidget* get_contacts_widget (NstPlugin *plugin)
>+{
>+	GtkListStore *store;
>+	GtkWidget *cb;
>+	GList *liter;

It's not an iter, and we usually call those just "*l"

>+	GtkTreeIter iter;
>+	GtkCellRenderer *renderer;
>+
>+	store = gtk_list_store_new (1, G_TYPE_STRING);

No icon for the devices?

<snip>
>+static
>+void copy_dir (GFile* source, GFile* dest)
>+{
>+	GFileEnumerator* en = g_file_enumerate_children (source, "*", G_FILE_QUERY_INFO_NONE, NULL, NULL);

No big assignments like that in the declarations.

>+	GFileInfo* info;
>+
>+	g_file_make_directory (dest, NULL, NULL);

You should check for errors here.

>+	while ((info = g_file_enumerator_next_file (en, NULL, NULL)) != NULL)
>+	{

Braces on the same line as the conditional.

>+		gchar *name = (gchar*)g_file_info_get_name (G_FILE_INFO (info));

Why do you need a cast here?

>+		if (name != NULL)
>+		{
>+			GFile *child = g_file_get_child (source, name);
>+			gchar *file_name = g_file_get_basename (child);
>+			GFile *dst = g_file_get_child (dest, file_name);
>+
>+			if (g_file_query_file_type (child, G_FILE_QUERY_INFO_NONE, NULL) == G_FILE_TYPE_DIRECTORY) copy_dir (child, dst);
>+			else g_file_copy (child, dst, G_FILE_COPY_NONE, NULL, NULL, NULL, NULL);
>+
>+			g_free (file_name);
>+			g_object_unref (child);
>+			g_object_unref (dst);
>+		}

All this code looks like it's the same as in send_files below. Could you please merge those?

>+		

There's a tab here on an empty line, please remove those.

>+		g_free (name);
>+	}
>+
>+	g_object_unref (en);
>+}
>+
>+static
>+gboolean send_files (NstPlugin *plugin, GtkWidget *contact_widget,
>+			 GList *file_list)

Please use:
static gboolean <- same line
send_files (...) <- another line

>+{
>+	int choice = gtk_combo_box_get_active (GTK_COMBO_BOX (contact_widget));

Same problem with the declaration/assignment as above.

>+	GMount *dest_vol;
>+	GFile *vol_root;
>+	gchar *vol_uri;
>+	GList *iter;
>+
>+	if (choice < 0) return TRUE;

Please use 2 lines for that.

>+	dest_vol = g_list_nth_data (vol, choice);
>+	if (!G_IS_MOUNT (dest_vol)) return TRUE;
>+	vol_root = g_mount_get_root (dest_vol);
>+	vol_uri = g_file_get_uri (vol_root);
>+
>+
>+	for (iter = file_list; iter != NULL; iter = g_list_next (iter))
>+	{
>+		GFile *source;
>+		gchar *file_name;
>+		GFile *dest;
>+
>+		if (g_uri_parse_scheme (iter->data) == NULL) source = g_file_new_for_path (iter->data);
>+		else source = g_file_new_for_uri (iter->data);
>+
>+		file_name = g_file_get_basename (source);
>+		dest = g_file_get_child (vol_root, file_name);
>+
>+		if (g_file_query_file_type (source, G_FILE_QUERY_INFO_NONE, NULL) == G_FILE_TYPE_DIRECTORY) copy_dir (source, dest);
>+		else g_file_copy (source, dest, G_FILE_COPY_NONE, NULL, NULL, NULL, NULL);
>+
>+		g_free (file_name);
>+		g_object_unref (source);
>+		g_object_unref (dest);
>+	}
>+
>+	g_object_unref (vol_root);
>+	g_free (vol_uri);
>+
>+	return TRUE;
>+}
>+
>+static
>+gboolean destroy (NstPlugin *plugin){
>+	g_list_free (vol);

You're leaking all the objects in the list. Each one of them needs a g_object_unref(). Use g_list_foreach for that.

Created attachment 126398 [details] [review]
corrected patch

Comment on attachment 126398 [details] [review]
corrected patch

<snip>

>+static GtkWidget*
>+get_contacts_widget (NstPlugin *plugin)
>+{
>+	GtkListStore *store;
>+	GtkWidget *cb;
>+	GList *l;
>+	GtkTreeIter iter;
>+	GtkCellRenderer *text_renderer, *icon_renderer;
>+
>+	store = gtk_list_store_new (2, G_TYPE_STRING, G_TYPE_ICON);
>+
>+	for (l = volumes; l != NULL; l = g_list_next (l)) {

I prefer l = l->next instead of g_list_next().

>+
>+		gtk_list_store_append (store, &iter);
>+		gtk_list_store_set (store, &iter, 0, g_mount_get_name (l->data), 1,
>+				    g_mount_get_icon (l->data), -1);
>+	}
>+
>+	cb = gtk_combo_box_new_with_model (GTK_TREE_MODEL (store));
>+
>+	text_renderer = gtk_cell_renderer_text_new ();
>+	icon_renderer = gtk_cell_renderer_pixbuf_new ();
>+	gtk_cell_layout_pack_start (GTK_CELL_LAYOUT (cb), icon_renderer, FALSE);
>+	gtk_cell_layout_pack_start (GTK_CELL_LAYOUT (cb), text_renderer, TRUE);
>+
>+	gtk_cell_layout_set_attributes (GTK_CELL_LAYOUT (cb), text_renderer, "text", 0,  NULL);
>+	gtk_cell_layout_set_attributes (GTK_CELL_LAYOUT (cb), icon_renderer, "gicon", 1,  NULL);
>+	gtk_combo_box_set_active (GTK_COMBO_BOX (cb), 0);

You're not updating the combobox selection when volumes are added or removed. You should listen to the "volume-added", "volume-changed" and "volume-removed" signals.

>+static gboolean
>+copy_fobject (GFile* source, GFile* dst)
>+{
>+	GFileEnumerator* en;
>+	GFileInfo* info;
>+	GError *err = NULL;
>+	gchar *file_name = g_file_get_basename (source);
>+	GFile *dest = g_file_get_child (dst, file_name);

Again, please no assignments during declaration.

>+	if (g_file_query_file_type (source, G_FILE_QUERY_INFO_NONE, NULL) != G_FILE_TYPE_DIRECTORY) {
>+		gboolean ret = g_file_copy (source, dest, G_FILE_COPY_NONE, NULL, NULL, NULL, NULL);

Same again.

>+	while ((info = g_file_enumerator_next_file (en, NULL, &err)) != NULL) {
>+		const gchar *name = g_file_info_get_name (G_FILE_INFO (info));

Same.

>+		if (name != NULL) {
>+			GFile *child = g_file_get_child (source, name);

And again.

>+		if (g_uri_parse_scheme (l->data) == NULL) source = g_file_new_for_path (l->data);
>+		else source = g_file_new_for_uri (l->data);

Please indent properly. You're leaking the output of g_uri_parse_scheme().

Created attachment 126547 [details] [review]
corrected patch

add listen to the "mount-added", "mount-changed" and "mount-removed" && remove assignments during declaration.

+		gchar* scheme;

Use char, not gchar (everywhere)

+		if (scheme == NULL) source = g_file_new_for_path (l->data);
+		else source = g_file_new_for_uri (l->data);

Still the broken identation

+	g_signal_connect_swapped (G_OBJECT (vol_monitor), "mount-removed", G_CALLBACK (get_contacts_widget), plugin);
+	g_signal_connect_swapped (G_OBJECT (vol_monitor), "mount-added", G_CALLBACK (get_contacts_widget), plugin);
+	g_signal_connect_swapped (G_OBJECT (vol_monitor), "mount-changed", G_CALLBACK (get_contacts_widget), plugin);

Sorry about misleading you to use the volume-* signals.

This isn't quite good enough. We know which mount was added/removed, so we should remove just that one from the liststore, or add just that one, otherwise if the user removes a device when nautilus-sendto is running, then the user's selection will be lost. You were also leaking a ListStore for every change, as you weren't destroying the old one.

The rest looks alright.

Created attachment 126924 [details] [review]
corrected patch

Made a few changes to your patch and committed. FYI, the changes were:
- no need to use the UUID (and some devices won't have any), just compare the mounts instead
- Remove use of "magic" numbers, use an enum for the column numbers instead
- No need to have a list replicating the same data as the GtkListStore, just use the list store itself

2009-02-06  Bastien Nocera  <hadess@hadess.net>

        * configure.in:
        * src/plugins/removable-devices/Makefile.am:
        * src/plugins/removable-devices/removable-devices.c
        (cb_mount_removed), (cb_mount_changed), (cb_mount_added), (init),
        (get_contacts_widget), (copy_fobject), (send_files), (destroy):
        Patch from Maxim Ermilov <zaspire@rambler.ru> adding a "Removable
        Devices" plugin (Closes: #453758)


Please test and open a new bug if it doesn't work as expected.