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

Correctly handle URIs like c: on Windows #4606

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

Roemer
Copy link
Contributor

@Roemer Roemer commented Feb 2, 2024

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:

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

Where applicable:

  • Any breaking changes have a deprecation path or have been discussed.

@@ -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) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the variables now.

Copy link
Contributor

@dweymouth dweymouth left a comment

Choose a reason for hiding this comment

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

Can we add a Windows case to TestURI_Path as well?

s = "file:///C:/over/there"
u, err = storage.ParseURI(s)
assert.Nil(t, err)
assert.Equal(t, "/C:/over/there", u.Path())
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@andydotxyz andydotxyz left a comment

Choose a reason for hiding this comment

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

Thanks for this, a couple of small things inline

@@ -16,6 +18,9 @@ import (
// for string processing
const fileSchemePrefix string = "file://"

// Regex pattern to match Windows drive paths
Copy link
Member

Choose a reason for hiding this comment

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

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]:`)
Copy link
Member

Choose a reason for hiding this comment

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

is prefixh a typo?

@@ -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")
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

Choose a reason for hiding this comment

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

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

@andydotxyz
Copy link
Member

I think there are some missed test changes as well...

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.

4 participants