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

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]

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

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]

[-- Attachment #1: Type: text/plain, Size: 512 bytes --]

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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-New-function-browse-url-with-function.patch --]
[-- Type: text/x-diff, Size: 10147 bytes --]

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.

* lisp/net/browse-url.el (browse-url--transform): New function,
extracted from `browse-url'.
(browse-url): Use it.
(browse-url-with-function): New function.
(browse-url-with-browser-kind, browse-url-button-open)
(browse-url-button-open-url):
* lisp/net/shr.el (shr-browse-url):
* lisp/net/eww.el (eww-browse-with-external-browser):
* lisp/gnus/gnus-sum.el (gnus-summary-browse-url):
* lisp/emacs-lisp/package.el (package-browse-url): Use it.
---
 lisp/emacs-lisp/package.el |  4 +--
 lisp/gnus/gnus-sum.el      | 10 +++-----
 lisp/net/browse-url.el     | 50 ++++++++++++++++++++++++--------------
 lisp/net/eww.el            |  2 +-
 lisp/net/goto-addr.el      |  6 ++---
 lisp/net/shr.el            | 18 ++++++--------
 6 files changed, 49 insertions(+), 41 deletions(-)

diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index 5f785071ea3..ebd56c75677 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -4689,9 +4689,7 @@ package-browse-url
   (let ((url (cdr (assoc :url (package-desc-extras desc)))))
     (unless url
       (user-error "No website for %s" (package-desc-name desc)))
-    (if secondary
-        (funcall browse-url-secondary-browser-function url)
-      (browse-url url))))
+    (browse-url-with-function secondary url)))
 
 (declare-function ietf-drums-parse-address "ietf-drums"
                   (string &optional decode))
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
@@ -9429,9 +9429,7 @@ gnus-summary-browse-url
 				 (gnus-shorten-url (car urls) 40))
 		  urls nil t nil nil (car urls))))))
     (if target
-	(if external
-	    (funcall browse-url-secondary-browser-function target)
-	  (browse-url target))
+	(browse-url-with-function secondary target)
       (message "No URLs found."))))
 
 (defun gnus-summary-isearch-article (&optional regexp-p)
diff --git a/lisp/net/browse-url.el b/lisp/net/browse-url.el
index 9dd990108df..acc61162e15 100644
--- a/lisp/net/browse-url.el
+++ b/lisp/net/browse-url.el
@@ -888,6 +888,18 @@ browse-url-of-region
 
 ;; A generic command to call the current browse-url-browser-function
 
+(defun browse-url--transform (url)
+  "Transform URL according to `browse-url-transform-alist'."
+  (when browse-url-transform-alist
+    (dolist (trans browse-url-transform-alist)
+      (when (string-match (car trans) url)
+        (setq url (replace-match (cdr trans) nil t url)))))
+  (when (and url-handler-mode
+             (not (file-name-absolute-p url))
+             (not (string-match "\\`[a-z]+:" url)))
+    (setq url (expand-file-name url)))
+  url)
+
 (declare-function pgtk-backend-display-class "pgtkfns.c" (&optional terminal))
 
 ;;;###autoload
@@ -916,14 +928,7 @@ browse-url
   (interactive (browse-url-interactive-arg "URL: "))
   (unless (called-interactively-p 'interactive)
     (setq args (or args (list browse-url-new-window-flag))))
-  (when browse-url-transform-alist
-    (dolist (trans browse-url-transform-alist)
-      (when (string-match (car trans) url)
-        (setq url (replace-match (cdr trans) nil t url)))))
-  (when (and url-handler-mode
-             (not (file-name-absolute-p url))
-             (not (string-match "\\`[a-z]+:" url)))
-    (setq url (expand-file-name url)))
+  (setq url (browse-url--transform url))
   (let ((process-environment (copy-sequence process-environment))
 	(function (or (browse-url-select-handler url)
                       browse-url-browser-function))
@@ -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)))
+
 ;;;###autoload
 (defun browse-url-at-point (&optional arg)
   "Open URL at point using a configurable method.
@@ -1008,7 +1026,7 @@ browse-url-with-browser-kind
                    browse-url-secondary-browser-function
                    #'browse-url-default-browser
                    #'eww))))
-    (funcall function url arg)))
+    (browse-url-with-function function url arg)))
 
 ;;;###autoload
 (defun browse-url-at-mouse (event)
@@ -1815,27 +1833,23 @@ browse-url-add-buttons
                                          browse-url-data ,(match-string 0)))))))
 
 ;;;###autoload
-(defun browse-url-button-open (&optional external mouse-event)
+(defun browse-url-button-open (&optional secondary mouse-event)
   "Follow the link under point using `browse-url'.
-If EXTERNAL (the prefix if used interactively), open with the
-external browser instead of the default one."
+If SECONDARY (the prefix if used interactively), open with the
+secondary browser instead of the default one."
   (interactive (list current-prefix-arg last-nonmenu-event))
   (mouse-set-point mouse-event)
   (let ((url (get-text-property (point) 'browse-url-data)))
     (unless url
       (error "No URL under point"))
-    (if external
-        (funcall browse-url-secondary-browser-function url)
-      (browse-url url))))
+    (browse-url-with-function secondary url)))
 
 ;;;###autoload
 (defun browse-url-button-open-url (url)
   "Open URL using `browse-url'.
 If `current-prefix-arg' is non-nil, use
 `browse-url-secondary-browser-function' instead."
-  (if current-prefix-arg
-      (funcall browse-url-secondary-browser-function url)
-    (browse-url url)))
+  (browse-url-with-function current-prefix-arg url))
 
 (defun browse-url-button-copy ()
   "Copy the URL under point."
diff --git a/lisp/net/eww.el b/lisp/net/eww.el
index 842db3f27f4..a40232d977b 100644
--- a/lisp/net/eww.el
+++ b/lisp/net/eww.el
@@ -2166,7 +2166,7 @@ eww-browse-with-external-browser
   (setq url (or url (plist-get eww-data :url)))
   (if (eq 'external (browse-url--browser-kind
                      browse-url-secondary-browser-function url))
-      (funcall browse-url-secondary-browser-function url)
+      (browse-url-with-function browse-url-secondary-browser-function url)
     (browse-url-with-browser-kind 'external url)))
 
 (defun eww-remove-tracking (url)
diff --git a/lisp/net/goto-addr.el b/lisp/net/goto-addr.el
index 3e5bc1ec090..59d6a01f585 100644
--- a/lisp/net/goto-addr.el
+++ b/lisp/net/goto-addr.el
@@ -227,9 +227,9 @@ goto-address-at-point
 Compose message to address at point.  See documentation for
 `goto-address-find-address-at-point'.
 
-If no e-mail address is found at point, open the URL at or before
-point using `browse-url'.  With a prefix argument, open the URL
-using `browse-url-secondary-browser-function' instead."
+If no e-mail address is found at point, open the URL at or before point
+using `browse-url-button-open-url'.  With a prefix argument the
+secondary browser is used."
   (interactive (list last-input-event))
   (save-excursion
     (if event (posn-set-point (event-end event)))
diff --git a/lisp/net/shr.el b/lisp/net/shr.el
index 6d8b235a2b8..da77bbf75a8 100644
--- a/lisp/net/shr.el
+++ b/lisp/net/shr.el
@@ -1097,24 +1097,22 @@ shr-mouse-browse-url-new-window
   (mouse-set-point ev)
   (shr-browse-url nil nil t))
 
-(defun shr-browse-url (&optional external mouse-event new-window)
+(defun shr-browse-url (&optional secondary mouse-event new-window)
   "Browse the URL at point using `browse-url'.
-If EXTERNAL is non-nil (interactively, the prefix argument), browse
-the URL using `browse-url-secondary-browser-function'.
+If SECONDARY is non-nil (interactively, the prefix argument), browse
+the URL with the secondary browser.
 If this function is invoked by a mouse click, it will browse the URL
 at the position of the click.  Optional argument MOUSE-EVENT describes
 the mouse click event."
   (interactive (list current-prefix-arg last-nonmenu-event))
   (mouse-set-point mouse-event)
   (let ((url (get-text-property (point) 'shr-url)))
-    (cond
-     ((not url)
+    (if (not url)
       (message "No link under point"))
-     (external
-      (funcall browse-url-secondary-browser-function url)
-      (shr--blink-link))
-     (t
-      (browse-url url (xor new-window browse-url-new-window-flag))))))
+    (browse-url-with-function secondary url
+                              (xor new-window browse-url-new-window-flag))
+    (when secondary
+      (shr--blink-link))))
 
 (defun shr-save-contents (directory)
   "Save the contents from URL in a file."
-- 
2.45.2

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

[Stefan Kangas]

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?

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]

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

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

[Stefan Kangas]

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?

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]

Stefan Kangas <stefankangas@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

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]

[-- Attachment #1: Type: text/plain, Size: 159 bytes --]

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

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

Updated patch attached to this mail.

> Daniel


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Call-browser-functions-via-browse-url.patch --]
[-- Type: text/x-diff, Size: 6839 bytes --]

From a859519c83e73b9a61795c2c9200e1b853136b9d 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] Call browser functions via `browse-url'

`browse-url' applies `browse-url-transform-alist'
transformations to the URL argument and initializes appropriate
environment variables.  Therefore always call browser functions
via `browse-url' and not directly, by let-binding
`browse-url-browser-function'.

* lisp/net/browse-url.el (browse-url-with-browser-kind)
(browse-url-button-open, browse-url-button-open-url):
* lisp/net/shr.el (shr-browse-url):
* lisp/net/eww.el (eww-browse-with-external-browser):
* lisp/gnus/gnus-sum.el (gnus-summary-browse-url):
* lisp/emacs-lisp/package.el (package-browse-url): Let-bind
`browse-url-browser-function' and call `browse-url'.
---
 lisp/emacs-lisp/package.el |  6 ++++--
 lisp/gnus/gnus-sum.el      | 10 ++++++----
 lisp/net/browse-url.el     | 23 +++++++++++++++--------
 lisp/net/eww.el            |  3 ++-
 lisp/net/shr.el            |  9 +++++----
 5 files changed, 32 insertions(+), 19 deletions(-)

diff --git a/lisp/emacs-lisp/package.el b/lisp/emacs-lisp/package.el
index b4a33db1a77..6a04108b9f8 100644
--- a/lisp/emacs-lisp/package.el
+++ b/lisp/emacs-lisp/package.el
@@ -4692,8 +4692,10 @@ package-browse-url
   (let ((url (cdr (assoc :url (package-desc-extras desc)))))
     (unless url
       (user-error "No website for %s" (package-desc-name desc)))
-    (if secondary
-        (funcall browse-url-secondary-browser-function url)
+    (let ((browse-url-browser-function
+           (if secondary
+               browse-url-secondary-browser-function
+             browse-url-browser-function)))
       (browse-url url))))
 
 (declare-function ietf-drums-parse-address "ietf-drums"
diff --git a/lisp/gnus/gnus-sum.el b/lisp/gnus/gnus-sum.el
index c909d9cfd5c..06367b7293e 100644
--- a/lisp/gnus/gnus-sum.el
+++ b/lisp/gnus/gnus-sum.el
@@ -9408,7 +9408,7 @@ 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
@@ -9429,9 +9429,11 @@ gnus-summary-browse-url
 				 (gnus-shorten-url (car urls) 40))
 		  urls nil t nil nil (car urls))))))
     (if target
-	(if external
-	    (funcall browse-url-secondary-browser-function target)
-	  (browse-url target))
+        (let ((browse-url-browser-function
+               (if secondary
+                   browse-url-secondary-browser-function
+                 browse-url-browser-function)))
+          (browse-url target))
       (message "No URLs found."))))
 
 (defun gnus-summary-isearch-article (&optional regexp-p)
diff --git a/lisp/net/browse-url.el b/lisp/net/browse-url.el
index b9610fec150..e67c2fd79da 100644
--- a/lisp/net/browse-url.el
+++ b/lisp/net/browse-url.el
@@ -999,7 +999,10 @@ browse-url-with-browser-kind
                    browse-url-secondary-browser-function
                    #'browse-url-default-browser
                    #'eww))))
-    (funcall function url arg)))
+    (let ((browse-url-browser-function function)
+          (browse-url-handlers nil)
+          (browse-url-default-handlers nil))
+      (browse-url url))))
 
 ;;;###autoload
 (defun browse-url-at-mouse (event)
@@ -1786,17 +1789,19 @@ browse-url-add-buttons
                                          browse-url-data ,(match-string 0)))))))
 
 ;;;###autoload
-(defun browse-url-button-open (&optional external mouse-event)
+(defun browse-url-button-open (&optional secondary mouse-event)
   "Follow the link under point using `browse-url'.
-If EXTERNAL (the prefix if used interactively), open with the
-external browser instead of the default one."
+If SECONDARY (the prefix if used interactively), open with the
+secondary browser instead of the default one."
   (interactive (list current-prefix-arg last-nonmenu-event))
   (mouse-set-point mouse-event)
   (let ((url (get-text-property (point) 'browse-url-data)))
     (unless url
       (error "No URL under point"))
-    (if external
-        (funcall browse-url-secondary-browser-function url)
+    (let ((browse-url-browser-function
+           (if secondary
+               browse-url-secondary-browser-function
+             browse-url-browser-function)))
       (browse-url url))))
 
 ;;;###autoload
@@ -1804,8 +1809,10 @@ browse-url-button-open-url
   "Open URL using `browse-url'.
 If `current-prefix-arg' is non-nil, use
 `browse-url-secondary-browser-function' instead."
-  (if current-prefix-arg
-      (funcall browse-url-secondary-browser-function url)
+  (let ((browse-url-browser-function
+         (if current-prefix-arg
+             browse-url-secondary-browser-function
+           browse-url-browser-function)))
     (browse-url url)))
 
 (defun browse-url-button-copy ()
diff --git a/lisp/net/eww.el b/lisp/net/eww.el
index 95513a67acd..df9e5ad88a7 100644
--- a/lisp/net/eww.el
+++ b/lisp/net/eww.el
@@ -2166,7 +2166,8 @@ eww-browse-with-external-browser
   (setq url (or url (plist-get eww-data :url)))
   (if (eq 'external (browse-url--browser-kind
                      browse-url-secondary-browser-function url))
-      (funcall browse-url-secondary-browser-function url)
+      (let ((browse-url-browser-function browse-url-secondary-browser-function))
+        (browse-url url))
     (browse-url-with-browser-kind 'external url)))
 
 (defun eww-remove-tracking (url)
diff --git a/lisp/net/shr.el b/lisp/net/shr.el
index 24c779524dc..c2d01bd2e0d 100644
--- a/lisp/net/shr.el
+++ b/lisp/net/shr.el
@@ -1097,9 +1097,9 @@ shr-mouse-browse-url-new-window
   (mouse-set-point ev)
   (shr-browse-url nil nil t))
 
-(defun shr-browse-url (&optional external mouse-event new-window)
+(defun shr-browse-url (&optional secondary mouse-event new-window)
   "Browse the URL at point using `browse-url'.
-If EXTERNAL is non-nil (interactively, the prefix argument), browse
+If SECONDARY is non-nil (interactively, the prefix argument), browse
 the URL using `browse-url-secondary-browser-function'.
 If this function is invoked by a mouse click, it will browse the URL
 at the position of the click.  Optional argument MOUSE-EVENT describes
@@ -1110,8 +1110,9 @@ shr-browse-url
     (cond
      ((not url)
       (message "No link under point"))
-     (external
-      (funcall browse-url-secondary-browser-function url)
+     (secondary
+      (let ((browse-url-browser-function browse-url-secondary-browser-function))
+        (browse-url url))
       (shr--blink-link))
      (t
       (browse-url url (xor new-window browse-url-new-window-flag))))))
-- 
2.45.2