Re: [PATCHv3 5/6] web--browse: use *www-browser if available

From: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
Date: Dec 8, 2010, 07:38 UTC
Message-ID: <AANLkTimP+yLLH+arKeg-VHZ4OH5_JgKmdv90j3wGTVUk@mail.gmail.com>
In-Reply-To: <7vaakhnok7.fsf@alter.siamese.dyndns.org>

On Wed, Dec 8, 2010 at 12:36 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 14 quoted lines

> Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:
>
>> +     # if the linked executable doesn't match a browser name we know about,
>> +     # look at the version string
>> +
>> +     # even though most browsers (and applications, in fact) will show their
>> +     # name and version on the first line of the --version output, this is
>> +     # not true in particular for the KDE apps (e.g. konqueror and kfmclient),
>> +     # which display their name and version on the LAST line. So we cannot
>> +     # clip the version string at the first line when retrieving it. Rather,
>> +     # we keep it whole and then limit it when we know what we're dealing with.
>
> I'd be more worried about the ones that do not understand --version and
> spawn a new window.

That's indeed a potential issue (although I _do_ wonder if there are browsers that act like that; the only one I couldn't try, on Linux, was dillo).

Show 6 quoted lines

>> +     verstring="$("$testexe" --version 2> /dev/null)"
>> +     browser="$(echo "$verstring" | head -n 1 | cut -f1 -d' ' | tr A-Z a-z)"
>> +     case "$browser" in
>> +             mozilla)
>
> What was the first patch in this series about again ;-)?

Oh my! LOL. Ok, I can fix that 8-D

Show 29 quoted lines

>> +                     verstring="$(echo "$verstring" | head -n 1)"
>> +                     browser="$(echo "$verstring" | cut -f2 -d' ' | tr A-Z a-z)"
>> +                     ;;
>> +             google)
>> +                     verstring="$(echo "$verstring" | head -n 1)"
>> +                     browser="google-chrome"
>> +                     ;;
>> +             qt:)
>> +                     # konqueror, kfmclient or other KDE app
>> +                     verstring="$(echo "$verstring" | tail -n 1)"
>> +                     browser="$(echo "$verstring" | cut -f1 -d:)"
>> +                     ;;
>> +             *)
>> +                     verstring="$(echo "$verstring" | head -n 1)"
>> +                     ;;
>> +
>> +     esac
>> +     if valid_tool "$browser" ; then
>> +             browser_path="$i"
>> +             return 0
>> +     fi
>> +
>> +     echo >&2 "$testexe (detected as $browser) is not a supported browser, skipping"
>> +     browser=""
>> +     return 1
>> +}
>
> Sorry, but I simply do not think it is worth this ugliness to get slight
> customization between -new-tab, newTab, and nothingness.

I personally don't have a particular feeling for forcing the opening in a new tab (and I might have over-engineered it, although it's not really much more complex than the previous versions of this patchset, it's just refactored.

If this requirement is relaxed, the patch becomes much simpler, and we can include direct support for xdg-open, sensible-browser, htlmview & so forth.

-- 
Giuseppe "Oblomov" Bilotta