Re: patch-ahoy: gnome_url_show stuff

[Alexander Larsson <alexl redhat com>]

On Wed, 12 Mar 2003, Frank Worsley wrote:

> Original message:
> 
> > Here goes round two with a much improved set of patches. This is pretty
> > much how I would like to commit this if people are ok with that. Patches
> > for the panel and (time permitting) the run dialog are to follow. There
> > are also new component_viewer and terminal schemas that go into
> > gnome-vfs which I have not attached.
> > 
> > Changes:
> > 
> > - fix a bunch of bugs and crashers in the previous patch
> > - introduce new GnomeVFSResult error codes
> > - move all the url showing stuff into gnome-vfs including the url
> > handler business
> > - gnome_url_show reduced to wrapping gnome_vfs_url_show. it will fill in
> > a GError with nice error messages for the different gnome-vfs error
> > codes.
> > - make nautilus work properly with this stuff
> > - in the process fix #42391 in nautilus
> > - two new eel timed wait functions: suspend/resume
> > 
> > So, this is looking pretty good. It would be nice if I could commit
> > these different parts. Some code review would be good.
> > 
> > I am just writing this email by double-clicking on a launcher with
> > "mailto:desktop-devel-list gnome org" as the target. Pretty sweet.
> > Couldn't do that before.

Ok. I had a quick look at this.

In general I'm uncertain of the usefulness of the component stuff. If 
you're launching a uri from something that doesn't support component 
embedding itself I think you almost always prefer an application to open 
over a component in a generic shell (do we even have a usable shell for 
this?). I guess if a component is all you have you'd prefer to use that 
over nothing though.

Also, I'm not sure how this will interact with the work on the mime system 
jrb is working on. We need to make sure to not introduce any new stuff 
that uses things which may become deprecated soon.

The gnome-vfs changes are API additions, and the addition of this means we 
have to branch gnome vfs for 2.2 now. I guess thats ok, since we need to 
get on with 2.4 gnome-vfs work. However, nautilus will not yet branch for 
2.4 (we want to do more stablilization and bugfixing work), so the 
nautilus part can't go in yet.

Some comments on the patches:

gnomevfs.patch.1:
-----------------

In expand_parameters():

+		if (!app->can_open_multiple_files && uri_list != NULL) {
+			break;
+		}

That needs to be !can_open_multiple_files && uris != NULL

And in general I think it might be nicer if the command line generation 
stuff worked on argvs instead of commandlines that are escaped and then 
parsed+unescaped at the end. The handling of %s and %c is also a bit 
strange. Typically when you introduce something like that you also allow 
escapes if you need the escape sequences in the string.

+	g_warning ("viewer command is: %s", command);

Leftover spew.

+gnome_vfs_use_handler_for_scheme (const char *scheme)
+gnome_vfs_url_show_using_handler (const char *url)
+gnome_vfs_url_show_using_handler_with_env (const char *url,
+			                   char      **envp)

These should probably be internal functions. I don't think they are much 
use in the public API.

+	desktop_default_applications.schemas

This is not on the site.

+gnome_vfs_uri_new_with_unsupported (const gchar *text_uri)

This is some pretty strange API, and a bit badly named to (*with* 
unsupported?). Given the usage of it I think a function to extract the 
schema from a text-uri would be better.

libgnome.patch.1:
-----------------

+gnome_url_get_from_input()

The way this function assumes any ':' in the filename means its an 
uri makes it fail on anything with a colon. We should be able to do much 
better than that. And unless i misread the code absolute filenames are 
broken. 

 typedef enum {
-  GNOME_URL_ERROR_PARSE
+  GNOME_URL_ERROR_URL,
+  GNOME_URL_ERROR_PARSE,
+  GNOME_URL_ERROR_LAUNCH,
+  GNOME_URL_ERROR_NO_DEFAULT,
+  GNOME_URL_ERROR_NOT_SUPPORTED,
+  GNOME_URL_ERROR_VFS
 } GnomeURLError;

I'm not sure about the API guarantees we have for libgnome. Is extending 
enums ok? Anyway, the value for GNOME_URL_ERROR_PARSE which breaks the 
ABI. At least thats not ok.

Why are you removing all the schemas stuff? Is that in the missing 
desktop_default_applications.schemas file?

eel.patch.1:
------------

I don't think these changes are necessary. The way you use them in 
nautilus seems wrong to me. Why would some part of the wait not count 
towards opening the cancel dialog?

nautilus.patch.1:
-----------------

+void nautilus_launch_show_file (NautilusFile *file,
+                                GtkWindow *parent_window)
+void nautilus_launch_action (GnomeVFSMimeAction *action,
+                             NautilusFile *file,
+			     GtkWindow *parent_window)

Some indentation issues like these. The return type go on the line before 
the functionname.

+
+		} else if (launch_action == GNOME_VFS_MIME_ACTION_TYPE_COMPONENT &&
+		           !use_external_viewer_auto_value) {   

Why would you ever want to not view the component in nautilus? I thought 
the main idea for having components in the mime data was that then a 
component-supporting browser could show these inline?

I have to admit i slacked off here and didn't review the nautilus parts 
that well...

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander Larsson                                            Red Hat, Inc 
                   alexl redhat com    alla lysator liu se 
He's a notorious ninja paranormal investigator with a mysterious suitcase 
handcuffed to his arm. She's a plucky thirtysomething queen of the dead on the 
trail of a serial killer. They fight crime!