[Nautilus-list] Bugs 1457 and 1462

[Kenneth Kocienda <kocienda eazel com>]

For 1457, I can attempt to make a more general uri beautifier function,
one that handles escapes, etc. and is targeted at formatting uris for
human, rather than machine use.I will add such a function to
nautilus-file-utilities.[ch]. 

For 1462 - File names containing space don't launch right, your idea to
use single quotes won't work in one very important respect, and I quote
the bash man page: "Enclosing characters in single quotes preserves the
literal value of each character within the quotes. A single quote may
not occur between single quotes, even when preceded by a backslash."
Double quotes are also problematic, as the '!' character is interpreted
as the 'event' specifier, and cannot be escaped by a backslash. Hence, I
made my choice to escape every metacharacter and eschew the use of
quotes altogether. In my casual tests, it works for all cases, and
avoids the logic of multiple strpbrk() calls to determine which kind of
quoting is safe. (...of course, no type of quoting will work for files
with both ! and single quotes....) In all, I am happy to make
modifications to this function to bring it into line with your wishes.
To wit, I will send another patch that exposes this function in a
header, and moves the call to
nautilus_launch_application_from_command(). Once this function is in
place, we can continue to resolve the details surrounding the
implemetation. I can also investigate adding tests as you have
suggested. 

BTW: You'll notice, that I have cc'ed the nautilus list with this reply,
in case other hackers have interest in this thread.

Darin Adler wrote:
> 
> Kenneth,
> 
> Thanks for diving in and looking at some bugs! It's a great way to learn
> about how we code on Nautilus. In fact, I feel a little guilty about having
> this discussion in private email instead of nautilus-list, since the other
> people on the list would no doubt be interested too.
> 
> John:
> >> For the "file:// in bookmarks window" bug, here's what I know: after this
> >> code was implemented, somebody (I think Seth the intern) made a change to
> >> the location bar (where you see/type the URI for the current location) such
> >> that it didn't display file:// and didn't require file:// when entering
> >> URIs. I haven't looked at that code, so I don't know exactly how the change
> >> was made, but we need the same end result here. Hopefully the change in the
> >> location bar was made in a nicely abstracted way such that you can call a
> >> bottleneck routine when reading the URI from the bookmark and another when
> >> storing the new URI into the bookmark. If the change in the location bar was
> >> made in a less abstract localized way, then the best way to fix this would
> >> be to first abstract the location bar one by creating the nice bottleneck
> >> functions and moving them to the right place. The "right place" is probably
> >> libnautilus-extensions/nautilus-file-utilities.[ch].
> 
> Kenneth:
> > As for 1457, I've given a quick look to nautilus-file-utilities.[ch]. It
> > seems that these functions have been abstracted nicely, and there is a
> > nautilus_get_local_path_from_uri() function that does the job. So,
> > fixing this is a two-liner. A patch for this is attached as well.
> 
> Unfortunately that's not a correct solution. The
> nautilus_get_local_path_from_uri function extracts a local path from a URI,
> but returns NULL if the URI is not a URI for a local file or if the URI has
> any base escape sequences or includes any escaped slash or null characters
> ("%2F" or "%00"). The job of formatting a URI "nicely" and omitting
> "file://" to make it look nice for the user would need a function that's
> different in at least one respect -- we don't want it to return NULL for any
> non-local URIs!
> 
> So your patch isn't usable as-is.
> 
> >>> Darin, the second one is yours, 1462 - File names containing spaces
> >>> don't launch right). For this one, I read the bash man page to find a
> >>> list of possible offending characters in filenames. I'm not completely
> >>> sure that only covering the bash shell is the right approach, but that's
> >>> what my patch does .
> 
> This looks pretty good. I have a few complaints.
> 
> I'd like to see the shell_escape_path function be named with a nautilus
> prefix and put in a public header file. We'll definitely have use for this
> in more than one place. If you can't find any other place for it,
> nautilus-file-utilities.h would be an OK place for starters.
> 
> I had planned to use single-quote quoting, instead of escaping every illegal
> character (of course single quotes would still need to be escaped). This
> will result in a nicer-looking string and would require less handling of the
> individual characters; a single call to strpbrk() could be used to test if
> the quoting is necessary.
> 
> A function like this one needs some tests, and we have a standard way of
> doing that in Nautilus. If you look at the bottom of nautilus-string.c, for
> example, you'll see how we normally do tests for these sorts of functions.
> These tests can be invoked by starting Nautilus with the "--check"
> command-line option, or by doing "make check".
> 
> The escaping should be done inside the
> nautilus_launch_application_from_command() function, not in the caller of
> that function. We want to pass the application name around as a normal path
> or URI. The escaping should be done at the level that is invoking the shell.
> 
> > Index: src/nautilus-bookmarks-window.c
> > ===================================================================
> > RCS file: /cvs/gnome/nautilus/src/nautilus-bookmarks-window.c,v
> > retrieving revision 1.33
> > diff -U3 -r1.33 nautilus-bookmarks-window.c
> > --- src/nautilus-bookmarks-window.c    2000/08/11 09:29:35    1.33
> > +++ src/nautilus-bookmarks-window.c    2000/08/22 16:13:05
> > @@ -28,6 +28,7 @@
> > #include <config.h>
> > #include "nautilus-bookmarks-window.h"
> > #include <libnautilus/nautilus-undo.h>
> > +#include <libnautilus-extensions/nautilus-file-utilities.h>
> > #include <libnautilus-extensions/nautilus-global-preferences.h>
> > #include <libnautilus-extensions/nautilus-gtk-extensions.h>
> > #include <libnautilus-extensions/nautilus-icon-factory.h>
> > @@ -504,7 +505,7 @@
> > uri = nautilus_bookmark_get_uri (selected);
> >
> > nautilus_entry_set_text (NAUTILUS_ENTRY (name_field), name);
> > -    nautilus_entry_set_text (NAUTILUS_ENTRY (uri_field), uri);
> > +    nautilus_entry_set_text (NAUTILUS_ENTRY (uri_field),
> > nautilus_get_local_path_from_uri (uri));
> >
> > g_free (name);
> > g_free (uri);
> 
> Besides the problems mentioned at the top of the message, the change above
> has a storage leak. The result of nautilus_get_local_path_from_uri must be
> freed with g_free after using it.
> 
>     -- Darin

begin:vcard 
n:Kocienda;Kenneth
tel;cell:415-235-9956
tel;fax:650-210-9922
tel;work:650-210-9920
x-mozilla-html:FALSE
org:Eazel, Inc.
adr:;;1023 Corporation Way;Palo Alto;CA;94303;USA
version:2.1
email;internet:kocienda eazel com
title:Software Engineer
x-mozilla-cpt:;0
fn:Kenneth Kocienda
end:vcard