Re: [PATCH] Add DnD files onto archiver files with help of file-roller

[Alexander Larsson <alexl redhat com>]

On Sun, 2006-12-03 at 20:44 +0000, Nelson Benítez wrote:
> Alexander Larsson escribió:
> > On Sat, 2006-11-25 at 22:42 +0000, Nelson Benítez wrote:
> >> Alexander Larsson escribió:
> >>> On Sun, 2006-11-19 at 22:53 +0000, Nelson Benítez wrote:
> >>>> Hi, please see http://bugs.gnome.org/377157 for details and also a
> >>>> screencast of the feature. I also attach patch here.
> >>> Interesting. This interfers with implementing similar features using
> >>> chained uris. However, since those never really worked I guess we could
> >>> go this route instead.
> >>>
> >>> nautilus_is_archiver_file() is pretty weird, it looks for e.e.g "zip"
> >>> anywhere in a filename, not ".zip" at the end. Also we should probably
> >>> be using mimetypes, not extensions if possible.
> >> Now I added code so it looks for "zip" at the end of filename so now
> >> even 'photos.zip.backup' filename is not matched.
> >> I don't like very much using mimetypes because if I recall right mime
> >> functions make the code slow . Also strange things could happen like
> >> OpenOffice files are zip and so be offered as drop target, I think these
> >> well known archivers extensions are enough for the job, anyway I don't
> >> mind adding mime functions if you want that...
> > 
> > The way nautilus handles file types in general is through mimetypes. We
> > don't want to have everything use mimetypes except one place, because
> > that would make it act different in some particular case, completely
> > surprising users.
> 
> I've changed nautilus_is_archiver_file() to use mime-types, it tries
> first with the sniffed one, if that is not present it uses the guessed
> one (based on extension). Now it integrates more with nautilus, as it
> can be seen in this screencast[1]. Patch attached.

The check should really take a NautilusFile. There is no reason to pass
it via a URI. Also, its better as nautilus_file_is_archive().
Furthermore, using nautilus_file_get_mime_type() is enough, it will
return the non-sniffed mimetype if we haven't sniffed yet. 

+	file = nautilus_file_get_existing (uri);
+	g_return_val_if_fail (file != NULL, FALSE);
There is no guarantee that file != null here. (I'm not sure why you used
get_existing() at all.)

+	if (strcmp (mime_compare, GNOME_VFS_MIME_TYPE_UNKNOWN) == 0) {
+		g_free (mime_compare);
+		nautilus_file_unref (file);
+		return FALSE;
+	}
No need to special case this. It won't match against the archive
mimetypes.

The check for local files is done to late. It really needs to be done in
nautilus_drag_can_accept_files() so that we don't flag it as a possible
operation and then silently ignore the files on drop. (Even if that
requires some slight changes to nautilus_drag_can_accept_files so that
it accepts the source uri.)

We should also look for the file roller binary at runtime (once) and
only support this operation if it is installed.

There are also some coding style issues in the patch. Like use of c++
comments and missing spaces.

=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander Larsson                                            Red Hat, Inc 
                   alexl redhat com    alla lysator liu se 
He's a fiendish chivalrous cowboy who knows the secret of the alien invasion. 
She's a virginal junkie magician's assistant on the trail of a serial killer. 
They fight crime!