Re: gio, gvfs and nautilus fixes

[David Zeuthen <david fubar dk>]

Hey Alex,

(Hi i18n team: there's a string break request near the end of this
mail)

Here are some updated patches

http://people.freedesktop.org/~david/nautilus-fixes-2.patch
http://people.freedesktop.org/~david/gvfs-fixes-2.patch
http://people.freedesktop.org/~david/gio-fixes-2.patch

New fixes that weren't in the earlier patch set

 - Only consider mounts under /media and $HOME to be user visible. This
   makes it a lot easier for users to understand what they have to do
   if they want to hide or show a mount point. And users have been very
   loud about this issue. Also revert the commit where we hide NFS
   mounts; if the user don't want NFS mounts to be shown they just need
   to mount them outside /media. Or if they want it shown they need to
   mount it inside /media. (It would be weird to mount NFS stuff
   in /media anyway)

 - For some reason we didn't do anything if the user hit Eject or
   Unmount in the autorun dialog. Beats me. I remember testing this.
   Anyway, it was 7-8 lines of code so here goes.

On Thu, 2008-02-21 at 14:27 +0100, Alexander Larsson wrote:
> On Thu, 2008-02-21 at 02:46 -0500, David Zeuthen wrote:
> > Hey,
> > 
> > Here's a couple of patches for gio, gvfs and Nautilus. The description
> > is in the ChangeLog for each module. Some comments
> > 
> >  - I'm unsure my fix for nautilus_file_get_mount() is correct and I'm
> >    not sure what it's supposed to do. My assumption is that it will
> >    return a GMount for every file on a GMount. The older version only
> >    did this for the root folder. My fix is provably not correct; we
> >    may not have created NautilusFile objects to get to the root. One way
> >    of testing this is to have some removable media with a DCIM/
> >    directory in the root; for every folder you go into you should see
> >    a cluebar. Without this fix you will only see it for the root folder
> >    of the mount.
> 
> This is due to the check in lacks_mount:
> 
> static gboolean
> lacks_mount (NautilusFile *file)
> {
> 	return (!file->details->mount_is_up_to_date &&
> 		(
> 		 /* Unix mountpoint, could be a GMount */
> 		 file->details->is_mountpoint ||
> 		 
> 
> Without this we would be calling get_enclosing_mount() and doing the
> related stat-walk and mtab reading for each file nautilus displays, as
> we get the mount attribute for all files displayed (so we get the
> unmount/eject etc context menu items). This is gonna be pretty slow, and
> furthermore it sort of changes what nautilus_file_get_mount means. Atm
> it means "this location represents a mount", either by being a
> mountpoint or by being something like a desktop icon for a mount, a
> smb://server/share file or a file in computer:, but with something like
> what you propose it means "is on this mount".
> 
> For instance, it would put an eject menu item on all files on a cd,
> which i'm not sure is a good idea.

Right. So now we're just using g_file_find_enclosing_mount_async() in
the code where we need to determine whether a x-content cluebar should
be added. That's a lot simpler I think.

> >  - Autorunning was broken; this patch unbreaks it by moving the call
> >    to inhibit into only selected call sites. One thing we can't fix
> >    is when you mount from computer:// because it runs in another
> >    process. Ideas on how to fix this?
> 
> None atm, but we need to figure out some way to do global autorun
> inhibiting, because atm nautilus kicks in when you mount something from
> the panel (or other app) too.

I really don't know how to do that in a clean way... I don't know. It's
both nice and annoying that volume monitoring now is in-process. 

Frankly I'm not sure how big of a deal this is; ideally users will never
ever manually have to mount things - that's why we have automounting.
But, sure, some users will surely complain about this for 2.22.

So I propose we fix this later. Sounds OK?

> >  - g_file_find_enclosing_mount() for GDaemonFile was broken. We just
> >    cannot construct a new GMount on the fly as it won't have the
> >    the reference to a foreign GVolume that adopted it. My attempt
> >    to fix this involves a global variable; it's probably safe as
> >    GDaemonVolumeMonitor is a singleton anyway. It's not pretty though.
> >    But it works.
> 
> This is totally unthreadsafe, and called from the i/o thread in the
> async_find_enclosing_mount case...

OK, I've added some locking.

> >  - Before the feature freeze I commited half of the gphoto2 support
> >    namely the bits in the volume monitor to create, maintain and
> >    destroy GVolume for connected cameras. This patch is the second
> >    half, the actual filesystem code. It's lacking write support
> >    at the moment but is otherwise in pretty good shape (barring
> >    crashes in libgphoto2; it doesn't like my Sandisk Sansa MTP
> >    device that much).
> > 
> >    So this part of the patch set falls into a grey area wrt feature
> >    freeze; technically it's just a bug fix (shipping gvfs without
> >    it will appear broken as you can see the GVolume objects for
> >    the cameras.. but you have no filesystem code for it!) but I guess
> >    asking the release team whether it's OK to commit may be in order.
> >    Shrug.
> 
> In fact, a lot of changes in the gvfs patch adds strings, and we're in
> string freeze period right now, so you have to request permission on
> gnome-i18n to commit most of it.

i18n team: This is in the patch that adds new strings

http://people.freedesktop.org/~david/gvfs-fixes-2.patch

Most of the strings are in the gphoto2 backend. Almost all of them
(except one or two) are not user-visible under normal circumstances as
they are mostly error messages. Requesting permission to commit this.
Thanks.

> I commited the gio code and the nautilus code (sans the get_mount
> change). For the gvfs code you need to request string break permissions
> and handle the threadsafety issue.

Thanks.

    David