From: Mauro Aranda <maurooaranda@gmail.com>
To: 21355@debbugs.gnu.org
Cc: Greg Lucas <greg@glucas.net>
Subject: bug#21355: 24.4; Loading a theme causes session customizations to be saved
Date: Fri, 6 Nov 2020 09:31:43 -0300 [thread overview]
Message-ID: <CABczVwfSzxKTGONZihNgKLNG5j-bf8Ke1U5RnJBv0NW+Eux3LA@mail.gmail.com> (raw)
In-Reply-To: <CABczVwfB6WmyDjeEJ=0sW=f_AH-Lon5n+f9yPARrmdHuwZPcgg@mail.gmail.com>
[-- Attachment #1.1: Type: text/plain, Size: 178 bytes --]
Mauro Aranda <maurooaranda@gmail.com> writes:
> I attach a patch with three tests that demonstrate the bug.
And now I attach a patch to fix this, with the three tests updated.
[-- Attachment #1.2: Type: text/html, Size: 269 bytes --]
[-- Attachment #2: 0001-Avoid-saving-session-customizations-in-the-custom-fi.patch --]
[-- Type: text/x-patch, Size: 10848 bytes --]
From 24a895f54d2dc6333c34cca69e600c494e697a26 Mon Sep 17 00:00:00 2001
From: Mauro Aranda <maurooaranda@gmail.com>
Date: Fri, 6 Nov 2020 08:59:32 -0300
Subject: [PATCH] Avoid saving session customizations in the custom-file
* lisp/custom.el (custom-theme-recalc-variable): Only stash theme
settings for void variables.
(custom-declare-variable): After initializing a variable, unstash a
theme setting, if present.
(disable-theme): When disabling a theme, maybe unstash a theme
setting.
* test/lisp/custom-resources/custom--test-theme.el: Add two settings
for testing the fix.
* test/lisp/custom--tests.el (custom--test-bug-21355-before): New user
option for testing.
(custom-test-no-saved-value-after-enabling-theme)
(custom-test-no-saved-value-after-enabling-theme-2)
(custom-test-no-saved-value-after-customizing-option): New tests.
---
lisp/custom.el | 40 ++++++-
.../custom-resources/custom--test-theme.el | 4 +-
test/lisp/custom-tests.el | 104 ++++++++++++++++++
3 files changed, 142 insertions(+), 6 deletions(-)
diff --git a/lisp/custom.el b/lisp/custom.el
index cee4589543..0b23726361 100644
--- a/lisp/custom.el
+++ b/lisp/custom.el
@@ -205,7 +205,22 @@ custom-declare-variable
(put symbol 'custom-requests requests)
;; Do the actual initialization.
(unless custom-dont-initialize
- (funcall initialize symbol default)))
+ (funcall initialize symbol default)
+ ;; If there is a value under saved-value that wasn't saved by the user,
+ ;; reset it: we used that property to stash the value, but we don't need
+ ;; it anymore.
+ ;; This can happen given the following:
+ ;; 1. The user loaded a theme that had a setting for an unbound
+ ;; variable, so we stashed the theme setting under the saved-value
+ ;; property in `custom-theme-recalc-variable'.
+ ;; 2. Then, Emacs evaluated the defcustom for the option
+ ;; (e.g., something required the file where the option is defined).
+ ;; If we don't reset it and the user later sets this variable via
+ ;; Customize, we might end up saving the theme setting in the custom-file.
+ ;; See the test `custom-test-no-saved-value-after-customizing-option'.
+ (let ((theme (caar (get symbol 'theme-value))))
+ (when (and theme (not (eq theme 'user)) (get symbol 'saved-value))
+ (put symbol 'saved-value nil)))))
(run-hooks 'custom-define-hook)
symbol)
@@ -1458,7 +1473,15 @@ disable-theme
(custom-push-theme prop symbol theme 'reset)
(cond
((eq prop 'theme-value)
- (custom-theme-recalc-variable symbol))
+ (custom-theme-recalc-variable symbol)
+ ;; We might have to reset the stashed value of the variable, if
+ ;; no other theme is customizing it. Without this, loading a theme
+ ;; that has a setting for an unbound user option and then disabling
+ ;; it will leave this lingering setting for the option, and if then
+ ;; Emacs evaluates the defcustom the saved-value might be used to
+ ;; set the variable. (Bug#20766)
+ (unless (get symbol 'theme-value)
+ (put symbol 'saved-value nil)))
((eq prop 'theme-face)
;; If the face spec specified by this theme is in the
;; saved-face property, reset that property.
@@ -1507,9 +1530,16 @@ custom-variable-theme-value
(defun custom-theme-recalc-variable (variable)
"Set VARIABLE according to currently enabled custom themes."
(let ((valspec (custom-variable-theme-value variable)))
- (if valspec
- (put variable 'saved-value valspec)
- (setq valspec (get variable 'standard-value)))
+ ;; We used to save VALSPEC under the saved-value property unconditionally,
+ ;; but that is a recipe for trouble because we might end up saving session
+ ;; customizations if the user loads a theme. (Bug#21355)
+ ;; It's better to only use the saved-value property to stash the value only
+ ;; if we really need to stash it (i.e., VARIABLE is void).
+ (condition-case nil
+ (default-toplevel-value variable) ; See if it doesn't fail.
+ (void-variable (when valspec
+ (put variable 'saved-value valspec))))
+ (or valspec (setq valspec (get variable 'standard-value)))
(if (and valspec
(or (get variable 'force-value)
(default-boundp variable)))
diff --git a/test/lisp/custom-resources/custom--test-theme.el b/test/lisp/custom-resources/custom--test-theme.el
index 4ced98a50b..6b1cca3c15 100644
--- a/test/lisp/custom-resources/custom--test-theme.el
+++ b/test/lisp/custom-resources/custom--test-theme.el
@@ -6,6 +6,8 @@ custom--test
(custom-theme-set-variables
'custom--test
'(custom--test-user-option 'bar)
- '(custom--test-variable 'bar))
+ '(custom--test-variable 'bar)
+ '(custom--test-bug-21355-before 'before)
+ '(custom--test-bug-21355-after 'after))
(provide-theme 'custom--test)
diff --git a/test/lisp/custom-tests.el b/test/lisp/custom-tests.el
index a1451cf0ce..15661fb78f 100644
--- a/test/lisp/custom-tests.el
+++ b/test/lisp/custom-tests.el
@@ -156,4 +156,108 @@ check-for-wrong-custom-types
(load custom-test-admin-cus-test)
(should (null (cus-test-opts t))))
+;; The following three tests demonstrate Bug#21355.
+;; In this one, we set an user option for the current session and then
+;; we enable a theme that doesn't have a setting for it, ending up with
+;; a non-nil saved-value property. Since the `caar' of the theme-value
+;; property is user (i.e., the user theme setting is active), we might
+;; save the setting to the custom-file, even though it was meant for the
+;; current session only. So there should be a nil saved-value property
+;; for this test to pass.
+(ert-deftest custom-test-no-saved-value-after-enabling-theme ()
+ "Test that we don't record a saved-value property when we shouldn't."
+ (let ((custom-theme-load-path `(,(ert-resource-directory))))
+ (customize-option 'mark-ring-max)
+ (let* ((field (seq-find (lambda (widget)
+ (eq mark-ring-max (widget-value widget)))
+ widget-field-list))
+ (parent (widget-get field :parent)))
+ ;; Move to the editable widget, modify the value and save it.
+ (goto-char (widget-field-text-end field))
+ (insert "0")
+ (widget-apply parent :custom-set)
+ ;; Just setting for the current session should not store a saved-value
+ ;; property.
+ (should-not (get 'mark-ring-max 'saved-value))
+ ;; Now enable and disable the test theme.
+ (load-theme 'custom--test 'no-confirm)
+ (disable-theme 'custom--test)
+ ;; Since the user customized the option, this is OK.
+ (should (eq (caar (get 'mark-ring-max 'theme-value)) 'user))
+ ;; The saved-value property should still be nil.
+ (should-not (get 'mark-ring-max 'saved-value)))))
+
+;; In this second test, we load a theme that has a setting for the user option
+;; above. We must check that we don't end up with a non-nil saved-value
+;; property and a user setting active in the theme-value property, which
+;; means we might inadvertently save the session setting in the custom-file.
+(defcustom custom--test-bug-21355-before 'foo
+ "User option for `custom-test-no-saved-value-after-enabling-theme-2'."
+ :type 'symbol :group 'emacs)
+
+(ert-deftest custom-test-no-saved-value-after-enabling-theme-2 ()
+ "Test that we don't record a saved-value property when we shouldn't."
+ (let ((custom-theme-load-path `(,(ert-resource-directory))))
+ (customize-option 'custom--test-bug-21355-before)
+ (let* ((field (seq-find
+ (lambda (widget)
+ (eq custom--test-bug-21355-before (widget-value widget)))
+ widget-field-list))
+ (parent (widget-get field :parent)))
+ ;; Move to the editable widget, modify the value and save it.
+ (goto-char (widget-field-text-end field))
+ (insert "bar")
+ (widget-apply parent :custom-set)
+ ;; Just setting for the current session should not store a saved-value
+ ;; property.
+ (should-not (get 'custom--test-bug-21355-before 'saved-value))
+ ;; Now load our test theme, which has a setting for
+ ;; `custom--test-bug-21355-before'.
+ (load-theme 'custom--test 'no-confirm 'no-enable)
+ (enable-theme 'custom--test)
+ ;; Since the user customized the option, this is OK.
+ (should (eq (caar (get 'custom--test-bug-21355-before 'theme-value))
+ 'user))
+ ;; But the saved-value property has to be nil, since the user didn't mark
+ ;; this variable to save for future sessions.
+ (should-not (get 'custom--test-bug-21355-before 'saved-value)))))
+
+(defvar custom--test-bug-21355-after)
+
+;; In this test, we check that stashing a theme value for a not yet defined
+;; option works, but that later on if the user customizes the option for the
+;; current session, we might save the theme setting in the custom file.
+(ert-deftest custom-test-no-saved-value-after-customizing-option ()
+ "Test for a nil saved-value after setting an option for the current session."
+ (let ((custom-theme-load-path `(,(ert-resource-directory))))
+ ;; Check that we correctly stashed the value.
+ (load-theme 'custom--test 'no-confirm 'no-enable)
+ (enable-theme 'custom--test)
+ (should (and (not (boundp 'custom--test-bug-21355-after))
+ (eq (eval
+ (car (get 'custom--test-bug-21355-after 'saved-value)))
+ 'after)))
+ ;; Now Emacs finds the defcustom.
+ (defcustom custom--test-bug-21355-after 'initially "..."
+ :type 'symbol :group 'emacs)
+ ;; And we used the stashed value correctly.
+ (should (and (boundp 'custom--test-bug-21355-after)
+ (eq custom--test-bug-21355-after 'after)))
+ ;; Now customize it.
+ (customize-option 'custom--test-bug-21355-after)
+ (let* ((field (seq-find (lambda (widget)
+ (eq custom--test-bug-21355-after
+ (widget-value widget)))
+ widget-field-list))
+ (parent (widget-get field :parent)))
+ ;; Move to the editable widget, modify the value and save it.
+ (goto-char (widget-field-text-end field))
+ (insert "bar")
+ (widget-apply parent :custom-set)
+ ;; The user customized the variable, so this is OK.
+ (should (eq (caar (get 'custom--test-bug-21355-after 'theme-value))
+ 'user))
+ ;; But it was only for the current session, so this should not happen.
+ (should-not (get 'custom--test-bug-21355-after 'saved-value)))))
+
;;; custom-tests.el ends here
--
2.29.2
next prev parent reply other threads:[~2020-11-06 12:31 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-08-26 21:04 bug#21355: 24.4; Loading a theme causes session customizations to be saved Greg Lucas
2020-09-07 19:50 ` Mauro Aranda
2020-09-08 12:32 ` Mauro Aranda
2020-11-06 12:31 ` Mauro Aranda [this message]
2021-05-10 11:35 ` Lars Ingebrigtsen
2021-05-11 11:53 ` Mauro Aranda
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: https://www.gnu.org/software/emacs/
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CABczVwfSzxKTGONZihNgKLNG5j-bf8Ke1U5RnJBv0NW+Eux3LA@mail.gmail.com \
--to=maurooaranda@gmail.com \
--cc=21355@debbugs.gnu.org \
--cc=greg@glucas.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).