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

[J.P.]

Stefan Kangas <stefankangas@gmail.com> writes:

> "J.P." <jp@neverwas.me> 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.

Thinking about this more, I guess it's fine if a modern ERC running on
an older Emacs uses the port alone to decide whether to connect over
TLS. As such, I've abandoned the whole `url-irc-function' thing in favor
of adding a proper argument, as suggested. The small fraction of users
with their own `url-irc-function' (if any) may feel some churn though.

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

Makes sense. I have thus added the missing ingredients and wired them
up.

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

Added.

> I also added some notes inline below:

Much appreciated!

>> From 0d191d30b15ea2d5b6042f51c6cf421b82feb7e5 Mon Sep 17 00:00:00 2001
>> From: "F. Jason Park" <jp@neverwas.me>
>> 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.

Installed.

>> From 6fd2f75707f123abfbcfae2d4f2837efed5b7adc Mon Sep 17 00:00:00 2001
>> From: "F. Jason Park" <jp@neverwas.me>
>> 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.

Added.

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

I guess I was trying to avoid growing lisp/loaddefs.el on account of a
couple URL schemes that haven't caught on in the wild (and don't seem
poised to). Still, I might as well ask around with some IRC folk just to
be sure.

>> From a9b47f5a6079fb3030c9e1514b4cbbda86dafff8 Mon Sep 17 00:00:00 2001
>> From: "F. Jason Park" <jp@neverwas.me>
>> 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.

Right, I definitely plan on mentioning this and most other ERC changes
in some fashion.

>> @@ -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.

I've removed the option entirely because I've come to realize it's
unlikely a new user would ever set a port parameter to an IANA name in
the first place (via :port, `erc-port', or whatever). And existing users
accustomed to doing so obviously already know what to expect (namely,
quasi-obsolete port numbers, like 194).

>> From 3658e89614cbe3b5b27f09271b7bc738a1c7ec38 Mon Sep 17 00:00:00 2001
>> From: "F. Jason Park" <jp@neverwas.me>
>> Date: Mon, 11 Jul 2022 05:14:57 -0700
>> Subject: [PATCH 6/6] Improve new connections in erc-handle-irc-url
> [...]
>> +
>> +@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.

Agreed! That's basically nonsense (and so removed).

>> +;; 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.

ERC has always supported the auth:pass@ stuff, but my comment was
confusing, so I've deleted it.


Thanks so much for looking at these changes!

0000-v4-v5.diff
Description: Text Data

0002-Accommodate-ircs-URLs-in-url-irc-and-browse-url.patch
Description: Text Data

0003-Refactor-erc-select-read-args.patch
Description: Text Data

0004-Default-to-TLS-port-when-calling-erc-tls-from-lisp.patch
Description: Text Data

0005-Add-optional-server-param-to-erc-networks-determine.patch
Description: Text Data

0006-Improve-new-connections-in-erc-handle-irc-url.patch
Description: Text Data