bug#34027: 27.0.50; disable-theme resets variables to their initial values

[Mauro Aranda <maurooaranda@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 1153 bytes --]

Michael Albinus <michael.albinus@gmx.de> writes:

> Scenario:
>
> - Declare a variable (either defvar or defcustom)
> - Change the initial value to something else
> - Enable a theme, which changes the value to something different, again
> - Disable the theme
>
> I would expect that the variable has been reset to the value prior
> enabling the theme. But it is reset to the initial value.
>

There is code in custom-push-theme that attempts to handle this case,
but it was being skipped because custom didn't think it should apply the
settings of the theme (and at the same time, preserve priors
customizations).

That is controlled by the variable custom--inhibit-theme-enable, and
we should bind it to nil in enable-theme, because we are definitely
enabling it.  Once we do that, it is just a matter of using
custom-push-theme to handle the case like it's supposed to.

My patch does that, and introduces an extra check in custom-push-theme,
because while testing I found another instance of Bug#28904.  The rest
this patch does is changing the test because of the comments I made in
another post to this bug, and we now can expect the test to pass.

[-- Attachment #1.2: Type: text/html, Size: 1343 bytes --]

[-- Attachment #2: 0001-Preserve-user-customizations-after-disabling-a-theme.patch --]
[-- Type: text/x-patch, Size: 4764 bytes --]

From fa9326f7e77f85cab9627face82465ea1af261bd Mon Sep 17 00:00:00 2001
From: Mauro Aranda <maurooaranda@gmail.com>
Date: Fri, 4 Sep 2020 11:46:33 -0300
Subject: [PATCH] Preserve user customizations after disabling a theme

* lisp/custom.el (enable-theme): Since we are enabling the theme, bind
custom--inhibit-theme-enable to nil.  Then rely on custom-push-theme
to do the right thing with the theme settings and prior user settings,
instead of manipulating the property here.  This way, when disabling a
theme, we restore user preferences, even when the values were changed
outside of customize.
(disable-theme): Call custom-push-theme instead of handling theme
settings directly.
(custom-push-theme): Avoid another instance of Bug#28904: we don't
need the changed theme when the value recorded for it is going to be
the same as the recorded for the user theme.

* test/lisp/custom-tests.el (custom--test-theme-variables): Get rid of
a portion of the test that will always fail, because the user theme
has priority over every other theme.  Expect the test to pass now that
we preserve user customizations after disabling a theme.  (Bug#34027)
---
 lisp/custom.el            | 21 +++++++++++++++------
 test/lisp/custom-tests.el |  8 +-------
 2 files changed, 16 insertions(+), 13 deletions(-)

diff --git a/lisp/custom.el b/lisp/custom.el
index 7581457ce8..cc445fe765 100644
--- a/lisp/custom.el
+++ b/lisp/custom.el
@@ -907,7 +907,15 @@ custom-push-theme
 		     (boundp symbol))
 	    (let ((sv  (get symbol 'standard-value))
 		  (val (symbol-value symbol)))
-	      (unless (and sv (equal (eval (car sv)) val))
+	      (unless (or
+                       ;; We only do this trick if the current value
+                       ;; is different from the standard value.
+                       (and sv (equal (eval (car sv)) val))
+                       ;; And we don't do it if we would end up recording
+                       ;; the same value for the user theme.  This way we avoid
+                       ;; having ((user VALUE) (changed VALUE)).  That would be
+                       ;; useless, because we don't disable the user theme.
+                       (and (eq theme 'user) (equal (custom-quote val) value)))
 		(setq old `((changed ,(custom-quote val))))))))
 	(put symbol prop (cons (list theme value) old)))
       (put theme 'theme-settings
@@ -1368,13 +1376,14 @@ enable-theme
 		       obarray (lambda (sym) (get sym 'theme-settings)) t))))
   (unless (custom-theme-p theme)
     (error "Undefined Custom theme %s" theme))
-  (let ((settings (get theme 'theme-settings)))
+  (let ((settings (get theme 'theme-settings)) ; '(prop symbol theme value)
+        ;; We are enabling the theme, so don't inhibit enabling it.  (Bug#34027)
+        (custom--inhibit-theme-enable nil))
     ;; Loop through theme settings, recalculating vars/faces.
     (dolist (s settings)
       (let* ((prop (car s))
-	     (symbol (cadr s))
-	     (spec-list (get symbol prop)))
-	(put symbol prop (cons (cddr s) (assq-delete-all theme spec-list)))
+	     (symbol (cadr s)))
+        (custom-push-theme prop symbol theme 'set (nth 3 s))
 	(cond
 	 ((eq prop 'theme-face)
 	  (custom-theme-recalc-face symbol))
@@ -1443,7 +1452,7 @@ disable-theme
 	(let* ((prop   (car s))
 	       (symbol (cadr s))
 	       (val (assq-delete-all theme (get symbol prop))))
-	  (put symbol prop val)
+          (custom-push-theme prop symbol theme 'reset)
 	  (cond
 	   ((eq prop 'theme-value)
 	    (custom-theme-recalc-variable symbol))
diff --git a/test/lisp/custom-tests.el b/test/lisp/custom-tests.el
index 766e484498..7853c84bb6 100644
--- a/test/lisp/custom-tests.el
+++ b/test/lisp/custom-tests.el
@@ -99,7 +99,6 @@ custom--test-variable
 ;; This is demonstrating bug#34027.
 (ert-deftest custom--test-theme-variables ()
   "Test variables setting with enabling / disabling a custom theme."
-  :expected-result :failed
   ;; We load custom-resources/custom--test-theme.el.
   (let ((custom-theme-load-path
          `(,(expand-file-name "custom-resources" (file-name-directory #$)))))
@@ -115,15 +114,10 @@ custom--test-theme-variables
     (should (equal custom--test-user-option 'baz))
     (should (equal custom--test-variable 'baz))
 
+    ;; Enable and then disable.
     (enable-theme 'custom--test)
-    ;; The variables have the theme values.
-    (should (equal custom--test-user-option 'bar))
-    (should (equal custom--test-variable 'bar))
-
     (disable-theme 'custom--test)
     ;; The variables should have the changed values, by reverting.
-    ;; This doesn't work as expected.  Instead, they have their
-    ;; initial values `foo'.
     (should (equal custom--test-user-option 'baz))
     (should (equal custom--test-variable 'baz))))
 
-- 
2.28.0