Skip to content

Added OpenBrowser function, opens browser in Windows, GNU and XNU #153

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

Merged
merged 4 commits into from
Apr 18, 2021

Conversation

Rotario
Copy link
Contributor

@Rotario Rotario commented Apr 16, 2021

Added a simple OpenBrowser function that runs just before RedBean begins processing requests.
Fixes #98
Maybe this should be added to be optionally turned off, it would be quite annoying if you're just hosting a socket and don't need GUI. I think it should probably default to on though, because then a double click on the executable would open your browser, rather than clunkily opening CMD and passing the argument.
I'm not much of a C programmer (at all!) so feedback is always appreciated!
Thanks

@fredericosilva
Copy link

I've seen the command "start" being used for that purpose on windows. Do you know how it differs from explorer?

Copy link
Owner

@jart jart 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 sending this!

@@ -2631,6 +2647,8 @@ void RedBean(int argc, char *argv[]) {
inbuf.p = xvalloc(inbuf.n);
hdrbuf.n = 4 * 1024;
hdrbuf.p = xvalloc(hdrbuf.n);
// TODO: Maybe make this an optional argv?
Copy link
Owner

Choose a reason for hiding this comment

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

I'd recommend -b. All you have to do is add it to the getopt() invocation as well as the man page string near the top of the redbean.c file. We need it because redbean is designed to be automated. I've been working on unit tests that launch it as a subprocess within a unit test, using the redbean.com -zp0 flags which bind to a random port and then prints it to stdout.

Here's the work in progress on the integration test: https://github.com/jart/cosmopolitan/blob/master/test/tool/net/redbean_test.c Most of the other tests for redbean are stored in https://github.com/jart/cosmopolitan/tree/master/test/net/http right now.

Copy link
Owner

Choose a reason for hiding this comment

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

Another thing you might consider is adding a Lua API that plugs into your launcher function. That way, if someone wants to distribute a redbean that doesn't need the -b flag, they can just put something like LaunchBrowser() in their /tool/net/.init.lua file which would make launching the browser the default behavior.

Copy link
Contributor Author

@Rotario Rotario Apr 16, 2021

Choose a reason for hiding this comment

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

That's a really good idea, I think having to pass the -b flag is a little cumbersome, as it would be nice to just "double click" the exe (which obviously won't pass any args) but still have a browser launch, so adding the lua init would fix that.
Also - I've noticed -b turns on logbodies, shall I move that to something else?

Copy link
Owner

@jart jart Apr 17, 2021

Choose a reason for hiding this comment

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

Oh right that's true. Judging by your reaction, I think your instinct is right. The Lua API alone seems perfect. Let's hold off on the flag unless someone explicitly requests it.

} else {
prog = "xdg-open";
}
snprintf(openbrowsercommand, sizeof(openbrowsercommand), "%s http://%s", prog,
Copy link
Owner

Choose a reason for hiding this comment

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

One issue with serveraddrname is it might be 0.0.0.0:8080. What I'd recommend doing is always use 127.0.0.1 and then the port can be obtained via ntohs(serveraddr.sin_port).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also a note on 0.0.0.0 is that yeah that is serveraddrstr resolves to, but the server still works with 0.0.0.0 as well as 127.0.0.1. After doing some reading - 0.0.0.0 is "every local ip on this machine" whereas 127.0.0.1 is just "loopback" or localhost. Should I still change it to 127.0.0.1 for functional reasons? or is it just a cosmetic thing.

Copy link
Owner

Choose a reason for hiding this comment

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

Oh wow you're correct. I knew 0.0.0.0 would bind() to all local interfaces. I never considered it would work on the client side with connect(). Linux and FreeBSD appear to explicitly define connect(0.0.0.0) as connect(127.0.0.1) but that does not appear to be the case in all the os / browser / client combinations I've tried. I also just realized the user might explicitly bind to 192.168.0.x or something. So the right thing to do here might be:

  uint32_t addr;
  char ip4buf[16];
  addr = serveraddr.sin_addr.s_addr;
  if (ntohl(addr) == INADDR_ANY) addr = htonl(INADDR_LOOPBACK);
  snprintf(openbrowsercommand, sizeof(openbrowsercommand), "%s http://%s:%d\n",
           prog, inet_ntop(AF_INET, &addr, ip4buf, sizeof(ip4buf)),
           ntohs(serveraddr.sin_port));

@Rotario
Copy link
Contributor Author

Rotario commented Apr 16, 2021

I've added Lua API and -b flag now

@Rotario
Copy link
Contributor Author

Rotario commented Apr 18, 2021

Alright, I've added this new code, removed the -b flags and tested multiple different -l flags (127.0.0.1 and 127.0.0.2) on Ubuntu 20.04, browser launches at the right addresses.
I guess this needs testing on windows and BSD etc?

Copy link
Owner

@jart jart left a comment

Choose a reason for hiding this comment

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

Looks good! Don't worry too much about testing different platforms. The only requirement for contributors is that automated Travis tests are green. I'm happy to carry the burden the rest of the way on your behalf, since I've got a pretty good automated environment running on my local network that I haven't integrated into GitHub workflows yet.

@jart jart merged commit 69c5087 into jart:master Apr 18, 2021
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.

Suggestion: Launch browser when opening binary
3 participants