-
-
Notifications
You must be signed in to change notification settings - Fork 736
changed the processing of loadUrl at CreateWindowAsync #631
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
Conversation
…t and port. You can also insert the port with {port}
ElectronNET.API/WindowManager.cs
Outdated
|
|
||
| private string ParseLoadUrl(string loadUrl) | ||
| { | ||
| var match = Regex.Match(loadUrl, @"(ht|f)tp(s?)\:\/\/([a-z]+\.)+[a-z]+"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not just use UriBuilder and check the port?
var uri = new UriBuilder(loadUrl);
// check host and port on uri and modify as necessary
return uri.ToString();This would be more readable and much easier to grok and understand than doing a regex match.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just tried that out and yes, it is much easier to read but if you don't have the protocol and host given in the uri string, the UriBuilder gives either a completely wrong response or throws an exception.
The first two examples give a complete nonsense response and the last one throws an exception
Console.WriteLine(ParseLoadUrl("github.com/ElectronNET/Electron.NET/pull/631"));
Console.WriteLine(ParseLoadUrl("ElectronNET/Electron.NET/pull/631"));
Console.WriteLine(ParseLoadUrl("/ElectronNET/Electron.NET/pull/631"));
static string ParseLoadUrl(string loadUrl) {
var uri = new UriBuilder(loadUrl);
return uri.ToString();
}Do you have any solution for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the intention here is to go to github.com, then you wouldn't see a string format of github.com you'd see https://github.com. I think the contrive examples your showing here doesn't really account for the reality of what loadUrl is for. However to cover the case of receiving a relative url you can make an assumption that any url that doesn't start with http passed in (i.e. any relative url) is intended to be localhost. Thus the following...
Obviously this is an incomplete solution but it works for the given urls of the test case.
Console.WriteLine(ParseLoadUrl("github.com/ElectronNET/Electron.NET/pull/631"));
Console.WriteLine(ParseLoadUrl("ElectronNET/Electron.NET/pull/631"));
Console.WriteLine(ParseLoadUrl("/ElectronNET/Electron.NET/pull/631"));
static string ParseLoadUrl(string loadUrl) {
Uri BaseUri = new Uri("http://localhost:8000");
if (Uri.TryCreate(loadUrl, UriKind.Absolute, out var url) ||
Uri.TryCreate(BaseUri, loadUrl, out url)) {
Console.WriteLine(url.ToString());
var uri = new UriBuilder(url.ToString());
return uri.ToString();
}
throw new Exception($"Unable to parse {loadUrl}");
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I knew that github.com isn't what loadUrl is really for but I wanted an easy example. I implemented the method now into the WindowManager and commited it, I hope that you like it now better. Thanks for your help.
|
@danatcofo What is your opinion on this PR? Does it make sense to take this? |
|
I don't see an issue with this PR. Not sure if its needed but I definitely think it hardens the paths better. |
adds if missing
http://localhost