bug#75052: 31; browse-url-transform-alist is not used by secondary browser

[Daniel Mendler via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>]

Stefan Kangas <stefankangas@gmail.com> writes:

> Daniel Mendler <mail@daniel-mendler.de> writes:
>
>> Hello Stefan,
>>
>> you recently added `browse-url-transform-alist' to `browse-url'.  At a
>> few places in Emacs browser functions are called directly. For direct
>> browser function calls the transformation alist is not applied.
>>
>> Examples:
>>
>> - `browse-url-secondary-browser-function' is called by
>>   `browse-url-button-open' or by `package-browse-url' and others.
>> - `browse-url-with-browser-kind' calls browser functions directly.
>
> Thanks for identifying this issue, and for the patch!
> Sorry for the delay in reviewing it.

Thanks for taking a look. The problem goes a little bit further even.
`browse-url' contains some setup code (`setenv' etc), which is also
relevant when invoking secondary browser functions.

> I find the calls here rather unintuitive, for example I see calls like:
>
>     (browse-url-with-function current-prefix-arg url)
>     (browse-url-with-function secondary url)
>
> The `current-prefix-arg' and `secondary' are not really functions, so
> when I first read this code, I jumped at it and had to consult the
> docstring.

Maybe it would be more clear if the function is renamed to
`browse-url-via`? Then the VIA argument could either be a function or a
flag.

(browse-url-via some-function url)
(browse-url-via secondary url)

The goal here is also to make it simpler for the caller to select
browsers via a prefix argument, which happens at a few places.

> I believe that the API would be simpler if we had two functions instead:
>
>     (browse-url-with-function func url &rest args)
>     (browse-url-with-secondary-browser url &rest args)

This would work too but would impose slightly more work on the callers,
since they would then decide if they want to call
`browse-url-with-secondary-browser' or `browse-url'.

> We could also consider this idea:
>
>> An alternative could be to add keyword arguments to `browse-url' for
>> consolidation:
>>
>> For example:
>> (browse-url url :kind 'external)    ;; replaces `browse-url-with-browser-kind'
>> (browse-url url :via some-function) ;; alternative to `browse-url-with-function'
>> (browse-url url :via secondary)
>
> First, it seems like `browse-url-with-browser-kind' is intended as a
> user-facing command, so replacing it with a keyword argument to
> browse-url won't work.

Yes, indeed.

> As for the rest, I'd rather use something more direct and boring,
> because I find :via to be unclear.  Here's what I'd do:
>
>     (browse-url :function function)
>     (browse-url :secondary t)

Okay. I find the :via quite readable, also the `browse-url-via`
suggested above since it just follows the language flow. This is a
matter of taste. I am not sure if we necessarily want to touch the
`browse-url' calling convention.

Another perhaps simpler and significantly more minimal approach would be
to let-bind `browse-url-browser-function' to
`browse-url-secondary-browser-function' at all call-sites instead of
calling `browse-url-secondary-browser-function' directly. Inside the
let, `browse-url' would be called as usual.

However the problem with that approach is that it wouldn't be entirely
backward compatible, since the `browse-url-handlers' take precedence
over the `browse-url-browser-function'. In order to circumvent this
problem we could introduce a new variable
`browse-url-override-browser-function' which would take precedence in
`browse-url'. `package-browse-url' would become the following:

#+begin_src emacs-lisp
(defun package-browse-url (desc &optional secondary)
  (interactive (list (package--query-desc) current-prefix-arg)
               package-menu-mode)
  (unless desc
    (user-error "No package here"))
  (let ((url (cdr (assoc :url (package-desc-extras desc))))
        (browse-url-override-browser-function
         (and secondary browse-url-secondary-browser-function)))
    (unless url
      (user-error "No website for %s" (package-desc-name desc)))
    (browse-url url)))
#+end_src

However this small incompatibility may even be acceptable. In case URL
handlers are defined, you may want them to take precedence in any case.
I believe the problem is only pronounced in
`browse-url-with-browser-kind' which under all circumstances wants to
use the browser function it has chosen. In order to achieve that it
could let-bind `browse-url-handlers' and `browse-url-default-handlers'
to nil. The function would become this:

#+begin_src emacs-lisp
(defun browse-url-with-browser-kind (kind url &optional arg)
  (interactive ...)
  (let ((function (browse-url-select-handler url kind)))
    (unless function
      (setq function
            (seq-find
             (lambda (fun)
               (eq kind (browse-url--browser-kind fun url)))
             (list browse-url-browser-function
                   browse-url-secondary-browser-function
                   #'browse-url-default-browser
                   #'eww))))
    ;; Ensure that function is used
    (let ((browse-url-browser-function function)
          (browse-url-default-handlers nil)
          (browse-url-handlers))
      (browse-url url arg))))
#+end_src

Daniel