From: Giuseppe Bilotta Date: Tue, 30 Nov 2010 08:22:20 GMT Subject: Re: [PATCH 5/6] web--browse: use (x-)www-browser if available Message-ID: In-Reply-To: On Tue, Nov 30, 2010 at 5:02 AM, Christian Couder wrote: > On Mon, Nov 29, 2010 at 8:05 PM, Giuseppe Bilotta > wrote: >> On Mon, Nov 29, 2010 at 5:18 PM, Christian Couder >> wrote: >>> Hi, >>> >>> On Mon, Nov 29, 2010 at 3:47 PM, Giuseppe Bilotta >>> 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