bug#74730: [PATCH] 30.0.92; eww-browse-with-external-browser and eww-follow-link should use browse-url-with-browser-kind

bug#74730: [PATCH] 30.0.92; eww-browse-with-external-browser and eww-follow-link should use browse-url-with-browser-kind

[Daniel Mendler via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Tags: patch

The command `eww-browse-with-external-browser' uses
`browse-url-secondary-browser-function'. This makes the command
ineffective in the common setup where
`browse-url-secondary-browser-function' is set to `eww-browse-url' and
`browse-url-browser-function' is set to an external browser.

Fortunately we can use `browse-url-with-browser-kind' instead, which
guarantees that an external browser is launched.

Furthermore `eww-follow-link' should use `browse-url-with-browser-kind'
if the EXTERNAL prefix argument is non-nil.

I've looked at all other uses of `browse-url-browser-function' and
`browse-url-secondary-browser-function' in the Emacs code base and I
have not found other problems. Most commands use a prefix argument to
switch to the secondary browser, relying on the primary browser by
default. The problem is limited to `eww-browse-with-external-browser'
and `eww-follow-link'.

In GNU Emacs 30.0.92 (build 2, x86_64-pc-linux-gnu, X toolkit, cairo
 version 1.18.2, Xaw scroll bars) of 2024-11-20


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-eww-Use-browse-url-browser-kind-for-external-browsin.patch --]
[-- Type: text/patch, Size: 2280 bytes --]

From 361b9d970631b3547bd859fb8686597e651447ce Mon Sep 17 00:00:00 2001
From: Daniel Mendler <mail@daniel-mendler.de>
Date: Sat, 7 Dec 2024 22:56:15 +0100
Subject: [PATCH] eww: Use browse-url-browser-kind for external browsing

Guarantee that an external browser is used by Eww even if
`browse-url-secondary-browser-function' is set to `eww-browse-url'.

* lisp/net/eww.el (eww-browse-with-external-browser): Use
`browse-url-with-browser-kind'.
(eww-follow-link): Use `browse-url-with-browser-kind' if the
EXTERNAL prefix argument is non-nil.
---
 lisp/net/eww.el | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/lisp/net/eww.el b/lisp/net/eww.el
index 4d4d4d6beac..bace519c7c4 100644
--- a/lisp/net/eww.el
+++ b/lisp/net/eww.el
@@ -2141,12 +2141,11 @@ eww-submit
 	(mm-url-encode-www-form-urlencoded values))))))
 
 (defun eww-browse-with-external-browser (&optional url)
-  "Browse the current URL with an external browser.
-The browser to used is specified by the
-`browse-url-secondary-browser-function' variable."
+  "Browse the current URL with an external browser."
   (interactive nil eww-mode)
-  (funcall browse-url-secondary-browser-function
-           (or url (plist-get eww-data :url))))
+  (browse-url-with-browser-kind
+   'external
+   (or url (plist-get eww-data :url))))
 
 (defun eww-remove-tracking (url)
   "Remove the commong utm_ tracking cookies from URLs."
@@ -2161,9 +2160,7 @@ eww--transform-url
 
 (defun eww-follow-link (&optional external mouse-event)
   "Browse the URL under point.
-If EXTERNAL is single prefix, browse the URL using
-`browse-url-secondary-browser-function'.
-
+If EXTERNAL is single prefix, browse the URL using an external browser.
 If EXTERNAL is double prefix, browse in new buffer."
   (interactive
    (list current-prefix-arg last-nonmenu-event)
@@ -2180,7 +2177,7 @@ eww-follow-link
       ;; and `browse-url-mailto-function'.
       (browse-url url))
      ((and (consp external) (<= (car external) 4))
-      (funcall browse-url-secondary-browser-function url)
+      (browse-url-with-browser-kind 'external url)
       (shr--blink-link))
      ;; This is a #target url in the same page as the current one.
      ((and (setq target (url-target (url-generic-parse-url url)))
-- 
2.45.2

bug#74730: [PATCH] 30.0.92; eww-browse-with-external-browser and eww-follow-link should use browse-url-with-browser-kind

[Eli Zaretskii]

> Date: Sat, 07 Dec 2024 23:02:09 +0100
> From:  Daniel Mendler via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> The command `eww-browse-with-external-browser' uses
> `browse-url-secondary-browser-function'. This makes the command
> ineffective in the common setup where
> `browse-url-secondary-browser-function' is set to `eww-browse-url' and
> `browse-url-browser-function' is set to an external browser.

The doc string of browse-url-secondary-browser-function explicitly
says not to set it to eww.  So users who do the above are acting
against the design and the recommended usage, and I'm not sure we
should support that at all, let alone with a (seemingly)
backward-incompatible change such as the one you propose.

> Fortunately we can use `browse-url-with-browser-kind' instead, which
> guarantees that an external browser is launched.
> 
> Furthermore `eww-follow-link' should use `browse-url-with-browser-kind'
> if the EXTERNAL prefix argument is non-nil.
> 
> I've looked at all other uses of `browse-url-browser-function' and
> `browse-url-secondary-browser-function' in the Emacs code base and I
> have not found other problems. Most commands use a prefix argument to
> switch to the secondary browser, relying on the primary browser by
> default. The problem is limited to `eww-browse-with-external-browser'
> and `eww-follow-link'.

What will happen as result of this change to users who customize
browse-url-secondary-browser-function as its doc string says, and then
invoke the command eww-browse-with-external-browser?

bug#74730: [PATCH] 30.0.92; eww-browse-with-external-browser and eww-follow-link should use browse-url-with-browser-kind

[Daniel Mendler via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> Date: Sat, 07 Dec 2024 23:02:09 +0100
>> From:  Daniel Mendler via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>> 
>> The command `eww-browse-with-external-browser' uses
>> `browse-url-secondary-browser-function'. This makes the command
>> ineffective in the common setup where
>> `browse-url-secondary-browser-function' is set to `eww-browse-url' and
>> `browse-url-browser-function' is set to an external browser.
>
> The doc string of browse-url-secondary-browser-function explicitly
> says not to set it to eww.  So users who do the above are acting
> against the design and the recommended usage, and I'm not sure we
> should support that at all, let alone with a (seemingly)
> backward-incompatible change such as the one you propose.

How can I then use an external browser as the default and Eww as
alternative? I argue that this is a legitimate use case - an external
browser as primary and Eww as secondary browser for distraction-free
reading.

As I wrote, the only place which leads to problems is in
`eww-browse-with-external-browser' - I checked all other places in
Emacs. I argue that the behavior and implementation will be even more
explicit, since `browse-url-with-browser-kind' explicitly supports the
`external' kind. But you are right about the backward compatibility
problem, see below.

>> Fortunately we can use `browse-url-with-browser-kind' instead, which
>> guarantees that an external browser is launched.
>> 
>> Furthermore `eww-follow-link' should use `browse-url-with-browser-kind'
>> if the EXTERNAL prefix argument is non-nil.
>> 
>> I've looked at all other uses of `browse-url-browser-function' and
>> `browse-url-secondary-browser-function' in the Emacs code base and I
>> have not found other problems. Most commands use a prefix argument to
>> switch to the secondary browser, relying on the primary browser by
>> default. The problem is limited to `eww-browse-with-external-browser'
>> and `eww-follow-link'.
>
> What will happen as result of this change to users who customize
> browse-url-secondary-browser-function as its doc string says, and then
> invoke the command eww-browse-with-external-browser?

In this case the change will indeed be backward-incompatible, if
multiple external browsers are used and the secondary browser is
configured to a different one than the one which will be selected by
`browse-url-with-browser-kind'.

The patch can be extended however. I can change it such that the
`browse-url-secondary-browser-function' is checked first if it is indeed
external (via the `browser-kind' property). And only if it is not
external, `browse-url-with-browser-kind' will be used. One could be even
more strict and compare `browse-url-secondary-browser-function' to
`eww-browse-url' and only in this case fall back to
`browse-url-with-browser-kind'.

Daniel

bug#74730: [PATCH] 30.0.92; eww-browse-with-external-browser and eww-follow-link should use browse-url-with-browser-kind

[Eli Zaretskii]

> From: Daniel Mendler <mail@daniel-mendler.de>
> Cc: 74730@debbugs.gnu.org
> Date: Sun, 08 Dec 2024 07:13:54 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > The doc string of browse-url-secondary-browser-function explicitly
> > says not to set it to eww.  So users who do the above are acting
> > against the design and the recommended usage, and I'm not sure we
> > should support that at all, let alone with a (seemingly)
> > backward-incompatible change such as the one you propose.
> 
> How can I then use an external browser as the default and Eww as
> alternative? I argue that this is a legitimate use case - an external
> browser as primary and Eww as secondary browser for distraction-free
> reading.

It is a legitimate use case, but please describe it in more detail:
what do you mean by "using external browser as primary and eww as
secondary"?  That is, which Emacs commands should behave like that?

E.g., is "use browse-url-with-browser-kind" a valid solution for the
use case you describe?  If not, why not?

> As I wrote, the only place which leads to problems is in
> `eww-browse-with-external-browser' - I checked all other places in
> Emacs.

You haven't describe the results of your investigation in detail.  I
see 6 callers of browse-url-secondary-browser-function, so please
explain how this change is not a problem in each one of them.  Your
original message said that "most commands use a prefix argument to
switch to the secondary browser", but I don't understand how saying
that allows you to conclude that this change will not cause trouble?

> I argue that the behavior and implementation will be even more
> explicit, since `browse-url-with-browser-kind' explicitly supports the
> `external' kind. But you are right about the backward compatibility
> problem, see below.
> [...]
> > What will happen as result of this change to users who customize
> > browse-url-secondary-browser-function as its doc string says, and then
> > invoke the command eww-browse-with-external-browser?
> 
> In this case the change will indeed be backward-incompatible, if
> multiple external browsers are used and the secondary browser is
> configured to a different one than the one which will be selected by
> `browse-url-with-browser-kind'.
> 
> The patch can be extended however. I can change it such that the
> `browse-url-secondary-browser-function' is checked first if it is indeed
> external (via the `browser-kind' property). And only if it is not
> external, `browse-url-with-browser-kind' will be used. One could be even
> more strict and compare `browse-url-secondary-browser-function' to
> `eww-browse-url' and only in this case fall back to
> `browse-url-with-browser-kind'.

If we can make it so that existing customizations of
browse-url-secondary-browser-function will keep working as they did,
then the backward incompatibility issue will disappear, and such a
change becomes possible.  But in any case, the doc string of
browse-url-secondary-browser-function should be amended if we are
going to support its setting to eww.

Also, are we sure all the relevant functions always have the
'browse-url-browser-kind property? what about user-defined functions,
for example?

bug#74730: [PATCH] 30.0.92; eww-browse-with-external-browser and eww-follow-link should use browse-url-with-browser-kind

[Daniel Mendler via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Daniel Mendler <mail@daniel-mendler.de>
>> Cc: 74730@debbugs.gnu.org
>> Date: Sun, 08 Dec 2024 07:13:54 +0100
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > The doc string of browse-url-secondary-browser-function explicitly
>> > says not to set it to eww.  So users who do the above are acting
>> > against the design and the recommended usage, and I'm not sure we
>> > should support that at all, let alone with a (seemingly)
>> > backward-incompatible change such as the one you propose.
>> 
>> How can I then use an external browser as the default and Eww as
>> alternative? I argue that this is a legitimate use case - an external
>> browser as primary and Eww as secondary browser for distraction-free
>> reading.
>
> It is a legitimate use case, but please describe it in more detail:
> what do you mean by "using external browser as primary and eww as
> secondary"?  That is, which Emacs commands should behave like that?

In Emacs there are many commands which use `browse-url-browser-function'
or `browse-url-secondary-browser-function' depending on the prefix
argument. Many of the commands are generic and act on URLs or buttons at
point in different kinds of buffers. Also many external packages use
`browse-url'.

- `goto-address-at-point'
- `browse-url-button-open'
- `browse-url-button-open-url'
- `package-browse-url'
- `gnus-summary-browse-url'

When any of these commands on links is invoked, the
`browse-url-browser-function' is used by default. The primary browser is
configured by `browse-url-browser-function' while the secondary browser
is configured by `browse-url-secondary-browser-function'. Both use cases
are plausible: `browse-url-browser-function' can be external and
`browse-url-secondary-browser-function' can be internal and vice versa.

> E.g., is "use browse-url-with-browser-kind" a valid solution for the
> use case you describe?  If not, why not?

`browse-url-with-browser-kind' can be used when one explicitly wants to
call an external or internal browser. The command in Eww
`eww-browse-with-external-browser' has `*-external-*' as part of its
name, so in this case I think is a valid solution.

>> As I wrote, the only place which leads to problems is in
>> `eww-browse-with-external-browser' - I checked all other places in
>> Emacs.
>
> You haven't describe the results of your investigation in detail.  I
> see 6 callers of browse-url-secondary-browser-function, so please
> explain how this change is not a problem in each one of them.  Your
> original message said that "most commands use a prefix argument to
> switch to the secondary browser", but I don't understand how saying
> that allows you to conclude that this change will not cause trouble?

Each of the other call sites does not make assumptions about the
internal or external distinction. All that is important is that two
browser functions exist - a primary browser function and a secondary
function. By default the primary function is called, and if explicitly
requested by the user via a prefix argument, the secondary function is
used. It does not matter if the functions point to external or internal
browsers.

>> I argue that the behavior and implementation will be even more
>> explicit, since `browse-url-with-browser-kind' explicitly supports the
>> `external' kind. But you are right about the backward compatibility
>> problem, see below.
>> [...]
>> > What will happen as result of this change to users who customize
>> > browse-url-secondary-browser-function as its doc string says, and then
>> > invoke the command eww-browse-with-external-browser?
>> 
>> In this case the change will indeed be backward-incompatible, if
>> multiple external browsers are used and the secondary browser is
>> configured to a different one than the one which will be selected by
>> `browse-url-with-browser-kind'.
>> 
>> The patch can be extended however. I can change it such that the
>> `browse-url-secondary-browser-function' is checked first if it is indeed
>> external (via the `browser-kind' property). And only if it is not
>> external, `browse-url-with-browser-kind' will be used. One could be even
>> more strict and compare `browse-url-secondary-browser-function' to
>> `eww-browse-url' and only in this case fall back to
>> `browse-url-with-browser-kind'.
>
> If we can make it so that existing customizations of
> browse-url-secondary-browser-function will keep working as they did,
> then the backward incompatibility issue will disappear, and such a
> change becomes possible.  But in any case, the doc string of
> browse-url-secondary-browser-function should be amended if we are
> going to support its setting to eww.

Okay, I can do that.

> Also, are we sure all the relevant functions always have the
> 'browse-url-browser-kind property? what about user-defined functions,
> for example?

User-defined functions may not have the property. But we can be
conservative and preserve the existing behavior in the case where the
property is unavailable, treating the missing property like the value
`external'. Only if the property is found and `internal', the
`browse-url-with-browser-kind' will be used to make sure that an
external browser is used. As I mentioned, alternatively one can be even
more conservative and only use `browse-url-with-browser-kind' if
`browse-url-secondary-browser-function' is set to `eww-browse-url'. That
might be a little bit too restrictive, but it would be completely
backward compatible.

Daniel

bug#74730: [PATCH] 30.0.92; eww-browse-with-external-browser and eww-follow-link should use browse-url-with-browser-kind

[Eli Zaretskii]

> From: Daniel Mendler <mail@daniel-mendler.de>
> Cc: 74730@debbugs.gnu.org
> Date: Sun, 08 Dec 2024 08:27:20 +0100
> 
> > You haven't describe the results of your investigation in detail.  I
> > see 6 callers of browse-url-secondary-browser-function, so please
> > explain how this change is not a problem in each one of them.  Your
> > original message said that "most commands use a prefix argument to
> > switch to the secondary browser", but I don't understand how saying
> > that allows you to conclude that this change will not cause trouble?
> 
> Each of the other call sites does not make assumptions about the
> internal or external distinction.

Except that anything that calls them in eww must make some
assumptions, because it could otherwise cause infinite recursion.

> > If we can make it so that existing customizations of
> > browse-url-secondary-browser-function will keep working as they did,
> > then the backward incompatibility issue will disappear, and such a
> > change becomes possible.  But in any case, the doc string of
> > browse-url-secondary-browser-function should be amended if we are
> > going to support its setting to eww.
> 
> Okay, I can do that.
> 
> > Also, are we sure all the relevant functions always have the
> > 'browse-url-browser-kind property? what about user-defined functions,
> > for example?
> 
> User-defined functions may not have the property. But we can be
> conservative and preserve the existing behavior in the case where the
> property is unavailable, treating the missing property like the value
> `external'. Only if the property is found and `internal', the
> `browse-url-with-browser-kind' will be used to make sure that an
> external browser is used. As I mentioned, alternatively one can be even
> more conservative and only use `browse-url-with-browser-kind' if
> `browse-url-secondary-browser-function' is set to `eww-browse-url'. That
> might be a little bit too restrictive, but it would be completely
> backward compatible.

Looking forward to seeing an updated patch which preserves the current
behavior wrt browse-url-secondary-browser-function's customization.

Thanks.

bug#74730: [PATCH] 30.0.92; eww-browse-with-external-browser and eww-follow-link should use browse-url-with-browser-kind

[Daniel Mendler via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> > If we can make it so that existing customizations of
>> > browse-url-secondary-browser-function will keep working as they did,
>> > then the backward incompatibility issue will disappear, and such a
>> > change becomes possible.  But in any case, the doc string of
>> > browse-url-secondary-browser-function should be amended if we are
>> > going to support its setting to eww.
>> 
>> Okay, I can do that.
>> 
>> > Also, are we sure all the relevant functions always have the
>> > 'browse-url-browser-kind property? what about user-defined functions,
>> > for example?
>> 
>> User-defined functions may not have the property. But we can be
>> conservative and preserve the existing behavior in the case where the
>> property is unavailable, treating the missing property like the value
>> `external'. Only if the property is found and `internal', the
>> `browse-url-with-browser-kind' will be used to make sure that an
>> external browser is used. As I mentioned, alternatively one can be even
>> more conservative and only use `browse-url-with-browser-kind' if
>> `browse-url-secondary-browser-function' is set to `eww-browse-url'. That
>> might be a little bit too restrictive, but it would be completely
>> backward compatible.
>
> Looking forward to seeing an updated patch which preserves the current
> behavior wrt browse-url-secondary-browser-function's customization.
>
> Thanks.

Updated patch attached.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-eww-Improve-eww-browse-with-external-browser.patch --]
[-- Type: text/x-diff, Size: 3333 bytes --]

From 9d782ed2123b34fd09268f6c83305d7d9d4acf7d Mon Sep 17 00:00:00 2001
From: Daniel Mendler <mail@daniel-mendler.de>
Date: Sat, 7 Dec 2024 22:56:15 +0100
Subject: [PATCH] eww: Improve eww-browse-with-external-browser

Guarantee that an external browser is used by EWW if
`browse-url-secondary-browser-function' is set to
`eww-browse-url'.

* lisp/net/eww.el (eww-browse-with-external-browser): Use
`browse-url-secondary-browser-function' if it does not
correspond to EWW, otherwise fall back to `browse-url'.
(eww-follow-link): Use `eww-browse-with-external-browser' if the
EXTERNAL prefix argument is non-nil.
* lisp/net/browse-url.el
(browse-url-secondary-browser-function): Update docstring.
---
 lisp/net/browse-url.el |  8 ++++----
 lisp/net/eww.el        | 14 ++++++++------
 2 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/lisp/net/browse-url.el b/lisp/net/browse-url.el
index c10bc671a88..c9d8b2ee80d 100644
--- a/lisp/net/browse-url.el
+++ b/lisp/net/browse-url.el
@@ -198,10 +198,10 @@ browse-url-browser-function
 
 (defcustom browse-url-secondary-browser-function 'browse-url-default-browser
   "Function used to launch an alternative browser.
-This is usually an external browser (that is, not eww or w3m),
-used as the secondary browser choice, typically when a prefix
-argument is given to a URL-opening command in those modes that
-support this (for instance, eww/shr).
+
+This is browser is used as the secondary browser choice, typically
+when a prefix argument is given to a URL-opening command in those
+modes that support this (for instance `goto-addr-at-point', eww or shr).
 
 Also see `browse-url-browser-function'."
   :version "27.1"
diff --git a/lisp/net/eww.el b/lisp/net/eww.el
index 4d4d4d6beac..b2d295a4dcf 100644
--- a/lisp/net/eww.el
+++ b/lisp/net/eww.el
@@ -2142,10 +2142,13 @@ eww-submit
 
 (defun eww-browse-with-external-browser (&optional url)
   "Browse the current URL with an external browser.
-The browser to used is specified by the
-`browse-url-secondary-browser-function' variable."
+Calls `browse-url-secondary-browser-function' if it does not correspond
+to EWW.  Otherwise `browse-url' is used."
   (interactive nil eww-mode)
-  (funcall browse-url-secondary-browser-function
+  (funcall (if (memq browse-url-secondary-browser-function
+                     '(eww eww-browse-url))
+               #'browse-url
+             browse-url-secondary-browser-function)
            (or url (plist-get eww-data :url))))
 
 (defun eww-remove-tracking (url)
@@ -2162,8 +2165,7 @@ eww--transform-url
 (defun eww-follow-link (&optional external mouse-event)
   "Browse the URL under point.
 If EXTERNAL is single prefix, browse the URL using
-`browse-url-secondary-browser-function'.
-
+`eww-browse-with-external-browser'.
 If EXTERNAL is double prefix, browse in new buffer."
   (interactive
    (list current-prefix-arg last-nonmenu-event)
@@ -2180,7 +2182,7 @@ eww-follow-link
       ;; and `browse-url-mailto-function'.
       (browse-url url))
      ((and (consp external) (<= (car external) 4))
-      (funcall browse-url-secondary-browser-function url)
+      (eww-browse-with-external-browser url)
       (shr--blink-link))
      ;; This is a #target url in the same page as the current one.
      ((and (setq target (url-target (url-generic-parse-url url)))
-- 
2.45.2

bug#74730: [PATCH] 30.0.92; eww-browse-with-external-browser and eww-follow-link should use browse-url-with-browser-kind

[Eli Zaretskii]

> From: Daniel Mendler <mail@daniel-mendler.de>
> Cc: 74730@debbugs.gnu.org
> Date: Sun, 08 Dec 2024 14:00:56 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> > If we can make it so that existing customizations of
> >> > browse-url-secondary-browser-function will keep working as they did,
> >> > then the backward incompatibility issue will disappear, and such a
> >> > change becomes possible.  But in any case, the doc string of
> >> > browse-url-secondary-browser-function should be amended if we are
> >> > going to support its setting to eww.
> >> 
> >> Okay, I can do that.
> >> 
> >> > Also, are we sure all the relevant functions always have the
> >> > 'browse-url-browser-kind property? what about user-defined functions,
> >> > for example?
> >> 
> >> User-defined functions may not have the property. But we can be
> >> conservative and preserve the existing behavior in the case where the
> >> property is unavailable, treating the missing property like the value
> >> `external'. Only if the property is found and `internal', the
> >> `browse-url-with-browser-kind' will be used to make sure that an
> >> external browser is used. As I mentioned, alternatively one can be even
> >> more conservative and only use `browse-url-with-browser-kind' if
> >> `browse-url-secondary-browser-function' is set to `eww-browse-url'. That
> >> might be a little bit too restrictive, but it would be completely
> >> backward compatible.
> >
> > Looking forward to seeing an updated patch which preserves the current
> > behavior wrt browse-url-secondary-browser-function's customization.
> >
> > Thanks.
> 
> Updated patch attached.

Thanks, a few comments below.

>  (defcustom browse-url-secondary-browser-function 'browse-url-default-browser
>    "Function used to launch an alternative browser.
> -This is usually an external browser (that is, not eww or w3m),
> -used as the secondary browser choice, typically when a prefix
> -argument is given to a URL-opening command in those modes that
> -support this (for instance, eww/shr).
> +
> +This is browser is used as the secondary browser choice, typically
> +when a prefix argument is given to a URL-opening command in those
> +modes that support this (for instance `goto-addr-at-point', eww or shr).

This doc string should explain the assumptions about this and the
other variable, browse-url-browser-function: that either one or the
other invokes the external browser, and the other one should then NOT
do the same.  Users should be aware and understand the rules of the
game here, which are now slightly more complex than they were before,
so removing the previous assumption without replacing it with anything
makes the doc string less useful.

>  (defun eww-browse-with-external-browser (&optional url)
>    "Browse the current URL with an external browser.
> -The browser to used is specified by the
> -`browse-url-secondary-browser-function' variable."
> +Calls `browse-url-secondary-browser-function' if it does not correspond
> +to EWW.  Otherwise `browse-url' is used."
                      ^^^^^^^^^^^^^^^^^^^^
Passive tense alert!

>    (interactive nil eww-mode)
> -  (funcall browse-url-secondary-browser-function
> +  (funcall (if (memq browse-url-secondary-browser-function
> +                     '(eww eww-browse-url))
> +               #'browse-url
> +             browse-url-secondary-browser-function)

I think we should allow here (and document) more just 2 literal
functions hard-coded in this command.  Perhaps some special property
on the function's symbol?  Then these two specific functions can be
supported via the property, and what's more important, users and
applications could use other functions with the same semantics.

>  (defun eww-follow-link (&optional external mouse-event)
>    "Browse the URL under point.
>  If EXTERNAL is single prefix, browse the URL using
   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
What does this mean? does it mean that EXTERNAL is non-nil in
interactive usage if the command is invoked with C-u?  If so, let's
say that.  (Yes, I know that problem came from the original doc
string.)

> @@ -2180,7 +2182,7 @@ eww-follow-link
>        ;; and `browse-url-mailto-function'.
>        (browse-url url))
>       ((and (consp external) (<= (car external) 4))
> -      (funcall browse-url-secondary-browser-function url)
> +      (eww-browse-with-external-browser url)

I'm not sure I agree.  The call to eww-browse-with-external-browser
will no longer ensure an external browser is used, after these
changes.  Why not call browse-url-default-browser instead?

In any case, the doc string should say that if no external browser
could be found, this will fall back to eww.

bug#74730: [PATCH] 30.0.92; eww-browse-with-external-browser and eww-follow-link should use browse-url-with-browser-kind

[Daniel Mendler via Bug reports for GNU Emacs, the Swiss army knife of text editors]

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

Eli Zaretskii <eliz@gnu.org> writes:

Updated patch attached.

>>  (defcustom browse-url-secondary-browser-function 'browse-url-default-browser
>>    "Function used to launch an alternative browser.
>> -This is usually an external browser (that is, not eww or w3m),
>> -used as the secondary browser choice, typically when a prefix
>> -argument is given to a URL-opening command in those modes that
>> -support this (for instance, eww/shr).
>> +
>> +This is browser is used as the secondary browser choice, typically
>> +when a prefix argument is given to a URL-opening command in those
>> +modes that support this (for instance `goto-addr-at-point', eww or shr).
>
> This doc string should explain the assumptions about this and the
> other variable, browse-url-browser-function: that either one or the
> other invokes the external browser, and the other one should then NOT
> do the same.  Users should be aware and understand the rules of the
> game here, which are now slightly more complex than they were before,
> so removing the previous assumption without replacing it with anything
> makes the doc string less useful.

I expanded the docstring.

>>    (interactive nil eww-mode)
>> -  (funcall browse-url-secondary-browser-function
>> +  (funcall (if (memq browse-url-secondary-browser-function
>> +                     '(eww eww-browse-url))
>> +               #'browse-url
>> +             browse-url-secondary-browser-function)
>
> I think we should allow here (and document) more just 2 literal
> functions hard-coded in this command.  Perhaps some special property
> on the function's symbol?  Then these two specific functions can be
> supported via the property, and what's more important, users and
> applications could use other functions with the same semantics.

In this case I think it is better to use `browse-url-with-browser-kind'
as I had initially suggested. This will be better than introducing yet
another property only for this purpose.

>>  (defun eww-follow-link (&optional external mouse-event)
>>    "Browse the URL under point.
>>  If EXTERNAL is single prefix, browse the URL using
>    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> What does this mean? does it mean that EXTERNAL is non-nil in
> interactive usage if the command is invoked with C-u?  If so, let's
> say that.  (Yes, I know that problem came from the original doc
> string.)

I improved the docstring.

>> @@ -2180,7 +2182,7 @@ eww-follow-link
>>        ;; and `browse-url-mailto-function'.
>>        (browse-url url))
>>       ((and (consp external) (<= (car external) 4))
>> -      (funcall browse-url-secondary-browser-function url)
>> +      (eww-browse-with-external-browser url)
>
> I'm not sure I agree.  The call to eww-browse-with-external-browser
> will no longer ensure an external browser is used, after these
> changes.  Why not call browse-url-default-browser instead?

Now, it is ensured that the external browser is called.

Daniel


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-eww-Use-browse-url-with-browser-kind-in-eww-browse-w.patch --]
[-- Type: text/x-diff, Size: 4170 bytes --]

From f56022cc4b1f9c007ba583ece8f2c714a92eefc6 Mon Sep 17 00:00:00 2001
From: Daniel Mendler <mail@daniel-mendler.de>
Date: Sat, 7 Dec 2024 22:56:15 +0100
Subject: [PATCH] eww: Use browse-url-with-browser-kind in
 eww-browse-with-external-browser

Guarantee that an external browser is used by EWW if
`browse-url-secondary-browser-function' is set to
`eww-browse-url'.

* lisp/net/eww.el (eww-browse-with-external-browser): Use
`browse-url-secondary-browser-function' only if it is an
external browser, otherwise fall back to
`browse-url-with-browser-kind'.
(eww-follow-link): Use `eww-browse-with-external-browser' if the
EXTERNAL prefix argument is non-nil.  Improve docstring.
* lisp/net/browse-url.el
(browse-url-secondary-browser-function): Update docstring.
---
 lisp/net/browse-url.el | 14 ++++++++++----
 lisp/net/eww.el        | 23 ++++++++++++++---------
 2 files changed, 24 insertions(+), 13 deletions(-)

diff --git a/lisp/net/browse-url.el b/lisp/net/browse-url.el
index c10bc671a88..8ec025d017b 100644
--- a/lisp/net/browse-url.el
+++ b/lisp/net/browse-url.el
@@ -198,10 +198,16 @@ browse-url-browser-function
 
 (defcustom browse-url-secondary-browser-function 'browse-url-default-browser
   "Function used to launch an alternative browser.
-This is usually an external browser (that is, not eww or w3m),
-used as the secondary browser choice, typically when a prefix
-argument is given to a URL-opening command in those modes that
-support this (for instance, eww/shr).
+
+This browser is used as the secondary browser choice, typically
+when a prefix argument is given to a URL-opening command in those
+modes that support this (for instance `browse-url-at-point',
+`goto-addr-at-point', eww or shr).
+
+This assumption is that `browse-url-secondary-browser-function'
+and `browse-url-browser-function' are set to distinct browsers.
+Either one of the two functions should call an external browser
+and the other one should not do the same.
 
 Also see `browse-url-browser-function'."
   :version "27.1"
diff --git a/lisp/net/eww.el b/lisp/net/eww.el
index 4d4d4d6beac..4609755a902 100644
--- a/lisp/net/eww.el
+++ b/lisp/net/eww.el
@@ -2142,11 +2142,15 @@ eww-submit
 
 (defun eww-browse-with-external-browser (&optional url)
   "Browse the current URL with an external browser.
-The browser to used is specified by the
-`browse-url-secondary-browser-function' variable."
+Use `browse-url-secondary-browser-function' if it is an external
+browser, otherwise use `browse-url-with-browser-kind' to open an
+external browser."
   (interactive nil eww-mode)
-  (funcall browse-url-secondary-browser-function
-           (or url (plist-get eww-data :url))))
+  (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-browser-kind 'external url)))
 
 (defun eww-remove-tracking (url)
   "Remove the commong utm_ tracking cookies from URLs."
@@ -2160,11 +2164,12 @@ eww--transform-url
     url))
 
 (defun eww-follow-link (&optional external mouse-event)
-  "Browse the URL under point.
-If EXTERNAL is single prefix, browse the URL using
-`browse-url-secondary-browser-function'.
+  "Browse the URL at point, optionally the position of MOUSE-EVENT.
 
-If EXTERNAL is double prefix, browse in new buffer."
+EXTERNAL is the prefix argument.  If called interactively with
+\\[universal-argument] pressed once, browse the URL using
+`eww-browse-with-external-browser'.  If called interactively, with
+\\[universal-argument] pressed twice, browse in new buffer."
   (interactive
    (list current-prefix-arg last-nonmenu-event)
    eww-mode)
@@ -2180,7 +2185,7 @@ eww-follow-link
       ;; and `browse-url-mailto-function'.
       (browse-url url))
      ((and (consp external) (<= (car external) 4))
-      (funcall browse-url-secondary-browser-function url)
+      (eww-browse-with-external-browser url)
       (shr--blink-link))
      ;; This is a #target url in the same page as the current one.
      ((and (setq target (url-target (url-generic-parse-url url)))
-- 
2.45.2

bug#74730: [PATCH] 30.0.92; eww-browse-with-external-browser and eww-follow-link should use browse-url-with-browser-kind

[Eli Zaretskii]

> From: Daniel Mendler <mail@daniel-mendler.de>
> Cc: 74730@debbugs.gnu.org
> Date: Sun, 08 Dec 2024 18:39:28 +0100
> 
> Updated patch attached.

Thanks, installed on the master branch, and closing the bug.