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:

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

> I've attached a patch which implements a `browse-url-with-function'.
> However I am not sure if we want to add more `browse-url-*' function
> variants, since this will make the `browse-url' API increasingly
> unclear.

There is definitely room to simplify the browse-url API, but let's
separate the discussion about the Lisp programmer API from the user
facing commands.  I think the latter would be important to simplify, but
in my view there is no catastrophe if we were to add a few more
functions for Lisp programmers.

> From 0b7edfc61aace7c692c04f1d0f7a6b6eb987b657 Mon Sep 17 00:00:00 2001
> From: Daniel Mendler <mail@daniel-mendler.de>
> Date: Tue, 24 Dec 2024 01:31:35 +0100
> Subject: [PATCH] New function browse-url-with-function
>
> `browse-url-with-function' takes a browser function as argument,
> the URL and rest arguments, which are passed to the browser
> function.  `browse-url-with-function' transforms the URL first
> with `browse-url-transform-alist' before calling the browser
> function.
>
> Calling browser functions directly is discouraged.  Instead
> `browse-url' or `browse-url-with-function' should be used, such
> that URL transformations are applied.

This last paragraph is fine, but should probably be moved to a code
comment instead.  Maybe it should be in Commentary?

> diff --git a/lisp/gnus/gnus-sum.el b/lisp/gnus/gnus-sum.el
> index cebeb6d4c37..931952a2885 100644
> --- a/lisp/gnus/gnus-sum.el
> +++ b/lisp/gnus/gnus-sum.el
> @@ -9408,11 +9408,11 @@ gnus-shorten-url
>                             (concat "#" target)))))
>        (concat host (string-truncate-left rest (- max (length host)))))))
>
> -(defun gnus-summary-browse-url (&optional external)
> +(defun gnus-summary-browse-url (&optional secondary)
>    "Scan the current article body for links, and offer to browse them.
>
> -Links are opened using `browse-url' unless a prefix argument is
> -given: then `browse-url-secondary-browser-function' is used instead.
> +Links are opened using the primary browser unless a prefix argument is
> +given. Then the secondary browser is used instead.
>  If only one link is found, browse that directly, otherwise use
>  completion to select a link.  The first link marked in the

I'm not sure this is an improvement, because this hides away the fact
that you can customize which browser is being used.

> @@ -957,6 +962,19 @@ browse-url
>          (apply function url args)
>        (error "No suitable browser for URL %s" url))))
>
> +;;;###autoload
> +(defun browse-url-with-function (func url &rest args)
> +  "Open URL with browser FUNC.
> +If FUNC is a function use this function.
> +If FUNC is nil use the `browse-url', which either calls a handler or the
> +primary `browse-url-browser-function'.
> +For other non-nil values use `browse-url-secondary-browser-function'."
> +  (if (not func)
> +      (apply #'browse-url url args)
> +    (apply
> +     (if (functionp func) func browse-url-secondary-browser-function)
> +     (browse-url--transform url) args)))

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.

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 is not something you would jump at, but quite self-explanatory.

I understand that you don't want to add more to an already messy API,
but I think we should do more to simplify it elsewhere instead, which
would offset the cost of doing that to some extent.  Avoiding functions
at the cost of more complex function interfaces doesn't strike me as a
good tradeoff.

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.

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)

Maybe this option is better than adding the new functions I proposed
above?