Re: [Utopia] Re: [patch] improved hal support for gnome-vfs

[David Zeuthen <david fubar dk>]

Hi,

On Fri, 2005-07-01 at 23:01 -0400, David Zeuthen wrote: 

(I shouldn't be writing patches that late in the night especially just
before going on vacation :-)

> > This doesn't work. First of all, drives and volumes, once initialized
> > are generally read-only (except the volume-drive relationships). This is
> > partly a threading issue, as all thread protection on these datatypes is
> > based on them being read-only. It also lacks any code to push such
> > changes from the daemon to the clients, so nobody will actually see this
> > change.
> 
> Right, sorry, I should have deleted this code. Now we just delete the
> drive if it was already there and then add a new one.

OK, this did make gnome-vfs-daemon crash especially after the reprobe()
that happens after e.g. mount. Since any code shouldn't be relying on
the activation_uri of the GnomeVFSDrive (instead, use the activation_uri
of the associated volume) we just reuse the GnomeVFSDrive object without
updating the activation_uri at all. 

I also forgot to query for vendor provided icons.

Both things are fixed in the attached patch. May I commit this?

Thanks,
David

? compile
? gnome-vfs-zip
? imported/fnmatch/Makefile
? imported/fnmatch/Makefile.in
Index: ChangeLog
===================================================================
RCS file: /cvs/gnome/gnome-vfs/ChangeLog,v
retrieving revision 1.2214
diff -u -p -r1.2214 ChangeLog
--- ChangeLog	5 Jul 2005 07:33:04 -0000	1.2214
+++ ChangeLog	7 Jul 2005 19:13:00 -0000
@@ -1,3 +1,11 @@
+2005-07-07  David Zeuthen  <davidz redhat com>
+
+	* libgnomevfs/gnome-vfs-hal-mounts.c (_hal_drive_policy_get_icon):
+	Support storage.icon.drive property for icon supplied by vendor
+	(_hal_volume_policy_get_icon): -do- for storage.icon.volume
+	(_hal_add_volume): Reuse GnomeVFSDrive object if it already
+	exists
+
 2005-07-05  Christian Kellner  <gicmo gnome org>
 
 	Second part of the even-more-rocking-hal-support by
Index: libgnomevfs/gnome-vfs-hal-mounts.c
===================================================================
RCS file: /cvs/gnome/gnome-vfs/libgnomevfs/gnome-vfs-hal-mounts.c,v
retrieving revision 1.13
diff -u -p -r1.13 gnome-vfs-hal-mounts.c
--- libgnomevfs/gnome-vfs-hal-mounts.c	5 Jul 2005 07:33:04 -0000	1.13
+++ libgnomevfs/gnome-vfs-hal-mounts.c	7 Jul 2005 19:13:00 -0000
@@ -225,6 +225,10 @@ _hal_drive_policy_get_icon (GnomeVFSVolu
 	LibHalDriveBus bus;
 	LibHalDriveType drive_type;
 
+	name = libhal_drive_get_dedicated_icon_drive (hal_drive);
+	if (name != NULL)
+		goto out;
+
 	bus        = libhal_drive_get_bus (hal_drive);
 	drive_type = libhal_drive_get_type (hal_drive);
 
@@ -254,6 +258,7 @@ _hal_drive_policy_get_icon (GnomeVFSVolu
 		name = _hal_lookup_icon (0x10000 + drive_type*0x100);
 	}
 
+out:
 	if (name != NULL)
 		return g_strdup (name);
 	else {
@@ -271,6 +276,10 @@ _hal_volume_policy_get_icon (GnomeVFSVol
 	LibHalDriveType drive_type;
 	LibHalVolumeDiscType disc_type;
 
+	name = libhal_drive_get_dedicated_icon_volume (hal_drive);
+	if (name != NULL)
+		goto out;
+
 	/* by design, the enums are laid out so we can do easy computations */
 
 	if (libhal_volume_is_disc (hal_volume)) {
@@ -871,47 +880,45 @@ _hal_add_volume (GnomeVFSVolumeMonitorDa
 		_gnome_vfs_volume_monitor_disconnected (volume_monitor, drive);
 	}
 
-	/* if we had a drive from here but where we weren't mounted, just use that drive and update
-	 * activation_uri as we now have a definite mount point...
+	/* if we had a drive from here but where we weren't mounted, just use that drive since nothing actually
+	 * changed 
 	 */
 	drive = _gnome_vfs_volume_monitor_find_drive_by_hal_udi (volume_monitor, libhal_volume_get_udi (hal_volume));
-	if (drive != NULL) {
-		_gnome_vfs_volume_monitor_disconnected (volume_monitor, drive);
-	}
-
-	drive = g_object_new (GNOME_VFS_TYPE_DRIVE, NULL);
-	if (libhal_volume_disc_has_audio (hal_volume)) {
-		drive->priv->activation_uri = g_strdup_printf ("cdda://%s", 
-							       libhal_volume_get_device_file (hal_volume));
-	} else if (libhal_volume_disc_is_blank (hal_volume)) {
-		drive->priv->activation_uri = g_strdup ("burn:///");
-	} else if (libhal_volume_is_mounted (hal_volume)) {
-		drive->priv->activation_uri = gnome_vfs_get_uri_from_local_path (
-			libhal_volume_get_mount_point (hal_volume));
-	} else {
-		/* This sucks but it doesn't make sense to talk about the activation_uri if we're not mounted!
-		 * So just set it to the empty string
-		 */
-		drive->priv->activation_uri = g_strdup ("");
-	}
-	drive->priv->is_connected = TRUE;
-	drive->priv->device_path = g_strdup (libhal_volume_get_device_file (hal_volume));
-	drive->priv->device_type = _hal_get_gnome_vfs_device_type (hal_drive);
+	if (drive == NULL) {
+		drive = g_object_new (GNOME_VFS_TYPE_DRIVE, NULL);
+		if (libhal_volume_disc_has_audio (hal_volume)) {
+			drive->priv->activation_uri = g_strdup_printf ("cdda://%s", 
+								       libhal_volume_get_device_file (hal_volume));
+		} else if (libhal_volume_disc_is_blank (hal_volume)) {
+			drive->priv->activation_uri = g_strdup ("burn:///");
+		} else if (libhal_volume_is_mounted (hal_volume)) {
+			drive->priv->activation_uri = gnome_vfs_get_uri_from_local_path (
+				libhal_volume_get_mount_point (hal_volume));
+		} else {
+			/* This sucks but it doesn't make sense to talk about the activation_uri if we're not mounted!
+			 * So just set it to the empty string
+			 */
+			drive->priv->activation_uri = g_strdup ("");
+		}
+		drive->priv->is_connected = TRUE;
+		drive->priv->device_path = g_strdup (libhal_volume_get_device_file (hal_volume));
+		drive->priv->device_type = _hal_get_gnome_vfs_device_type (hal_drive);
 	
-	/* TODO: could add an icon of a drive with media in it since this codepath only
-	 * handles drives with media in them
-	 */
-	drive->priv->icon = _hal_drive_policy_get_icon (volume_monitor_daemon, hal_drive, NULL);
-	name = _hal_drive_policy_get_display_name (volume_monitor_daemon, hal_drive, hal_volume);
-	drive->priv->display_name = _gnome_vfs_volume_monitor_uniquify_drive_name (volume_monitor, name);
-	g_free (name);
-	drive->priv->is_user_visible = TRUE;
-	drive->priv->volumes = NULL;
-	drive->priv->hal_udi = g_strdup (libhal_volume_get_udi (hal_volume));
-	drive->priv->hal_drive_udi = g_strdup (libhal_drive_get_udi (hal_drive));
+		/* TODO: could add an icon of a drive with media in it since this codepath only
+		 * handles drives with media in them
+		 */
+		drive->priv->icon = _hal_drive_policy_get_icon (volume_monitor_daemon, hal_drive, NULL);
+		name = _hal_drive_policy_get_display_name (volume_monitor_daemon, hal_drive, hal_volume);
+		drive->priv->display_name = _gnome_vfs_volume_monitor_uniquify_drive_name (volume_monitor, name);
+		g_free (name);
+		drive->priv->is_user_visible = TRUE;
+		drive->priv->volumes = NULL;
+		drive->priv->hal_udi = g_strdup (libhal_volume_get_udi (hal_volume));
+		drive->priv->hal_drive_udi = g_strdup (libhal_drive_get_udi (hal_drive));
 	
-	_gnome_vfs_volume_monitor_connected (volume_monitor, drive);
-	gnome_vfs_drive_unref (drive);
+		_gnome_vfs_volume_monitor_connected (volume_monitor, drive);
+		gnome_vfs_drive_unref (drive);
+	}
 
 	vol = _gnome_vfs_volume_monitor_find_volume_by_hal_udi (volume_monitor, libhal_volume_get_udi (hal_volume));
 	if (vol == NULL &&