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". > Since > it would need special-casing anyway (e.g. chromium-browser -> > chromium), I opted out for the --version way so that (1) we could > catch one of our friendly cases even if the binary was called > something else and (2) some invocation paths try to get the version > anyway, so we could do it once and for all. > >> And are you sure that when they support --version, the first word of >> the output is >> better than "$(readlink $i)"? > > Probably depends on the metric used for 'better' ;-) > >> What if we add support for a new one? > > Then when adding it we should also look at its --version output and > see if it needs special treatment. > >>> + case "$browser" in >>> + mozilla) >>> + browser="$(echo "$verstring" | cut -f2 -d' ' | tr A-Z a-z)" >>> + ;; >>> + google) >>> + browser="google-chrome" >>> + ;; >>> + esac >>> + if valid_tool "$browser" ; then >> >> valid_tool "$browser" is called once here... >> >>> -else >>> +else if test -z "$browser_path"; then >>> valid_tool "$browser" || die "Unknown browser '$browser'." >> >> ...valid_tool "$browser" is called again here if it failed above, and >> here we die, >> so isn't it clearer to die as soon as we call it and it fails? > > 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. > 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. Thanks, Christian. -- 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