Re: Your last change to browse-url is bogus.

[YAMAMOTO Mitsuharu]

Your point of view is low-level (string operation), and mine is
high-level (URL operation).  The problems is that the current
implementation of browse-url-encode-url is neither of them: it looks
like a result of superficial code factoring.

(defun browse-url-encode-url (url &optional filename-p)
  "Encode all `confusing' characters in URL.
If FILENAME-P is nil, the confusing characters are [,)$].
Otherwise, the confusing characters are [*\"()',=;?% ]."
  (let ((conf-char (if filename-p "[*\"()',=;?% ]" "[,)$]"))
        (encoded-url (copy-sequence url))
        (s 0))
    (while (setq s (string-match conf-char encoded-url s))
      (setq encoded-url
            (replace-match (format "%%%x"
                                   (string-to-char (match-string 0 
encoded-url)))
                           t t encoded-url)
            s (1+ s)))
    encoded-url))

It is inappropriate from a high-level view because it is a mixture of
two semantically different operations.  Also inappropriate from a
low-level view because it is not general enough and the function name
doesn't represent what it does.

>>>>> On Thu, 13 Sep 2007 00:50:59 +0200, address@hidden (Michaël Cadilhac) 
>>>>> said:

>>> What would you do?
>> 
>> Maybe I would revert browse-url-file-url to the one that doesn't
>> use browse-url-encode-url [...].  Also I would use more explicit
>> and specific name in place of browse-url-encode-url (e.g.,
>> browse-url-escape-confusing-characters).

>> An alternative way would be, as you suggested, to give characters
>> to be escaped as an argument to browse-url-encode-url, but rename
>> the function so it looks like a low-level string operation.

> Well, for now on I'm not sure which one of the possibilities is the
> good one (and I'm getting sleepy :)).  If someone has a strong
> feeling about that around here, it'd be nice to hear him!

I propose the combination of above two, which is similar to what Davis
suggested:

  1. Add a low-level string replacing function (say,
     browse-url-encode-chars-in-string as you mentioned) that takes an
     argument representing the characters to be escaped.

  2. In browse-url-file-url, directly call the above low-level
     function instead of calling browse-url-encode-url.

  3. Change browse-url-encode-url so it calls the the low-level
     function introduced in 1, and just do one task (URL -> URL).

The difference between direct and indirect calls to the low-level
function comes from whether it is meaningful as a self-contained task
or not.  Escaping confusing characters in a URL deserves a specialized
function, but escaping characters in a filename string is just a part
of the conversion from the filename to a URL.

                                     YAMAMOTO Mitsuharu
                                address@hidden