Warn when customizing minor-mode vars for ERC modules

* lisp/erc/erc-common.el: (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--favor-changed-reverted-modules-state): Add new helper to show a
"SET" Custom state for `erc-modules' except when reverting to the
default value because \"STANDARD\" always takes precedence, as
explained somewhat in bug#12864.
(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 in new users.  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.)

3 files changed, 126 insertions, 9 deletions

diff --git a/lisp/erc/erc-common.el b/lisp/erc/erc-common.el
index 91ddce8fbd9..c01c0323453 100644
--- a/lisp/erc/erc-common.el
+++ b/lisp/erc/erc-common.el
@@ -31,12 +31,18 @@
 (defvar erc-channel-users)
 (defvar erc-dbuf)
 (defvar erc-log-p)
+(defvar erc-modules)
 (defvar erc-server-users)
 (defvar 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)
@@ -93,6 +99,40 @@
     (setq symbol canonical))
   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--favor-changed-reverted-modules-state (name op)
+  "Be more nuanced in displaying Custom state of `erc-modules'.
+When `customized-value' differs from `saved-value', allow widget
+to behave normally and show \"SET for current session\", as
+though `customize-set-variable' or similar had been applied.
+However, when `customized-value' and `standard-value' match but
+differ from `saved-value', prefer showing \"CHANGED outside
+Customize\" to prevent the widget from seeing a `standard'
+instead of a `set' state, which precludes any actual saving."
+  ;; Although the button "Apply and save" is fortunately grayed out,
+  ;; `Custom-save' doesn't actually save (users must click the magic
+  ;; state button instead).  The default behavior described in the doc
+  ;; string is intentional and was introduced by bug#12864 "Make state
+  ;; button interaction less confusing".  However, it is unfriendly to
+  ;; rogue libraries (like ours) that insist on mutating user options
+  ;; as a matter of course.
+  (custom-load-symbol 'erc-modules)
+  (funcall (get 'erc-modules 'custom-set) 'erc-modules
+           (funcall op (erc--normalize-module-symbol name) erc-modules))
+  (when (equal (pcase (get 'erc-modules 'saved-value)
+                 (`((quote ,saved) saved)))
+               erc-modules)
+    (customize-mark-as-set 'erc-modules)))
+
 (defun erc--assemble-toggle (localp name ablsym mode val body)
   (let ((arg (make-symbol "arg")))
     `(defun ,ablsym ,(if localp `(&optional ,arg) '())
@@ -110,11 +150,17 @@
                        (,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))
+                 (erc--favor-changed-reverted-modules-state
+                  ',name #',(if val 'cons 'delq))))
              (setq ,mode ,val)
              ,@body)))))
 
@@ -149,6 +195,61 @@
         (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
+    (unless w
+      (error "Failed to find %s in `erc-modules' checklist" name))
+    (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]"))))
+
+(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.
@@ -195,6 +296,8 @@ if ARG is omitted or nil.
 %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 cc5cac87da8..ea581c17661 100644
--- a/lisp/erc/erc.el
+++ b/lisp/erc/erc.el
@@ -1898,7 +1898,8 @@ removed from the list will be disabled."
                             (nreverse third-party))))
          ;; 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 45d8cae5125..b1df04841a4 100644
--- a/test/lisp/erc/erc-tests.el
+++ b/test/lisp/erc/erc-tests.el
@@ -1476,7 +1476,10 @@
                           ((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
@@ -1487,6 +1490,8 @@ if ARG is omitted or nil.
 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)))
@@ -1494,14 +1499,22 @@ Some docstring"
                       (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))
+                            (erc--favor-changed-reverted-modules-state
+                             'mname #'cons)))
                         (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))
+                            (erc--favor-changed-reverted-modules-state
+                             'mname #'delq)))
                         (setq erc-mname-mode nil)
                         (ignore c) (ignore d))