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

From: "J.P." <[email-masked]>
Date: Wed, 18 Jan 2023 06:50:15 -0800

Tags: patch

While ERC could certainly play nicer with the Customize interface in
general, a couple particularly annoying pain points may deserve more
immediate attention:

 1. ERC's disregard of nonexistent customization groups

    It's well known that ERC struggles when relating module names to
    minor modes and library features. Partly responsible is the
    many-to-one arrangement of multiple modules being housed in a
    singular library. (Third-party packages are, thankfully, denied this
    freedom.) Complicating this is the intertwining of module aliases
    with the various preferred-name migrations that have occurred over
    the years, never mind the twin mode toggles and recent local vs.
    global scoping.

    With all this chaos afoot, it's no wonder customization-group
    mappings have always taken a back seat. The first patch in the
    attached series attempts to address this in a defensive way, mainly
    using heuristics and testing rather than, say, opening
    `define-erc-module' up to arbitrary `defcustom' keyword arguments
    (which is too drastic, IMO).

 2. ERC's presentation of minor-mode variables in Custom-mode buffers

    A (global) module's minor-mode variable appears in its group's
    customization buffer by design. But its presence there is a constant
    source of confusion, even for longtime casual users (as I've
    recently learned). If you'll recall, the only meaningful factor
    affecting automatic module activation is a module's membership in
    `erc-modules'. Thus, to ensure its survival across sessions, a
    module's minor-mode toggle happily mutates `erc-modules', currently
    without informing Customize (or users).

    While this happens to work, the fact that a module's activation
    state only reflects changes after they've been made is apparently
    nonobvious to some. Alas, such users have come to regard minor-mode
    variables as magically declarative, ostensibly prompted by their
    presence in ERC's Custom buffers. But before writing off these folks
    as hopeless, consider the overly convoluted state of affairs here.
    As instructed by many an article on the Emacs Wiki (not to mention
    the Commentary section of at least one built-in module), a good many
    people never deal with `erc-modules' directly and instead simply
    call a series of activation toggles in their init file.

    The second patch in this series aims to address (some of) this by
    taking a slightly circuitous route. Rather than removing the
    minor-mode variable's widget entirely, it opts to confront the
    problem by explaining the situation and offering a modified toggle
    button that, when clicked, opens yet another customize buffer (for
    `erc-modules') and ticks or unticks the item in question, as
    appropriate. The idea is to hopefully force people to confront the
    reality of `erc-modules' being the single source of truth when it
    comes to module activation. For use in lisp-code, the patch also
    changes the custom-set function shared by these minor-mode variables
    so that it updates `erc-modules' via the Customize library API.

The above descriptions are pretty inadequate, in part due to the
UX-centric nature of this bug set. Please try them out for a clearer
picture of what's being proposed. Thanks.

In GNU Emacs 30.0.50 (build 2, x86_64-pc-linux-gnu, GTK+ Version
 3.24.35, cairo version 1.17.6) of 2023-01-17 built on localhost
Repository revision: 281f48f19ecad706a639d57cb937afb0b97eded7
Repository branch: master
Windowing system distributor 'The X.Org Foundation', version 11.0.12014000
System Description: Fedora Linux 36 (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 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 iso8601 time-date
auth-source cl-seq eieio eieio-core cl-macs password-cache json subr-x
map thingatpt pp format-spec cl-loaddefs cl-lib erc-backend erc-goodies
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 move-toolbar gtk x-toolkit xinput2 x multi-tty
make-network-process emacs)

Memory information:
((conses 16 64390 6319)
 (symbols 48 8639 0)
 (strings 32 23673 1623)
 (string-bytes 1 685926)
 (vectors 16 15259)
 (vector-slots 8 209777 7692)
 (floats 8 24 35)
 (intervals 56 232 0)
 (buffers 976 10))
>From e504a84e698982cec6830450bb2554be2f50e30d Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <[email-masked]>
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 (erc-capab-mode): Add new alias for
`erc-capab-identify-mode'.
* lisp/erc/erc-common.el (erc--features-to-modules): Update
`erc-capab' entry to prefer `capab' as the canonical name because it
matches the library feature.
(erc--find-group): Add new function, a helper for finding an existing
ERC custom group based on `define-erc-module' params.  Use
`group-documentation' as a sentinel instead of `custom-group', which
may not be present if it 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.
* lisp/erc/erc.el (erc-modules): Remove entry for nonexistent module
`hecomplete' and change choice value for `erc-capab' from
`capab-identify' to the now canonical `capab'.
* 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.
(Depends on bug#60784.)
---
 lisp/erc/erc-capab.el      |  2 +-
 lisp/erc/erc-common.el     | 36 ++++++++++++++++++++++++++++++------
 lisp/erc/erc.el            |  3 +--
 test/lisp/erc/erc-tests.el | 33 +++++++++++++++++++++++++++++++--
 4 files changed, 63 insertions(+), 11 deletions(-)

diff --git a/lisp/erc/erc-capab.el b/lisp/erc/erc-capab.el
index 650c5fa84a..25cdea9e77 100644
--- a/lisp/erc/erc-capab.el
+++ b/lisp/erc/erc-capab.el
@@ -89,7 +89,7 @@ erc-capab-identify-unidentified
 ;;; Define module:
 
 ;;;###autoload(autoload 'erc-capab-identify-mode "erc-capab" nil t)
-(define-erc-module capab-identify nil
+(define-erc-module capab-identify capab
   "Handle dancer-ircd's CAPAB IDENTIFY-MSG and IDENTIFY-CTCP."
   ;; append so that `erc-server-parameters' is already set by `erc-server-005'
   ((add-hook 'erc-server-005-functions #'erc-capab-identify-setup t)
diff --git a/lisp/erc/erc-common.el b/lisp/erc/erc-common.el
index 994555acec..60bb5dd6fc 100644
--- a/lisp/erc/erc-common.el
+++ b/lisp/erc/erc-common.el
@@ -91,7 +91,7 @@ erc--target
 ;; TODO move goodies modules here after 29 is released.
 (defconst erc--features-to-modules
   '((erc-pcomplete completion pcomplete)
-    (erc-capab capab-identify)
+    (erc-capab capab capab-identify)
     (erc-join autojoin)
     (erc-page page ctcp-page)
     (erc-sound sound ctcp-sound)
@@ -148,6 +148,33 @@ 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 will appear 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 where this fails and where the catch-all of `erc'
+;; is inappropriate, (global) modules can declare a top-level
+;;
+;;   (custom-add-to-group 'erc-foo 'erc-bar-mode 'custom-variable)
+;;
+;; *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
+    (while symbols
+      (when-let* ((s (pop symbols))
+                  (downed (concat "erc-" (downcase (symbol-name s))))
+                  (known (intern-soft downed)))
+        (when-let ((found (custom-group-of-mode known)))
+          (throw 'found found))
+        (when (or (get known 'group-documentation)
+                  (rassq known custom-current-group-alist))
+          (throw 'found known))))
+    'erc))
+
 (defmacro define-erc-module (name alias doc enable-body disable-body
                                   &optional local-p)
   "Define a new minor mode using ERC conventions.
@@ -182,7 +209,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
@@ -193,10 +219,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/lisp/erc/erc.el b/lisp/erc/erc.el
index 7f51b7bfb2..3bf6019783 100644
--- a/lisp/erc/erc.el
+++ b/lisp/erc/erc.el
@@ -1854,10 +1854,9 @@ erc-modules
     (const :tag "autojoin: Join channels automatically" autojoin)
     (const :tag "button: Buttonize URLs, nicknames, and other text" button)
     (const :tag "capab: Mark unidentified users on servers supporting CAPAB"
-           capab-identify)
+           capab)
     (const :tag "completion: Complete nicknames and commands (programmable)"
            completion)
-    (const :tag "hecomplete: Complete nicknames and commands (obsolete, use \"completion\")" hecomplete)
     (const :tag "dcc: Provide Direct Client-to-Client support" dcc)
     (const :tag "fill: Wrap long lines" fill)
     (const :tag "identd: Launch an identd server on port 8113" identd)
diff --git a/test/lisp/erc/erc-tests.el b/test/lisp/erc/erc-tests.el
index 40a2d2de65..c07f249343 100644
--- a/test/lisp/erc/erc-tests.el
+++ b/test/lisp/erc/erc-tests.el
@@ -1209,6 +1209,35 @@ 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) ""))
+    (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))))
+
+(ert-deftest erc--find-group--real ()
+  :tags '(:unstable)
+  (let (out)
+    (pcase-dolist (`(,feat . ,syms) erc--features-to-modules)
+      (require feat)
+      (push (cons syms (apply #'erc--find-group syms)) out))
+    ;; Remove local modules, which are mapped to the catch-all
+    (while (and-let* ((m (rassq 'erc out))) ; while-let
+             (setq out (remq m out))))
+    (should (equal out
+                   '(((services nickserv) . erc-services)
+                     ((stamp timestamp) . erc-stamp)
+                     ((sound ctcp-sound) . erc-sound)
+                     ((page ctcp-page) . erc-page)
+                     ((autojoin) . erc-autojoin)
+                     ((capab capab-identify) . erc-capab)
+                     ((completion pcomplete) . erc-pcomplete))))))
+
 (ert-deftest erc--update-modules ()
   (let (calls
         erc-modules
@@ -1290,7 +1319,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 +1365,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.38.1

>From e96136d6ac747ec7cec5831480997e16cefbf51a Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <[email-masked]>
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--custom-set-minor-mode): Add new custom-set function for module
minor modes.  Update `erc-modules' before calling a minor-mode toggle
via `custom-set-minor-mode'.
(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 `:set' and `:type' keywords 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.  (Depends on bug#60784.)
---
 lisp/erc/erc-common.el     | 78 +++++++++++++++++++++++++++++++++++---
 lisp/erc/erc.el            |  3 +-
 test/lisp/erc/erc-tests.el | 13 +++++--
 3 files changed, 85 insertions(+), 9 deletions(-)

diff --git a/lisp/erc/erc-common.el b/lisp/erc/erc-common.el
index 60bb5dd6fc..326b970bdb 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)
@@ -123,6 +129,15 @@ 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 mode toggle is updating module membership.
+This serves as a flag to inhibit the mutual recursion that
+otherwise occurs 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.")
+
 (defun erc--assemble-toggle (localp name ablsym mode val body)
   (let ((arg (make-symbol "arg")))
     `(defun ,ablsym ,(if localp `(&optional ,arg) '())
@@ -140,11 +155,13 @@ 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)))
+           `((unless erc--inside-mode-toggle-p
+               ,(if val
+                    `(cl-pushnew ',(erc--normalize-module-symbol name)
+                                 erc-modules)
+                  `(setq erc-modules
+                         (delq ',(erc--normalize-module-symbol name)
+                               erc-modules))))
              (setq ,mode ,val)
              ,@body)))))
 
@@ -175,6 +192,55 @@ erc--find-group
           (throw 'found known))))
     'erc))
 
+(defun erc--custom-set-minor-mode (variable value)
+  (let ((name (get variable 'erc-module))
+        (erc--inside-mode-toggle-p t))
+    (customize-set-variable
+     'erc-modules
+     (if value (cl-pushnew name erc-modules) (delq name erc-modules)))
+    (custom-set-minor-mode variable value)))
+
+;; 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 '(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.")
+       :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.
@@ -221,6 +287,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 '(:set #'erc--custom-set-minor-mode))
+         ,@(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 3bf6019783..e77ac8b4af 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 c07f249343..b8dd930141 100644
--- a/test/lisp/erc/erc-tests.el
+++ b/test/lisp/erc/erc-tests.el
@@ -1309,7 +1309,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
@@ -1320,6 +1323,8 @@ define-erc-module--global
 Some docstring"
                         :global t
                         :group (erc--find-group 'mname 'malias)
+                        :set #'erc--custom-set-minor-mode
+                        :type "mname"
                         (if erc-mname-mode
                             (erc-mname-enable)
                           (erc-mname-disable)))
@@ -1327,14 +1332,16 @@ define-erc-module--global
                       (defun erc-mname-enable ()
                         "Enable ERC mname mode."
                         (interactive)
-                        (cl-pushnew 'mname erc-modules)
+                        (unless erc--inside-mode-toggle-p
+                          (cl-pushnew '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 erc--inside-mode-toggle-p
+                          (setq erc-modules (delq 'mname erc-modules)))
                         (setq erc-mname-mode nil)
                         (ignore c) (ignore d))
 
-- 
2.38.1

>From 8cf6de3d73e1226ce92eeedafcc699ee6aa3467a Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <[email-masked]>
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.
---
 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 326b970bdb..f488244ef9 100644
--- a/lisp/erc/erc-common.el
+++ b/lisp/erc/erc-common.el
@@ -141,7 +141,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
@@ -241,6 +241,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.
@@ -280,11 +294,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 '(:set #'erc--custom-set-minor-mode))
diff --git a/test/lisp/erc/erc-tests.el b/test/lisp/erc/erc-tests.el
index b8dd930141..703b26dcea 100644
--- a/test/lisp/erc/erc-tests.el
+++ b/test/lisp/erc/erc-tests.el
@@ -1305,7 +1305,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)))))
 
@@ -1317,10 +1317,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)
                         :set #'erc--custom-set-minor-mode
@@ -1355,7 +1356,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))
@@ -1367,10 +1368,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
@@ -1379,7 +1381,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
@@ -1391,7 +1394,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.38.1