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

Added support for windows paths #2276

Merged
merged 2 commits into from
Jul 28, 2021
Merged

Conversation

sopyer
Copy link
Contributor

@sopyer sopyer commented Jul 13, 2021

No description provided.

@matteblair matteblair self-assigned this Jul 13, 2021
@matteblair
Copy link
Member

matteblair commented Jul 18, 2021

Thanks for breaking this into a separate PR!

I have concerns about the #ifdefs in the code here. Platform-branching like this is an easy way to create bugs or confusion, especially when it occurs in a core function of the library. So I was thinking about how we could achieve the goal of running on Windows without adding unnecessary complexity and I came up with an idea:

We can leave the URL code un-changed (that is, revert the changes in this PR) and instead request that Windows application code follows two rules:

  • All file paths use forward slash separators
  • Absolute file paths (that is, those including a drive letter) should start with file:///

With these two changes on just the Windows application side, the URL code can remain simpler and stay the same for all platforms. What do you think?

@sopyer
Copy link
Contributor Author

sopyer commented Jul 19, 2021

@matteblair I thought about it but in this case ifdefs will just move to another place. I was considering using msys convention like /c/user/name/Downloads/somefile.txt. I'll need for this to work to modify all places that use URLs to access local files. Curewntly I am aware about urlCLient.cpp, but there can be others.
I'll try to prepare PR with that, but unfortunately it will take time as my vacation is over.

@sopyer
Copy link
Contributor Author

sopyer commented Jul 20, 2021

I think latest commit is minimal changes we need to support Windows platform.
In general problem rises from guessing whether : is separator for scheme or disk drive.
Proper fix for this problem is to change API:

  • make URL immutable
  • Hide generic string constructor and introduce named "constructors" Url::FromAbsPath, Url::FromRelPath, Url::FromUrlString
    Those preconditions should allow to simplify URL handling logic - specifically resolve(no need to guess abs vs rel path), and also will allow seamless path handling cross platform(scheme will be always valid for abs urls)

@sopyer
Copy link
Contributor Author

sopyer commented Jul 20, 2021

I decided against including windows specific tests as they are very little reassurance, / at start of path already works but is very brittle, so I decided against specifying this behavior as current approach is more on hackish side.

@sopyer
Copy link
Contributor Author

sopyer commented Jul 26, 2021

@matteblair Is new approach good? Can we proceed?

@matteblair
Copy link
Member

This seems good enough for now! I will think on this more and propose any changes I come up with in a new PR.

One last thing before I merge this - is there a reason for increasing the width of the Range members? If it isn't necessary, I'd prefer leaving those as 16 bits to keep the size of Url smaller.

@sopyer
Copy link
Contributor Author

sopyer commented Jul 28, 2021

Reason is multiple warning related to slicing size_t to uint16_t. There were too many of them and using size_t was simplest way to avoid them.

@matteblair matteblair merged commit a240323 into tangrams:main Jul 28, 2021
ericnelsonaz pushed a commit to ericnelsonaz/tangram-es that referenced this pull request Aug 5, 2021
* Added support for windows paths

* Minimal changes for Windows path support
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