Subject: bug#64478: 30.0.50; ERC 5.6: Improve and deprecate erc-server-select

From: "J.P." <[email-masked]>
Date: Wed, 05 Jul 2023 07:18:30 -0700

Tags: patch

The command `erc-server-select' has long been considered a UX liability
by many familiar with ERC, and yet it still appears among the handful of
"erc-" prefixed entry points on offer when typing M-x erc TAB. So it's
no wonder that folks complaining about ERC's brokenness often mention
this command when pressed for details. What I imagine typically happens
is that new folks find it succeeds initially but quickly grow frustrated
once authentication becomes a concern (which it necessarily does because
network documentation often emphasizes it). And even if these folks
eventually figure out that the lack of prompting for vital parameters
(like a nickname) means they're expected to convey such info via user
options, they likely don't take kindly to discovering certain channels
are off limits because the command isn't TLS aware.

Although I'd prefer we somehow salvage `erc-server-select' (and other
public items residing near the bottom of erc-networks.el [1]), we must
also account for our meager manpower reserves and factor in all the
would-be users continuously lost to negative experiences while we await
whatever volunteer do-gooder may eventually emerge to take this on. At a
minimum, I'd expect such a makeover to include some means of offering
TLS, likely by extending `erc-server-alist' somehow while also shedding
obsolete members in its default value (and hunting down updated values
where warranted). I'd also think some mention in the doc string of
essential user options that influence the connection, like `erc-nick',
would be in order, as well as possibly migrating the command to erc.el.
I've taken a stab at the lowest-hanging bits but have left the most
laborious, namely, updating `erc-server-select', for any takers who feel
up for the challenge. While it's heavy on leg work, it's relatively
light on ERC know how and thus a good(TM) first issue. If that's you
[2], please keep us in the loop by announcing your intentions so we can
possibly reconsider deprecation.

Thanks.

[1] Someday, I do plan on revisiting at least one specific user-facing
    item at the bottom of erc-networks.el, namely, `erc-settings', which
    seems oriented toward users coming from traditional clients where
    connections are configured centrally. As I've mentioned elsewhere,
    we need something akin to (but more flexible than) connection-local
    variables to allow users to leverage existing user options but
    tailor them to various context-specific "scopes," like
    network/nick/channel, etc. Because this option is currently
    nonfunctional and its doc string notes it as being experimental, I
    believe we'd be free to repurpose it with impunity for some role in
    this new imagined paradigm.

[2] The list weighs in at almost 400 lines, so you'll need to assign
    copyright if not already on the books.

In GNU Emacs 30.0.50 (build 3, x86_64-pc-linux-gnu, GTK+ Version
 3.24.38, cairo version 1.17.6) of 2023-07-04 built on localhost
Repository revision: e37b96432b89521871e947f322545f154547dd1b
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12014000
System Description: Fedora Linux 37 (Workstation Edition)

Configured using:
 'configure --enable-check-lisp-object-type --enable-checking=yes,glyphs
 'CFLAGS=-O0 -g3'
 PKG_CONFIG_PATH=:/usr/lib64/pkgconfig:/usr/share/pkgconfig'

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
JSON LCMS2 LIBOTF LIBSELINUX LIBSYSTEMD LIBXML2 M17N_FLT MODULES NOTIFY
INOTIFY PDUMPER PNG RSVG SECCOMP SOUND SQLITE3 THREADS TIFF
TOOLKIT_SCROLL_BARS WEBP X11 XDBE XIM XINPUT2 XPM GTK3 ZLIB

Important settings:
  value of $LANG: en_US.UTF-8
  value of $XMODIFIERS: @im=ibus
  locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message mailcap yank-media puny dired
dired-loaddefs rfc822 mml mml-sec epa derived epg rfc6068 epg-config
gnus-util text-property-search time-date mm-decode mm-bodies mm-encode
mail-parse rfc2231 mailabbrev gmm-utils mailheader sendmail rfc2047
rfc2045 ietf-drums mm-util mail-prsvr mail-utils erc auth-source cl-seq
eieio eieio-core cl-macs password-cache json subr-x map format-spec
cl-loaddefs cl-lib erc-backend erc-networks byte-opt gv bytecomp
byte-compile erc-common erc-compat erc-loaddefs rmc iso-transl tooltip
cconv eldoc paren electric uniquify ediff-hook vc-hooks lisp-float-type
elisp-mode mwheel term/x-win x-win term/common-win x-dnd tool-bar dnd
fontset image regexp-opt fringe tabulated-list replace newcomment
text-mode lisp-mode prog-mode register page tab-bar menu-bar rfn-eshadow
isearch easymenu timer select scroll-bar mouse jit-lock font-lock syntax
font-core term/tty-colors frame minibuffer nadvice seq simple cl-generic
indonesian philippine cham georgian utf-8-lang misc-lang vietnamese
tibetan thai tai-viet lao korean japanese eucjp-ms cp51932 hebrew greek
romanian slovak czech european ethiopic indian cyrillic chinese
composite emoji-zwj charscript charprop case-table epa-hook
jka-cmpr-hook help abbrev obarray oclosure cl-preloaded button loaddefs
theme-loaddefs faces cus-face macroexp files window text-properties
overlay sha1 md5 base64 format env code-pages mule custom widget keymap
hashtable-print-readable backquote threads dbusbind inotify lcms2
dynamic-setting system-font-setting font-render-setting cairo gtk
x-toolkit xinput2 x multi-tty move-toolbar make-network-process emacs)

Memory information:
((conses 16 64567 9442) (symbols 48 8587 0) (strings 32 23303 1404)
 (string-bytes 1 675756) (vectors 16 15005)
 (vector-slots 8 207155 9292) (floats 8 24 27) (intervals 56 228 0)
 (buffers 976 10))
>From 570fe39e70f048eb9da0b9712f3ebe8d778ef596 Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <[email-masked]>
Date: Thu, 29 Jun 2023 07:12:46 -0700
Subject: [PATCH] [5.6] Deprecate erc-server-alist and erc-server-select

* etc/ERC-NEWS: Announce deprecation of `erc-server-alist' and
`erc-server-select'.
* lisp/erc/erc-networks.el: Comment out call to `erc-get' at the end
of the file.
(erc-server-alist) Change shape to
accommodate a fifth member: TLS ports.  Add default TLS ports for
Libera.Chat and OFTC.  Deprecate option.
(erc-ports-list): Overload to accept a number instead of a list, but
don't advertise this fact.
(erc-networks--server-select): Convert `erc-server-select' into a
function that performs the same prompting but returns a URL instead of
calling `erc'.
(erc-server-select): Move to erc.el.
* lisp/erc/erc.el (erc--prompt-for-server-functions): New variable.
(erc-select-read-args): Defer to `erc--prompt-for-server-function'
when non-nil.
(erc-server-select): Deprecate and defer to `erc'.
* test/lisp/erc/erc-networks-tests.el (erc-ports-list): New test.
* test/lisp/erc/erc-tests.el (erc-server-select): New test.
---
 etc/ERC-NEWS                        | 16 ++++++
 lisp/erc/erc-networks.el            | 77 ++++++++++++++++-------------
 lisp/erc/erc.el                     | 16 +++++-
 test/lisp/erc/erc-networks-tests.el | 18 +++++++
 test/lisp/erc/erc-tests.el          | 38 ++++++++++++++
 5 files changed, 130 insertions(+), 35 deletions(-)

diff --git a/etc/ERC-NEWS b/etc/ERC-NEWS
index 5665b760ea9..808d7dcb64f 100644
--- a/etc/ERC-NEWS
+++ b/etc/ERC-NEWS
@@ -75,6 +75,22 @@ the 'log' module may want to customize 'erc-log-filter-function' to
 'erc-stamp-prefix-log-filter' to avoid ragged right-hand stamps
 appearing in their saved logs.
 
+** Awkward entry point 'erc-server-select' improved but deprecated.
+The alternate entry point 'erc-server-select' has mainly served to
+confuse users in more recent years because it requires certain
+options, like 'erc-nick', to be configured ahead of time, and it
+doesn't support TLS.  Its main selling point, historically, has been
+interactive completion based on the option 'erc-server-alist', which
+is a table of networks, servers, and ports.  But most of the option's
+400-odd entries are sadly defunct or otherwise outdated.  And, these
+days, most networks promote a well known load-balancing end point over
+individual servers anyway.  Regardless, the command has now been
+improved to prompt for the same slate of parameters sought by
+'erc-tls'.  Similarly, 'erc-server-alist' entries now support a fifth
+member in TLS ports (though this option too has been deprecated).  If
+you feel these deprecations rash or unwarranted, please file a bug
+report and petition the maintainers for a reprieve.
+
 ** Smarter reconnect handling for users on the move.
 ERC now offers a new, experimental reconnect strategy in the function
 'erc-server-delayed-check-reconnect', which tests for underlying
diff --git a/lisp/erc/erc-networks.el b/lisp/erc/erc-networks.el
index dd481032e7e..81ce18ad82f 100644
--- a/lisp/erc/erc-networks.el
+++ b/lisp/erc/erc-networks.el
@@ -29,8 +29,6 @@
 ;;
 ;; This is the "networks" module.
 ;;
-;; M-x erc-server-select provides an alternative way to connect to servers by
-;; choosing networks.
 ;; You can use (eq (erc-network) 'Network) if you'd like to set variables or do
 ;; certain actions according to which network you're connected to.
 ;; If a network you use is not listed in `erc-networks-alist', you can put
@@ -258,6 +256,7 @@ erc-server-alist
   ("IRChat: Random server" IRChat "irc.irchat.net" ((6660 6669)))
   ("IrcLordz: Random server" IrcLordz "irc.irclordz.com" 6667)
   ("IrcMalta: Random server" IrcMalta "irc.ircmalta.org" ((6660 6667)))
+  ;; This one is dead but used in testing.  Please retain.
   ("IRCnet: EU, FR, Random" IRCnet "irc.fr.ircnet.net" 6667)
   ("IRCnet: EU, IT, Random" IRCnet "irc.ircd.it" ((6665 6669)))
   ("IRCnet: AS, IL, Haifa" IRCnet "ircnet.netvision.net.il" ((6661 6668)))
@@ -318,13 +317,15 @@ erc-server-alist
   ("LagNet: Random server" LagNet "irc.lagnet.org.za" 6667)
   ("LagNet: AF, ZA, Cape Town" LagNet "reaper.lagnet.org.za" 6667)
   ("LagNet: AF, ZA, Johannesburg" LagNet "mystery.lagnet.org.za" 6667)
-  ("Libera.Chat: Random server" Libera.Chat "irc.libera.chat" 6667)
-  ("Libera.Chat: Random Europe server" Libera.Chat "irc.eu.libera.chat" 6667)
-  ("Libera.Chat: Random US & Canada server" Libera.Chat "irc.us.libera.chat" 6667)
-  ("Libera.Chat: Random Australia & New Zealand server" Libera.Chat "irc.au.libera.chat" 6667)
-  ("Libera.Chat: Random East Asia server" Libera.Chat "irc.ea.libera.chat" 6667)
-  ("Libera.Chat: IPv4 only server" Libera.Chat "irc.ipv4.libera.chat" 6667)
-  ("Libera.Chat: IPv6 only server" Libera.Chat "irc.ipv6.libera.chat" 6667)
+  ("Libera.Chat: Random server" Libera.Chat "irc.libera.chat"
+   ((6665 6667) (8000 8002)) (6697 7000 7070))
+  ;; If not deprecating this option, use ^ for the rest of these servers.
+  ("Libera.Chat: Random Europe server" Libera.Chat "irc.eu.libera.chat" 6667 6697)
+  ("Libera.Chat: Random US & Canada server" Libera.Chat "irc.us.libera.chat" 6667 6697)
+  ("Libera.Chat: Random Australia & New Zealand server" Libera.Chat "irc.au.libera.chat" 6667 6697)
+  ("Libera.Chat: Random East Asia server" Libera.Chat "irc.ea.libera.chat" 6667 6697)
+  ("Libera.Chat: IPv4 only server" Libera.Chat "irc.ipv4.libera.chat" 6667 6697)
+  ("Libera.Chat: IPv6 only server" Libera.Chat "irc.ipv6.libera.chat" 6667 6697)
   ("Librenet: Random server" Librenet "irc.librenet.net" 6667)
   ("LinkNet: Random server" LinkNet "irc.link-net.org" ((6667 6669)))
   ("LinuxChix: Random server" LinuxChix "irc.linuxchix.org" 6667)
@@ -349,7 +350,7 @@ erc-server-alist
   ("Novernet: Random server" Novernet "irc.novernet.com" ((6665 6669) 7000 ))
   ("Nullrouted: Random server" Nullrouted "irc.nullrouted.org" ((6666 6669) 7000 ))
   ("NullusNet: Random server" NullusNet "irc.nullus.net" 6667)
-  ("OFTC: Random server" OFTC "irc.oftc.net" ((6667 6670) 7000))
+  ("OFTC: Random server" OFTC "irc.oftc.net" ((6667 6670) 7000) (6697 9999))
   ("OpChat: Random server" OpChat "irc.opchat.org" ((6667 6669)))
   ("Othernet: Random server" Othernet "irc.othernet.org" 6667)
   ("Othernet: US, FL, Miami" Othernet "miami.fl.us.othernet.org" 6667)
@@ -472,13 +473,14 @@ erc-server-alist
   ("ZUHnet: Random server" ZUHnet "irc.zuh.net" 6667)
   ("Zurna: Random server" Zurna "irc.zurna.net" 6667))
   "Alist of irc servers.
-Each server is a list (NAME NET HOST PORTS) where
+Each server is a list (NAME NET HOST PORTS TLS-PORTS) where
 NAME is a name for that server,
 NET is a symbol indicating to which network from `erc-networks-alist'
   this server corresponds,
 HOST is the servers hostname and
 PORTS is either a number, a list of numbers, or a list of port ranges."
-  :type '(alist :key-type (string :tag "Name")
+  :package-version '(ERC . "5.6") ; FIXME sync on release
+  :type `(alist :key-type (string :tag "Name")
 		:value-type
 		(group symbol (string :tag "Hostname")
 		       (choice :tag "Ports"
@@ -486,7 +488,15 @@ erc-server-alist
 			       (repeat :tag "List of ports or ranges"
 				       (choice (integer :tag "Port number")
 					       (list :tag "Port range"
-						     integer integer)))))))
+						     integer integer))))
+                       (choice :tag "TLS ports"
+                               (integer :tag "TLS port number")
+                               (repeat :tag "List of TLS ports or ranges"
+                                       (choice (integer :tag "TLS port number")
+                                               (list :tag "TLS port range"
+                                                     integer integer)))))))
+(make-obsolete-variable 'erc-server-alist
+                        "specify `:server' with `erc-tls'." "30.1")
 
 (defcustom erc-networks-alist
   '((4-irc "4-irc.com")
@@ -1535,7 +1545,7 @@ erc-ports-list
   (erc-ports-list \\='((1 5))) => (1 2 3 4 5)
   (erc-ports-list \\='(1 (3 5))) => (1 3 4 5)"
   (let (result)
-    (dolist (p ports)
+    (dolist (p (ensure-list ports))
       (cond ((numberp p)
 	     (push p result))
 	    ((listp p)
@@ -1544,31 +1554,32 @@ erc-ports-list
 				 result)))))
     (nreverse result)))
 
-;;;###autoload
-(defun erc-server-select ()
-  "Interactively select a server to connect to using `erc-server-alist'."
-  (interactive)
+(defun erc-networks--server-select ()
+  "Prompt for a server in `erc-server-alist' and return its irc(s):// URL.
+Choose port at random if multiple candidates exist, but always
+prefer TLS without asking.  When a port can't be determined,
+return the host alone sans URL formatting (for compatibility)."
   (let* ((completion-ignore-case t)
 	 (net (intern
 	       (completing-read "Network: "
 				(delete-dups
 				 (mapcar (lambda (x)
-					   (list (symbol-name (nth 1 x))))
+                                           (list (nth 1 x)))
 					 erc-server-alist)))))
-	 (srv (assoc
-	       (completing-read "Server: "
-				(delq nil
-				      (mapcar (lambda (x)
-						(when (equal (nth 1 x) net)
-						  x))
-					      erc-server-alist)))
-	       erc-server-alist))
+         (s-choose (lambda (entry)
+                     (and (equal (nth 1 entry) net)
+                          (if-let ((b (string-search ": " (car entry))))
+                              (cons (format "%s (%s)" (nth 2 entry)
+                                            (substring (car entry) (+ b 2)))
+                                    (cdr entry))
+                            entry))))
+         (s-entries (delq nil (mapcar s-choose erc-server-alist)))
+         (srv (assoc (completing-read "Server: " s-entries) s-entries))
 	 (host (nth 2 srv))
-	 (ports (if (listp (nth 3 srv))
-		    (erc-ports-list (nth 3 srv))
-		  (list (nth 3 srv))))
-         (port (and ports (seq-random-elt ports))))
-    (erc :server host :port port)))
+         (pspec (nthcdr 3 srv))
+         (ports (erc-ports-list (or (cadr pspec) (car pspec))))
+         (scheme (if (cdr pspec) "ircs" "irc")))
+    (if ports (format "%s://%s:%d" scheme host (seq-random-elt ports)) host)))
 
 ;;; The following experimental
 ;; It does not work yet, help me with it if you
@@ -1605,7 +1616,7 @@ erc-get
 		     items nil)))))
     val))
 
-(erc-get 'pals 'Libera.Chat)
+;; (erc-get 'pals 'Libera.Chat)
 
 (provide 'erc-networks)
 
diff --git a/lisp/erc/erc.el b/lisp/erc/erc.el
index e23185934f7..cb6794a40d2 100644
--- a/lisp/erc/erc.el
+++ b/lisp/erc/erc.el
@@ -2354,13 +2354,17 @@ erc--ensure-url
     (setq input (concat "irc://" input)))
   input)
 
+(defvar erc--prompt-for-server-function nil)
+
 ;;;###autoload
 (defun erc-select-read-args ()
   "Prompt the user for values of nick, server, port, and password.
 With prefix arg, also prompt for user and full name."
   (let* ((input (let ((d (erc-compute-server)))
-                  (read-string (format "Server or URL (default is %S): " d)
-                               nil 'erc-server-history-list d)))
+                  (if erc--prompt-for-server-function
+                      (funcall erc--prompt-for-server-function)
+                    (read-string (format "Server or URL (default is %S): " d)
+                                 nil 'erc-server-history-list d))))
          ;; For legacy reasons, also accept a URL without a scheme.
          (url (url-generic-parse-url (erc--ensure-url input)))
          (server (url-host url))
@@ -2419,6 +2423,14 @@ erc--with-entrypoint-environment
        (cl-progv ,syms ,vals
          ,@body))))
 
+;;;###autoload
+(defun erc-server-select ()
+  "Interactively connect to a server from `erc-server-alist'."
+  (declare (obsolete erc-tls "30.1"))
+  (interactive)
+  (let ((erc--prompt-for-server-function #'erc-networks--server-select))
+    (call-interactively #'erc)))
+
 ;;;###autoload
 (cl-defun erc (&key (server (erc-compute-server))
                     (port   (erc-compute-port))
diff --git a/test/lisp/erc/erc-networks-tests.el b/test/lisp/erc/erc-networks-tests.el
index f0fcbbc81c6..e95d99c128f 100644
--- a/test/lisp/erc/erc-networks-tests.el
+++ b/test/lisp/erc/erc-networks-tests.el
@@ -1750,4 +1750,22 @@ erc-networks--determine
     (should (eq (erc-networks--determine)
                 erc-networks--name-missing-sentinel))))
 
+(ert-deftest erc-ports-list ()
+  (with-suppressed-warnings ((obsolete erc-server-alist))
+    (let* ((srv (assoc "Libera.Chat: Random server" erc-server-alist)))
+      (should (equal (erc-ports-list (nth 3 srv))
+                     '(6665 6666 6667 8000 8001 8002)))
+      (should (equal (erc-ports-list (nth 4 srv))
+                     '(6697 7000 7070))))
+
+    (let* ((srv (assoc "Libera.Chat: Random Europe server" erc-server-alist)))
+      (should (equal (erc-ports-list (nth 3 srv)) '(6667)))
+      (should (equal (erc-ports-list (nth 4 srv)) '(6697))))
+
+    (let* ((srv (assoc "OFTC: Random server" erc-server-alist)))
+      (should (equal (erc-ports-list (nth 3 srv))
+                     '(6667 6668 6669 6670 7000)))
+      (should (equal (erc-ports-list (nth 4 srv))
+                     '(6697 9999))))))
+
 ;;; erc-networks-tests.el ends here
diff --git a/test/lisp/erc/erc-tests.el b/test/lisp/erc/erc-tests.el
index 80c7c708fc5..930bef2aaa6 100644
--- a/test/lisp/erc/erc-tests.el
+++ b/test/lisp/erc/erc-tests.el
@@ -1657,6 +1657,44 @@ erc--interactive
                          (erc-server-connect-function
                           erc-open-network-stream))))))))
 
+(ert-deftest erc-server-select ()
+  (let (calls env)
+    (cl-letf (((symbol-function 'user-login-name)
+               (lambda (&optional _) "tester"))
+              ((symbol-function 'erc-open)
+               (lambda (&rest r)
+                 (push `((erc-join-buffer ,erc-join-buffer)
+                         (erc--display-context ,@erc--display-context)
+                         (erc-server-connect-function
+                          ,erc-server-connect-function))
+                       env)
+                 (push r calls))))
+
+      (ert-info ("Selects Libera.Chat Europe, automatic TSL")
+        (ert-simulate-keys "Libera.Chat\rirc.eu.\t\r\r\r"
+          (with-suppressed-warnings ((obsolete erc-server-select))
+            (call-interactively #'erc-server-select)))
+        (should (equal (pop calls)
+                       '("irc.eu.libera.chat" 6697 "tester" "unknown" t nil
+                         nil nil nil nil "user" nil)))
+        (should (equal (pop env)
+                       '((erc-join-buffer window)
+                         (erc--display-context (erc-interactive-display . erc))
+                         (erc-server-connect-function erc-open-tls-stream)))))
+
+      (ert-info ("Selects entry that doesn't support TLS")
+        (ert-simulate-keys "IRCnet\rirc.fr.\t\rdummy\r\r"
+          (with-suppressed-warnings ((obsolete erc-server-select))
+            (call-interactively #'erc-server-select)))
+        (should (equal (pop calls)
+                       '("irc.fr.ircnet.net" 6667 "dummy" "unknown" t nil
+                         nil nil nil nil "user" nil)))
+        (should (equal (pop env)
+                       '((erc-join-buffer window)
+                         (erc--display-context (erc-interactive-display . erc))
+                         (erc-server-connect-function
+                          erc-open-network-stream))))))))
+
 (defun erc-tests--make-server-buf (name)
   (with-current-buffer (get-buffer-create name)
     (erc-mode)
-- 
2.41.0