After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 585545 - Split automounting code into a g-s-d plugin
Split automounting code into a g-s-d plugin
Status: RESOLVED FIXED
Product: gnome-settings-daemon
Classification: Core
Component: plugins
2.91.x
Other Linux
: Normal enhancement
: ---
Assigned To: gnome-settings-daemon-maint
gnome-settings-daemon-maint
Depends on:
Blocks: 633288
 
 
Reported: 2009-06-12 11:09 UTC by Ross Burton
Modified: 2010-11-25 14:25 UTC
See Also:
GNOME target: 3.0
GNOME version: ---


Attachments
mount plugin (10.55 KB, application/x-gzip)
2009-06-12 11:12 UTC, Ross Burton
  Details
g-s-d automount plugin (108.48 KB, patch)
2010-11-24 15:36 UTC, Tomas Bzatek
needs-work Details | Review
g-s-d automount plugin (102.86 KB, patch)
2010-11-24 18:02 UTC, Tomas Bzatek
none Details | Review

Description Ross Burton 2009-06-12 11:09:00 UTC
In Moblin 2 we're using gnome-settings-daemon.  We're also shipping nautilus but not starting it on login because we don't have a traditional file manager desktop.  This means that we don't have anything responding to volume insertion events and mounting new media.

The solution we have is a g-s-d plugin which watches for devices and mounts them, and then opens the mount point using gtk_show_uri (which will open a file manager window).  

Would you considering adding this to gnome-settings-daemon?  Obviously the plugin would be disabled by default because in GNOME nautilus handles this, but I can see this being useful for not just us but other people who don't want to use nautilus to manage their desktop.
Comment 1 Ross Burton 2009-06-12 11:12:06 UTC
Created attachment 136412 [details]
mount plugin

This is the mount plugin, as a standalone module.  If you agree that this can be integrated then I'll submit a patch.
Comment 2 Ray Strode [halfline] 2009-06-12 17:51:52 UTC
So the real problem here is that nautilus exits if it has no windows open but also does certain 'under the cover' operations unrelated to its open windows, right?

I guess now that nautilus does these types of daemon-y things it exiting doesn't make as much sense.

Maybe the right fix is to make nautilus stick around even if show_desktop is false?
Comment 3 Ray Strode [halfline] 2009-06-12 18:04:16 UTC
The other side of the coin would be to drop support from nautilus for automounting and do this in gnome-settings-daemon all the time.
Comment 4 David Zeuthen (not reading bugmail) 2009-06-12 18:13:14 UTC
Not completely opposed to moving this from Nautilus to g-s-d but note that Nautilus does a bit more as well including autorun dialogs and x-content/* dialogs. That should be moved as well.
Comment 5 Jens Granseuer 2009-06-12 18:19:16 UTC
I tihnk I'd prefer if the g-s-d module would continue to be maintained with nautilus.
Comment 6 Ross Burton 2009-06-13 08:43:45 UTC
Ray: The real problem is that we don't want to start Nautilus on boot because its heavy and in the ideal world the user will never use it.

The autorun/content dialogs are an interesting case and we haven't considered what to do with those yet.

My personal preference is to move the all of the mount machinery to gsd from nautilus (maybe leaving autorun in nautilus?).  Then if we want a different UI we can patch g-s-d.
Comment 7 Bastien Nocera 2009-08-12 14:34:21 UTC
(In reply to comment #6)
> Ray: The real problem is that we don't want to start Nautilus on boot because
> its heavy and in the ideal world the user will never use it.
> 
> The autorun/content dialogs are an interesting case and we haven't considered
> what to do with those yet.
> 
> My personal preference is to move the all of the mount machinery to gsd from
> nautilus (maybe leaving autorun in nautilus?).  Then if we want a different UI
> we can patch g-s-d.

But the code should live in nautilus, even if it is as a gnome-settings-daemon plugin
Comment 8 David Zeuthen (not reading bugmail) 2009-08-12 14:38:49 UTC
(In reply to comment #7)
> But the code should live in nautilus, even if it is as a gnome-settings-daemon
> plugin

Agreed. Downstreams can then just package this up as a nautilus-automount subpackage. 

Do note that this component (Nautilus) will does more than automounting - it also does auto-unlocking (for LUKS volumes) and, in the future, it will also do auto-assembly of RAID / LVM2 / btrfs etc. So maybe the plug-in, and subpackage, should be called something like nautilus-storage-policy-manager.

Comment 9 William Jon McCann 2010-04-02 14:23:18 UTC
Any update on this?  I think this is something we want for GNOME 3.  Ross' plan sounds good to me.
Comment 10 Alexander Larsson 2010-04-27 09:31:30 UTC
The plan is to move all the nautilus code to g-s-d, but I don't really have time for this right now unfortunately...
Comment 11 Ross Burton 2010-04-27 09:43:01 UTC
We (MeeGo) ended up maintaining our own automounter plugin because we wanted a different user experience.  Our plugin mounts and then pops up a notification offering to bring down one of the panels, no more.
Comment 12 Tomas Bzatek 2010-11-15 14:32:30 UTC
I've published 'automounter' branch of gnome-settings-daemon. The code there is mostly copy&paste of Nautilus parts, glued together in a g-s-d plugin. There's still a lot of work to do and all this should be considered as a functional preview.

Simple list of my plans:
 - code cleanup, some parts are not needed
 - fix all FIXME comments
 - refactor dialogs (incl. GtkMountOperation) to be able to bind either Gtk or gnome-shell interface
Comment 13 Tomas Bzatek 2010-11-24 15:36:59 UTC
Created attachment 175172 [details] [review]
g-s-d automount plugin

Posting squashed patch, please review.

Since the control-center 'media-panel' branch shares the original Nautilus code, I've incorporated feedback from bug 634917. And when NautilusOpenWithDialog goes to Gtk+ (bug 582557), I will update the sources, nuking nautilus-open-with-dialog.c completely.

We still haven't agreed how integration with gnome-shell should look like but we can fix that later.
Comment 14 Bastien Nocera 2010-11-24 16:53:27 UTC
Review of attachment 175172 [details] [review]:

Please trim the commit log, there's no need for us to see the full work on the branch.

Other than the few comments, looks good to go.

::: plugins/automount/automount.gnome-settings-plugin.in
@@ +5,3 @@
+_Description=Automounter plugin
+Authors=
+Copyright=Copyright © 2010

Missing something here?

::: plugins/automount/gsd-automount-manager.c
@@ +133,3 @@
+
+static void
+nautilus_file_operations_mount_volume (GtkWindow *parent_window,

No nautilus namespace in g-s-d code (apart from the copy/paste code).

@@ +207,3 @@
+{
+	manager->priv->volume_monitor = g_volume_monitor_get ();
+	g_signal_connect_object (manager->priv->volume_monitor, "mount_added",

"mount-added" instead? I know both work, but "mount-added" is what is documented.
Comment 15 Cosimo Cecchi 2010-11-24 17:13:45 UTC
Review of attachment 175172 [details] [review]:

Some more mostly small comments.

::: plugins/automount/gsd-automount-manager.c
@@ +89,3 @@
+		}
+		g_list_foreach (volumes, (GFunc) g_object_unref, NULL);
+		g_list_free (volumes);

You can use g_list_free_full(volumes, g_object_unref) here.

@@ +260,3 @@
+                                     guint           prop_id,
+                                     const GValue   *value,
+                                     GParamSpec     *pspec)

Aren't this and the following functions redundant? (get/set_property,  constructor, dispose and finalize)

::: plugins/automount/gsd-automount-plugin.c
@@ +58,3 @@
+        plugin = GSD_AUTOMOUNT_PLUGIN (object);
+
+        g_return_if_fail (plugin->priv != NULL);

I don't think this (and also |object|) can ever be NULL
Comment 16 Tomas Bzatek 2010-11-24 18:02:09 UTC
Created attachment 175191 [details] [review]
g-s-d automount plugin

Thanks for the review.

(In reply to comment #14)
> Please trim the commit log, there's no need for us to see the full work on the
> branch.

That was just a quick output from git. I meant this squashed patch just for review purposes, my work is continuously pushed in the automounter g-s-d branch.

> ::: plugins/automount/automount.gnome-settings-plugin.in
> @@ +5,3 @@
> +_Description=Automounter plugin
> +Authors=
> +Copyright=Copyright © 2010
> 
> Missing something here?
Just like background plugin is :-) Added.

> ::: plugins/automount/gsd-automount-manager.c
> @@ +133,3 @@
> +
> +static void
> +nautilus_file_operations_mount_volume (GtkWindow *parent_window,
> 
> No nautilus namespace in g-s-d code (apart from the copy/paste code).
Right, changed this one. I left nautilus-autorun.c and nautilus-open-with-dialog.c (soon-to-be-removed) unchanged since it's 99% original copy from Nautilus tree.

> @@ +207,3 @@
> +{
> +    manager->priv->volume_monitor = g_volume_monitor_get ();
> +    g_signal_connect_object (manager->priv->volume_monitor, "mount_added",
> 
> "mount-added" instead? I know both work, but "mount-added" is what is
> documented.
Fixed.


(In reply to comment #15)
> ::: plugins/automount/gsd-automount-manager.c
> @@ +89,3 @@
> +        }
> +        g_list_foreach (volumes, (GFunc) g_object_unref, NULL);
> +        g_list_free (volumes);
> 
> You can use g_list_free_full(volumes, g_object_unref) here.
Thanks, didn't know of this existence. Changed.

 
> @@ +260,3 @@
> +                                     guint           prop_id,
> +                                     const GValue   *value,
> +                                     GParamSpec     *pspec)
> 
> Aren't this and the following functions redundant? (get/set_property, 
> constructor, dispose and finalize)
> 
> ::: plugins/automount/gsd-automount-plugin.c
> @@ +58,3 @@
> +        plugin = GSD_AUTOMOUNT_PLUGIN (object);
> +
> +        g_return_if_fail (plugin->priv != NULL);
> 
> I don't think this (and also |object|) can ever be NULL
Right, copy&paste garbage. Removed.
Comment 17 Matthias Clasen 2010-11-24 20:22:15 UTC
Review of attachment 175172 [details] [review]:

::: plugins/automount/gsd-automount-manager.c
@@ +261,3 @@
+                                     const GValue   *value,
+                                     GParamSpec     *pspec)
+{

You need finalize, but the rest can go.
Comment 18 Cosimo Cecchi 2010-11-25 10:22:37 UTC
Review of attachment 175191 [details] [review]:

Looks good to me, except for this little typo.

::: plugins/automount/gsd-automount-plugin.h
@@ +31,3 @@
+G_BEGIN_DECLS
+
+#define GSD_TYPE_GSD_AUTOMOUNT_PLUGIN            (gsd_automount_plugin_get_type ())

This should probably be #define GSD_TYPE_AUTOMOUNT_PLUGIN (gst_automount_plugin_get_type())
Comment 19 Tomas Bzatek 2010-11-25 10:49:21 UTC
(In reply to comment #17)
> ::: plugins/automount/gsd-automount-manager.c
> @@ +261,3 @@
> +                                     const GValue   *value,
> +                                     GParamSpec     *pspec)
> +{
> 
> You need finalize, but the rest can go.

Do we really need finalize? All memory deallocation is done in gsd_automount_manager_stop() which is being called from GsdAutomountPluginClass, together with manager object unref.

(In reply to comment #18)
> Looks good to me, except for this little typo.
> 
> ::: plugins/automount/gsd-automount-plugin.h
> @@ +31,3 @@
> +G_BEGIN_DECLS
> +
> +#define GSD_TYPE_GSD_AUTOMOUNT_PLUGIN           
> (gsd_automount_plugin_get_type ())
> 
> This should probably be #define GSD_TYPE_AUTOMOUNT_PLUGIN
> (gst_automount_plugin_get_type())

Missed this, fixed.

Changes pushed to the automounter branch: http://git.gnome.org/browse/gnome-settings-daemon/log/?h=automounter
Comment 20 Cosimo Cecchi 2010-11-25 12:57:43 UTC
(In reply to comment #19)

> Changes pushed to the automounter branch:
> http://git.gnome.org/browse/gnome-settings-daemon/log/?h=automounter

Looks good to me.
Comment 21 Tomas Bzatek 2010-11-25 14:25:20 UTC
Merged with master. Thanks for your reviews.