GNOME Bugzilla – Bug 326173
Copy disc... option in right click menu over blank cd is not useful.
Last modified: 2006-08-04 18:02:06 UTC
The Copy disc... option of the right click menu over a blank cd is not useful, so it might be hidden of the menu if the cd is blank. Other information:
Created attachment 65797 [details] [review] Fix for this bug
I think that the option should be hidden, instead of disabled if there's a blank CD in the drive, and it should also hide the entry if the drive is empty.
Created attachment 65820 [details] [review] New patch This patch disables the item for empty drives as well. I don't agree with you about removing the menu item, the HIG says "Do not add or remove individual menu items while the application is running, make them insensitive instead." (http://developer.gnome.org/projects/gup/hig/2.0/menus-types.html)
This is a contextual menu, not a "proper" application menu. For instance, right-click on the background (ie. not on a file) in nautilus. Do you see "Move to trash"? Right-click on a file, you will see "Move to trash".
I think that that parallelism is not fair: you are comparing two semantically different objects and, because of this, the context changes and so does the contextual menu (of course). In my opinion, the mounted disc icon and the drive icon is nearly the same place/thing to the user, it just gives a little clue when a disc is present.
Jon, what's your take on this? I guess that it should also disable/hide the menu item if there's no disc in the drive. This was also reported downstream: https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=171030
Comment on attachment 65820 [details] [review] New patch >+ volumes = gnome_vfs_drive_get_mounted_volumes (drive); >+ vol = g_list_nth_data (volumes, 0); Why do you assume that the current volume is the 0th in the list of mounted volumes? That doesn't seem right. >+ /* Check if the volume is blank, in that case set the "Copy disc" >+ * menu item to insensitive */ >+ if (vol) { Please use an explicit != NULL check here. We should only check boolean values for truth. >+ if (dbus_error_is_set (&error)) { >+ g_warning ("%s\n", error.message); >+ dbus_error_free (&error); >+ >+ goto out; >+ } Seems like an indentation problem here. >+ if (is_blank) { >+ value = g_new0 (GValue, 1); >+ >+ g_value_init (value, G_TYPE_BOOLEAN); >+ g_value_set_boolean (value, FALSE); >+ >+ g_object_set_property (G_OBJECT (item), "sensitive", value); >+ >+ g_free (value); >+ } Why not just use: g_object_set (item, "sensitive", !is_blank, NULL); That said, I think we should not show it instead of making it insensitive. For example, we wouldn't show the "Mount Drive" option if the drive is already mounted (or we shouldn't). >+out: >+ gnome_vfs_drive_volume_list_free (volumes); >+ I think you also need to unref the volumes.
Sorry, I was thinking about gnome_vfs_volume_monitor_get_mounted_volumes() your usage of gnome_vfs_drive_get_mounted_volumes seems fine. And also fine on the free/unref.
Created attachment 69681 [details] [review] patch How about this one?
Looks fine to me. Can this go into 2.16 ?
Yes.