Re: A patch for uniform mime handling

[Alexander Larsson <alexl redhat com>]

On Wed, 2003-09-10 at 06:54, Bryan Forbes wrote:
> Hello,
>     I am emailing this list to propose a patch to gnome_vfs_get_mime_type to return
> GNOME_VFS_MIME_TYPE_UNKNOWN instead of NULL as it does now.  The patch on bug 121553
> (http://bugzilla.gnome.org/show_bug.cgi?id=121553) fixes this issue and also handles it
> the way gnome_vfs_get_mime_type_common, gnome_vfs_get_file_mime_type,
> gnome_vfs_get_mime_type_for_data, and gnome_vfs_get_mime_type_from_file_data (by using
> _gnome_vfs_get_mime_type_internal instead of returning NULL).  Not only does it make the
> function more uniform to the other 4 functions, but it also fixes a crash on Solaris when
> the function returns NULL (because you can't do printf("%s\n", NULL);).
>     You may be saying to yourself, "This breaks API", but I would say to you that all
> apps that use this function have to handle this anyway, and most set the mime-type to
> GNOME_VFS_MIME_TYPE_UNKNOWN themselves. For example:

I don't like the patch as it stands. The call is documented to do the
same thing as gnome_vfs_get_file_info(), and it is my opinion that if
you pass GNOME_VFS_FILE_INFO_GET_MIME_TYPE to get_file_info the module
should *always* put something in the mime field, even if it is only
GNOME_VFS_MIME_TYPE_UNKNOWN. If some module is not doing this we should
fix that. The file method internally calls
gnome_vfs_get_file_mime_type() which ends up calling 
_gnome_vfs_get_mime_type_internal() already, so its ok.

I do agree that we should probably pass GNOME_VFS_MIME_TYPE_UNKNOWN if
the get_file_info() fails though. But we shouldn't be trying to guess
the type of a non-existing file, that will just confuse the caller,
since i.e. launching an app to handle the file or trying to load it will
not work.

> At line 97 of src/egg-recent-item.c in totem cvs HEAD:
> 	item->mime_type = gnome_vfs_get_mime_type (item->uri);
> 
> 	if (!item->mime_type)
> 		item->mime_type = g_strdup (GNOME_VFS_MIME_TYPE_UNKNOWN);
> 
> As you can see, this code just works around the NULL return and wouldn't break with the
> new patch. EOG, File-Roller, gedit, gpdf, nautilus

This would be fixed by passing GNOME_VFS_MIME_TYPE_UNKNOWN if the
get_file_info fails.

> Another example is at line 1027 of lib/rb-playlist.c in rhythmbox cvs HEAD:
> 	mimetype = gnome_vfs_get_mime_type (url);
> 
> 	if (mimetype == NULL)
> 		return rb_playlist_add_url_from_data (playlist, url);
> 
> rb_playlist_add_url_from_data calls a hack to open up the file and read the mime-type
> using gnome_vfs_get_mime_type_for_data (which looks up mime-types the same way the patch
> will make gnome_vfs_get_mime_type look them up).  Totem uses a similar hack in
> src/totem-playlist.c at line 1935.

I don't understand why they did this. If mimetype returns NULL its
probably because opening the file failed (e.g it didn't exist), so will
the open + gnome_vfs_get_mime_type_for_data work? If it does something
sounds broken about gnome_vfs_get_file_info().


=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander Larsson                                            Red Hat, Inc 
                   alexl redhat com    alla lysator liu se 
He's a suicidal crooked rock star haunted by an iconic dead American 
confidante She's a tortured mute socialite with the power to bend men's minds. 
They fight crime!