Fix for https://github.com/fyne-io/fyne/issues/5207

[brucealthompson]

Description:

Fixes #(issue)

Checklist:

• Tests included.
• Lint and formatter run with no errors.
• Tests all pass.

Where applicable:

• Public APIs match existing style and have Since: line.
• Any breaking changes have a deprecation path or have been discussed.
• Check for binary size increases when importing new modules.

[andydotxyz]

This doesn't seem right - the new URI is constructed essentially from a file path, you can see it starts with:

buildDir := filepath.VolumeName(localdir)

but then is created as:

newURL := dir.Scheme() + "://" + filepath.ToSlash(buildDir)

This does not seem right. I cannot tell exactly what is happening here, but simply swapping the correct scheme where "file://" is used will not be the right fix as file uris/paths are being assumed in this whole code chunk.

I think the right fix will be to traverse this properly with the storage API to iterate on the storage.Parent result to walk back up the "tree".

[brucealthompson]

I was trying to make as minimal a change as possible. The fix certainly works for the "file" repository and my "httpfile" repository that I created.

This does not seem right. I cannot tell exactly what is happening here, but simply swapping the correct scheme where "file://" is used will not be the right fix as file uris/paths are being assumed in this whole code chunk.

I think the right fix will be to traverse this properly with the storage API to iterate on the storage. Parent result to walk back up the "tree".

I did not touch the code that manipulated the path. I believe the data structure represented by a fyne URI is the same as an RFC 3986 URI and the same as is represented by the golang URI https://pkg.go.dev/go.lsp.dev/uri#URI. I believe the path manipulation code you are referring to is correct for manipulating an RFC 3986 compliant path.

The only potential issue with the code you are referring to is that the "file" repository as well as the the "httpfile" repository can include DOS drive letters in the path. IE, a valid "file" repository URL is "file://c:/foo". This is a valid URI according to RFC 8089 https://datatracker.ietf.org/doc/html/rfc8089#page-13. The fyne storage.ParseURI() call does not parse this type of URI correctly and ends up putting the drive letter in the Authority field of the URI. I had to work around this issue in my "httpfile" repository code. So there is a bug in the fyne fyne storage.ParseURI() code. I think the storage.ParseURI() implementation should call the golang uri.ParseURI function instead of having the fyne repository interface provide its own implementation for this.

The only issue I could see with the fix I made is this:
newURL := dir.Scheme() + "://" + filepath.ToSlash(buildDir)

The filepath.ToSlash() call should not be necessary since buildDir should be an RFC 3986 compliant path, I do not remember why I did this. I will try removing it and test to make sure everything works.

[brucealthompson]

OK. I fixed the path manipulation code. Here are the changes

Notice newURL := dir.Scheme() + "://" + filepath.ToSlash(buildDir) >> newURL := dir.Scheme() + "://" + buildDir

I also found the issue in the fyne storage.ParseURI() code. Here it is:
if strings.EqualFold(scheme, "file") {
// Does this really deserve to be special? In principle, the
// purpose of this check is to pass it to NewFileURI, which
// allows platform path seps in the URI (against the RFC, but
// easier for people building URIs naively on Windows). Maybe
// we should punt this to whoever generated the URI in the
// first place?

This code should be independent of scheme. This is a separate issue.

[brucealthompson]

I also found the issue in the fyne storage.ParseURI() code. Here it is:
if strings.EqualFold(scheme, "file") {
// Does this really deserve to be special? In principle, the
// purpose of this check is to pass it to NewFileURI, which
// allows platform path seps in the URI (against the RFC, but
// easier for people building URIs naively on Windows). Maybe
// we should punt this to whoever generated the URI in the
// first place?

This code should be independent of scheme. This is a separate issue.

I have fixed this issue for my httpfile repository. The fix is to add a ParseURI method to the repository. Here is what I imples code should be independent of scheme. This is a separate issue.

I have fixed this issue for my httpfile repository. The fix is to add a ParseURI method to the repository. Here is what I implemented for the httpfile repository:

func (r *HttpFileRepository) ParseURI(uristring string) (fyne.URI, error) {
	localurl, err := url.Parse(uristring)
	if err != nil {
		return nil, err
	}
	uri := &uri{
		scheme: localurl.Scheme,
		// url.Parse interprets windows drive letters as a Host. Include this in the path.
		path:      localurl.Host + localurl.Path,
		authority: "",
		fragment:  "",
		query:     "",
	}
	return uri, nil
}

I believe the same fix cam be applied to the "file" repository. Then the repository specific code in fyne storage.ParseURI() that I highlighted above can be removed.

[andydotxyz]

I believe the path manipulation code you are referring to is correct for manipulating an RFC 3986 compliant path

The issue is that the code assumes a URI is just scheme://path - this Is not true when there is a host component, or port (or query, but that may be out of scope).

The fix is to add a ParseURI method to the repository.

I don't understand why this would be the case - as you say a URI parse is pretty standard (if there is a fix in our parser we should fix it). We can iterate through the file tree of any URI using storage.Parent iteratively as I suggested before. I don't think new parsing or file path manipulation is needed to do this in a way that works for all repository schemes.

[andydotxyz]

I think you should check out an earlier piece of work for handling windows volume names and remote servers for windows format at #4606

[brucealthompson]

There are actually 2 separate topics now in this thread.

The main topic is the fix for the issue described in the original bug report. I have modified my proposed fix and created a new push request.

Please review.

[brucealthompson]

The 2nd topic is about an issue I founding during code inspection. Here is the issue I found during code inspection:

I also found the issue in the fyne storage.ParseURI() code. Here it is:
if strings.EqualFold(scheme, "file") {
// Does this really deserve to be special? In principle, the
// purpose of this check is to pass it to NewFileURI, which
// allows platform path seps in the URI (against the RFC, but
// easier for people building URIs naively on Windows). Maybe
// we should punt this to whoever generated the URI in the
// first place?

This code should be independent of scheme. This is a separate issue.

This issue definitely impacted my httpfile repository. If you remove this code from storage.ParseURI() then the file repository interface will not work with dialog.NewFileOpen()

I don't understand why this would be the case - as you say a URI parse is pretty standard (if there is a fix in our parser we should fix it). We can iterate through the file tree of any URI using storage.Parent iteratively as I suggested before. I don't think new parsing or file path manipulation is needed to do this in a way that works for all repository schemes.

I believe my testing from above shows that there is an issue with the fyne parser. It does not work with "file"//" URIs that include DOS paths. The code above must be a workaround for this issue. You can either:

1. Leave things alone.
2. fix the fyne parser to make it completely general
3. create a ParseURI method for the "file" repository

[andydotxyz]

Is hiding favourites necessary because you have specified a non-file starting location? The two feel unconnected.

Anyhow, the reason for asking for changes is the false URI assumption mentioned inline and in the issue this fixes.

[andydotxyz]

This assumes that a URI (not URL!) is a scheme followed by a path. This is only true for file URIs. Most contain a host and/or port portion as well.

[brucealthompson]

This assumes that a URI (not URL!) is a scheme followed by a path. This is only true for file URIs. Most contain a host and/or port portion as well.

This variable is actually for a URI, not a URL. I will rename it and check to see if there are other components of the fyne URI with variable name "dir" that need to be included in this constructed URI.

I will create a separate branch for this fix based on your dev branch and resubmit. Give me a little time as I am working on something else right now.

[andydotxyz]

check to see if there are other components of the fyne URI with variable name "dir" that need to be included in this constructed URI.

But why do you pursue the URI re-construction instead of looking into storage.Child? The parsing has all been done already and string manipulation is inherently less safe.

[brucealthompson]

I did not write the code you are referring to. I was trying to do the simplest thing possible without rewriting a bunch of existing code.

Not sure why the original author did not choose to use storage.Child(). Was it not evailable when this was originally written?

In any case, I can check into that solution as well.

[andydotxyz]

I did not write the code you are referring to. I was trying to do the simplest thing possible without rewriting a bunch of existing code.

I appreciate that, but sometimes when the existing code contains the bug we need to fix that directly instead of following the old pattern and working around it to minimise the change.

Not sure why the original author did not choose to use storage.Child(). Was it not evailable when this was originally written?

Well, when this only worked with file:// the assumptions held - because it does not (or did not) have any other URI elements like host or port. Those assumptions no longer hold.

Thanks for looking into it.

[brucealthompson]

Is hiding favourites necessary because you have specified a non-file starting location? The two feel unconnected.

Anyhow, the reason for asking for changes is the false URI assumption mentioned inline and in the issue this fixes.

Ugh...

This is github issues. I did a pull request for the file revision before this one. The revision you are looking at includes a local I fix I made for #5204. This fix works but I did not intend to push this back to fyne.

[andydotxyz]

Ugh...

This is github issues. I did a pull request for the file revision before this one. The revision you are looking at includes a local I fix I made for #5204. This fix works but I did not intend to push this back to fyne.

Ah. If you want to have two Pull Requests where one does not depend on another you'll need a different branch.

[andydotxyz]

Can you please update the title and description so it is clear what should (vs should not!) be in this PR/review?