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" <bug-gnu-emacs@gnu.org>]

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