Re: [RFC] gvfs connected servers

[Alexander Larsson <alexl redhat com>]

On Tue, 2008-10-07 at 00:08 -0400, David Zeuthen wrote:
> On Mon, 2008-10-06 at 21:37 +0200, Alexander Larsson wrote:

> > > I'm actually pretty close to finishing a patchset for shadow mounts [1],
> > > e.g. the in-process proxy volume monitor will automagically create and
> > > remove GProxyShadowMount's for volumes with an activation_uri according
> > > to if there's a real mount for the activation_uri. E.g. we'll get
> > 
> > Sounds good.
> 
> I've attached the patches to these bugs
> 
>  http://bugzilla.gnome.org/show_bug.cgi?id=555331 (gio)
>  http://bugzilla.gnome.org/show_bug.cgi?id=555332 (gvfs)
>  http://bugzilla.gnome.org/show_bug.cgi?id=555333 (nautilus)
>  http://bugzilla.gnome.org/show_bug.cgi?id=555334 (gtk+)
> 
> These patches should be fine to commit as is; that is, I've tidied them
> up, tested for locking problems and looked over them a few times. Works
> fine on my box.
> 
> I think the UI rework in Nautilus can be done as a separate patch. 


gio patch:
==========

Looks ok

gvfs patch:
===========

This is yet another separate daemon There is lots of daemons now. Maybe
we can combine some of the daemons that are basically always running?
I'm not sure what the overhead is atm, maybe we should measure it first.

 if (volume->volume_monitor != NULL)
-    g_object_unref (volume->volume_monitor);
+    {
+      g_signal_handlers_disconnect_by_func (volume->union_monitor, union_monitor_mount_added, volume);
+      g_signal_handlers_disconnect_by_func (volume->union_monitor, union_monitor_mount_removed, volume);
+      g_signal_handlers_disconnect_by_func (volume->union_monitor, union_monitor_mount_changed, volume);
+      g_object_unref (volume->volume_monitor);
+    }
 
+  if (volume->union_monitor != NULL)
+    g_object_unref (volume->union_monitor);

These two seems to mix up volume_monitor and union_monitor.

+#define PATH_GCONF_CONNECTED_SERVERS
"/desktop/gnome/gvfs-connected-servers"

Why not use the old "connected_servers" configs?

+static GIcon *
+g_proxy_shadow_mount_get_icon (GMount *mount)
+{
+  GProxyShadowMount *proxy_shadow_mount = G_PROXY_SHADOW_MOUNT (mount);
+  GIcon *icon;
+
+  G_LOCK (proxy_shadow_mount);
+  icon = g_volume_get_icon (G_VOLUME (proxy_shadow_mount->volume));
+  G_UNLOCK (proxy_shadow_mount);
+  return icon;
+}

Don't we want the volume and mount to be able to have different icons?
So you can see its been mounted?

Also, we better make sure we never create a shadow mount of a shadow
mount, or we will deadlock here. Maybe we should g_assert that
somewhere?
Or maybe it should ref in a lock and then call the function outside the
lock.

Nautilus patch:
===============

I'm not sure about the behaviour of the desktop icons here. We're on
purpose not showing volumes on the desktop but only mounts as desktop
space is rather tight, and we show the volumes in several other places
already to make it easy to mount them (side bar, computer:, panel places
menu). What makes the connected server volumes different from the other
unmounted volumes in the system?

I'd rather have a (default off) option to show all volumes on the
desktop. (The desktop is after all a very in-your-face thing, and
allowing a bunch of config options to tweak your use of it doesn't seem 
too excessive.)

 	/* add mounts that has no volume (/etc/mtab mounts, ftp, sftp,...) */
 	mounts = g_volume_monitor_get_mounts (volume_monitor);
 	for (l = mounts; l != NULL; l = l->next) {
 		mount = l->data;
+		if (mount_referenced_by_volume_activation_root (volumes, mount))
+			continue;

This leaks mount (needs to unref it)

+	if (mount != NULL)
+		g_object_unref (mount);

Got a bunch of missing brackets in places like this and the continue
call above.

Gtk Patch:
==========

Looks ok.