#3131: Allow help browser to be chosen independent of web-browser env… #3484
Conversation
…ironment variable setting. Set environment variable w3m -o confirm_qq=0 to the desired browser command. This variable may be an array; the entries after the first are treated as command-line options to the browser named in the first array entry.
| @@ -109,6 +108,9 @@ function help --description 'Show help for the fish shell' | |||
| if test -f "$man_arg" | |||
| man $man_arg | |||
| return | |||
| else if test -f "$man_arg.gz" | |||
|
@TieDyedDevil: You'll want to remove the other commits here. a74ff49 in particular is not only a completely different change, but won't even work since the later checks will still fail (which cursor sequences do those terminals use?) The rest is just noise. |
|
I understand how the PR got into this state, but am having difficulties figuring out how to unwind the unwanted commits. I'll update this as soon as I can figure out how... :\ |
|
you might try playing with |
Standardize how the host name is included in the prompts that do so. Fixes #3480
There was one block of code modified by commit 42458ff that had convoluted, inverted, logic. In the process of collapsing nested "if" blocks the logic was modified to avoid using "!" everywhere the bool was tested. Unfortunately I neglected to modify two of the conditions used to set that var to reflect the changed polarity.
|
Thanks for the git rebase -i suggestion. Unfortunately, I only dug a deeper hole for myself. What I'd like to do at this point is destroy my fork, start over and submit a new PR. In the new PR, I'll include the requested documentation and I'll double-check on the code that's currently in place for the gzip'd man pages. Does that sound reasonable, or will killing off my original fork cause other problems for you? |
|
That's OK. |
|
You might find |
…ironment variable setting. Set environment variable w3m -o confirm_qq=0 to the desired browser command. This variable may be an array; the entries after the first are treated as command-line options to the browser named in the first array entry.
|
Thank you for the git assistance. I believe that the PR is now in good (or at least better) shape. |
|
What's the next step with this PR? Should I restore the code that handles gzip'd fish help pages? |
|
I attempted to review this change but the diffs reported by Github are very confusing. I think you need to create a new git branch based on a |
| end | ||
|
|
||
| if test -z $fish_browser | ||
| if test -z $fish_browser[1] |
There was a problem hiding this comment.
You should always quote the argument to test -z and test -n. But in this instance it's better to do it the fish way: if set -q fish_browser[1].
| if contains -- $BROWSER $graphical_browsers | ||
| set fish_browser_bg 1 | ||
| end | ||
| if type -q "$fish_help_browser[1]" |
There was a problem hiding this comment.
Is fish_help_browser guaranteed to have at least one element at this point? Because if not (and I don't see any such guarantee) this will fail. Also, it's not obvious to me that the code you based this on should be using type -q. If the user has set fish_help_browser we should try to use it regardless of whether or not type -q thinks it is a valid command. I recommend just doing if set -q fish_help_browser[1] (not the lack of a dollar-sign).
| for i in $graphical_browsers | ||
| set -l text_browsers htmlview www-browser links elinks lynx w3m | ||
|
|
||
| if type -q "$BROWSER" |
There was a problem hiding this comment.
I know this is what the prior version did but as I commented above I believe this should simply be if set -q BROWSER.
| @@ -13,6 +13,8 @@ If a `SECTION` is specified, the help for that command is shown. | |||
|
|
|||
| If the BROWSER environment variable is set, it will be used to display the documentation. Otherwise, fish will search for a suitable browser. | |||
|
|
|||
| If you prefer to use a different browser (other than as described above) for fish help, you can set the fish_help_browser environment variable. This variable may be set as an array, where the first element is the browser command and the rest are browser options. | |||
There was a problem hiding this comment.
s/environment variable/variable/ as there is nothing that requires this to be an environment variable.
| @@ -71,7 +71,7 @@ function help --description 'Show help for the fish shell' | |||
| end | |||
| end | |||
|
|
|||
| if test -z $fish_browser[1] | |||
| if set -q $fish_browser[1] | |||
There was a problem hiding this comment.
You need to remove the $. Otherwise this looks good to me. If you fix this and confirm the resulting code works for you I'll merge it.
| end | ||
|
|
||
| if test -z $fish_browser | ||
| if set -q $fish_browser[1] |
There was a problem hiding this comment.
That "$" needs to be removed, otherwise you're checking if the value of $fish_browser is the name of a set variable.
set -q takes just a variable name. Also set -q var[1] is equivalent to set -q var.
There was a problem hiding this comment.
@faho, I think you had a brain cramp. set -q var[1] tests that the var has at least one element whereas set -q var tests that it is defined, possibly with zero elements. They are not equivalent. The former is equivalent to test (count $var) -ge 1 while the latter has not direct test analog. You can get away with the former is you know that the var will never be defined with zero elements.
|
OK, I think it's done. Thanks for guidance. One more thing: Should I reinstate the code that handles gzip'd man pages (i.e. to handle the case where Fedora gzips the man pages before installation)? |
|
Despite Github showing a good diff this branch has so many conflicts with the master branch that I can't cleanly apply it. I can generate just a context diff of your changes and apply that but that will lose your authorship. Or you can do the same thing and generate a new PR from a branch forked from the current master branch. Your choice, @TieDyedDevil. |
|
If you change didn't remove support for gzipped files then, no, don't reinstate that as part of this change. |
| @@ -23,7 +23,7 @@ function help --description 'Show help for the fish shell' | |||
| # | |||
| set -l graphical_browsers htmlview x-www-browser firefox galeon mozilla konqueror epiphany opera netscape rekonq google-chrome chromium-browser | |||
|
|
|||
| if set -q fish_help_browser[1] | |||
| if set -q fish_help_browser | |||
There was a problem hiding this comment.
@faho lead you astray. The two forms are not equivalent. The if set -q fish_help_browser[1] is safer as it ensure the var has at least one element. The form without the subscript will evaluate to true if the user did set fish_help_browser; i.e., defined it without giving it a value. That's an odd thing to do but it's good practice to protect against it.
|
I've created a fresh, clean PR here:#3583 . |
|
Superceded by #3583. |
Description
Allow help browser to be chosen independent of $BROWSER environment variable setting. Set environment variable $fish_help_browser to the desired browser command. This variable may be an array; the entries after the first are treated as command-line options to the browser named in the first array entry.
Fixes issue #3131
TODOs:
…ironment variable setting. Set environment variable w3m -o confirm_qq=0 to the desired browser command. This variable may be an array; the entries after the first are treated as command-line options to the browser named in the first array entry.