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

[Stefan Kangas <stefankangas@gmail.com>]

Daniel Mendler <mail@daniel-mendler.de> writes:

> Stefan Kangas <stefankangas@gmail.com> writes:
>
>> 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.

OK, let's look into it.

> 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)

That would be an improvement, I think.

>> 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'.

If code is read more often than it is written, this is an acceptable
tradeoff.  But see below.

> 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

I quite like this idea, and I think it buys us some improvements even,
as you detail above, while the compatibility issues are fixed with the
new variable.

This is the only idea so far that guarantees that the environment
variables, etc., that we set now and in future in `browse-url`, are used
also for the secondary browser.  IIUC, it would also give us exactly one
function that Lisp programs should normally call: `browse-url`.

Could you perhaps send a patch?