unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
* bug#60935: 30.0.50; ERC >5.5: Improve ERC's treatment of customization groups
@ 2023-01-18 14:50 J.P.
  2023-02-07 15:23 ` J.P.
                   ` (4 more replies)
  0 siblings, 5 replies; 7+ messages in thread
From: J.P. @ 2023-01-18 14:50 UTC (permalink / raw)
  To: 60935; +Cc: emacs-erc

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

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

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-5.6-Don-t-associate-ERC-modules-with-undefined-group.patch --]
[-- Type: text/x-patch, Size: 8333 bytes --]

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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-5.6-Warn-when-setting-minor-mode-vars-for-ERC-module.patch --]
[-- Type: text/x-patch, Size: 9571 bytes --]

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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-5.6-Fill-doc-strings-for-ERC-modules.patch --]
[-- Type: text/x-patch, Size: 5763 bytes --]

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


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* bug#60935: 30.0.50; ERC >5.5: Improve ERC's treatment of customization groups
  2023-01-18 14:50 bug#60935: 30.0.50; ERC >5.5: Improve ERC's treatment of customization groups J.P.
@ 2023-02-07 15:23 ` J.P.
  2023-02-19 15:03 ` J.P.
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 7+ messages in thread
From: J.P. @ 2023-02-07 15:23 UTC (permalink / raw)
  To: 60935; +Cc: emacs-erc

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

v2. Use autoloads for resolving groups in corner cases. Don't change
decades-old preferred module name.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0000-v1-v2.diff --]
[-- Type: text/x-patch, Size: 6867 bytes --]

From e4e270a4bf556ce238bc42c8549d4c443f3a969a Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <jp@neverwas.me>
Date: Tue, 7 Feb 2023 00:08:34 -0800
Subject: [PATCH 0/3] *** NOT A PATCH ***

*** BLURB HERE ***

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     | 136 +++++++++++++++++++++++++++++++++----
 lisp/erc/erc.el            |   3 +-
 test/lisp/erc/erc-tests.el |  78 ++++++++++++++++-----
 4 files changed, 187 insertions(+), 31 deletions(-)

Interdiff:
diff --git a/lisp/erc/erc-capab.el b/lisp/erc/erc-capab.el
index 25cdea9e777..bb0921da7f0 100644
--- a/lisp/erc/erc-capab.el
+++ b/lisp/erc/erc-capab.el
@@ -89,7 +89,8 @@ erc-capab-identify-unidentified
 ;;; Define module:
 
 ;;;###autoload(autoload 'erc-capab-identify-mode "erc-capab" nil t)
-(define-erc-module capab-identify capab
+(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'
   ((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 f488244ef9c..cc278428d5d 100644
--- a/lisp/erc/erc-common.el
+++ b/lisp/erc/erc-common.el
@@ -97,7 +97,7 @@ erc--target
 ;; TODO move goodies modules here after 29 is released.
 (defconst erc--features-to-modules
   '((erc-pcomplete completion pcomplete)
-    (erc-capab capab capab-identify)
+    (erc-capab capab-identify)
     (erc-join autojoin)
     (erc-page page ctcp-page)
     (erc-sound sound ctcp-sound)
@@ -173,23 +173,27 @@ erc--assemble-toggle
 ;; 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)
+;;   (put 'foo 'erc-group 'erc-bar)
 ;;
-;; *before* the module's definition.  If `define-erc-module' ever
-;; accepts arbitrary keywords, passing an explicit `:group' will
+;; 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
-    (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))))
+    (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))
 
 (defun erc--custom-set-minor-mode (variable value)
diff --git a/lisp/erc/erc.el b/lisp/erc/erc.el
index 70ba82fd4bf..5e971f15f5d 100644
--- a/lisp/erc/erc.el
+++ b/lisp/erc/erc.el
@@ -1854,9 +1854,10 @@ 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)
+           capab-identify)
     (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 703b26dcea0..e5db78ef2e1 100644
--- a/test/lisp/erc/erc-tests.el
+++ b/test/lisp/erc/erc-tests.el
@@ -1214,29 +1214,33 @@ erc--find-group
   (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) ""))
+  (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 'foo nil) 'erc))
+    (should (eq (erc--find-group 'fake 'baz) 'erc-foo))))
 
 (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))))))
+  (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
-- 
2.39.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0001-5.6-Don-t-associate-ERC-modules-with-undefined-group.patch --]
[-- Type: text/x-patch, Size: 7148 bytes --]

From 380ea8edc391aa75be9b7c7019bfc8c3db57e4f4 Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <jp@neverwas.me>
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 994555acecf..bf3bce74cfd 100644
--- a/lisp/erc/erc-common.el
+++ b/lisp/erc/erc-common.el
@@ -148,6 +148,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 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
+;;
+;;   (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.
@@ -182,7 +213,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 +223,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 40a2d2de657..24a4f9d0639 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.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0002-5.6-Warn-when-setting-minor-mode-vars-for-ERC-module.patch --]
[-- Type: text/x-patch, Size: 9563 bytes --]

From 84bdae341ec2e19ba45c6dd9547dc3a5db43cbea Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <jp@neverwas.me>
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.  (Bug#60935.)
---
 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 bf3bce74cfd..ec3a7cf9e85 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)))))
 
@@ -179,6 +196,55 @@ 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))
+    (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.
@@ -225,6 +291,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 ff1820cfaf2..5e971f15f5d 100644
--- a/lisp/erc/erc.el
+++ b/lisp/erc/erc.el
@@ -1845,7 +1845,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 24a4f9d0639..e254b00454b 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)
+                        :set #'erc--custom-set-minor-mode
+                        :type "mname"
                         (if erc-mname-mode
                             (erc-mname-enable)
                           (erc-mname-disable)))
@@ -1331,14 +1336,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.39.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #5: 0003-5.6-Fill-doc-strings-for-ERC-modules.patch --]
[-- Type: text/x-patch, Size: 5781 bytes --]

From e4e270a4bf556ce238bc42c8549d4c443f3a969a Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <jp@neverwas.me>
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 ec3a7cf9e85..cc278428d5d 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
@@ -245,6 +245,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.
@@ -284,11 +298,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 e254b00454b..e5db78ef2e1 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)
                         :set #'erc--custom-set-minor-mode
@@ -1359,7 +1360,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))
@@ -1371,10 +1372,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
@@ -1383,7 +1385,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
@@ -1395,7 +1398,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.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* bug#60935: 30.0.50; ERC >5.5: Improve ERC's treatment of customization groups
  2023-01-18 14:50 bug#60935: 30.0.50; ERC >5.5: Improve ERC's treatment of customization groups J.P.
  2023-02-07 15:23 ` J.P.
@ 2023-02-19 15:03 ` J.P.
  2023-03-14 13:43 ` J.P.
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 7+ messages in thread
From: J.P. @ 2023-02-19 15:03 UTC (permalink / raw)
  To: 60935; +Cc: emacs-erc

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

v3. Run custom :set function inside toggles. (Now depends on bug#60954.)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0000-v2-v3.diff --]
[-- Type: text/x-patch, Size: 5384 bytes --]

From 23b193168c03c9d36307986f87f2789cafae9b41 Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <jp@neverwas.me>
Date: Thu, 16 Feb 2023 22:51:42 -0800
Subject: [PATCH 0/3] *** NOT A PATCH ***

*** BLURB HERE ***

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     | 143 +++++++++++++++++++++++++++++++++----
 lisp/erc/erc.el            |   3 +-
 test/lisp/erc/erc-tests.el |  84 +++++++++++++++++-----
 4 files changed, 200 insertions(+), 31 deletions(-)

Interdiff:
diff --git a/lisp/erc/erc-common.el b/lisp/erc/erc-common.el
index cc278428d5d..6d09ac25a0a 100644
--- a/lisp/erc/erc-common.el
+++ b/lisp/erc/erc-common.el
@@ -130,13 +130,14 @@ erc--normalize-module-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
+  "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.")
+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")))
@@ -155,23 +156,29 @@ erc--assemble-toggle
                        (,ablsym))
                    (setq ,mode ,val)
                    ,@body)))
-           `((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))))
+           ;; 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))
+                 (customize-set-variable
+                  'erc-modules (,(if val 'cons 'delq)
+                                ',(erc--normalize-module-symbol name)
+                                erc-modules))))
              (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
+;; 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 where this fails and where the catch-all of `erc'
-;; is inappropriate, (global) modules can declare a top-level
+;; 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)
 ;;
diff --git a/test/lisp/erc/erc-tests.el b/test/lisp/erc/erc-tests.el
index e5db78ef2e1..8e41d428ce9 100644
--- a/test/lisp/erc/erc-tests.el
+++ b/test/lisp/erc/erc-tests.el
@@ -1337,16 +1337,22 @@ define-erc-module--global
                       (defun erc-mname-enable ()
                         "Enable ERC mname mode."
                         (interactive)
-                        (unless erc--inside-mode-toggle-p
-                          (cl-pushnew 'mname erc-modules))
+                        (unless (or erc--inside-mode-toggle-p
+                                    (memq 'mname erc-modules))
+                          (let ((erc--inside-mode-toggle-p t))
+                            (customize-set-variable
+                             'erc-modules (cons 'mname erc-modules))))
                         (setq erc-mname-mode t)
                         (ignore a) (ignore b))
 
                       (defun erc-mname-disable ()
                         "Disable ERC mname mode."
                         (interactive)
-                        (unless erc--inside-mode-toggle-p
-                          (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))
+                            (customize-set-variable
+                             'erc-modules (delq 'mname erc-modules))))
                         (setq erc-mname-mode nil)
                         (ignore c) (ignore d))
 
-- 
2.39.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0001-5.6-Don-t-associate-ERC-modules-with-undefined-group.patch --]
[-- Type: text/x-patch, Size: 7145 bytes --]

From f976c06bd1bff9921ecef743076d0c4c5cfeb9fd Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <jp@neverwas.me>
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 994555acecf..1503dc99d72 100644
--- a/lisp/erc/erc-common.el
+++ b/lisp/erc/erc-common.el
@@ -148,6 +148,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.
@@ -182,7 +213,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 +223,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 40a2d2de657..24a4f9d0639 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.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0002-5.6-Warn-when-setting-minor-mode-vars-for-ERC-module.patch --]
[-- Type: text/x-patch, Size: 10412 bytes --]

From c790bfcac0275222cc5d54c95db4717bb67c6bfc Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <jp@neverwas.me>
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.  (Bug#60935.)
---
 lisp/erc/erc-common.el     | 85 +++++++++++++++++++++++++++++++++++---
 lisp/erc/erc.el            |  3 +-
 test/lisp/erc/erc-tests.el | 19 +++++++--
 3 files changed, 98 insertions(+), 9 deletions(-)

diff --git a/lisp/erc/erc-common.el b/lisp/erc/erc-common.el
index 1503dc99d72..bc67bf94a67 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,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) '())
@@ -140,11 +156,19 @@ 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))
+                 (customize-set-variable
+                  'erc-modules (,(if val 'cons 'delq)
+                                ',(erc--normalize-module-symbol name)
+                                erc-modules))))
              (setq ,mode ,val)
              ,@body)))))
 
@@ -179,6 +203,55 @@ 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))
+    (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.
@@ -225,6 +298,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 d35907a1677..0087c7a09a1 100644
--- a/lisp/erc/erc.el
+++ b/lisp/erc/erc.el
@@ -1845,7 +1845,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 24a4f9d0639..26d986823b3 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)
+                        :set #'erc--custom-set-minor-mode
+                        :type "mname"
                         (if erc-mname-mode
                             (erc-mname-enable)
                           (erc-mname-disable)))
@@ -1331,14 +1336,22 @@ 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))
+                            (customize-set-variable
+                             '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))
+                            (customize-set-variable
+                             'erc-modules (delq 'mname erc-modules))))
                         (setq erc-mname-mode nil)
                         (ignore c) (ignore d))
 
-- 
2.39.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #5: 0003-5.6-Fill-doc-strings-for-ERC-modules.patch --]
[-- Type: text/x-patch, Size: 5781 bytes --]

From 23b193168c03c9d36307986f87f2789cafae9b41 Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <jp@neverwas.me>
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 bc67bf94a67..6d09ac25a0a 100644
--- a/lisp/erc/erc-common.el
+++ b/lisp/erc/erc-common.el
@@ -142,7 +142,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
@@ -252,6 +252,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.
@@ -291,11 +305,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 26d986823b3..8e41d428ce9 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)
                         :set #'erc--custom-set-minor-mode
@@ -1365,7 +1366,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))
@@ -1377,10 +1378,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
@@ -1389,7 +1391,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
@@ -1401,7 +1404,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.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* bug#60935: 30.0.50; ERC >5.5: Improve ERC's treatment of customization groups
  2023-01-18 14:50 bug#60935: 30.0.50; ERC >5.5: Improve ERC's treatment of customization groups J.P.
  2023-02-07 15:23 ` J.P.
  2023-02-19 15:03 ` J.P.
@ 2023-03-14 13:43 ` J.P.
  2023-03-15 14:05 ` J.P.
       [not found] ` <87edpqglf1.fsf@neverwas.me>
  4 siblings, 0 replies; 7+ messages in thread
From: J.P. @ 2023-03-14 13:43 UTC (permalink / raw)
  To: 60935; +Cc: emacs-erc

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

v4. Use `custom-set-variables' instead of `customize-set-variable'.
Munge `standard-value' of deprecated `erc-foo-mode' variables when
called by `erc-update-modules' (but not interactively).


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0000-v3-v4.diff --]
[-- Type: text/x-patch, Size: 4065 bytes --]

From a6662c04e156b1dd37661991a045acaf3c5cb9b4 Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <jp@neverwas.me>
Date: Tue, 14 Mar 2023 06:25:32 -0700
Subject: [PATCH 0/3] *** NOT A PATCH ***

*** BLURB HERE ***

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     | 146 +++++++++++++++++++++++++++++++++----
 lisp/erc/erc.el            |   3 +-
 test/lisp/erc/erc-tests.el |  88 +++++++++++++++++-----
 4 files changed, 207 insertions(+), 31 deletions(-)

Interdiff:
diff --git a/lisp/erc/erc-common.el b/lisp/erc/erc-common.el
index 4332ed00cfb..522803b91e2 100644
--- a/lisp/erc/erc-common.el
+++ b/lisp/erc/erc-common.el
@@ -162,11 +162,14 @@ erc--assemble-toggle
                                        erc-modules)))
                          `(,(if val v `(not ,v)))))
                (let ((erc--inside-mode-toggle-p t))
-                 (customize-set-variable
-                  'erc-modules (,(if val 'cons 'delq)
-                                ',(erc--normalize-module-symbol name)
-                                erc-modules))))
+                 (custom-set-variables
+                  `(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,9 +206,9 @@ erc--find-group
 (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-variables
+     `(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
diff --git a/test/lisp/erc/erc-tests.el b/test/lisp/erc/erc-tests.el
index 62480c0604e..ef742c853d6 100644
--- a/test/lisp/erc/erc-tests.el
+++ b/test/lisp/erc/erc-tests.el
@@ -1340,9 +1340,11 @@ define-erc-module--global
                         (unless (or erc--inside-mode-toggle-p
                                     (memq 'mname erc-modules))
                           (let ((erc--inside-mode-toggle-p t))
-                            (customize-set-variable
-                             'erc-modules (cons 'mname erc-modules))))
+                            (custom-set-variables
+                             `(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 ()
@@ -1351,9 +1353,11 @@ define-erc-module--global
                         (unless (or erc--inside-mode-toggle-p
                                     (not (memq 'mname erc-modules)))
                           (let ((erc--inside-mode-toggle-p t))
-                            (customize-set-variable
-                             'erc-modules (delq 'mname erc-modules))))
+                            (custom-set-variables
+                             `(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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0001-5.6-Don-t-associate-ERC-modules-with-undefined-group.patch --]
[-- Type: text/x-patch, Size: 7145 bytes --]

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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0002-5.6-Warn-when-setting-minor-mode-vars-for-ERC-module.patch --]
[-- Type: text/x-patch, Size: 11018 bytes --]

From 764dc63582a440495ab53a9986003d5d1a794bcf Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <jp@neverwas.me>
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.  (Bug#60935.)
---
 lisp/erc/erc-common.el     | 88 +++++++++++++++++++++++++++++++++++---
 lisp/erc/erc.el            |  3 +-
 test/lisp/erc/erc-tests.el | 23 ++++++++--
 3 files changed, 105 insertions(+), 9 deletions(-)

diff --git a/lisp/erc/erc-common.el b/lisp/erc/erc-common.el
index 0eabd3a2fe9..cd0fc79e568 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,12 +153,23 @@ 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))
+                 (custom-set-variables
+                  `(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'
@@ -176,6 +203,55 @@ 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)))
+
+;; 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.
@@ -222,6 +298,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 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..a31c2c530a3 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)
+                        :set #'erc--custom-set-minor-mode
+                        :type "mname"
                         (if erc-mname-mode
                             (erc-mname-enable)
                           (erc-mname-disable)))
@@ -1331,15 +1336,27 @@ 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))
+                            (custom-set-variables
+                             `(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 ()
                         "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))
+                            (custom-set-variables
+                             `(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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #5: 0003-5.6-Fill-doc-strings-for-ERC-modules.patch --]
[-- Type: text/x-patch, Size: 5781 bytes --]

From a6662c04e156b1dd37661991a045acaf3c5cb9b4 Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <jp@neverwas.me>
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 cd0fc79e568..522803b91e2 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
@@ -252,6 +252,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.
@@ -291,11 +305,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 a31c2c530a3..ef742c853d6 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)
                         :set #'erc--custom-set-minor-mode
@@ -1369,7 +1370,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))
@@ -1381,10 +1382,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
@@ -1393,7 +1395,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
@@ -1405,7 +1408,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


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* bug#60935: 30.0.50; ERC >5.5: Improve ERC's treatment of customization groups
  2023-01-18 14:50 bug#60935: 30.0.50; ERC >5.5: Improve ERC's treatment of customization groups J.P.
                   ` (2 preceding siblings ...)
  2023-03-14 13:43 ` J.P.
@ 2023-03-15 14:05 ` J.P.
       [not found] ` <87edpqglf1.fsf@neverwas.me>
  4 siblings, 0 replies; 7+ messages in thread
From: J.P. @ 2023-03-15 14:05 UTC (permalink / raw)
  To: 60935; +Cc: emacs-erc

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

v5. Be more responsible with custom var states. Allow CHANGED for
`erc-modules' but use getter to spoof STANDARD for deprecated mode vars
so widgets stay collapsed.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0000-v4-v5.diff --]
[-- Type: text/x-patch, Size: 7435 bytes --]

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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0001-5.6-Don-t-associate-ERC-modules-with-undefined-group.patch --]
[-- Type: text/x-patch, Size: 7145 bytes --]

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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0002-5.6-Warn-when-setting-minor-mode-vars-for-ERC-module.patch --]
[-- Type: text/x-patch, Size: 11725 bytes --]

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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #5: 0003-5.6-Fill-doc-strings-for-ERC-modules.patch --]
[-- Type: text/x-patch, Size: 5795 bytes --]

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


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* bug#60935: 30.0.50; ERC >5.5: Improve ERC's treatment of customization groups
       [not found] ` <87edpqglf1.fsf@neverwas.me>
@ 2023-04-14 14:03   ` J.P.
       [not found]   ` <87a5zay31h.fsf@neverwas.me>
  1 sibling, 0 replies; 7+ messages in thread
From: J.P. @ 2023-04-14 14:03 UTC (permalink / raw)
  To: 60935; +Cc: emacs-erc

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

Seems I was under the mistaken impression that all customizations with a
"CHANGED" state get saved when hitting C-x C-s (`Custom-save'), which is
obviously not the case. Thus, I think it's best not to mislead users by
spoofing the modification state because the only benefit of doing so now
is that carats aren't automatically thrown open on module-var widgets,
which is a comparatively minor annoyance. The attached patch reverts
this spoofing nonsense and also restores a working toggle button so
users again have the option of dismissing the warning.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-5.6-Restore-module-var-toggles-in-ERC-s-Custom-buffe.patch --]
[-- Type: text/x-patch, Size: 7512 bytes --]

From f8ec0e7646f8d1ed0c30ec6d11e22235edb5a316 Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <jp@neverwas.me>
Date: Fri, 14 Apr 2023 00:07:31 -0700
Subject: [PATCH] [5.6] Restore module var toggles in ERC's Custom buffers

* lisp/erc/erc-common.el (erc--neuter-custom-variable-state): Remove
function.  ERC famously toggles global minor-mode vars during normal
operations, which adds noise to its customization buffers because
`customize-variable-state' always sees an activated module's mode
variable as having "CHANGED".  To suppress this annoyance, a
workaround was employed that used a dishonest `:get' function to
simply return the "saved value," if any.  While this improved the
Customize experience it also misled users, which likely wasn't
justified.
(erc--make-show-me-widget): Add helper to avoid forward declarations.
(erc--prepare-custom-module-type): Don't deprive users of a working
minor-mode toggle.
(erc--find-feature): New function to guess the feature of a module's
containing library.
(define-erc-module): Remove `:get' keyword.  Specify `:require'
instead, whose value may be nil.  Users who currently have mode vars
in their `custom-file' won't be impacted by this addition because
those `custom-set-variables' entries will still lack a REQUEST list
and hence won't incur a startup penalty.  And new users intent on
using the toggle will hopefully do so with the knowledge they're
opting in to requiring ERC on startup, which is not the case if they
follow the recommended practice of using `erc-modules' instead.
* test/lisp/erc/erc-tests.el (define-erc-module--global): Change
expected expansion.  (Bug#60935.)
---
 lisp/erc/erc-common.el     | 62 ++++++++++++++++++++++++--------------
 test/lisp/erc/erc-tests.el |  2 +-
 2 files changed, 40 insertions(+), 24 deletions(-)

diff --git a/lisp/erc/erc-common.el b/lisp/erc/erc-common.el
index 6c015c71ff9..7b63a4b1eca 100644
--- a/lisp/erc/erc-common.el
+++ b/lisp/erc/erc-common.el
@@ -40,6 +40,9 @@ erc-session-server
 (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-create-child-and-convert "wid-edit"
+                  (parent type &rest args))
+(declare-function widget-default-format-handler "wid-edit" (widget escape))
 (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))
@@ -195,16 +198,6 @@ 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.
@@ -230,25 +223,42 @@ erc--tick-module-checkbox
              (substitute-command-keys "\\[Custom-set]")
              (substitute-command-keys "\\[Custom-save]"))))
 
+;; This stands apart to avoid needing forward declarations for
+;; `wid-edit' functions in every file requiring `erc-common'.
+(defun erc--make-show-me-widget (widget escape &rest plist)
+  (if (eq escape ?i)
+      (apply #'widget-create-child-and-convert widget 'push-button plist)
+    (widget-default-format-handler widget escape)))
+
 (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"
+       :format "%{%t%}: %i %[Deprecated Toggle%] %v \n%h\n"
+       :format-handler
+       ,(lambda (widget escape)
+          (erc--make-show-me-widget
+           widget escape
+           :button-face '(custom-variable-obsolete custom-button)
+           :tag "Show Me"
+           :action (apply-partially #'erc--tick-module-checkbox name)
+           :help-echo (lambda (_)
+                        (let ((hasp (memq name erc-modules)))
+                          (concat (if hasp "Remove" "Add") fmtd
+                                  (if hasp "from" "to")
+                                  " `erc-modules'.")))))
+       :action widget-toggle-action
        :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))))
+            (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 "
+             (propertize "Show Me" 'face 'custom-variable-obsolete)
+             " above."))))))
 
 (defun erc--fill-module-docstring (&rest strings)
   (with-temp-buffer
@@ -264,6 +274,12 @@ erc--fill-module-docstring
     (goto-char (point-min))
     (nth 3 (read (current-buffer)))))
 
+(defmacro erc--find-feature (name alias)
+  `(pcase (erc--find-group ',name ,(and alias (list 'quote alias)))
+     ('erc (and-let* ((file (or (macroexp-file-name) buffer-file-name)))
+             (intern (file-name-base file))))
+     (v v)))
+
 (defmacro define-erc-module (name alias doc enable-body disable-body
                                   &optional local-p)
   "Define a new minor mode using ERC conventions.
@@ -310,7 +326,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 '(:get #'erc--neuter-custom-variable-state))
+         ,@(unless local-p `(:require ',(erc--find-feature name alias)))
          ,@(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 5aaf7e499e3..7fc885c7b9d 100644
--- a/test/lisp/erc/erc-tests.el
+++ b/test/lisp/erc/erc-tests.el
@@ -2018,7 +2018,7 @@ define-erc-module--global
 Some docstring."
                         :global t
                         :group (erc--find-group 'mname 'malias)
-                        :get #'erc--neuter-custom-variable-state
+                        :require 'nil
                         :type "mname"
                         (if erc-mname-mode
                             (erc-mname-enable)
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* bug#60935: 30.0.50; ERC >5.5: Improve ERC's treatment of customization groups
       [not found]   ` <87a5zay31h.fsf@neverwas.me>
@ 2023-05-06  0:54     ` J.P.
  0 siblings, 0 replies; 7+ messages in thread
From: J.P. @ 2023-05-06  0:54 UTC (permalink / raw)
  To: 60935; +Cc: emacs-erc

"J.P." <jp@neverwas.me> writes:

> The attached patch reverts this spoofing nonsense and also restores a
> working toggle button so users again have the option of dismissing the
> warning.

This correction has been added as:

  https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=b0d761be

Thanks.





^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2023-05-06  0:54 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-18 14:50 bug#60935: 30.0.50; ERC >5.5: Improve ERC's treatment of customization groups J.P.
2023-02-07 15:23 ` J.P.
2023-02-19 15:03 ` J.P.
2023-03-14 13:43 ` J.P.
2023-03-15 14:05 ` J.P.
     [not found] ` <87edpqglf1.fsf@neverwas.me>
2023-04-14 14:03   ` J.P.
     [not found]   ` <87a5zay31h.fsf@neverwas.me>
2023-05-06  0:54     ` J.P.

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).