Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix unable to parse filename with spaces #27

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

michaelrk02
Copy link

This is done by parsing file URI's with urllib standard library instead of picking a substring at a specified offset

Therefore file:///my%20dir/my%20file.c will be read as /my dir/my file.c instead of /my%20dir/my%20file.c which results in file not found and causes plugin to not working

Copy link
Contributor

@ferdnyc ferdnyc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dropping the file:/// check potentially confuses remote and local paths.

Comment on lines 47 to 48
if file_uri.startswith('file:///'):
return self.get_properties_from_filename(file_uri[7:])
return self.get_properties_from_filename(unquote(urlparse(file_uri).path))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems wrong to me, because what if the file_uri is something like sftp://host.domain/path/to/some%20remote%20file.txt? At least in theory that's possible, with Gio... so it seems like the if file_uri.startswith('file:///') check is still necessary, no?

In fact, I just ran gedit sftp://my.fileserver/home/ferd/.zshrc and GEdit opened up my remote .zshrc just fine. But it'd break things, if the plugin turns that location into the path to my local .zshrc. (Which also exists, but is not the same file!)

If the file_uri does start with file:///, I agree using urlparse and unquote is a better approach to converting URLs into local pathnames.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright thanks for the correction. So I guess it's better to keep the file:/// URI checking then, let me fix this soon

@michaelrk02
Copy link
Author

@ferdnyc sorry for the very late delay, I got occupied with my projects. I have re-enabled the URI checking for file names. Please kindly check it out. Thanks

Comment on lines 46 to +49
if location:
file_uri = location.get_uri()
if file_uri.startswith('file:///'):
return self.get_properties_from_filename(file_uri[7:])
if file_uri.startswith('file://'):
return self.get_properties_from_filename(unquote(urlparse(file_uri).path))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies, @michaelrk02, for not checking this out earlier, but I was just reading the terminal plugin source, and it turns out this plugin is using outdated APIs. This function can be written like this, and not require urllib at all:

location = document.get_file().get_location()
if location and location.has_uri_scheme('file'):
    return self.get_properties_from_filename(location.get_path())

(document.get_file() returns a GtkSourceFile, and its get_location() method returns a GFile with access to all of that interface's methods.)

Comment on lines -42 to +44
if file_uri and file_uri.startswith("file:///"):
return self.get_properties_from_filename(file_uri[7:])
if file_uri and file_uri.startswith('file://'):
return self.get_properties_from_filename(unquote(urlparse(file_uri).path))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll have to track down GEdit 2 API documentation, to figure out if this one can use similar calls. Though it's debatable whether providing a gedit2 plugin really makes any sense anymore.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. It seems that while document.get_uri() returned a gchar string containing the URI, there was also a document.get_location() which, similar to GEdit 3, returned an ancient-history version of a GFile. (ref)

What did the GFile API look like in 2010, tho? Absolutely no idea.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...Huh, I guess even back in 2010 GFile was provided by Gio, which means its API should be fairly similar to what it is today. GLib has evolved very little over the years. And neither has_uri_scheme() nor get_path() is shown as being a later addition to the GFile API, so they should exist just the same even in GEdit 2.

Looks like this one can be written the same as in GEdit 3, just using document.get_location() instead of document.get_file().get_location().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants