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

Previous Next

To reply to this bug, email your comments to 75052 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Daniel Mendler <mail <at> daniel-mendler.de>
To: bug-gnu-emacs <at> gnu.org
Subject: 31; browse-url-transform-alist is not used by secondary browser
Date: Mon, 23 Dec 2024 19:41:30 +0100

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.

A possible solution could be to add a function
`browse-url-with-function' which takes a function argument:

(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 default `browse-url'.
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)))

All call sites which call a browse function directly could use
`browse-url-with-function' instead. Calling browser functions directly
should be discouraged. For example `package-browse-url' would simplify
to this:

(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)))))
    (unless url
      (user-error "No website for %s" (package-desc-name desc)))
    (browse-url-with-function secondary url)))

Similarly `browse-url-with-browser-kind':

(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))))
    (browse-url-with-function function url arg)))

Does this sound like an acceptable plan? Thank you.

Daniel

Message #8 received at 75052 <at> debbugs.gnu.org (full text, mbox):

From: Daniel Mendler <mail <at> daniel-mendler.de>
To: 75052 <at> debbugs.gnu.org
Cc: Stefan Kangas <stefankangas <at> gmail.com>
Subject: Re: bug#75052: 31; browse-url-transform-alist is not used by
 secondary browser
Date: Tue, 24 Dec 2024 02:00:04 +0100

[Message part 1 (text/plain, inline)]

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.

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)

Daniel

[0001-New-function-browse-url-with-function.patch (text/x-diff, attachment)]

Message #11 received at 75052 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Daniel Mendler <mail <at> daniel-mendler.de>, 75052 <at> debbugs.gnu.org
Subject: Re: bug#75052: 31; browse-url-transform-alist is not used by
 secondary browser
Date: Fri, 3 Jan 2025 10:18:09 -0600

Daniel Mendler <mail <at> 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 <at> 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?

Message #14 received at 75052 <at> debbugs.gnu.org (full text, mbox):

From: Daniel Mendler <mail <at> daniel-mendler.de>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: 75052 <at> debbugs.gnu.org
Subject: Re: bug#75052: 31; browse-url-transform-alist is not used by
 secondary browser
Date: Fri, 03 Jan 2025 17:47:03 +0100

Stefan Kangas <stefankangas <at> gmail.com> writes:

> Daniel Mendler <mail <at> 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

Message #17 received at 75052 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Kangas <stefankangas <at> gmail.com>
To: Daniel Mendler <mail <at> daniel-mendler.de>
Cc: 75052 <at> debbugs.gnu.org
Subject: Re: bug#75052: 31; browse-url-transform-alist is not used by
 secondary browser
Date: Fri, 3 Jan 2025 20:55:56 -0600

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

> Stefan Kangas <stefankangas <at> 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?

Message #20 received at 75052 <at> debbugs.gnu.org (full text, mbox):

From: Daniel Mendler <mail <at> daniel-mendler.de>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: 75052 <at> debbugs.gnu.org
Subject: Re: bug#75052: 31; browse-url-transform-alist is not used by
 secondary browser
Date: Sat, 04 Jan 2025 12:17:59 +0100

Stefan Kangas <stefankangas <at> gmail.com> writes:

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

Yes, it is best if all calls just go through `browse-url'. Also we avoid
adding more functions such that the API isn't made more complex.

Just to be clear - I think we don't actually need the override variable
if we accept a minor change in behavior with respect to the handlers. If
a specific handler is defined we may always want it to take precedence,
except in the case of `browse-url-with-browser-kind', but there we can
simply rebind the handler variables to nil.

> Could you perhaps send a patch?

Yes, I'll send a patch and then you can take another look.

Daniel

Message #23 received at 75052 <at> debbugs.gnu.org (full text, mbox):

From: Daniel Mendler <mail <at> daniel-mendler.de>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: 75052 <at> debbugs.gnu.org
Subject: Re: bug#75052: 31; browse-url-transform-alist is not used by
 secondary browser
Date: Sat, 04 Jan 2025 12:59:00 +0100

[Message part 1 (text/plain, inline)]

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

> Yes, I'll send a patch and then you can take another look.

Updated patch attached to this mail.

> Daniel

[0001-Call-browser-functions-via-browse-url.patch (text/x-diff, attachment)]

Message #26 received at 75052 <at> debbugs.gnu.org (full text, mbox):

From: Daniel Mendler <mail <at> daniel-mendler.de>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: 75052 <at> debbugs.gnu.org
Subject: Re: bug#75052: 31; browse-url-transform-alist is not used by
 secondary browser
Date: Sun, 26 Jan 2025 10:14:03 +0100

> Updated patch attached to this mail.

Stefan, did you have a chance to look at the last patch? I only let-bind
the browser function around the call sites, to avoid an intrusive
change. Thanks!

Daniel

Message #29 received at 75052 <at> debbugs.gnu.org (full text, mbox):

From: Daniel Mendler <mail <at> daniel-mendler.de>
To: Stefan Kangas <stefankangas <at> gmail.com>
Cc: 75052 <at> debbugs.gnu.org
Subject: Re: bug#75052: 31; browse-url-transform-alist is not used by
 secondary browser
Date: Thu, 13 Mar 2025 18:37:52 +0100

[Message part 1 (text/plain, inline)]

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

>> Updated patch attached to this mail.
>
> Stefan, did you have a chance to look at the last patch? I only let-bind
> the browser function around the call sites, to avoid an intrusive
> change. Thanks!

Ping. Stefan, how do you plan to proceed with `browse-url'? I attached
the latest version of the patch.

Daniel

[0001-Call-browser-functions-via-browse-url.patch (text/x-diff, attachment)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.