Created attachment 288754 [details] [review]
gvfs-open: Fix incorrect use of g_app_info_launch_default_for_uri()

First, as we never check whether the passed arguments are indeed URIs,
we'd be failing to work correctly for relative paths.
Secondly, we would be running g_app_info_launch_default_for_uri() twice
for no good reason.
Finally, we were leaking the output of g_file_get_uri()

Review of attachment 288754 [details] [review]:

It introduces a warning:
make[2]: Entering directory '/home/ross/src/gnome/gvfs/programs'
  CC       gvfs-open.o
gvfs-open.c: In function ‘main’:
gvfs-open.c:53:10: warning: unused variable ‘file’ [-Wunused-variable]
   GFile *file;
          ^
  CCLD     gvfs-open


Otherwise looks good.

With the unused variable removed.

Attachment 288754 [details] pushed as 95aac17 - gvfs-open: Fix incorrect use of g_app_info_launch_default_for_uri()

The patch broke the gvfs-open for schemes i.e. mailto, magnet, irc. We should support such uris, while gvfs-open is called from xdg-open.

See Bug 748600 for more details. So the patch should be reverted, leak fixed separately and comment added to explain why we call g_app_info_launch_default_for_uri twice...

(In reply to Ondrej Holy from comment #4)
> The patch broke the gvfs-open for schemes i.e. mailto, magnet, irc. We
> should support such uris, while gvfs-open is called from xdg-open.
> 
> See Bug 748600 for more details. So the patch should be reverted, leak fixed
> separately and comment added to explain why we call
> g_app_info_launch_default_for_uri twice...

I already said so in bug 748600, the patch is not broken. g_app_info_launch_default_for_uri() is for those URIs.

Sorry I missed the comment in Bug 748600.

But the problem is that we "preprocess" the uri using g_file_get_uri (g_file_new_for_commandline_arg (location)), which e.g. for mailto:address returns mailto:///address...

g_app_info_launch_default_for_uri() opens correct application, but with wrong uri...

(In reply to Ondrej Holy from comment #6)
> Sorry I missed the comment in Bug 748600.
> 
> But the problem is that we "preprocess" the uri using g_file_get_uri
> (g_file_new_for_commandline_arg (location)), which e.g. for mailto:address
> returns mailto:///address...

mailto:address to mailto:///address looks very wrong.

It shouldn't mangle the URI for schemes it cannot express in GFile.

> g_app_info_launch_default_for_uri() opens correct application, but with
> wrong uri...

Created attachment 302563 [details] [review]
client: Do not create GDaemonFile for unknown schemes

It is because new GDaemonFile is created and the uri is constructed from mountspec (see g_daemon_vfs_get_file_for_uri, g_daemon_file_get_uri). I'm not sure why we are creating new GDaemonFile for schemes, which we can't handle. Probably we should call g_local_vfs_get_file_for_uri instead and let them create GDummyFile. But not sure we don't brake anything with this change...

(In reply to Bastien Nocera from comment #7)
> (In reply to Ondrej Holy from comment #6)
> > Sorry I missed the comment in Bug 748600.
> > 
> > But the problem is that we "preprocess" the uri using g_file_get_uri
> > (g_file_new_for_commandline_arg (location)), which e.g. for mailto:address
> > returns mailto:///address...
> 
> mailto:address to mailto:///address looks very wrong.
> 
> It shouldn't mangle the URI for schemes it cannot express in GFile.

See Bug 748912, there could be also problem with altering uris which we can express using GFile...

"ssh://user@host" becomes "ssh://user@host/"

...but it is true that the original uri is probably invalid according RFC, isn't it? Anyway it isn't good idea to modify the uri if it won't be used in GVfs...

Created attachment 303585 [details] [review]
gvfs-open: Do not alter uris before use

Uri is altered before g_app_info_launch_default_for_uri, because of the following code (from the commit 95aac17):

file = g_file_new_for_commandline_arg (location[i])
uri = g_file_get_uri (file);

Examples of uri changes:

mailto:email -> mailto:///email
ssh://user@host -> sftp://user@host/

This patch cause that uri isn't preprocessed for locations with scheme (however absolute and relative paths are still preprocessed).

*** Bug 748912 has been marked as a duplicate of this bug. ***

*** Bug 748600 has been marked as a duplicate of this bug. ***

Review of attachment 303585 [details] [review]:

::: programs/gvfs-open.c
@@ -107,3 @@
-      char *uri;
-
-      file = g_file_new_for_commandline_arg (locations[i]);

Here, check the output of g_file_get_uri_scheme() as you do in your patch.

@@ +111,3 @@
+       * we don't want. See:
+       * https://bugzilla.gnome.org/show_bug.cgi?id=738690 */
+      uri_scheme = g_uri_parse_scheme (locations[i]);

We shouldn't do that, the local path passed might "look like" a URI.

@@ +120,3 @@
+          g_object_unref (file);
+
+          g_free (usi_scheme);

I doubt that it was even compile-tested.

(In reply to Bastien Nocera from comment #13)
> Review of attachment 303585 [details] [review] [review]:
> 
> ::: programs/gvfs-open.c
> @@ -107,3 @@
> -      char *uri;
> -
> -      file = g_file_new_for_commandline_arg (locations[i]);
> 
> Here, check the output of g_file_get_uri_scheme() as you do in your patch.

As far as I know g_file_get_uri_scheme(g_file_new_for_commandline_arg()) never return NULL or empty scheme... So should I use g_file_get_uri() only if "file" scheme is returned, or what to do with?

> @@ +111,3 @@
> +       * we don't want. See:
> +       * https://bugzilla.gnome.org/show_bug.cgi?id=738690 */
> +      uri_scheme = g_uri_parse_scheme (locations[i]);
> 
> We shouldn't do that, the local path passed might "look like" a URI.

Do you think files with name e.g. "mailto:myfile"? Ok, this won't work, but it doesn't work correctly neither with g_file_new_for_commandline_arg() and g_file_get_uri_scheme returns "mailto"...

> @@ +120,3 @@
> +          g_object_unref (file);
> +
> +          g_free (usi_scheme);
> 
> I doubt that it was even compile-tested.

I am sorry, I forgot recreated patch after amending...

(In reply to Ondrej Holy from comment #14)
> (In reply to Bastien Nocera from comment #13)
> > Review of attachment 303585 [details] [review] [review] [review]:
> > @@ +111,3 @@
> > +       * we don't want. See:
> > +       * https://bugzilla.gnome.org/show_bug.cgi?id=738690 */
> > +      uri_scheme = g_uri_parse_scheme (locations[i]);
> > 
> > We shouldn't do that, the local path passed might "look like" a URI.
> 
> Do you think files with name e.g. "mailto:myfile"? Ok, this won't work, but
> it doesn't work correctly neither with g_file_new_for_commandline_arg() and
> g_file_get_uri_scheme returns "mailto"...

But hard to say what is correct, if you call e.g.:
gvfs-open mailto\:myfile

It can be handled like uri, or it can be handled like relative path. g_file_new_for_commandline_arg handles this like an uri, same as the proposed patch. So I think the patch is correct, or do you have in mind something else?

Fine by me then...

Created attachment 303678 [details] [review]
gvfs-open: Do not alter uris before use

Ok, so there is amended version...

Review of attachment 303678 [details] [review]:

Seems ok.

Comment on attachment 303678 [details] [review]
gvfs-open: Do not alter uris before use

Thanks for the review!

Pushed to master as:
commit dd650d4a82ee369df960bc8e40024e75cad0ba88

and gnome-3-16 as:
commit 98c3b3ce6f1261d3198a07b7f90644d6cb417556

*** Bug 748912 has been marked as a duplicate of this bug. ***