custom-set-variables considered harmful

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

custom-set-variables considered harmful

Stefan Monnier
I keep seeing people copy and pasting part of calls to
custom-set-variables, and getting all confused about it.

I think we should stop using it.  Here's a proposal for that.

When writing customizations, instead of writing

    (custom-set-variables
     ;; Big ugly warning which doesn't help enough.
     '(VAR1 VAL1)
     '(VAR2 VAL2 nil '(REQUEST) COMMENT)
     '(VAR3 VAL3)
     ...)

we write

    (autogenerated-custom-settings
      ;; Big warning, still, but less important.
      (setq VAR1 VAL1)
      (require 'REQUEST)
      (customize-set-variable VAR2 VAL2 COMMENT)
      (customize-set-variable VAR3 VAL3)
      ...)

where `autogenerated-custom-settings` is a macro which turns that call
back into the form expected by custom-set-variables.

The idea is basically to use a syntax which happens to look close to
what you'd write by hand if you weren't using Customize, so that users
who copy&paste snippets of code end up copying more or less valid code,
instead of copying "code" like

    '(toto 2)

and then be surprised that it doesn't do anything.

[ The difference between VAR1 and VAR3 is that VAR1 doesn't have
  a setter.  I guess in some cases we should emit `(setq-default VAR VAL)`
  also.  ]

One question, tho: could someone explain to me what the NOW is used
for, really?  I've read the docstrings, comments, and the code that
seems related to it, and I have some idea of what it supposedly does,
but I don't have a clear idea of a scenario where it's used.


        Stefan

Reply | Threaded
Open this post in threaded view
|

Re: custom-set-variables considered harmful

Philippe Vaucher

The idea is basically to use a syntax which happens to look close to
what you'd write by hand if you weren't using Customize, so that users
who copy&paste snippets of code end up copying more or less valid code,
instead of copying "code" like

    '(toto 2)

and then be surprised that it doesn't do anything.

Good idea! It's also consistent with "Show saved lisp expression" from customize buffers.

In my case, I always delete the whole custom-set-variables block and move the relevant parts in my config, but have to transform the sexps into their `setq` equivalent, which is annoying.

Philippe
Reply | Threaded
Open this post in threaded view
|

Re: custom-set-variables considered harmful

Clément Pit-Claudel
On 2017-11-06 04:02, Philippe Vaucher wrote:
> but have to transform the sexps into their `setq` equivalent, which is annoying.

And often they're not equivalent, either, which causes plenty of issues for beginners.


Reply | Threaded
Open this post in threaded view
|

Re: custom-set-variables considered harmful

raman
In reply to this post by Philippe Vaucher
Here i a useful defmacro that helps with setting things that are
customized via custom without having to think about it:

(defmacro c-setq (variable value)
  "Exactly like setq, but handles custom."
  `(funcall (or (get ',variable 'custom-set) 'set-default) ',variable ,value))
--

Reply | Threaded
Open this post in threaded view
|

Re: custom-set-variables considered harmful

Stefan Monnier
> Here i a useful defmacro that helps with setting things that are
> customized via custom without having to think about it:

> (defmacro c-setq (variable value)
>   "Exactly like setq, but handles custom."
>   `(funcall (or (get ',variable 'custom-set) 'set-default) ',variable ,value))

I think you should use customize-set-variable instead
(or make your macro use it).


        Stefan


Reply | Threaded
Open this post in threaded view
|

Re: custom-set-variables considered harmful

Vivek Dasmohapatra-2
In reply to this post by Stefan Monnier
> The idea is basically to use a syntax which happens to look close to
> what you'd write by hand if you weren't using Customize, so that users
> who copy&paste snippets of code end up copying more or less valid code,
> instead of copying "code" like

Won't they then be surprised by the normal value sanitisation/hook-firing
etc that some settings rely on not happening and have even less idea
why it's not working when they inevitably cargo-cult the contents?


Reply | Threaded
Open this post in threaded view
|

Re: custom-set-variables considered harmful

Stefan Monnier
> Won't they then be surprised by the normal value sanitisation/hook-firing
> etc that some settings rely on not happening and have even less idea
> why it's not working when they inevitably cargo-cult the contents?

Hmm... what you say rings some kind of bell, but not clearly enough:
could give a scenario to clarify the issue?


        Stefan


Reply | Threaded
Open this post in threaded view
|

Re: custom-set-variables considered harmful

Vivek Dasmohapatra-2
On Wed, 8 Nov 2017, Stefan Monnier wrote:

>> Won't they then be surprised by the normal value sanitisation/hook-firing

> Hmm... what you say rings some kind of bell, but not clearly enough:
> could give a scenario to clarify the issue?

Sure, take something like glyphless-char-display-control which says:

==========================================================================
Do not set its value directly from Lisp; the value takes effect
only via a custom ‘:set’
function (‘update-glyphless-char-display’), which updates
‘glyphless-char-display’.

You can customize this variable.
==========================================================================

Or a variable which has a restricted set of possible values, as determined
by its defcustom declaration...

In the former case (or similar cases) the user might copy the setq and then
be surprised that it does not work as expected. In the latter, they would
copy the setq, alter the value (possibly to an "illegal" value) and then
be surprised it didnt work.

Maybe instead of setq a (users-will-copy-this-even-though-we-said-not-to …)
form which _could_ be copied out of the custom-set-variables expression
independently and would still trigger the relevant custom-related hooks
and value-sanitisation (I believe someone suggested custom-set or similar
instead of setq which might well do that).
Reply | Threaded
Open this post in threaded view
|

Re: custom-set-variables considered harmful

Stefan Monnier
I think you misunderstood my suggestion.  E.g. look at how VAR3 is
handled in my example:

     '(VAR3 VAL3)

is turned into

      (customize-set-variable VAR3 VAL3)

rather than into

      (setq VAR3 VAL3)

The idea was that VAR3 is a variable with a :setter.

> In the former case (or similar cases) the user might copy the setq and then
> be surprised that it does not work as expected.

This is already a problem since '(VAR VAL3) doesn't do what the user
intended when copied outside of the custom-set-variables block.  The new
doesn't aim to guarantee that copy&paste always works.  It just aims to
get a bit closer.

E.g. note that using customize-set-variable doesn't solve the problem
completely either (there are differences in terms of *when* the settings
is applied), but it will often work.

> In the latter, they would copy the setq, alter the value (possibly to
> an "illegal" value) and then be surprised it didnt work.

While the change I propose doesn't solve this problem, it doesn't make
it worse either, AFAICT.


        Stefan

Reply | Threaded
Open this post in threaded view
|

Re: custom-set-variables considered harmful

Vivek Dasmohapatra-2
On Wed, 8 Nov 2017, Stefan Monnier wrote:

> I think you misunderstood my suggestion.  E.g. look at how VAR3 is
> handled in my example:
>
>     '(VAR3 VAL3)
>
> is turned into
>
>      (customize-set-variable VAR3 VAL3)

Ah, ok. Fair enough.


Reply | Threaded
Open this post in threaded view
|

Re: custom-set-variables considered harmful

Stefan Monnier
In reply to this post by Stefan Monnier
> When writing customizations, instead of writing
>
>     (custom-set-variables
>      ;; Big ugly warning which doesn't help enough.
>      '(VAR1 VAL1)
>      '(VAR2 VAL2 nil '(REQUEST) COMMENT)
>      '(VAR3 VAL3)
>      ...)
>
> we write
>
>     (autogenerated-custom-settings
>       ;; Big warning, still, but less important.
>       (setq VAR1 VAL1)
>       (require 'REQUEST)
>       (customize-set-variable VAR2 VAL2 COMMENT)
>       (customize-set-variable VAR3 VAL3)
>       ...)

Here's a sample patch to do that.  Comments?


        Stefan


diff --git a/lisp/cus-edit.el b/lisp/cus-edit.el
index edf3545cad..0f9b38974d 100644
--- a/lisp/cus-edit.el
+++ b/lisp/cus-edit.el
@@ -4374,6 +4382,16 @@ custom-file
 if only the first line of the docstring is shown."))
   :group 'customize)
 
+(defcustom custom-mimic-plain-elisp t
+  "If non-nil, save user settings with the new format.
+This new format tries to mimick the code that would be written by hand
+in plain Elisp.  But it relies on `custom-autogenerated-user-settings' which
+is a new macro in Emacs-27, so settings saved with this will either
+require a recent enough Emacs, or some ad-hoc hack such
+as (defalias 'custom-autogenerated-user-settings #'ignore)."
+  :type 'boolean
+  :group 'customize)
+
 (defun custom-file (&optional no-error)
   "Return the file name for saving customizations."
   (if (or (null user-init-file)
@@ -4505,6 +4523,7 @@ custom-save-variables
   "Save all customized variables in `custom-file'."
   (save-excursion
     (custom-save-delete 'custom-set-variables)
+    (custom-save-delete 'custom-autogenerated-user-settings)
     (let ((standard-output (current-buffer))
   (saved-list (make-list 1 0))
   sort-fold-case)
@@ -4519,8 +4538,12 @@ custom-save-variables
       (setq saved-list (sort (cdr saved-list) 'string<))
       (unless (bolp)
  (princ "\n"))
-      (princ "(custom-set-variables
- ;; custom-set-variables was added by Custom.
+      (princ (if custom-mimic-plain-elisp
+                 "(custom-autogenerated-user-settings
+ ;; This custom-autogenerated-user-settings was added by Custom."
+               "(custom-set-variables
+ ;; This custom-set-variables was added by Custom."))
+      (princ "
  ;; If you edit it by hand, you could mess it up, so be careful.
  ;; Your init file should contain only one such instance.
  ;; If there is more than one, they won't work right.\n")
@@ -4555,28 +4578,43 @@ custom-save-variables
     ;; with the customize facility.
     (unless (bolp)
       (princ "\n"))
-    (princ " '(")
-    (prin1 symbol)
-    (princ " ")
-    (let ((val (prin1-to-string (car value))))
-      (if (< (length val) 60)
-  (insert val)
- (newline-and-indent)
- (let ((beginning-of-val (point)))
-  (insert val)
-  (save-excursion
-    (goto-char beginning-of-val)
-    (indent-pp-sexp 1)))))
-    (when (or now requests comment)
+            (if custom-mimic-plain-elisp
+                ;; Do the inverse conversion of
+                ;; custom-autogenerated-user-settings.
+                (let* ((e (cond
+                           ((get symbol 'custom-set)
+                            `(customize-set-variable ',symbol ,(car value)))
+                           ((local-variable-if-set-p
+                             symbol (get-buffer-create "*scratch*"))
+                            `(setq-default ,symbol ,(car value)))
+                           (t `(setq ,symbol ,(car value))))))
+                  (dolist (e (nconc (mapcar (lambda (r) `(require ',r)) requests)
+                                    (if comment (list comment e) (list e))))
+                    (princ " ")
+                    (pp e)
+                    (unless (bolp) (princ "\n"))))
+      (princ " '(")
+      (prin1 symbol)
       (princ " ")
-      (prin1 now)
-      (when (or requests comment)
- (princ " ")
- (prin1 requests)
- (when comment
+      (let ((val (prin1-to-string (car value))))
+        (if (< (length val) 60)
+    (insert val)
+  (newline-and-indent)
+  (let ((beginning-of-val (point)))
+    (insert val)
+    (save-excursion
+      (goto-char beginning-of-val)
+      (indent-pp-sexp 1)))))
+      (when (or now requests comment)
+        (princ " ")
+        (prin1 now)
+        (when (or requests comment)
   (princ " ")
-  (prin1 comment))))
-    (princ ")"))))
+  (prin1 requests)
+  (when comment
+    (princ " ")
+    (prin1 comment))))
+      (princ ")")))))
       (if (bolp)
   (princ " "))
       (princ ")")
@@ -4586,7 +4624,7 @@ custom-save-variables
 (defun custom-save-faces ()
   "Save all customized faces in `custom-file'."
   (save-excursion
-    (custom-save-delete 'custom-reset-faces)
+    (custom-save-delete 'custom-reset-faces) ;FIXME: never written!?
     (custom-save-delete 'custom-set-faces)
     (let ((standard-output (current-buffer))
   (saved-list (make-list 1 0))
@@ -4738,9 +4776,9 @@ customize-menu-create
 
 (defvar tool-bar-map)
 
-;;; `custom-tool-bar-map' used to be set up here.  This will fail to
-;;; DTRT when `display-graphic-p' returns nil during compilation.  Hence
-;;; we set this up lazily in `Custom-mode'.
+;; `custom-tool-bar-map' used to be set up here.  This will fail to
+;; DTRT when `display-graphic-p' returns nil during compilation.  Hence
+;; we set this up lazily in `Custom-mode'.
 (defvar custom-tool-bar-map nil
   "Keymap for toolbar in Custom mode.")
 
diff --git a/lisp/custom.el b/lisp/custom.el
index 352fc6bd53..f541b51420 100644
--- a/lisp/custom.el
+++ b/lisp/custom.el
@@ -1,4 +1,4 @@
-;;; custom.el --- tools for declaring and initializing options
+;;; custom.el --- tools for declaring and initializing options  -*- lexical-binding:t -*-
 ;;
 ;; Copyright (C) 1996-1997, 1999, 2001-2017 Free Software Foundation,
 ;; Inc.
@@ -926,6 +933,35 @@ custom-fix-face-spec
   (nreverse result))
       spec)))
 
+(defmacro custom-autogenerated-user-settings (&rest body)
+  "Install user customizations of variable values specified in ARGS.
+Expect the format output by `custom-save-variables'."
+  (let* ((settings '())
+         (requests '())
+         (comment nil)
+         (mk (lambda (x e)
+               (push
+                `'(,x ,e
+                      . ,(when (or comment requests)
+                           `(nil ,(prog1 (nreverse requests)
+                                    (setq requests '()))
+                                 . ,(when comment
+                                      (prog1 (list comment)
+                                        (setq comment nil))))))
+                settings))))
+    (dolist (e body)
+      (pcase e
+        (`(,(or 'setq 'setq-default) ,x ,e) (funcall mk x e))
+        (`(require ',p) (push p requests))
+        (`(customize-set-variable ',x ,e) (funcall mk x e))
+        (`(,x ,(and v (or 1 -1))) (funcall mk x (> v 0)))
+        ((pred stringp)
+         (and comment (message "Dropping extra comment: %S" comment))
+         (setq comment e))
+        (_
+         (message "Unrecognized expression in custom-autogenerated-user-settings: %S" e))))
+    `(custom-set-variables ,@(nreverse settings))))
+
 (defun custom-set-variables (&rest args)
   "Install user customizations of variable values specified in ARGS.
 These settings are registered as theme `user'.
@@ -940,7 +976,7 @@ custom-set-variables
 REQUEST is a list of features we must require in order to
 handle SYMBOL properly.
 COMMENT is a comment string about SYMBOL."
-  (apply 'custom-theme-set-variables 'user args))
+  (apply #'custom-theme-set-variables 'user args))
 
 (defun custom-theme-set-variables (theme &rest args)
   "Initialize variables for theme THEME according to settings in ARGS.
@@ -1505,7 +1541,7 @@ custom-reset-variables
     (VARIABLE IGNORED)
 
 This means reset VARIABLE.  (The argument IGNORED is ignored)."
-    (apply 'custom-theme-reset-variables 'user args))
+    (apply #'custom-theme-reset-variables 'user args))
 
 ;;; The End.