Re: bug#60935: 30.0.50; ERC >5.5: Improve ERC's treatment of customization groups

From e8a6cbfaffb91019fa9a4a9aa3cae3c7896a6ff5 Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <[email protected]>
Date: Wed, 15 Mar 2023 06:39:32 -0700
Subject: [PATCH 0/3] *** NOT A PATCH ***

v5. Don't touch `standard-value'. Instead, favor a "CHANGED" state for
`erc-modules'. Use a custom getter to spoof a "STANDARD" state for all module
mode vars so their widgets remain dormant and only protest when disturbed.

F. Jason Park (3):
  [5.6] Don't associate ERC modules with undefined groups
  [5.6] Warn when setting minor-mode vars for ERC modules
  [5.6] Fill doc strings for ERC modules.

 lisp/erc/erc-capab.el      |   1 +
 lisp/erc/erc-common.el     | 158 ++++++++++++++++++++++++++++++++++---
 lisp/erc/erc.el            |   3 +-
 test/lisp/erc/erc-tests.el |  82 +++++++++++++++----
 4 files changed, 213 insertions(+), 31 deletions(-)

Interdiff:
diff --git a/lisp/erc/erc-common.el b/lisp/erc/erc-common.el
index 522803b91e2..3a148dc1196 100644
--- a/lisp/erc/erc-common.el
+++ b/lisp/erc/erc-common.el
@@ -162,14 +162,20 @@ erc--assemble-toggle
                                        erc-modules)))
                          `(,(if val v `(not ,v)))))
                (let ((erc--inside-mode-toggle-p t))
-                 (custom-set-variables
-                  `(erc-modules ',(,(if val 'cons 'delq)
-                                   ',(erc--normalize-module-symbol name)
-                                   erc-modules)))))
+                 ;; Make the widget show "CHANGED outside Customize."
+                 ;; Ideally, it would show "SET for current session"
+                 ;; instead, but using `customize-set-variable' here
+                 ;; or calling `customize-mark-as-set' afterward isn't
+                 ;; enough if `customized-value' and `standard-value'
+                 ;; match but differ from `saved-value' because the
+                 ;; widget will see a `standard' instead of a `set'
+                 ;; state.  And clicking "apply and save" won't update
+                 ;; `custom-file'.  See bug#12864 "Make state button
+                 ;; interaction less confusing".
+                 (setopt erc-modules (,(if val 'cons 'delq)
+                                      ',(erc--normalize-module-symbol name)
+                                      erc-modules))))
              (setq ,mode ,val)
-             ;; Avoid "changed" state from `erc-update-modules'
-             (unless (called-interactively-p 'any)
-               (put ',mode 'standard-value (list ,val)))
              ,@body)))))
 
 ;; This is a migration helper that determines a module's `:group'
@@ -203,13 +209,15 @@ erc--find-group
         (throw 'found found)))
     'erc))
 
-(defun erc--custom-set-minor-mode (variable value)
-  (let ((name (get variable 'erc-module))
-        (erc--inside-mode-toggle-p t))
-    (custom-set-variables
-     `(erc-modules
-       ',(if value (cl-pushnew name erc-modules) (delq name erc-modules))))
-    (custom-set-minor-mode variable value)))
+(defun erc--neuter-custom-variable-state (variable)
+  "Lie to Customize about VARIABLE's true state.
+Do so by always returning its standard value, namely nil."
+  ;; Make a module's global minor-mode toggle blind to Customize, so
+  ;; that `customize-variable-state' never sees it as "changed",
+  ;; regardless of its value.  This snippet is
+  ;; `custom--standard-value' from Emacs 28+.
+  (cl-assert (null (eval (car (get variable 'standard-value)) t)))
+  nil)
 
 ;; This exists as a separate, top-level function to prevent the byte
 ;; compiler from warning about widget-related dependencies not being
@@ -240,12 +248,16 @@ erc--prepare-custom-module-type
   `(let* ((name (erc--normalize-module-symbol ',name))
           (fmtd (format " `%s' " name)))
      `(boolean
-       :button-face '(error custom-button)
-       :format "%{%t%}: %[Deprecated Toggle%] \n%d\n"
-       :doc ,(concat "Setting a module's minor-mode variable is "
-                     (propertize "ineffective" 'face 'error) ".\nPlease add"
-                     fmtd "directly to `erc-modules' instead.\nYou can do so"
-                     " now by clicking the scary button above.")
+       :button-face '(custom-variable-obsolete custom-button)
+       :format "%{%t%}: %[Deprecated Toggle%] \n%h\n"
+       :documentation-property
+       ,(lambda (_)
+          (let ((hasp (memq name erc-modules)))
+            (concat "Setting a module's minor-mode variable is "
+                    (propertize "ineffective" 'face 'error)
+                    ".\nPlease " (if hasp "remove" "add") fmtd
+                    (if hasp "from" "to") " `erc-modules' directly instead.\n"
+                    "You can do so now by clicking the scary button above.")))
        :help-echo ,(lambda (_)
                      (let ((hasp (memq name erc-modules)))
                        (concat (if hasp "Remove" "Add") fmtd
@@ -312,7 +324,7 @@ define-erc-module
 \n%s" name name doc))
          :global ,(not local-p)
          :group (erc--find-group ',name ,(and alias (list 'quote alias)))
-         ,@(unless local-p '(:set #'erc--custom-set-minor-mode))
+         ,@(unless local-p '(:get #'erc--neuter-custom-variable-state))
          ,@(unless local-p `(:type ,(erc--prepare-custom-module-type name)))
          (if ,mode
              (,enable)
diff --git a/test/lisp/erc/erc-tests.el b/test/lisp/erc/erc-tests.el
index ef742c853d6..baf6826ff97 100644
--- a/test/lisp/erc/erc-tests.el
+++ b/test/lisp/erc/erc-tests.el
@@ -1328,7 +1328,7 @@ define-erc-module--global
 Some docstring."
                         :global t
                         :group (erc--find-group 'mname 'malias)
-                        :set #'erc--custom-set-minor-mode
+                        :get #'erc--neuter-custom-variable-state
                         :type "mname"
                         (if erc-mname-mode
                             (erc-mname-enable)
@@ -1340,11 +1340,8 @@ define-erc-module--global
                         (unless (or erc--inside-mode-toggle-p
                                     (memq 'mname erc-modules))
                           (let ((erc--inside-mode-toggle-p t))
-                            (custom-set-variables
-                             `(erc-modules ',(cons 'mname erc-modules)))))
+                            (setopt erc-modules (cons 'mname erc-modules))))
                         (setq erc-mname-mode t)
-                        (unless (called-interactively-p 'any)
-                          (put 'erc-mname-mode 'standard-value (list t)))
                         (ignore a) (ignore b))
 
                       (defun erc-mname-disable ()
@@ -1353,11 +1350,8 @@ define-erc-module--global
                         (unless (or erc--inside-mode-toggle-p
                                     (not (memq 'mname erc-modules)))
                           (let ((erc--inside-mode-toggle-p t))
-                            (custom-set-variables
-                             `(erc-modules ',(delq 'mname erc-modules)))))
+                            (setopt erc-modules (delq 'mname erc-modules))))
                         (setq erc-mname-mode nil)
-                        (unless (called-interactively-p 'any)
-                          (put 'erc-mname-mode 'standard-value (list nil)))
                         (ignore c) (ignore d))
 
                       (defalias 'erc-malias-mode #'erc-mname-mode)
-- 
2.39.2

From d2ebcb79a86895ada512ce466174d4c0cd7e27e2 Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <[email protected]>
Date: Sat, 14 Jan 2023 19:05:59 -0800
Subject: [PATCH 1/3] [5.6] Don't associate ERC modules with undefined groups

* lisp/erc/erc-capab.el Add property crutch to help ERC find module's
custom group.
* lisp/erc/erc-common.el (erc--find-group): Add new function, a helper
for finding an existing ERC custom group based on `define-erc-module'
params.  Prefer `group-documentation' as a sentinel over symbol
properties owned by Customize because they might not be present if the
group isn't yet associated with any custom variables.
(define-erc-module): Set `:group' keyword value more accurately,
falling back to `erc' group when no associated group has been defined.
* test/lisp/erc/erc-tests.el (erc--find-group, erc--find-group--real):
New tests.
(define-erc-module--global, define-erc-module--local): Expect the
:group keyword to be the unevaluated `erc--find-group'
form.  (Bug#60935.)
---
 lisp/erc/erc-capab.el      |  1 +
 lisp/erc/erc-common.el     | 38 +++++++++++++++++++++++++++++++++-----
 test/lisp/erc/erc-tests.el | 37 +++++++++++++++++++++++++++++++++++--
 3 files changed, 69 insertions(+), 7 deletions(-)

diff --git a/lisp/erc/erc-capab.el b/lisp/erc/erc-capab.el
index 650c5fa84ac..bb0921da7f0 100644
--- a/lisp/erc/erc-capab.el
+++ b/lisp/erc/erc-capab.el
@@ -89,6 +89,7 @@ erc-capab-identify-unidentified
 ;;; Define module:
 
 ;;;###autoload(autoload 'erc-capab-identify-mode "erc-capab" nil t)
+(put 'capab-identify 'erc-group 'erc-capab)
 (define-erc-module capab-identify nil
   "Handle dancer-ircd's CAPAB IDENTIFY-MSG and IDENTIFY-CTCP."
   ;; append so that `erc-server-parameters' is already set by `erc-server-005'
diff --git a/lisp/erc/erc-common.el b/lisp/erc/erc-common.el
index 0279b0a0bc4..0eabd3a2fe9 100644
--- a/lisp/erc/erc-common.el
+++ b/lisp/erc/erc-common.el
@@ -145,6 +145,37 @@ erc--assemble-toggle
              (setq ,mode ,val)
              ,@body)))))
 
+;; This is a migration helper that determines a module's `:group'
+;; keyword argument from its name or alias.  A (global) module's minor
+;; mode variable appears under the group's Custom menu.  Like
+;; `erc--normalize-module-symbol', it must run when the module's
+;; definition (rather than that of `define-erc-module') is expanded.
+;; For corner cases in which this fails or the catch-all of `erc' is
+;; more inappropriate, (global) modules can declare a top-level
+;;
+;;   (put 'foo 'erc-group 'erc-bar)
+;;
+;; where `erc-bar' is the group and `foo' is the normalized module.
+;; Do this *before* the module's definition.  If `define-erc-module'
+;; ever accepts arbitrary keywords, passing an explicit `:group' will
+;; obviously be preferable.
+
+(defun erc--find-group (&rest symbols)
+  (catch 'found
+    (dolist (s symbols)
+      (let* ((downed (downcase (symbol-name s)))
+             (known (intern-soft (concat "erc-" downed))))
+        (when (and known
+                   (or (get known 'group-documentation)
+                       (rassq known custom-current-group-alist)))
+          (throw 'found known))
+        (when (setq known (intern-soft (concat "erc-" downed "-mode")))
+          (when-let ((found (custom-group-of-mode known)))
+            (throw 'found found))))
+      (when-let ((found (get (erc--normalize-module-symbol s) 'erc-group)))
+        (throw 'found found)))
+    'erc))
+
 (defmacro define-erc-module (name alias doc enable-body disable-body
                                   &optional local-p)
   "Define a new minor mode using ERC conventions.
@@ -179,7 +210,6 @@ define-erc-module
   (declare (doc-string 3) (indent defun))
   (let* ((sn (symbol-name name))
          (mode (intern (format "erc-%s-mode" (downcase sn))))
-         (group (intern (format "erc-%s" (downcase sn))))
          (enable (intern (format "erc-%s-enable" (downcase sn))))
          (disable (intern (format "erc-%s-disable" (downcase sn)))))
     `(progn
@@ -190,10 +220,8 @@ define-erc-module
 and disable it otherwise.  If called from Lisp, enable the mode
 if ARG is omitted or nil.
 %s" name name doc)
-         ;; FIXME: We don't know if this group exists, so this `:group' may
-         ;; actually just silence a valid warning about the fact that the var
-         ;; is not associated with any group.
-         :global ,(not local-p) :group (quote ,group)
+         :global ,(not local-p)
+         :group (erc--find-group ',name ,(and alias (list 'quote alias)))
          (if ,mode
              (,enable)
            (,disable)))
diff --git a/test/lisp/erc/erc-tests.el b/test/lisp/erc/erc-tests.el
index d6c63934163..13ef99be167 100644
--- a/test/lisp/erc/erc-tests.el
+++ b/test/lisp/erc/erc-tests.el
@@ -1209,6 +1209,39 @@ erc-migrate-modules
   ;; Default unchanged
   (should (equal (erc-migrate-modules erc-modules) erc-modules)))
 
+(ert-deftest erc--find-group ()
+  ;; These two are loaded by default
+  (should (eq (erc--find-group 'keep-place nil) 'erc))
+  (should (eq (erc--find-group 'networks nil) 'erc-networks))
+  ;; These are fake
+  (cl-letf (((get 'erc-bar 'group-documentation) "")
+            ((get 'baz 'erc-group) 'erc-foo))
+    (should (eq (erc--find-group 'foo 'bar) 'erc-bar))
+    (should (eq (erc--find-group 'bar 'foo) 'erc-bar))
+    (should (eq (erc--find-group 'bar nil) 'erc-bar))
+    (should (eq (erc--find-group 'foo nil) 'erc))
+    (should (eq (erc--find-group 'fake 'baz) 'erc-foo))))
+
+(ert-deftest erc--find-group--real ()
+  :tags '(:unstable)
+  (require 'erc-services)
+  (require 'erc-stamp)
+  (require 'erc-sound)
+  (require 'erc-page)
+  (require 'erc-join)
+  (require 'erc-capab)
+  (require 'erc-pcomplete)
+  (should (eq (erc--find-group 'services 'nickserv) 'erc-services))
+  (should (eq (erc--find-group 'stamp 'timestamp) 'erc-stamp))
+  (should (eq (erc--find-group 'sound 'ctcp-sound) 'erc-sound))
+  (should (eq (erc--find-group 'page 'ctcp-page) 'erc-page))
+  (should (eq (erc--find-group 'autojoin) 'erc-autojoin))
+  (should (eq (erc--find-group 'pcomplete 'Completion) 'erc-pcomplete))
+  (should (eq (erc--find-group 'capab-identify) 'erc-capab))
+  ;; No group specified.
+  (should (eq (erc--find-group 'smiley nil) 'erc))
+  (should (eq (erc--find-group 'unmorse nil) 'erc)))
+
 (ert-deftest erc--update-modules ()
   (let (calls
         erc-modules
@@ -1290,7 +1323,7 @@ define-erc-module--global
 if ARG is omitted or nil.
 Some docstring"
                         :global t
-                        :group 'erc-mname
+                        :group (erc--find-group 'mname 'malias)
                         (if erc-mname-mode
                             (erc-mname-enable)
                           (erc-mname-disable)))
@@ -1336,7 +1369,7 @@ define-erc-module--local
 if ARG is omitted or nil.
 Some docstring"
                         :global nil
-                        :group 'erc-mname
+                        :group (erc--find-group 'mname nil)
                         (if erc-mname-mode
                             (erc-mname-enable)
                           (erc-mname-disable)))
-- 
2.39.2

From eaaebc28c33300ec0b7e54b92076b83bc09923eb Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <[email protected]>
Date: Sat, 14 Jan 2023 19:08:11 -0800
Subject: [PATCH 2/3] [5.6] Warn when setting minor-mode vars for ERC modules

(erc--inside-mode-toggle-p): Add global var to inhibit mode toggles
from being run by `erc-update-modules'.  It must be non-nil inside
custom-set functions for mode toggles created by `define-erc-module'.
(erc--assemble-toggle): Don't modify `erc-modules' when run from
custom-set function.
(erc--neuter-custom-variable-state): Add new function to serve as a
phony getter that deceives Customize into thinking the variable is
always set to its standard value.  The justification for this is that
toggling a module's minor mode in Customize has never worked and has
only sewn confusion.  Without this hack, mode widgets show a state of
"CHANGED outside Customize", which alone is probably preferable,
except that they all end up toggled open, bringing them unwanted
attention and distracting the user.
(erc--tick-module-checkbox): Add helper to toggle the appropriate
checkbox in the `erc-modules' widget when a user interactively toggles
a minor-mode state variable.
(erc--prepare-custom-module-type): Create spec for minor-mode custom
`:type', deferring various aspects until module-definition time.
(define-erc-module): Add `:get' and `:type' keywords to be passed to
`defcustom' definition for global modules.
* lisp/erc/erc.el (erc-modules): Inhibit `erc-update-modules' when run
from a minor-mode toggle's custom-set function.
* test/lisp/erc/erc-tests.el
(define-erc-module--global, define-erc-module--local): Update
`erc-modules' mutations with `erc--inside-mode-toggle-p' guard
conditions.  (Bug#60935.)
---
 lisp/erc/erc-common.el     | 100 +++++++++++++++++++++++++++++++++++--
 lisp/erc/erc.el            |   3 +-
 test/lisp/erc/erc-tests.el |  17 +++++--
 3 files changed, 111 insertions(+), 9 deletions(-)

diff --git a/lisp/erc/erc-common.el b/lisp/erc/erc-common.el
index 0eabd3a2fe9..fa5d5eb52bd 100644
--- a/lisp/erc/erc-common.el
+++ b/lisp/erc/erc-common.el
@@ -29,6 +29,7 @@
 (defvar erc--casemapping-rfc1459)
 (defvar erc--casemapping-rfc1459-strict)
 (defvar erc-channel-users)
+(defvar erc-modules)
 (defvar erc-dbuf)
 (defvar erc-log-p)
 (defvar erc-server-users)
@@ -37,6 +38,11 @@ erc-session-server
 (declare-function erc--get-isupport-entry "erc-backend" (key &optional single))
 (declare-function erc-get-buffer "erc" (target &optional proc))
 (declare-function erc-server-buffer "erc" nil)
+(declare-function widget-apply-action "wid-edit" (widget &optional event))
+(declare-function widget-at "wid-edit" (&optional pos))
+(declare-function widget-get-sibling "wid-edit" (widget))
+(declare-function widget-move "wid-edit" (arg &optional suppress-echo))
+(declare-function widget-type "wid-edit" (widget))
 
 (cl-defstruct erc-input
   string insertp sendp)
@@ -120,6 +126,16 @@ erc--normalize-module-symbol
   (setq symbol (intern (downcase (symbol-name symbol))))
   (or (cdr (assq symbol erc--module-name-migrations)) symbol))
 
+(defvar erc--inside-mode-toggle-p nil
+  "Non-nil when a module's mode toggle is updating module membership.
+This serves as a flag to inhibit the mutual recursion that would
+otherwise occur between an ERC-defined minor-mode function, such
+as `erc-services-mode', and the custom-set function for
+`erc-modules'.  For historical reasons, the latter calls
+`erc-update-modules', which, in turn, enables the minor-mode
+functions for all member modules.  Also non-nil when a mode's
+widget runs its set function.")
+
 (defun erc--assemble-toggle (localp name ablsym mode val body)
   (let ((arg (make-symbol "arg")))
     `(defun ,ablsym ,(if localp `(&optional ,arg) '())
@@ -137,11 +153,28 @@ erc--assemble-toggle
                        (,ablsym))
                    (setq ,mode ,val)
                    ,@body)))
-           `(,(if val
-                  `(cl-pushnew ',(erc--normalize-module-symbol name)
-                               erc-modules)
-                `(setq erc-modules (delq ',(erc--normalize-module-symbol name)
-                                         erc-modules)))
+           ;; No need for `default-value', etc. because a buffer-local
+           ;; `erc-modules' only influences the next session and
+           ;; doesn't survive the major-mode reset that soon follows.
+           `((unless
+                 (or erc--inside-mode-toggle-p
+                     ,@(let ((v `(memq ',(erc--normalize-module-symbol name)
+                                       erc-modules)))
+                         `(,(if val v `(not ,v)))))
+               (let ((erc--inside-mode-toggle-p t))
+                 ;; Make the widget show "CHANGED outside Customize."
+                 ;; Ideally, it would show "SET for current session"
+                 ;; instead, but using `customize-set-variable' here
+                 ;; or calling `customize-mark-as-set' afterward isn't
+                 ;; enough if `customized-value' and `standard-value'
+                 ;; match but differ from `saved-value' because the
+                 ;; widget will see a `standard' instead of a `set'
+                 ;; state.  And clicking "apply and save" won't update
+                 ;; `custom-file'.  See bug#12864 "Make state button
+                 ;; interaction less confusing".
+                 (setopt erc-modules (,(if val 'cons 'delq)
+                                      ',(erc--normalize-module-symbol name)
+                                      erc-modules))))
              (setq ,mode ,val)
              ,@body)))))
 
@@ -176,6 +209,61 @@ erc--find-group
         (throw 'found found)))
     'erc))
 
+(defun erc--neuter-custom-variable-state (variable)
+  "Lie to Customize about VARIABLE's true state.
+Do so by always returning its standard value, namely nil."
+  ;; Make a module's global minor-mode toggle blind to Customize, so
+  ;; that `customize-variable-state' never sees it as "changed",
+  ;; regardless of its value.  This snippet is
+  ;; `custom--standard-value' from Emacs 28+.
+  (cl-assert (null (eval (car (get variable 'standard-value)) t)))
+  nil)
+
+;; This exists as a separate, top-level function to prevent the byte
+;; compiler from warning about widget-related dependencies not being
+;; loaded at runtime.
+
+(defun erc--tick-module-checkbox (name &rest _) ; `name' must be normalized
+  (customize-variable-other-window 'erc-modules)
+  ;; Move to `erc-modules' section.
+  (while (not (eq (widget-type (widget-at)) 'checkbox))
+    (widget-move 1 t))
+  ;; This search for a checkbox can fail when `name' refers to a
+  ;; third-party module that modifies `erc-modules' (improperly) on
+  ;; load.
+  (let (w)
+    (while (and (eq (widget-type (widget-at)) 'checkbox)
+                (not (and (setq w (widget-get-sibling (widget-at)))
+                          (eq (widget-value w) name))))
+      (setq w nil)
+      (widget-move 1 t)) ; the `suppress-echo' arg exists in 27.2
+    (if w
+        (progn (widget-apply-action (widget-at))
+               (message "Hit %s to apply or %s to apply and save."
+                        (substitute-command-keys "\\[Custom-set]")
+                        (substitute-command-keys "\\[Custom-save]")))
+      (error "Failed to find %s in `erc-modules' checklist" name))))
+
+(defun erc--prepare-custom-module-type (name)
+  `(let* ((name (erc--normalize-module-symbol ',name))
+          (fmtd (format " `%s' " name)))
+     `(boolean
+       :button-face '(custom-variable-obsolete custom-button)
+       :format "%{%t%}: %[Deprecated Toggle%] \n%h\n"
+       :documentation-property
+       ,(lambda (_)
+          (let ((hasp (memq name erc-modules)))
+            (concat "Setting a module's minor-mode variable is "
+                    (propertize "ineffective" 'face 'error)
+                    ".\nPlease " (if hasp "remove" "add") fmtd
+                    (if hasp "from" "to") " `erc-modules' directly instead.\n"
+                    "You can do so now by clicking the scary button above.")))
+       :help-echo ,(lambda (_)
+                     (let ((hasp (memq name erc-modules)))
+                       (concat (if hasp "Remove" "Add") fmtd
+                               (if hasp "from" "to") " `erc-modules'.")))
+       :action ,(apply-partially #'erc--tick-module-checkbox name))))
+
 (defmacro define-erc-module (name alias doc enable-body disable-body
                                   &optional local-p)
   "Define a new minor mode using ERC conventions.
@@ -222,6 +310,8 @@ define-erc-module
 %s" name name doc)
          :global ,(not local-p)
          :group (erc--find-group ',name ,(and alias (list 'quote alias)))
+         ,@(unless local-p '(:get #'erc--neuter-custom-variable-state))
+         ,@(unless local-p `(:type ,(erc--prepare-custom-module-type name)))
          (if ,mode
              (,enable)
            (,disable)))
diff --git a/lisp/erc/erc.el b/lisp/erc/erc.el
index 69bdb5d71b1..59ab1f1eab3 100644
--- a/lisp/erc/erc.el
+++ b/lisp/erc/erc.el
@@ -1846,7 +1846,8 @@ erc-modules
          (set sym val)
          ;; this test is for the case where erc hasn't been loaded yet
          (when (fboundp 'erc-update-modules)
-           (erc-update-modules)))
+           (unless erc--inside-mode-toggle-p
+             (erc-update-modules))))
   :type
   '(set
     :greedy t
diff --git a/test/lisp/erc/erc-tests.el b/test/lisp/erc/erc-tests.el
index 13ef99be167..cb63b04eac5 100644
--- a/test/lisp/erc/erc-tests.el
+++ b/test/lisp/erc/erc-tests.el
@@ -1313,7 +1313,10 @@ define-erc-module--global
                           ((ignore a) (ignore b))
                           ((ignore c) (ignore d)))))
 
-    (should (equal (macroexpand global-module)
+    (should (equal (cl-letf (((symbol-function
+                               'erc--prepare-custom-module-type)
+                              #'symbol-name))
+                     (macroexpand global-module))
                    `(progn
 
                       (define-minor-mode erc-mname-mode
@@ -1324,6 +1327,8 @@ define-erc-module--global
 Some docstring"
                         :global t
                         :group (erc--find-group 'mname 'malias)
+                        :get #'erc--neuter-custom-variable-state
+                        :type "mname"
                         (if erc-mname-mode
                             (erc-mname-enable)
                           (erc-mname-disable)))
@@ -1331,14 +1336,20 @@ define-erc-module--global
                       (defun erc-mname-enable ()
                         "Enable ERC mname mode."
                         (interactive)
-                        (cl-pushnew 'mname erc-modules)
+                        (unless (or erc--inside-mode-toggle-p
+                                    (memq 'mname erc-modules))
+                          (let ((erc--inside-mode-toggle-p t))
+                            (setopt erc-modules (cons 'mname erc-modules))))
                         (setq erc-mname-mode t)
                         (ignore a) (ignore b))
 
                       (defun erc-mname-disable ()
                         "Disable ERC mname mode."
                         (interactive)
-                        (setq erc-modules (delq 'mname erc-modules))
+                        (unless (or erc--inside-mode-toggle-p
+                                    (not (memq 'mname erc-modules)))
+                          (let ((erc--inside-mode-toggle-p t))
+                            (setopt erc-modules (delq 'mname erc-modules))))
                         (setq erc-mname-mode nil)
                         (ignore c) (ignore d))
 
-- 
2.39.2

From e8a6cbfaffb91019fa9a4a9aa3cae3c7896a6ff5 Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <[email protected]>
Date: Mon, 16 Jan 2023 20:18:32 -0800
Subject: [PATCH 3/3] [5.6] Fill doc strings for ERC modules.

* lisp/erc/erc-common.el (erc--fill-module-docstring): Add helper to
fill doc strings.
(erc--assemble-toggle, define-erc-module): Use helper to fill doc
string.
* test/lisp/erc/erc-tests.el (define-minor-mode--global,
define-minor-mode--local): Adjust expected output for generated doc
strings.  (Bug#60935.)
---
 lisp/erc/erc-common.el     | 20 +++++++++++++++++---
 test/lisp/erc/erc-tests.el | 28 ++++++++++++++++------------
 2 files changed, 33 insertions(+), 15 deletions(-)

diff --git a/lisp/erc/erc-common.el b/lisp/erc/erc-common.el
index fa5d5eb52bd..3a148dc1196 100644
--- a/lisp/erc/erc-common.el
+++ b/lisp/erc/erc-common.el
@@ -139,7 +139,7 @@ erc--inside-mode-toggle-p
 (defun erc--assemble-toggle (localp name ablsym mode val body)
   (let ((arg (make-symbol "arg")))
     `(defun ,ablsym ,(if localp `(&optional ,arg) '())
-       ,(concat
+       ,(erc--fill-module-docstring
          (if val "Enable" "Disable")
          " ERC " (symbol-name name) " mode."
          (when localp
@@ -264,6 +264,20 @@ erc--prepare-custom-module-type
                                (if hasp "from" "to") " `erc-modules'.")))
        :action ,(apply-partially #'erc--tick-module-checkbox name))))
 
+(defun erc--fill-module-docstring (&rest strings)
+  (with-temp-buffer
+    (emacs-lisp-mode)
+    (insert "(defun foo ()\n"
+            (format "%S" (apply #'concat strings))
+            "\n(ignore))")
+    (goto-char (point-min))
+    (forward-line 2)
+    (let ((emacs-lisp-docstring-fill-column 65)
+          (sentence-end-double-space t))
+      (fill-paragraph))
+    (goto-char (point-min))
+    (nth 3 (read (current-buffer)))))
+
 (defmacro define-erc-module (name alias doc enable-body disable-body
                                   &optional local-p)
   "Define a new minor mode using ERC conventions.
@@ -303,11 +317,11 @@ define-erc-module
     `(progn
        (define-minor-mode
          ,mode
-         ,(format "Toggle ERC %S mode.
+         ,(erc--fill-module-docstring (format "Toggle ERC %s mode.
 With a prefix argument ARG, enable %s if ARG is positive,
 and disable it otherwise.  If called from Lisp, enable the mode
 if ARG is omitted or nil.
-%s" name name doc)
+\n%s" name name doc))
          :global ,(not local-p)
          :group (erc--find-group ',name ,(and alias (list 'quote alias)))
          ,@(unless local-p '(:get #'erc--neuter-custom-variable-state))
diff --git a/test/lisp/erc/erc-tests.el b/test/lisp/erc/erc-tests.el
index cb63b04eac5..baf6826ff97 100644
--- a/test/lisp/erc/erc-tests.el
+++ b/test/lisp/erc/erc-tests.el
@@ -1309,7 +1309,7 @@ erc--merge-local-modes
 
 (ert-deftest define-erc-module--global ()
   (let ((global-module '(define-erc-module mname malias
-                          "Some docstring"
+                          "Some docstring."
                           ((ignore a) (ignore b))
                           ((ignore c) (ignore d)))))
 
@@ -1321,10 +1321,11 @@ define-erc-module--global
 
                       (define-minor-mode erc-mname-mode
                         "Toggle ERC mname mode.
-With a prefix argument ARG, enable mname if ARG is positive,
-and disable it otherwise.  If called from Lisp, enable the mode
-if ARG is omitted or nil.
-Some docstring"
+With a prefix argument ARG, enable mname if ARG is positive, and
+disable it otherwise.  If called from Lisp, enable the mode if
+ARG is omitted or nil.
+
+Some docstring."
                         :global t
                         :group (erc--find-group 'mname 'malias)
                         :get #'erc--neuter-custom-variable-state
@@ -1363,7 +1364,7 @@ define-erc-module--global
 
 (ert-deftest define-erc-module--local ()
   (let* ((global-module '(define-erc-module mname nil ; no alias
-                           "Some docstring"
+                           "Some docstring."
                            ((ignore a) (ignore b))
                            ((ignore c) (ignore d))
                            'local))
@@ -1375,10 +1376,11 @@ define-erc-module--local
                    `(progn
                       (define-minor-mode erc-mname-mode
                         "Toggle ERC mname mode.
-With a prefix argument ARG, enable mname if ARG is positive,
-and disable it otherwise.  If called from Lisp, enable the mode
-if ARG is omitted or nil.
-Some docstring"
+With a prefix argument ARG, enable mname if ARG is positive, and
+disable it otherwise.  If called from Lisp, enable the mode if
+ARG is omitted or nil.
+
+Some docstring."
                         :global nil
                         :group (erc--find-group 'mname nil)
                         (if erc-mname-mode
@@ -1387,7 +1389,8 @@ define-erc-module--local
 
                       (defun erc-mname-enable (&optional ,arg-en)
                         "Enable ERC mname mode.
-When called interactively, do so in all buffers for the current connection."
+When called interactively, do so in all buffers for the current
+connection."
                         (interactive "p")
                         (when (derived-mode-p 'erc-mode)
                           (if ,arg-en
@@ -1399,7 +1402,8 @@ define-erc-module--local
 
                       (defun erc-mname-disable (&optional ,arg-dis)
                         "Disable ERC mname mode.
-When called interactively, do so in all buffers for the current connection."
+When called interactively, do so in all buffers for the current
+connection."
                         (interactive "p")
                         (when (derived-mode-p 'erc-mode)
                           (if ,arg-dis
-- 
2.39.2