bug#60933: 30.0.50; ERC >5.5: Make buttonizing more extensible

["J.P." <jp@neverwas.me>]

[-- Attachment #1: Type: text/plain, Size: 1072 bytes --]

Some of the recent changes to this module were perhaps not handled as
sensibly as they could've been. The broad strokes seem sane enough, but
a few key aspects affecting the user experience were left too rough
around the edges for my taste. My main gripe involves the partial
deprecation of the `nicknames' entry for the option `erc-button-alist'.
After some reflection, I'm now of the opinion that imposing a specific
constraint on a specific slot in a specific default entry that may not
even be present after customization is just too messy a notion to foist
on users or to try and communicate in a doc string.

A better approach, I now feel, is to remove the troublesome `nicknames'
entry altogether and take special care behind the scenes to gracefully
accommodate users who still have it present in their configs. The
attached changes attempt something like this and also tidy up other odds
and ends in erc-button.el. Note that the second patch also appears in
bug#63569 and is only included here for its ERC-NEWS changes, which
touch on some of the above. Thanks.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-5.6-Deprecate-nicknames-entry-in-erc-button-alist.patch --]
[-- Type: text/x-patch, Size: 13426 bytes --]

From 627670ff54fbbd0b5278e538cea34c275ffc249d Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <jp@neverwas.me>
Date: Thu, 1 Jun 2023 22:07:03 -0700
Subject: [PATCH 1/2] [5.6] Deprecate nicknames entry in erc-button-alist

* lisp/erc/erc-button.el (erc-button-mode, erc-button-enable,
erc-button-disable): Restore running `erc-button-setup' on
`erc-mode-hook' and also immediately in all ERC buffers.
Do this instead of calling `erc-button--check-nicknames-entry.
(erc-button-buttonize-nicks): Mention that this must be non-nil for
all but the most basic client functionality.
(erc-button-alist): Remove `nicknames' entry.  Describe deprecation,
replacement behavior, and escape hatches in doc string.  Update and
improve custom type definition.
(erc-button-keys-added): Deprecate because unused and misleading: keys
are bound during module init.
(erc-button--has-nickname-entry): New variable to indicate whether to
follow legacy code path when a `nicknames' entry exists in
`erc-button-alist'.
(erc-button-setup): Rewrite to provide warnings about deprecated
values for `erc-button-alist'.
(erc-button-nickname-callback-function): Add escape hatch for those
needing a custom callback for what was the default `nickname' entry in
`erc-button-alist'.
(erc-button-add-buttons): Always run `erc-button-add-nickname-buttons'
unless `erc-button--has-nickname-entry' is non-nil.
(erc-button--maybe-warn-arbitrary-sexp, erc-button--extract-form):
Rename former to latter and abstain from emitting a warning.
(erc-button--check-nicknames-entry): Remove unused function.
(erc-button-add-nickname-buttons): Defer to `erc-button--extract-form'
for determining value of third FORM slot of entry.
(erc-button-add-buttons-1): Call renamed version of
`erc-button--maybe-warn-arbitrary-sexp'.  (Bug#60933)
---
 lisp/erc/erc-button.el | 134 ++++++++++++++++++++++++-----------------
 1 file changed, 78 insertions(+), 56 deletions(-)

diff --git a/lisp/erc/erc-button.el b/lisp/erc/erc-button.el
index 33b93ff6744..22ef3dc4846 100644
--- a/lisp/erc/erc-button.el
+++ b/lisp/erc/erc-button.el
@@ -52,13 +52,15 @@ erc-button
 ;;;###autoload(autoload 'erc-button-mode "erc-button" nil t)
 (define-erc-module button nil
   "This mode buttonizes all messages according to `erc-button-alist'."
-  ((erc-button--check-nicknames-entry)
-   (add-hook 'erc-insert-modify-hook #'erc-button-add-buttons 'append)
+  ((add-hook 'erc-insert-modify-hook #'erc-button-add-buttons 'append)
    (add-hook 'erc-send-modify-hook #'erc-button-add-buttons 'append)
+   (add-hook 'erc-mode-hook #'erc-button-setup)
+   (unless erc--updating-modules-p (erc-buffer-filter #'erc-button-setup))
    (add-hook 'erc--tab-functions #'erc-button-next)
    (erc--modify-local-map t "<backtab>" #'erc-button-previous))
   ((remove-hook 'erc-insert-modify-hook #'erc-button-add-buttons)
    (remove-hook 'erc-send-modify-hook #'erc-button-add-buttons)
+   (remove-hook 'erc-mode-hook #'erc-button-setup)
    (remove-hook 'erc--tab-functions #'erc-button-next)
    (erc--modify-local-map nil "<backtab>" #'erc-button-previous)))
 
@@ -103,7 +105,10 @@ erc-button-wrap-long-urls
   :type '(choice integer boolean))
 
 (defcustom erc-button-buttonize-nicks t
-  "Flag indicating whether nicks should be buttonized or not."
+  "Flag indicating whether nicks should be buttonized.
+Note that beginning in ERC 5.6, some functionality provided by
+other modules, such as `fill-wrap', may depend on this option
+being non-nil."
   :type 'boolean)
 
 (defcustom erc-button-rfc-url "https://tools.ietf.org/html/rfc%s"
@@ -126,8 +131,7 @@ erc-button-alist
   ;; a button, it makes no sense to optimize performance by
   ;; bytecompiling lambdas in this alist.  On the other hand, it makes
   ;; things hard to maintain.
-  '((nicknames 0 erc-button-buttonize-nicks erc-nick-popup 0)
-    (erc-button-url-regexp 0 t browse-url-button-open-url 0)
+  '((erc-button-url-regexp 0 t browse-url-button-open-url 0)
     ;; ("<URL: *\\([^<> ]+\\) *>" 0 t browse-url-button-open-url 1)
 ;;; ("(\\(\\([^~\n \t@][^\n \t@]*\\)@\\([a-zA-Z0-9.:-]+\\)\\)" 1 t finger 2 3)
     ;; emacs internal
@@ -159,41 +163,45 @@ erc-button-alist
   strings, or an alist with the strings in the car.  Note that
   entries in lists or alists are considered to be nicks or other
   complete words.  Therefore they are enclosed in \\< and \\>
-  while searching.  REGEXP can also be the symbol
-  `nicknames', which matches the nickname of any user on the
-  current server.
+  while searching.  Also, use of the special symbol `nicknames'
+  for this slot was deprecated in ERC 5.6, but users can still
+  use `erc-button-buttonize-nicks' to control whether nicks get
+  buttonized.  And because customizing a corresponding CALLBACK
+  is no longer possible, an escape hatch has been provided via
+  the variable `erc-button-nickname-callback-function'.
 
 BUTTON is the number of the regexp grouping actually matching the
-  button.  This is ignored if REGEXP is `nicknames'.
+  button.
 
 FORM is either a boolean or a special variable whose value must
-  be non-nil for the button to be added.  When REGEXP is the
-  special symbol `nicknames', FORM must be the symbol
-  `erc-button-buttonize-nicks'.  Anything else is deprecated.
-  For all other entries, FORM can also be a function to call in
-  place of `erc-button-add-button' with the exact same arguments.
-  When FORM is also a special variable, ERC disregards the
-  variable and calls the function.
+  be non-nil for the button to be added.  It can also be a
+  function to call in place of `erc-button-add-button' with the
+  exact same arguments.  When FORM is also a special variable,
+  ERC disregards the variable and calls the function.  Note that
+  arbitrary s-expressions were deprecated in ERC 5.6 and may not
+  be respected in the future.  If necessary, users can instead
+  supply a function that calls `erc-button-add-button' after
+  first checking some condition.
 
 CALLBACK is the function to call when the user push this button.
   CALLBACK can also be a symbol.  Its variable value will be used
   as the callback function.
 
 PAR is a number of a regexp grouping whose text will be passed to
-  CALLBACK.  There can be several PAR arguments.  If REGEXP is
-  `nicknames', these are ignored, and CALLBACK will be called with
-  the nickname matched as the argument."
+  CALLBACK.  There can be several PAR arguments."
   :package-version '(ERC . "5.6") ; FIXME sync on release
   :type '(repeat
           (list :tag "Button"
                 (choice :tag "Matches"
                         regexp
                         (variable :tag "Variable containing regexp")
-                        (const :tag "Nicknames" nicknames))
+                        (repeat :tag "List of words" string)
+                        (alist :key-type string :value-type sexp))
                 (integer :tag "Number of the regexp section that matches")
                 (choice :tag "When to buttonize"
                         (const :tag "Always" t)
-                        (sexp :tag "Only when this evaluates to non-nil"))
+                        (function :tag "Alternative buttonizing function")
+                        (variable :tag "Variable containing boolean"))
                 (function :tag "Function to call when button is pressed")
                 (repeat :tag "Sections of regexp to send to the function"
                         :inline t
@@ -239,15 +247,37 @@ erc-button-syntax-table
 (defvar erc-button-keys-added nil
   "Internal variable used to keep track of whether we've added the
 global-level ERC button keys yet.")
+(make-obsolete-variable 'erc-button-keys-added "no longer relevant" "30.1")
+
+(defvar-local erc-button--has-nickname-entry nil
+  "Whether `erc-button-alist' contains a legacy `nicknames' entry.")
 
-;; Maybe deprecate this function and `erc-button-keys-added' if they
-;; continue to go unused for a another version (currently 5.6).
 (defun erc-button-setup ()
-  "Add ERC mode-level button movement keys.  This is only done once."
-  ;; Add keys.
-  (unless erc-button-keys-added
-    (define-key erc-mode-map (kbd "<backtab>") #'erc-button-previous)
-    (setq erc-button-keys-added t)))
+  "Perform major-mode setup for ERC's button module.
+Note that prior to ERC 5.6, this function used to modify
+`erc-mode-map', but that's now handled by the mode toggles
+themselves."
+  (setq erc-button-keys-added t)
+  (cl-assert (derived-mode-p 'erc-mode))
+  (dolist (entry erc-button-alist)
+    (pcase entry
+      ((or `(nicknames ,_ ,sym . ,_) `('nicknames ,_ ,sym . ,_))
+       (setq erc-button--has-nickname-entry t)
+       (unless (eq sym 'erc-button-buttonize-nicks)
+         (erc--warn-once-before-connect 'erc-button-mode
+           "The legacy `nicknames' entry in `erc-button-alist'"
+           " is deprecated.  See doc string for details.")))
+      ((and `(,_ ,_ ,form . ,_)
+            (guard (not (or (and (symbolp form)
+                                 (special-variable-p form))
+                            (functionp form)))))
+       (erc--warn-once-before-connect 'erc-button-mode
+         "Arbitrary sexps for the third, FORM slot of `erc-button-alist'"
+         " entries are deprecated. Either use a variable or a function"
+         " that conditionally calls `erc-button-add-button'.")))))
+
+(defvar erc-button-nickname-callback-function #'erc-nick-popup
+  "Escape hatch for those needing a different nickname callback.")
 
 (defun erc-button-add-buttons ()
   "Find external references in the current buffer and make buttons of them.
@@ -261,6 +291,11 @@ erc-button-add-buttons
             (alist erc-button-alist)
             regexp)
         (erc-button-remove-old-buttons)
+        (unless (or erc-button--has-nickname-entry
+                    (not erc-button-buttonize-nicks))
+          (erc-button-add-nickname-buttons
+           `(_ _ erc-button--modify-nick-function
+               ,erc-button-nickname-callback-function)))
         (dolist (entry alist)
           (if (or (eq (car entry) 'nicknames)
                   ;; Old form retained for backward compatibility.
@@ -284,28 +319,18 @@ erc-button-add-buttons
                         (concat "\\<" (regexp-quote (car elem)) "\\>")
                         entry)))))))))))
 
-(defun erc-button--maybe-warn-arbitrary-sexp (form)
-  (cl-assert (not (booleanp form))) ; covered by caller
+(defun erc-button--extract-form (form)
   ;; If a special-variable is also a function, favor the function.
-  (cond ((functionp form) form)
-        ((and (symbolp form) (special-variable-p form)) (symbol-value form))
-        (t (unless (get 'erc-button--maybe-warn-arbitrary-sexp
-                        'warned-arbitrary-sexp)
-             (put 'erc-button--maybe-warn-arbitrary-sexp
-                  'warned-arbitrary-sexp t)
-             (lwarn 'erc :warning (concat "Arbitrary sexps for the third FORM"
-                                          " slot of `erc-button-alist' entries"
-                                          " have been deprecated.")))
-           (eval form t))))
-
-(defun erc-button--check-nicknames-entry ()
-  ;; This helper exists because the module is defined after its options.
-  (when (eq major-mode 'erc-mode)
-    (unless (eq (nth 1 (alist-get 'nicknames erc-button-alist))
-                'erc-button-buttonize-nicks)
-      (erc-button--display-error-notice-with-keys-and-warn
-       "Values other than `erc-button-buttonize-nicks' in the third slot of "
-       "the `nicknames' entry of `erc-button-alist' are deprecated."))))
+  (cond ((eq t form) t)
+        ((functionp form) form)
+        ((and (symbolp form) (special-variable-p form))
+         (while (let ((val (symbol-value form)))
+                  (prog1 (and (not (eq val form))
+                              (symbolp val)
+                              (special-variable-p val))
+                    (setq form val))))
+         form)
+        (t (eval form t))))
 
 (cl-defstruct erc-button--nick
   ( bounds nil :type cons
@@ -405,12 +430,10 @@ erc-button-add-nickname-buttons
   "Search through the buffer for nicknames, and add buttons."
   (let ((form (nth 2 entry))
         (fun (nth 3 entry))
+        (erc-button-buttonize-nicks (and erc-button-buttonize-nicks
+                                         erc-button--modify-nick-function))
         bounds word)
-    (when (eq form 'erc-button-buttonize-nicks)
-      (setq form (and (symbol-value form) erc-button--modify-nick-function)))
-    (when (or (functionp form)
-              (eq t form)
-              (and form (erc-button--maybe-warn-arbitrary-sexp form)))
+    (when (and form (setq form (erc-button--extract-form form)))
       (goto-char (point-min))
       (while (erc-forward-word)
         (when (setq bounds (erc-bounds-of-word-at-point))
@@ -456,8 +479,7 @@ erc-button-add-buttons-1
                        (and-let*
                            ((raw-form (nth 2 entry))
                             (res (or (eq t raw-form)
-                                     (erc-button--maybe-warn-arbitrary-sexp
-                                      raw-form))))
+                                     (erc-button--extract-form raw-form))))
                          (if (functionp res) res #'erc-button-add-button)))))
       (let ((start (match-beginning (nth 1 entry)))
             (end (match-end (nth 1 entry)))
-- 
2.40.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-5.6-Allow-ERC-modules-to-extend-erc-nick-popup-alist.patch --]
[-- Type: text/x-patch, Size: 6797 bytes --]

From 1c194ad4237d3544df9c6af7567d1764e1ba8411 Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <jp@neverwas.me>
Date: Sun, 18 Dec 2022 19:01:40 -0800
Subject: [PATCH 2/2] [5.6] Allow ERC modules to extend erc-nick-popup-alist

* etc/ERC-NEWS: Mention superficial changes to `erc-nick-popup-alist'.
* lisp/erc/erc-button.el (erc-nick-popup-alist): Accept alternate
shape for type with strings associated with functions instead of
arbitrary sexps.
(erc-button-cmd-KICK, erc-button-cmd-MSG): New functions to serve as
wrappers for `erc-cmd-KICK' and `erc-cmd-MSG', respectively.  The
first also fixes a bug in which all but the first token of a given
"reason" would be omitted from the ":trailing" portion of an outgoing
KICK message.
(erc-button--nick-popup-alist): New variable to help built-in modules
expose special actions to `erc-nick-popup' without touching
`erc-nick-popup-alist'.
(erc-nick-popup): Present both `erc--nick-popup-alist' and
`erc-nick-popup-alist' to the invoking user.  Accommodate functions as
well as arbitrary sexps.  (bug#63569)
---
 etc/ERC-NEWS           | 18 ++++++++----
 lisp/erc/erc-button.el | 64 +++++++++++++++++++++++++++++++-----------
 2 files changed, 59 insertions(+), 23 deletions(-)

diff --git a/etc/ERC-NEWS b/etc/ERC-NEWS
index e9ec9e2caab..836c6ff8ee8 100644
--- a/etc/ERC-NEWS
+++ b/etc/ERC-NEWS
@@ -116,13 +116,19 @@ asking users who've customized this option to switch to
 that some other solution, like automatic migration, is justified,
 please make that known on the bug list.
 
-** The 'nicknames' entry in 'erc-button-alist' is officially exceptional.
+** 'erc-button-alist' and 'erc-nick-popup-alist' have evolved slightly.
 It's no secret that the 'buttons' module treats potential nicknames
-specially.  To simplify ERC's move to next-gen "rich UI" extensions,
-this special treatment is being canonized.  From now on, all values
-other than the symbol 'erc-button-buttonize-nicks' appearing in the
-"FORM" field (third element) of this entry are considered deprecated
-and will incur a warning.
+specially.  This is perhaps most evident in its treatment of the
+'nicknames' entry in 'erc-button-alist'.  Indeed, to simplify ERC's
+move to next-gen "rich UI" extensions, this special treatment is being
+canonized.  From here on out, this entry will no longer appear in the
+option's default value but will instead be applied implicitly so long
+as the option 'erc-button-buttonize-nicks' is non-nil, which it is by
+default.  Relatedly, the option 'erc-nick-popup-alist' now favors
+functions, which ERC calls non-interactively, over arbitrary
+s-expressions, which ERC will continue to honor.  Although the default
+lineup remains functionally equivalent, its members have all been
+updated accordingly.
 
 ** Option 'erc-query-on-unjoined-chan-privmsg' restored and renamed.
 This option was accidentally removed from the default client in ERC
diff --git a/lisp/erc/erc-button.el b/lisp/erc/erc-button.el
index 22ef3dc4846..374d3fd0201 100644
--- a/lisp/erc/erc-button.el
+++ b/lisp/erc/erc-button.el
@@ -682,20 +682,20 @@ erc-browse-emacswiki-lisp
 ;;; Nickname buttons:
 
 (defcustom erc-nick-popup-alist
-  '(("DeOp"  . (erc-cmd-DEOP nick))
-    ("Kick"  . (erc-cmd-KICK (concat nick " "
-                                     (read-from-minibuffer
-                                      (concat "Kick " nick ", reason: ")))))
-    ("Msg"   . (erc-cmd-MSG (concat nick " "
-                                    (read-from-minibuffer
-                                     (concat "Message to " nick ": ")))))
-    ("Op"    . (erc-cmd-OP nick))
-    ("Query" . (erc-cmd-QUERY nick))
-    ("Whois" . (erc-cmd-WHOIS nick))
-    ("Lastlog" . (erc-cmd-LASTLOG nick)))
+  '(("DeOp"  . erc-cmd-DEOP)
+    ("Kick"  . erc-button-cmd-KICK)
+    ("Msg"   . erc-button-cmd-MSG)
+    ("Op"    . erc-cmd-OP)
+    ("Query" . erc-cmd-QUERY)
+    ("Whois" . erc-cmd-WHOIS)
+    ("Lastlog" . erc-cmd-LASTLOG))
   "An alist of possible actions to take on a nickname.
-An entry looks like (\"Action\" . SEXP) where SEXP is evaluated with
-the variable `nick' bound to the nick in question.
+For all entries (ACTION . FUNC), ERC offers ACTION as a possible
+completion item and calls the selected entry's FUNC with the
+buttonized nickname at point as the only argument.  For
+historical reasons, FUNC can also be an arbitrary sexp, in which
+case, ERC binds the nick in question to the variable `nick' and
+evaluates the expression.
 
 Examples:
  (\"DebianDB\" .
@@ -703,18 +703,48 @@ erc-nick-popup-alist
    (format
     \"ldapsearch -x -P 2 -h db.debian.org -b dc=debian,dc=org ircnick=%s\"
     nick)))"
+  :package-version '(ERC . "5.6") ; FIXME sync on release
   :type '(repeat (cons (string :tag "Op")
-                       sexp)))
+                       (choice function sexp))))
+
+(defun erc-button-cmd-KICK (nick)
+  "Prompt for a reason, then kick NICK via `erc-cmd-KICK'.
+In server buffers, also prompt for a channel."
+  (erc-cmd-KICK
+   (or (and erc--target (erc-default-target))
+       (let ((targets (mapcar (lambda (b)
+                                (cons (erc--target-string
+                                       (buffer-local-value 'erc--target b))
+                                      b))
+                              (erc-channel-list erc-server-process))))
+         (completing-read (format "Channel (%s): " (caar targets))
+                          targets (pcase-lambda (`(,_ . ,buf))
+                                    (with-current-buffer buf
+                                      (erc-get-channel-user nick)))
+                          t nil t (caar targets))))
+   nick
+   (read-from-minibuffer "Reason: ")))
+
+(defun erc-button-cmd-MSG (nick)
+  "Prompt for a message to NICK, and send it via `erc-cmd-MSG'."
+  (let ((msg (read-from-minibuffer (concat "Message to " nick ": "))))
+    (erc-cmd-MSG (concat nick " " msg))))
+
+(defvar-local erc-button--nick-popup-alist nil
+  "Internally controlled items for `erc-nick-popup-alist'.")
 
 (defun erc-nick-popup (nick)
   (let* ((completion-ignore-case t)
+         (alist (append erc-nick-popup-alist erc-button--nick-popup-alist))
          (action (completing-read (format-message
                                    "What action to take on `%s'? " nick)
-                                  erc-nick-popup-alist))
-         (code (cdr (assoc action erc-nick-popup-alist))))
+                                  alist))
+         (code (cdr (assoc action alist))))
     (when code
       (erc-set-active-buffer (current-buffer))
-      (eval code `((nick . ,nick))))))
+      (if (functionp code)
+          (funcall code nick)
+        (eval code `((nick . ,nick)))))))
 
 ;;; Callback functions
 (defun erc-button-describe-symbol (symbol-name)
-- 
2.40.1