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