Re: [Nautilus-list] Need to add Removabel Media Support for Solaris on gnome CVS

[Alex Larsson <alexl redhat com>]

On Sat, 6 Apr 2002, Rajeev  Karale wrote:

> Hi all,
> I have attached the diff(patch).
> Will be waiting for your feedback,
> thanx,

I don't like the configure.in changes at all. Tha is not the way autoconf 
is supposed to work. You're supposed to look for specific features instead 
of having architecture-specific ifdefs in the code.

A better way would be to have code in configure.in that looks for the 
existance of the helper apps and defines e.g. HAVE_MEDIA_PROTECT_COMMAND 
and sets a corresponding define MEDIA_PROTECT_COMMAND to 
"gmedia_prop -d %s". 

This should be done separately for format, media protect and media 
properties. And the code that looks for format should also look for 
gfloppy and use "gfloppy --device %s" for the command if gmedia_format was 
not found.

Specific comments on the code:

--- nautilus/src/file-manager/fm-desktop-icon-view.c	Fri Apr  5 01:29:25 2002
+++ nautilus_new/src/file-manager/fm-desktop-icon-view.c	Sat Apr  6 10:54:10 2002
@@ -119,6 +119,9 @@ static gboolean real_supports_zooming   
 static void     update_disks_menu                                 (FMDesktopIconView      *view);
 static void     free_volume_black_list                            (FMDesktopIconView      *view);
 static gboolean	volume_link_is_selection 			  (FMDirectoryView 	  *view);
+#ifdef SOLARIS_CODE
+static NautilusDeviceType volume_link_device_type                 (FMDirectoryView        *view);
+#endif
 

This function should not be ifdefed like this. Every architecture should 
use the same codepath.


 EEL_CLASS_BOILERPLATE (FMDesktopIconView,
 			      fm_desktop_icon_view,
@@ -687,6 +690,62 @@ unmount_volume_callback (BonoboUICompone
 	nautilus_file_list_free (selection);
 }
 
+/*
+ * Single callback function for format, protect and properties.
+ */
+
+static void
+media_callback (BonoboUIComponent *component, gpointer data, const char *verb)
+{
+        FMDirectoryView *view;
+        FILE *file1;
+        const gchar *command;
+        NautilusFile *file;
+        gchar *device_path;
+        NautilusVolume *volume;
+        gchar *uri, *path, *mount_uri, *mount_path;
+        GList *selection;
+
+        g_assert (FM_IS_DIRECTORY_VIEW (data));
+
+        view = FM_DIRECTORY_VIEW (data);
+
+        if (!volume_link_is_selection (view)) {
+                return;
+        }
+
+        selection = fm_directory_view_get_selection (view);
+
+        file = NAUTILUS_FILE (selection->data);
+        uri = nautilus_file_get_uri (file);
+        path = gnome_vfs_get_local_path_from_uri (uri);
+        if (path != NULL) {
+                mount_uri = nautilus_link_local_get_link_uri (path);
+                mount_path = gnome_vfs_get_local_path_from_uri (mount_uri);

You need to check for mount_path != NULL before you use it, since the path 
may not be local.

+                volume = nautilus_volume_monitor_get_volume_for_path (nautilus_volume_monitor_get (),mount_path);
+                device_path = (gchar *)nautilus_volume_get_device_path(volume);

Missing space before parenthesis.
Don't cast away the const from the device path.

+                        if (mount_path != NULL) {
+                                if (!strcmp(verb,"Format Conditional"))
+                                command = g_strdup_printf ("/opt/gnome-2.0/bin/gmedia_format -d %s", device_path);
+                                if (!strcmp(verb,"Media Properties Conditional"))
+                                command = g_strdup_printf ("/opt/gnome-2.0/bin/gmedia_prop -d %s", device_path);
+                                if (!strcmp(verb,"Protect Conditional"))
+                                command = g_strdup_printf ("/opt/gnome-2.0/bin/gmedia_prot -d %s", device_path);
+                                file1 = popen (command, "r");
+                                pclose (file1);
+			}

This part should be written more like:

command = NULL;

#ifdef HAVE_MEDIA_FORMAT_COMMAND
if (strcmp(verb, "Format Conditional)  == 0) {
	command = g_strdup_printf (MEDIA_FORMAT_COMMAND, device_path);
}
#endif
#ifdef HAVE_MEDIA_PROPERTIES_COMMAND
if (strcmp(verb, "Media Properties Conditional)  == 0) {
	command = g_strdup_printf (MEDIA_PROPERTIES_COMMAND, device_path);
}
#endif
#ifdef HAVE_MEDIA_PROTECT_COMMAND
if (strcmp(verb, "Protect Conditional)  == 0) {
	command = g_strdup_printf (MEDIA_PROTECT_COMMAND, device_path);
}
#endif

Then, if command != NULL it must fork off a process that runs command 
instead of hanging nautilus waiting for it to finish. And you much check 
the return value for popen().

+                g_free (mount_path);
+                g_free (mount_uri);
+                g_free (path);
+        }
+        g_free (uri);
+
+        nautilus_file_list_free (selection);
+}
+
+
+
 static gboolean
 trash_link_is_selection (FMDirectoryView *view)
 {
@@ -747,6 +806,36 @@ volume_link_is_selection (FMDirectoryVie
 	return result;
 }
 
+#ifdef SOLARIS_CODE

This code should not be #ifdefed


+/*
+ *  Returns Device Type for device icon on desktop
+ */
+static NautilusDeviceType
+volume_link_device_type (FMDirectoryView *view)

This function may be better named volume_link_get_device_type()

+{
+        GList *selection;
+        /*const GList *disk_list,*element;*/

Don't leave old code left commented out.

+        gboolean result;
+        gchar *uri, *path, *mount_uri, *mount_path;
+        const NautilusVolume *volume;
+        result = FALSE;

retult is not used.

+        selection = fm_directory_view_get_selection (view);
+        uri = nautilus_file_get_uri (NAUTILUS_FILE (selection->data));
+        path = gnome_vfs_get_local_path_from_uri (uri);

You must check that path != NULL

+        mount_uri = nautilus_link_local_get_link_uri (path);
+        mount_path = gnome_vfs_get_local_path_from_uri (mount_uri);

Check again.

+        volume = nautilus_volume_monitor_get_volume_for_path (nautilus_volume_monitor_get (),mount_path);

Missing a space after the comma.

+        g_free (mount_path);
+        g_free (mount_uri);
+        g_free (path);
+        g_free (uri);
+        nautilus_file_list_free (selection);
+        return nautilus_volume_get_device_type(volume);

Missing a space before the parenthesis

+}
+#endif
+
+
 static void
 fm_desktop_icon_view_trash_state_changed_callback (NautilusTrashMonitor *trash_monitor,
 						   gboolean state,
@@ -1170,23 +1259,110 @@ real_update_menus (FMDirectoryView *view
 		g_free (label);
 	}
 
-	/* Unmount Volume */
 	include_unmount_volume = volume_link_is_selection (view);


This variable would be better named include_media_commands instead.


-	nautilus_bonobo_set_hidden
-		(desktop_view->details->ui,
-		 DESKTOP_COMMAND_UNMOUNT_VOLUME_CONDITIONAL,
-		 !include_unmount_volume);
-	if (include_unmount_volume) {
-		label = g_strdup (_("Unmount Volume"));
-		nautilus_bonobo_set_label
-			(desktop_view->details->ui, 
-			 DESKTOP_COMMAND_UNMOUNT_VOLUME_CONDITIONAL,
-			 label);


I wonder why this code was done in this way. Please try to rename the 
actual menu item in the xml file to "Unmount Volume" instead of "Empty 
Trash" and see if that works. If so, the code that sets the label should 
be removed.


-		nautilus_bonobo_set_sensitive 
-			(desktop_view->details->ui, 
-			 DESKTOP_COMMAND_UNMOUNT_VOLUME_CONDITIONAL, TRUE);
-		g_free (label);
-	}
+        nautilus_bonobo_set_hidden
+                (desktop_view->details->ui,
+                 DESKTOP_COMMAND_UNMOUNT_VOLUME_CONDITIONAL,
+                 !include_unmount_volume);
+
+        nautilus_bonobo_set_hidden
+                (desktop_view->details->ui,
+                 DESKTOP_COMMAND_EJECT_VOLUME_CONDITIONAL,
+                 !include_unmount_volume);
+
+        nautilus_bonobo_set_hidden
+                (desktop_view->details->ui,
+                 DESKTOP_COMMAND_PROTECT_VOLUME_CONDITIONAL,
+                 !include_unmount_volume);
+
+        nautilus_bonobo_set_hidden
+                (desktop_view->details->ui,
+                 DESKTOP_COMMAND_FORMAT_VOLUME_CONDITIONAL,
+                 !include_unmount_volume);
+
+        nautilus_bonobo_set_hidden
+                (desktop_view->details->ui,
+                 DESKTOP_COMMAND_MEDIA_PROPERTIES_VOLUME_CONDITIONAL,
+                 !include_unmount_volume);
+
+#ifdef SOLARIS_CODE

Why do you have different code paths for SOLARIS and all other 
architectures? This should be one codepath and properly check 
HAVE_MEDIA_FORMAT_COMMAND etc.


+        nautilus_bonobo_set_hidden
+                (desktop_view->details->ui,
+                 DESKTOP_COMMAND_UNMOUNT_VOLUME_CONDITIONAL,
+                 TRUE);

Why do you always hide unmount on solaris? 

+
+	/* Iterate the removabel device list and hide/show specific menu items for each removable device
+         * (e.g. hide format for CD, show format for floppy)
+         */
+        if (include_unmount_volume) {
+                if (volume_link_device_type (view) == NAUTILUS_DEVICE_FLOPPY_DRIVE) {

You should only call volume_link_device_type() once. And store the value 
for all the checks.

+                        nautilus_bonobo_set_sensitive
+                                (desktop_view->details->ui,
+                                 DESKTOP_COMMAND_PROTECT_VOLUME_CONDITIONAL,
+                                 FALSE);
+
+                        nautilus_bonobo_set_sensitive
+                                (desktop_view->details->ui,
+                                 DESKTOP_COMMAND_FORMAT_VOLUME_CONDITIONAL,
+                                 TRUE);
+                }
+
+                if (volume_link_device_type (view) == NAUTILUS_DEVICE_CDROM_DRIVE) {
+                        nautilus_bonobo_set_sensitive
+                                (desktop_view->details->ui,
+                                 DESKTOP_COMMAND_FORMAT_VOLUME_CONDITIONAL,
+                                 FALSE);
+
+                        nautilus_bonobo_set_sensitive
+                                (desktop_view->details->ui,
+                                 DESKTOP_COMMAND_PROTECT_VOLUME_CONDITIONAL,
+                                 FALSE);
+                }
+
+                if (volume_link_device_type (view) == NAUTILUS_DEVICE_ZIP_DRIVE ||
+                    volume_link_device_type (view) == NAUTILUS_DEVICE_JAZ_DRIVE) {
+                        nautilus_bonobo_set_sensitive
+                                (desktop_view->details->ui,
+                                 DESKTOP_COMMAND_PROTECT_VOLUME_CONDITIONAL,
+                                 TRUE);
+                        nautilus_bonobo_set_sensitive
+                                (desktop_view->details->ui,
+                                 DESKTOP_COMMAND_FORMAT_VOLUME_CONDITIONAL,
+                                 TRUE);
+                }
+        }


I don't think it makes sense to have the operations that don't apply to 
the current device type visible, but insensitive. It seems better to only 
show the ones that are applicable to the current type and hide the rest.

+#else
+	/* Unmount Volume */
+
+                if (include_unmount_volume) {
+                        label = g_strdup (_("Unmount Volume"));
+                        nautilus_bonobo_set_label
+                                (desktop_view->details->ui,
+                                 DESKTOP_COMMAND_UNMOUNT_VOLUME_CONDITIONAL,
+                                 label);
+                        nautilus_bonobo_set_sensitive
+                                (desktop_view->details->ui,
+                                 DESKTOP_COMMAND_UNMOUNT_VOLUME_CONDITIONAL, TRUE);
+                        g_free (label);
+        nautilus_bonobo_set_hidden
+                (desktop_view->details->ui,
+                 DESKTOP_COMMAND_PROTECT_VOLUME_CONDITIONAL,
+                 TRUE);
+        nautilus_bonobo_set_hidden
+                (desktop_view->details->ui,
+                 DESKTOP_COMMAND_EJECT_VOLUME_CONDITIONAL,
+                 TRUE);
+        nautilus_bonobo_set_hidden
+                (desktop_view->details->ui,
+                 DESKTOP_COMMAND_FORMAT_VOLUME_CONDITIONAL,
+                 TRUE);
+        nautilus_bonobo_set_hidden
+                (desktop_view->details->ui,
+                 DESKTOP_COMMAND_MEDIA_PROPERTIES_VOLUME_CONDITIONAL,
+                 TRUE);
+        }
+
+#endif
 
 	bonobo_ui_component_thaw (desktop_view->details->ui, NULL);
 }
@@ -1202,6 +1378,10 @@ real_merge_menus (FMDirectoryView *view)
 		BONOBO_UI_VERB ("New Terminal", new_terminal_callback),
 		BONOBO_UI_VERB ("Reset Background", reset_background_callback),
 		BONOBO_UI_VERB ("Unmount Volume Conditional", unmount_volume_callback),
+		BONOBO_UI_VERB ("Eject Conditional", unmount_volume_callback),
Why 
+                BONOBO_UI_VERB ("Protect Conditional", media_callback),
+                BONOBO_UI_VERB ("Format Conditional", media_callback),
+                BONOBO_UI_VERB ("Media Properties Conditional", media_callback),
 		BONOBO_UI_VERB_END


Perhaps we should share the callback for all the media functions, if that 
makes sense.

Why do you have both Unmount Volume and Eject? They seem to be the same.
 
--- nautilus/src/file-manager/nautilus-desktop-icon-view-ui.xml	Sat Mar 24 06:55:39 2001
+++ nautilus_new/src/file-manager/nautilus-desktop-icon-view-ui.xml	Sat Apr  6 10:56:20 2002
@@ -8,6 +8,18 @@
         <cmd name="Empty Trash Conditional"
          _label="Empty Trash"
          _tip="Delete all items in the Trash"/>
+	<cmd name="Eject Conditional"
+         _label="Eject"
+         _tip="Eject Conditionally"/>
+        <cmd name="Format Conditional"
+         _label="Format"
+         _tip="Format Conditionally"/>
+        <cmd name="Protect Conditional"
+         _label="Protect"
+         _tip="Protect Conditionally"/>
+        <cmd name="Media Properties Conditional"
+         _label="Media Properties"
+         _tip="Media Properties Conditionally"/>
         <cmd name="New Terminal"
          _label="New Terminal"
          _tip="Open a new GNOME terminal window"/>
@@ -55,9 +67,13 @@
                 <placeholder name="Empty Trash Holder" delimit="top">
                         <menuitem name="Empty Trash" verb="Empty Trash Conditional"/>
                 </placeholder>
-                <placeholder name="Unmount Volume Holder" delimit="top">
-                        <menuitem name="Empty Trash" verb="Unmount Volume Conditional"/>
+<placeholder name="Removabel Media Holder" delimit="top">
This looks badly indented

+                        <menuitem name="Unmount Volume" _label="Unmount Volume" verb="Unmount Volume Conditional"/>
+                        <menuitem name="Eject Media" _label="Eject" verb="Eject Conditional"/>

Here you have both Eject and Unmount Volume again.

+                        <menuitem name="Protect Media" _label="Protect" verb="Protect Conditional"/>
+                        <menuitem name="Format Media" _label="Format" verb="Format Conditional"/>
+                        <menuitem name="Media Properties Media" _label="Media Properties" verb="Media Properties Conditional"/>

The naming of these, both name, label and verb, seems inconsistant. Why do 
you have Media twice in properties, and why does only properties have 
a "Media" in the label name?

Maybe better names would be:
"Unmount Volume"
"Format"
"Set Media Properties"
"Set Media Protection"

Maybe we should ask the UI team.

=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander Larsson                                            Red Hat, Inc 
                   alexl redhat com    alla lysator liu se 
He's a war-weary devious Green Beret with a mysterious suitcase handcuffed to 
his arm. She's an artistic winged detective looking for love in all the wrong 
places. They fight crime!