Adds different web backends and cleans up options #92

sloum · 2019-11-14T17:48:19Z

sloum commented

2019-11-14 17:48:19 +00:00

The manpage will need to be updated (and maybe the readme, not sure) to reflect this change. I am hopeful that this will be a good change that gives people more options and simplifies things a good amount.

I have been trying to find a way to detect whether a graphical environment is present or not so that we can add a guard clause to the gui setting for webmode. No great luck as of yet. I'll keep digging.

If I get time today I'll update the docs before fully merging this.

The manpage will need to be updated (and maybe the readme, not sure) to reflect this change. I am hopeful that this will be a good change that gives people more options and simplifies things a good amount. I have been trying to find a way to detect whether a graphical environment is present or not so that we can add a guard clause to the `gui` setting for `webmode`. No great luck as of yet. I'll keep digging. If I get time today I'll update the docs before fully merging this.

sloum added this to the 2.0.0 milestone 2019-11-14 17:48:19 +00:00

asdf was assigned by sloum

2019-11-14 17:48:20 +00:00

sloum added the

enhancement

http

labels 2019-11-14 17:48:20 +00:00

sloum commented

2019-11-14 18:17:02 +00:00

Ok. Updated the manpage. Hopefully I made it clear enough but I'd appreciate you taking a look and updating if something doesn't make sense or could use more info.

I also updated the linux version of open in browser to run type Xorg. If this errors then they do not have access to a graphical environment. This should help, but will not solve all of the problems. For example, sdf seems to be running an X server so this will not function to guard this for sdf users. It does work on circumlunar space, rawtext.club, and colorfield.space. It also, correctly, showed that my laptop had a graphical environment and proceeded to open in a browser. So while not perfect I think it is a solid step in the right direction and can serve until something better is discovered. OSX/Darwin should not need any additional checks since there is always a graphical environment available. We are not really handling BSD or Plan9 for open in browser and they will always get an error message.

Ok. Updated the manpage. Hopefully I made it clear enough but I'd appreciate you taking a look and updating if something doesn't make sense or could use more info. I also updated the linux version of open in browser to run `type Xorg`. If this errors then they do not have access to a graphical environment. This should _help_, but will not solve all of the problems. For example, sdf seems to be running an X server so this will not function to guard this for sdf users. It does work on circumlunar space, rawtext.club, and colorfield.space. It also, correctly, showed that my laptop had a graphical environment and proceeded to open in a browser. So while not perfect I think it is a solid step in the right direction and can serve until something better is discovered. OSX/Darwin should not need any additional checks since there is always a graphical environment available. We are not really handling BSD or Plan9 for open in browser and they will always get an error message.

sloum referenced this pull request

2019-11-14 18:18:42 +00:00

Opening web links in the default browser fails silently in a terminal only environment #91

sloum referenced this pull request

2019-11-14 18:19:18 +00:00

As a user, I'd like fewer options for handling of web links, so it's easier to configure Bombadillo #86

asdf commented

2019-11-15 00:47:58 +00:00

This might not be the latest version in the repo as there are compilation errors (fmt not imported, err := used twice), and type can't be run in this way because it is a shell builtin.

It's also an interesting approach to issue #91. It's detecting if Xorg is installed, but not necessarily if it is running? If that's the case, instead of type we can use exec.LookPath() instead of type.

I have found another alternative that might help too - check environment variables $DISPLAY or $WAYLAND_DISPLAY. If they are not set, I think this means there is no graphical environment available. This seems to work as expected on RTC, tilde town, and a local computer when accessed via SSH.

By the way, if you think #91 is going to delay this PR, I'm happy to put it in another PR.

This might not be the latest version in the repo as there are compilation errors (`fmt` not imported, `err :=` used twice), and `type` can't be run in this way because it is a shell builtin. It's also an interesting approach to issue #91. It's detecting if Xorg is installed, but not necessarily if it is running? If that's the case, instead of type we can use `exec.LookPath()` instead of `type`. I have found another alternative that might help too - check environment variables $DISPLAY or $WAYLAND_DISPLAY. If they are not set, I think this means there is no graphical environment available. This seems to work as expected on RTC, tilde town, and a local computer when accessed via SSH. By the way, if you think #91 is going to delay this PR, I'm happy to put it in another PR.

asdf referenced this pull request

2019-11-15 01:21:14 +00:00

Opening web links in the default browser fails silently in a terminal only environment #91

sloum commented

2019-11-15 03:34:58 +00:00

Whoops. Sorry about that. I made that update while I was at work and clearly neglected a few things. I have cleaned up the errors and linted the code for the http module (which had some issues since this update).

I updated to using exec.LookPath("Xorg") as well as using os.Getenv to look for $DISPLAY and $WAYLAND_DISPLAY. If LookPath errors and the other two are an empty string, then we bail out. Otherwise we continue with xdg-open.

Whoops. Sorry about that. I made that update while I was at work and clearly neglected a few things. I have cleaned up the errors and linted the code for the http module (which had some issues since this update). I updated to using `exec.LookPath("Xorg")` as well as using `os.Getenv` to look for `$DISPLAY` and `$WAYLAND_DISPLAY`. If `LookPath` errors and the other two are an empty string, then we bail out. Otherwise we continue with `xdg-open`.

👍 1

sloum referenced this pull request

2019-11-15 03:41:02 +00:00

Opening web links in the default browser fails silently in a terminal only environment #91

asdf reviewed 2019-11-15 03:50:40 +00:00

http/open_browser_linux.go

asdf commented

2019-11-15 03:50:40 +00:00

I think this might need to be:
if (disp == "" || wayland == "") && err != nil {
If either variable is set there could be a GUI environment.

I think this might need to be: `if (disp == "" || wayland == "") && err != nil {` If either variable is set there could be a GUI environment.

asdf reviewed 2019-11-15 03:57:00 +00:00

main.go Outdated

asdf commented

2019-11-15 03:57:00 +00:00

Should the additional settings be removed from here?

asdf reviewed 2019-11-15 03:58:15 +00:00

main.go Outdated

asdf commented

2019-11-15 03:58:15 +00:00

Also from lowerCaseOpt()?

Also from `lowerCaseOpt()`?

asdf commented

2019-11-15 04:23:24 +00:00

I've reviewed the documentation and made some further changes. There's also a couple of comments on the Files Changed tab. Let me know if you can't see them.

sloum reviewed 2019-11-15 04:34:02 +00:00

http/open_browser_linux.go

sloum commented

2019-11-15 04:34:02 +00:00

I think it should be the &&. The logic being if neither wayland nor x has a display variable then that is an indication that there is no gui present. Inversely if wayland had a value then there is a gui, so we do not want to enter the error block.

I think the logic is correct, but if I'm wrong definitely push back on it.

I think it should be the `&&`. The logic being if _neither_ wayland nor x has a display variable then that is an indication that there is no gui present. Inversely if wayland had a value then there is a gui, so we do not want to enter the error block. I think the logic is correct, but if I'm wrong definitely push back on it.

sloum commented

2019-11-15 04:37:13 +00:00

I made the change to remove the options that no longer exist and left a comment on the other one. I think the logic is right as is, though maybe there is a more clear way to write it? Or maybe a comment would be good?

asdf reviewed 2019-11-15 04:39:11 +00:00

http/open_browser_linux.go

asdf commented

2019-11-15 04:39:11 +00:00

Sorry I'm having trouble remembering how this logic works...you are 100% correct.