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

Towards gvfs-fuse independence #963

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

Towards gvfs-fuse independence #963

wants to merge 12 commits into from

Conversation

techee
Copy link
Member

@techee techee commented Mar 13, 2016

Based on the discussion here #921 I tried what is still missing to support opening remote files without gvfs-fuse. As I suspected, there wasn't much missing - mostly some g_file_test() calls which didn't work remotely, file name validation which didn't allow URIs, getting filenames instead of URIs from file dialogs etc.

What works after these patches:

  • opening links like https://developer.gnome.org/gio/stable/GFile.html both from command-line and file open dialog
  • browsing remote locations in file browser
  • opening/saving ftp/ssh/etc. locations when mounted - I guess they need to be mounted to work - even gedit needs them mounted (it would probably be possible to use g_file_mount_enclosing_volume () to pop up dialog to make user mount them, not sure if we want it or not)
  • opening/saving remote projects

What doesn't work:

  • TM parsing - the only reason here's the g_stat() in tm_source_file_init(), after commenting it out it works but TM at the moment doesn't know about Geany's file operation method used so it's not possible to implement this properly (I plan moving the TM - without ctags - into src making it effectively part of Geany after which it will have access to Geany's definitions)
  • File->Properties - I simply have been lazy to implement it
  • of course things like find in files, build commands etc. which we cannot run remotely

@elextr
Copy link
Member

elextr commented Mar 14, 2016

@techee nice!!!

Two questions:

  1. what does the file dialog show and what do we get for IRIs, a pretty UTF-8 version or the uglyfied punycode or percent encoded version?
  2. Does the save-actions plugin need adjusting too?

[Edit: 3. when the settings are not set to GIO does this mix GIO and non-GIO calls? (remembering that gave us problems with stat in the past) ]

@techee
Copy link
Member Author

techee commented Mar 14, 2016

what does the file dialog show and what do we get for IRIs, a pretty UTF-8 version or the uglyfied punycode or percent encoded version?

Could you provide an example of such an URI pointing to some text file so I can try opening it in Geany?

Does the save-actions plugin need adjusting too?

From what I can see the backup saving code would have to be updated to work remotely.

[Edit: 3. when the settings are not set to GIO does this mix GIO and non-GIO calls? (remembering that gave us problems with stat in the past) ]

Since I fixed this one I took care not to mix GIO/non-GIO for files which could be opened remotely. Now with local files like various Geany config files it shouldn't matter and these typically still use the non-GIO variants all the time. The exception will be the various g_file_test() calls I replaced in the first patch and which might use GIO on local files . There are quite many of them and I wanted to avoid evaluating every single case whether it needs GIO or not. But again, since they are local and since they have nothing to do with file updates there's no danger there would be some kind of race like there was in the mtime case.

/**
* Tests file existence using the configured IO method in Geany.
*
* @param fname File name.
Copy link
Member

Choose a reason for hiding this comment

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

"the path or URI"

Copy link
Member

Choose a reason for hiding this comment

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

Do we actually want those in the API? It's cute, but do plugin really care about that?

Copy link
Member Author

Choose a reason for hiding this comment

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

When you have a look at the plugins the patch affects, they do (maybe not all the g_file_test()s deal with possibly remote files but some definitely do).

But the question is good - we probably shouldn't make an utility function for every single IO function - instead there should be something like

utils_use_gio()

and plugins should use it as needed (we'll already need this for the saveaction plugin which needs to be updated to make a backup copy of a possibly remote file). This will lead to a minor code duplication in the plugins but I think from the API perspective this is the right way. What do you think?

The other question is whether to make such a function inside utils or somewhere else (prefs?) or whether to make it e.g. a variable in some config struct. Ideas?

@elextr
Copy link
Member

elextr commented Mar 19, 2016

Could you provide an example of such an URI pointing to some text file so I can try opening it in Geany?

No, I only speak ASCII remember :) You or @b4n would be more likely to know of a site with an accented character in the domain name.

@@ -1788,6 +1788,9 @@ void utils_tidy_path(gchar *filename)
const gchar *needle;
gboolean preserve_double_backslash = FALSE;

if (utils_is_uri(filename))
return;
Copy link
Member

Choose a reason for hiding this comment

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

what about skipping the scheme instead? around the line of a sane version of filename = strstr(filename, "://") + 3.

Copy link
Member Author

Choose a reason for hiding this comment

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

Can do. But can relative paths be part of a valid URI?

Copy link
Member

Choose a reason for hiding this comment

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

As I found out elsewhere not all schemes need the // so maybe not such a good idea.

Copy link
Member Author

Choose a reason for hiding this comment

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

@b4n I kept it this way in the new patch - the current implementation of the function would have to be changed because there's e.g.

utils_string_replace_all(str, "/", G_DIR_SEPARATOR_S)

on Windows which we don't want. And since URIs cannot be relative, I think it's more or less OK the way it is?

Copy link
Member Author

Choose a reason for hiding this comment

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

@elextr :// is used on other places to detect URIs - is there a better way?

Copy link
Member Author

Choose a reason for hiding this comment

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

Omitting "//" for URI is rather crazy and I don't expect operating systems or applications will ever pass uris this way. GTK for instance always returns "file://" when opening files and asking for URI. IMO the "//"-less case can be safely ignored.

Copy link
Member

Choose a reason for hiding this comment

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

@techee did you notice the release date THIS MONTH, of course nothing omits it at the moment, its only just been allowed.

Copy link
Member

@elextr elextr Feb 28, 2017

Choose a reason for hiding this comment

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

Though actually my exhaustive test with Chrome and Firefox shows that many ;) browsers are perfectly happy without the // already.

Copy link
Member Author

Choose a reason for hiding this comment

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

@techee did you notice the release date THIS MONTH

Sure, I'm aware of the release and the patches are definitely not meant for it. It's just that I had time now and the bigger pull requests are better to be merged early in the next release cycle to get better testing (of course if Colomban has time to review them).

Copy link
Member

Choose a reason for hiding this comment

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

Sure, I'm aware of the release and the patches are definitely not meant for it.

I didn't mean the release of Geany, I meant the release of RFC 8089 that allows for file: to not have the //es. As it was only released Feb 2017 nothing will take advantage of it yet, though as I noted both chrome and firefox accept file: URLs without the //.

@@ -1034,12 +1034,15 @@ static gboolean load_config(const gchar *filename)
GKeyFile *config;
GeanyProject *p;
GSList *node;
gchar *data;
gsize len;
Copy link
Member

Choose a reason for hiding this comment

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

needs freeing

@b4n
Copy link
Member

b4n commented Mar 19, 2016

locale_filename now can be an URI. Is it's encoding still good? Does all the code handle it being an URI rather than a path gracefully?

@b4n
Copy link
Member

b4n commented Mar 19, 2016

Looks mostly sensible. But all this GFile creation, and this making Geany use GIO more and more makes me wonder: don't we rather want to have a GFile member attached to a document in addition to the path, and have this NULL when not using GIO? That would re-use the same link all the way, and possibly might prevent some oddies when loading from an URI and writing to a path (when gvfs-fuse is available) -- I doubt it should be a problem, but still, could be suboptimal I guess.

@b4n
Copy link
Member

b4n commented Mar 19, 2016

@elextr

what does the file dialog show and what do we get for IRIs, a pretty UTF-8 version or the uglyfied punycode or percent encoded version?

That's most likely not up to us, and if it doesn't work now it'll probably get fixed in GIO at some point. And GEdit seems to display it just fine when I try to open http://bücher.ch/ (no, I don't have a better example than the WP one)

@elextr
Copy link
Member

elextr commented Mar 19, 2016

@b4n, agree its not up to us, I just wanted to know what the open dialog would show.

And what would show for the filename on the tab and in its tooltip? And in the documents sidebar? And in filebrowser plugin?

I will bet there will be confused users if some things show Unicoded names and some show punycode and percent escaped names :)

Note: both saveactions and filebrowser are core plugins and should be fixed (if they need to be) as part of this change.

@techee
Copy link
Member Author

techee commented Mar 20, 2016

@elextr Works fine as far as I can tell (once it works at one place, it should work everywhere - the rest is just a matter of correct locale/utf8 handling we should do anyway):

screenshot_2016-03-20_21-29-23

@techee
Copy link
Member Author

techee commented Mar 20, 2016

locale_filename now can be an URI. Is it's encoding still good? Does all the code handle it being an URI rather than a path gracefully?

Seems so for the core functionality - can't speak for all the plugins. The only exception at the moment is the tag manager which skips parsing because of the g_stat() I mentioned previously.

@techee
Copy link
Member Author

techee commented Mar 20, 2016

Looks mostly sensible. But all this GFile creation, and this making Geany use GIO more and more makes me wonder: don't we rather want to have a GFile member attached to a document in addition to the path, and have this NULL when not using GIO?

Good idea. I'd just make one change - always create the GFile no matter whether GIO is enabled or not. The problem is GIO can be enabled/disabled in preferences at any time so we'd have to nullify/set the field based on the settings, plugins would have to always check it for NULL and it would introduce just complications. Better to have something like the utils_use_gio() and whoever wants to use the GFile should use the utils_use_gio() or know what he is doing.

That would re-use the same link all the way, and possibly might prevent some oddies when loading from an URI and writing to a path (when gvfs-fuse is available) -- I doubt it should be a problem, but still, could be suboptimal I guess.

I hope when using GIO consistently this shouldn't happen. Anyway, the problem with mtime we had was quite unique I believe and shouldn't happen when e.g. just testing file existence.

@techee
Copy link
Member Author

techee commented Mar 20, 2016

Note: both saveactions and filebrowser are core plugins and should be fixed (if they need to be) as part of this change.

Agree. Filebrowser already works, saveactions need an update to be able to save backups of remote files.

@techee
Copy link
Member Author

techee commented Feb 27, 2017

Looks mostly sensible. But all this GFile creation, and this making Geany use GIO more and more makes me wonder: don't we rather want to have a GFile member attached to a document in addition to the path, and have this NULL when not using GIO?

Good idea. I'd just make one change - always create the GFile no matter whether GIO is enabled or not. The problem is GIO can be enabled/disabled in preferences at any time so we'd have to nullify/set the field based on the settings, plugins would have to always check it for NULL and it would introduce just complications. Better to have something like the utils_use_gio() and whoever wants to use the GFile should use the utils_use_gio() or know what he is doing.

I was thinking about it more and it won't eliminate most of the GFile creation. Most of the cases when GFile is created isn't for the already opened document, but when opening a file and e.g. validating it exists. So I didn't make this change in #1414.

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

Successfully merging this pull request may close these issues.

None yet

3 participants