Correctly handle URIs like c: on Windows #4606
Conversation
internal/repository/file.go
Outdated
| @@ -171,6 +173,11 @@ func (r *FileRepository) Parent(u fyne.URI) (fyne.URI, error) { | |||
| // trim the scheme | |||
| s = strings.TrimPrefix(s, fileSchemePrefix) | |||
|
|
|||
| // Only for Windows: If the path is a drive root, no parent is possible | |||
| if runtime.GOOS == "windows" && regexp.MustCompile(`(?i)^/[a-z]:$`).MatchString(s) { | |||
There was a problem hiding this comment.
Compiling the regexp each time is going to be incredibly slow. It is better to cache this in some way. The initial thought would be to use a global variable but it seems unnecessary to do that for all operating systems.
There was a problem hiding this comment.
I suspect there are not thousands of these calls per second in 99.99% of all applications, but I can for example move it to a local "global" variable and compile it only once there. Or to a utility method and put the global there. The downside is: globals are not nice.
The alternative could be a utility struct like:
var windowsDrivePattern = regexp.MustCompile(`(?i)^/[a-z]:$`)
type uriUtility struct {
}
func (f *uriUtility) MatchesDrivePattern(path string) bool {
return windowsDrivePattern .MatchString(path)
}
There was a problem hiding this comment.
Personally I have no problem with a var windowsDrivePattenRegex = regexp.MustCompile(...) or similar name added to the top of this file, and then just matching directly against it here (ie no need for uriUtility). @Jacalz do you have a different opinion?
There was a problem hiding this comment.
I added the variables now.
storage/uri_test.go
Outdated
| s = "file:///C:/over/there" | ||
| u, err = storage.ParseURI(s) | ||
| assert.Nil(t, err) | ||
| assert.Equal(t, "/C:/over/there", u.Path()) |
There was a problem hiding this comment.
Returning a path like this for Windows is wrong. The logic needs to be updated in the Path function to strip out the extra slash for file:// urls on Windows.
There was a problem hiding this comment.
True, I added the logic accordingly. There are more and more if runtime.GOOS == "windows" and even more will come when UNC will be supported as well but I don't see a way around this.
There was a problem hiding this comment.
Hmm, a question I guess (and maybe this is one for @andydotxyz) - do we want to use runtime.GOOS as the trigger or do we want to handle "windows-looking" file URLs the same (ie correctly) regardless of what OS we're running on?
There was a problem hiding this comment.
If this is discussed, the handling of UNC paths on Windows should also be discussed.
Simple example:
file://server/path in Windows almost always means that any file operation then should be done on \\server\path, which is the UNC path build out of this URI. All os and io methods on Windows can handle the UNC path correctly but would fail with server/path.
| @@ -16,6 +18,9 @@ import ( | |||
| // for string processing | |||
| const fileSchemePrefix string = "file://" | |||
|
|
|||
| // Regex pattern to match Windows drive paths | |||
There was a problem hiding this comment.
it's a drive identifier isn't it - not a path?
| @@ -11,6 +12,9 @@ import ( | |||
| "fyne.io/fyne/v2" | |||
| ) | |||
|
|
|||
| // Pattern to find Windows drive paths | |||
| var windowsDrivePrefixhPattern = regexp.MustCompile(`(?i)^[a-z]:`) | |||
| @@ -20,9 +20,9 @@ func TestParseURI(t *testing.T) { | |||
| assert.Nil(t, err) | |||
| assert.Equal(t, "file:///tmp/foo.txt", uri.String()) | |||
|
|
|||
| uri, err = ParseURI("file://C:/tmp/foo.txt") | |||
There was a problem hiding this comment.
You've stated that legacy URI will still work - so please leave the old test in so we can ensure it stays working.
| @@ -13,6 +15,9 @@ import ( | |||
| // Declare conformance with fyne.URI interface. | |||
| var _ fyne.URI = &uri{} | |||
|
|
|||
| // Regex pattern to match Windows drive paths | |||
There was a problem hiding this comment.
This probably needs a note about them being URI related or the slash in the middle may be unobvious or look like duplication of the other prefix pattern
|
I think there are some missed test changes as well... |
Description:
This change makes sure that file URIs on Windows with drive letters are handled correctly.
It kind of is a breaking change but old formats (
storage.NewURI("file://C:\\foo\\bar\\baz\\")) are still accepted but converted to the new format.Fixes #4605
Checklist:
Where applicable: