Re: bug#56514: 29.0.50; Improve ERC's URI scheme integration for irc:// links

"J.P." <[email protected]> writes:

> Questions (for anyone):
>
>   1. I added a couple autoloads in lisp/url/url-irc.el to avoid having
>      to create a url-ircs.el (or even a url-irc6{,s}.el). Is there a
>      better alternate means of getting `url-scheme-get-property' to
>      discover handlers that doesn't rely on autoloads?

I'm hoping someone else will weigh in about this.

>   2. In the function `url-irc', I bind `url-current-object' around the
>      call to `url-irc-function' to avoid adding another parameter to the
>      latter's interface (which mainly benefits ERC). Any obvious problem
>      with borrowing `url-current-object' for this purpose?

No real opinion here.  It feels slightly cleaner to add it as a proper
argument, if we expect that other IRC clients than ERC would be
interested in its value.

>   3. In browse-url, I basically ignore what looks like the favored
>      practice for adding handlers, namely, registering an internal
>      function with `browse-url-default-handlers' that calls a public
>      function assigned to a user option. An example of this pattern is:
>
>        internal: browse-url--mailto
>        option:   browse-url-mailto-function
>        public:   browse-url-mail
>
>      The reason for sidestepping the intervening indirection and adding
>      a public function directly to `browse-url-default-handlers' is that
>      I figure users wishing to override this can already do so via
>      `browse-url-handlers'. Is that misguided somehow?

You do have a point, but I think it's better to have the user option for
consistency, and for ease of customization.  Customizing an alist with
customize is always going to be harder than customizing a single-value
user option.

>   4. Are any of these non-ERC changes newsworthy enough for etc/NEWS?

I think teaching browse-url to recognize irc URLs is NEWS-worthy.

I also added some notes inline below:

> From 0d191d30b15ea2d5b6042f51c6cf421b82feb7e5 Mon Sep 17 00:00:00 2001
> From: "F. Jason Park" <[email protected]>
> Date: Wed, 13 Jul 2022 01:54:19 -0700
> Subject: [PATCH 1/6] Teach thing-at-point to recognize bracketed IPv6 URLs

I suggest pushing this patch so that we're sure to have it in Emacs 29.

I don't think it's NEWS-worthy, as it's more of a bug fix.

> From 6fd2f75707f123abfbcfae2d4f2837efed5b7adc Mon Sep 17 00:00:00 2001
> From: "F. Jason Park" <[email protected]>
> Date: Mon, 11 Jul 2022 05:14:57 -0700
> Subject: [PATCH 2/6] Accommodate ircs:// URLs in url-irc and browse-url
[...]
> +;;;; ircs://
> +
> +;; The function `url-scheme-get-property' tries and fails to load the
> +;; nonexistent url-ircs.el but falls back to using the following:
> +
> +;;;###autoload
> +(defconst url-ircs-default-port 6697 "Default port for IRCS connections.")
> +
> +;;;###autoload
> +(defalias 'url-ircs 'url-irc)

This change (support for ircs) should probably be in NEWS.

What about `irc6' and `irc6s'?  Should they have aliases?

> From a9b47f5a6079fb3030c9e1514b4cbbda86dafff8 Mon Sep 17 00:00:00 2001
> From: "F. Jason Park" <[email protected]>
> Date: Mon, 11 Jul 2022 05:14:57 -0700
> Subject: [PATCH 4/6] Default to TLS port when calling erc-tls from lisp
>
> * lisp/erc/erc.el (erc-legacy-port-names, erc-normalize-port): Add
> standard IANA port-name mappings for 6667 and 6697, as well as an
> option to opt in for saner but nonstandard behavior.
> (erc-open): Add note to doc string explaining that params `connect'
> and `channel' are mutually exclusive.
> (erc-tls): Call `erc-compute-port' with override.
> (erc-compute-port): Call `erc-normalize-port' with result'.
>
> * test/lisp/erc/erc-tests.el (erc-tls): Add simplistic test focusing
> on default parameters.

This belongs in NEWS.

> @@ -1550,8 +1564,16 @@ erc-normalize-port
>        (cond
>         ((> port-nr 0)
>          port-nr)
> -       ((string-equal port "irc")
> -        194)
> +       ((string-equal port "ircu") 6667)
> +       ((string-equal port "ircs-u") 6697)
> +       ((member port '("irc" "ircs"))
> +        (when (eq erc-legacy-port-names 'legacy)
> +          (lwarn 'ERC 'warning
> +                 (concat "`erc-legacy-port-names' will default to nil "
> +                         "in a future version of ERC.")))

Warning about the default seems unfortunate.  Then every user will be
warned until they customize this.

I think we should either disable the warning, or flip the default to
nil.

> From 3658e89614cbe3b5b27f09271b7bc738a1c7ec38 Mon Sep 17 00:00:00 2001
> From: "F. Jason Park" <[email protected]>
> Date: Mon, 11 Jul 2022 05:14:57 -0700
> Subject: [PATCH 6/6] Improve new connections in erc-handle-irc-url
[...]
> @@ -990,6 +992,43 @@ Sample Configuration
>  ;; (setq erc-kill-server-buffer-on-quit t)
>  @end lisp
>
> +@node Integrations
> +@section Integrations
> +@cindex integrations
> +
> +@subheading URL
> +For anything to work, you'll want to set @code{url-irc-function} to
> +@code{url-irc-erc}.  As a rule of thumb, libraries that rely directly
> +on @code{url-retrieve} should be good to go out the box from Emacs
> +29.1 onward.  On older versions of Emacs, you may need to
> +@code{(require 'erc)} beforehand. @pxref{Retrieving URLs,,, url, URL}.
> +
> +For other apps and libraries, such as those relying on the
> +higher-level @code{browse-url}, you'll oftentimes be asked to specify
> +a pattern, sometimes paired with a function that accepts a string URL
> +as a first argument.  For example, with EWW, you may need to tack
> +something like @code{"\\|\\`irc6?s?:"} onto the end of
> +@code{eww-use-browse-url}.  But with @code{gnus-button-alist}, you'll
> +need a function as well:
> +
> +@lisp
> +  '("\\birc6?s?://[][a-z0-9.,@@_:+%?&/#-]+"
> +    0 t erc-browse-url-handler 0)
> +@end lisp
> +
> +@defun erc-browse-url-handler url &rest args
> +An autoloaded convenience function for use in options like those
> +mentioned above.  @var{url} must be a string.  In Emacs 29 and above,
> +the function @code{browse-url-irc} can be used instead.
> +@end defun
> +
> +@noindent
> +Keep in mind that when fiddling with these options, it may be easier
> +(and more polite) to connect to a local server or a test network, like
> +@samp{ircs://testnet.ergo.chat/#test}, since these generally don't
> +require authentication.

Why would that be more polite?  It seems to me that, sure, if you're
developing an IRC client I can see why you'd want to use a test network.

But it seems like overkill just for user customization.

> @@ -7184,25 +7184,98 @@ erc-get-parsed-vector-type
>  ;; Teach url.el how to open irc:// URLs with ERC.
>  ;; To activate, customize `url-irc-function' to `url-irc-erc'.
>
> -;; FIXME change user to nick, and use API to find server buffer
> +(defcustom erc-url-connect-function nil
> +  "When non-nil, a function used to connect to an IRC URL.
> +Called with any number of keyword arguments recognized by `erc'
> +and `erc-tls'.  The variable `url-current-object', if non-nil,
> +can be used to help determine whether to connect using TLS."
> +  :group 'erc
> +  :package-version '(ERC . "5.4.1") ; FIXME increment on release
> +  :type '(choice (const nil) function))
> +
> +(defun erc--url-default-connect-function (&rest plist)
> +  (let* ((scheme (and url-current-object (url-type url-current-object)))
> +         (ircsp (if scheme
> +                    (string-suffix-p "s" scheme)
> +                  (or (eql 6697 (plist-get plist :port))
> +                      (yes-or-no-p "Connect using TLS? "))))
> +         (erc-server (plist-get plist :server))
> +         (erc-port (or (plist-get plist :port)
> +                       (and ircsp (erc-normalize-port 'ircs-u))
> +                       erc-port))
> +         (erc-nick (or (plist-get plist :nick) erc-nick))
> +         (erc-password (plist-get plist :password))
> +         (args (erc-select-read-args)))
> +    (unless ircsp
> +      (setq ircsp (eql 6697 erc-port)))
> +    (apply (if ircsp #'erc-tls #'erc) args)))
> +
> +;; The current spec, unlike the 2003 Butcher draft, doesn't explicitly
> +;; allow for an auth[:password]@ component (or trailing ,flags or
> +;; &options).
> +;;
> +;; https://www.iana.org/assignments/uri-schemes
> +;; https://datatracker.ietf.org/doc/html/draft-butcher-irc-url#section-6
> +

This is a breaking change, no?  I think it should be in NEWS, even if it
is only to make ERC more standards compliant.