From: F. Jason Park Date: Sun, 15 Jan 2023 03:08:11 +0000 (-0800) Subject: Warn when customizing minor-mode vars for ERC modules X-Git-Url: http://git.eshelyaron.com/gitweb/?a=commitdiff_plain;h=9c65ac73655c71a7f289d8c86ee6d7a314c32a05;p=emacs.git 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.) --- 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))