On Tue, Nov 30, 2010 at 5:02 AM, Christian Couder <christian.couder@xxxxxxxxx> wrote: > On Mon, Nov 29, 2010 at 8:05 PM, Giuseppe Bilotta > <giuseppe.bilotta@xxxxxxxxx> wrote: >> On Mon, Nov 29, 2010 at 5:18 PM, Christian Couder >> <christian.couder@xxxxxxxxx> wrote: >>> Hi, >>> >>> On Mon, Nov 29, 2010 at 3:47 PM, Giuseppe Bilotta >>> <giuseppe.bilotta@xxxxxxxxx> wrote: >>>> >>>> +# Debian and derivatives use x-www-browser or www-browser to set >>>> +# the default browser for the system >>>> +if test -z "$browser" ; then >>>> + wwwbrowser="/usr/bin/www-browser" >>>> + if test -n "$DISPLAY"; then >>>> + wwwbrowser="/usr/bin/x-www-browser $wwwbrowser" >>>> + fi >>>> + for i in $wwwbrowser; do >>>> + if test -x $i ; then >>>> + verstring="$($i --version 2> /dev/null | head -n 1)" >>>> + browser="$(echo "$verstring" | cut -f1 -d' ' | tr A-Z a-z)" >>> >>> Stupid questions: >>> >>> Did you check that all the browser we support accept the --version option? >> >> I don't have all of them readily available, so I checked with >> Mozilla-based browsers, Opera, Chromium, elinks, lynx, w3m. I'm >> missing konqueror and dillo. >> >>> What if we add support for a new one that doesn't ? >> >> I think that the worse issue would be (x-)www-browser linking to >> something that doesn't support --version regardless of wether we >> support it or not. >> >>> Shouldn't we add something like : >>> >>> test -z "$browser" && browser="$(readlink $i)" >> >> My first idea was to go for something like browser="$(basename >> $(readlink $i))" (not sure why you would need test -z before). > > We would need "test -z" before if we add it after: > > browser="$(echo "$verstring" | cut -f1 -d' ' | tr A-Z a-z)" > > so that we have a fallback if it doesn't support "--version". Oh, I see what you mean here. I thought you wanted to use it as an alternative to the --version. >> We allow valid_tool to be false in the x-www-browser case, in which >> case we test www-browser, and if it's still not valid we go on and use >> the previous paths. So we cannot die in case of an invalid >> (x-)www-browser. > > Yeah, you are right, but we could die after the "for i in $wwwbrowser" > loop if both are invalid. Why? If both are invalid, proceeding with the previous strategy of looking for a browser we _should_ be looking for any browser we know about, even if it's not set as the default system browser. >> But there's a bug in the www-brower testing, it needs >> an else that resets browser to the empty string. > > I thought it was by design that you did not reset it... > So yeah it is clearer and nicer for the user if you either reset > browser or just die if both (x-)www-browser are invalid. If you decide > to reset browser, perhaps a warning or an information message telling > that both are unknown would be nice. I can do that. Should it be a warning about reporting the lack of support to us, or just a warning that we are not going to use it even if it's defined? While we're at it: I was considering adding support for the BROWSER env var (a colon-separated list of browsers executables or "browser %s" strings). All of this is going to make the web--browse script very similar to the sensible-browser script in Debian, with the difference that we go at length to ensure that stuff is opened in a new tab, whereas Debian's sensible-browser doesn't. Should we just support sensible-browser instead of (x-)www-browser/BROWSER, and let it open anything? -- Giuseppe "Oblomov" Bilotta -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html