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

Implement a more robust algorithm for opening web links on unices #93

Merged
merged 4 commits into from
Oct 1, 2020
Merged

Implement a more robust algorithm for opening web links on unices #93

merged 4 commits into from
Oct 1, 2020

Conversation

bouncepaw
Copy link
Contributor

Changes

  • webbrowser.Open now tries to use $BROWSER env variable before giving up if there is no xdg-open installed.
  • Do not check for Xorg program anymore. I think it is safe to expect that there is X installed if $DISPLAY is set.
  • Add some comments.

TTY browsers

As a comment in this PR mentions, I couldn't implement it. Something goes wrong when trying to restore amfora after closing a terminal browser (running in foreground, of course). If anyone wishes to implement it, they could check this:

@bouncepaw bouncepaw changed the title Implement more robust algorithm for opening web links on unices Implement a more robust algorithm for opening web links on unices Oct 1, 2020
Copy link
Owner

@makew0rld makew0rld 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 adding this! I'm not sure how many people use $BROWSER but there's no harm in supporting it. As for TTY browsers, I'm okay with leaving that for now.

Please see my comments below.

return "", fmt.Errorf("no gui is available")
case xdgOpenNotFoundErr == nil || envBrowser != "":
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
case xdgOpenNotFoundErr == nil || envBrowser != "":
case envBrowser != "":
// Prefer $BROWSER over xdg-open
// Use start rather than run or output in order
// to make browser running in background.
if err := exec.Command(envBrowser, url).Start(); err != nil {
return "", err
}
return "Opened in $BROWSER", nil
case xdgOpenNotFoundErr == nil:

Your code does not seem to make use of the $BROWSER variable at all, I assume that's a mistake. My suggestion above makes use of the var if it's available, and use xdg-open otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's a mistake. There were two similar case-blocks and I smashed them together without second thought. Gotta fix this.

I think xdg-open should be used first because it's more idiomatic (I think) and is also similar to how this function is implemented for other platforms.

Copy link
Owner

Choose a reason for hiding this comment

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

I thought $BROWSER should be first because it's something the user manually sets, so it's more likely to be a custom specific choice than xdg-open. Plus it's only used by CLI software, so a user could use it to differentiate between links in CLI and GUI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good point (you've almost convinced me), but consider xdg-open's source code. It seems to me that xdg-open depends on the $BROWSER variable in some cases. Therefore, xdg-open is more universal and should be used first.

Copy link
Owner

Choose a reason for hiding this comment

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

Oh, I had no idea! Ok, I agree. I will merge this soon, consider it complete.

webbrowser/open_browser_unix.go Outdated Show resolved Hide resolved
@makew0rld makew0rld merged commit 4a71b69 into makew0rld:master Oct 1, 2020
makew0rld added a commit that referenced this pull request Oct 1, 2020
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