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.