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

["Rajeev Karale" <rajeev karale wipro com>]

Alex Larsson wrote :

> 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".

I will try to use some other method to serve the purpose.

>  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.

done

> +        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.

we will be doing all these activity only when volume is mounted.So I feel
that mount_path check is not needed here.

>
> +                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.

done

>
> +                        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().

Since media_callback itself is called within #ifdefs I don't think we need a
#ifdefs here.
I will do fork and do check popen return value.

>
> +                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

done

>
>
> +/*
> + *  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()

done

>
> +{
> +        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.

done

>
> +        volume = nautilus_volume_monitor_get_volume_for_path
(nautilus_volume_monitor_get (),mount_path);
>
> Missing a space after the comma.

done

>
> +        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

done

>
> +}
> +#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.

I think this flag will just check whether volume is removable or not. But
since we are using it to decide commands associated with icon, we can change
it to include_media_commands

>
>
> - 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.
>

I will check this.

>
> - 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.

I am looking into it.

>
> Why do you always hide unmount on solaris?

In solaris unmount volume is unmounting volume but not ejecting.

> +        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.

done

>
> +                        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.

that make sense. I will take care.

>   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.

looking into it.

> 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?

I have removed extra "Media" in menuitem name.
There is already  a menu item "show properties" added default with icon. So
to avoid confusion, Properties was prefixed with Media to convey that,
properties item is media specific.

> "Unmount Volume"
> "Format"
> "Set Media Properties"

thanx for comments,
_Rajeev.

**************************Disclaimer************************************
      


Information contained in this E-MAIL being proprietary to Wipro Limited
is 'privileged' and 'confidential' and intended for use only by the
individual or entity to which it is addressed. You are notified that any
use, copying or dissemination of the information contained in the E-MAIL
in any manner whatsoever is strictly prohibited.



 ********************************************************************