bug#15214: 24.3.50; `Revert This Session's Customization' does not do what it says

bug#15214: 24.3.50; `Revert This Session's Customization' does not do what it says

[Drew Adams]

1. emacs -Q

M-x customize-option backup-by-copying

Click Toggle, then State and Set For Current Session.
You have changed the value.

Click State and Revert This Session's Customization.

The current value does not change back to what it was before you changed
the setting for the session.  This session's customization was not
reverted.  However, even though the value was not changed, you see the
message "CHANGED outside Customize".

2. The State choice of Revert This Session's Customization is not even
documented at (emacs) `Changing a Variable'.  Other State actions are
documented.  So it's hard to tell whether #1 above is really a bug: it's
not even clear what this menu item is supposed to do.




In GNU Emacs 24.3.50.1 (i686-pc-mingw32)
 of 2013-08-23 on ODIEONE
Bzr revision: 113986 rgm@gnu.org-20130823185841-zoy6h1qk433ibrlf
Windowing system distributor `Microsoft Corp.', version 6.1.7601
Configured using:
 `configure --prefix=/c/Devel/emacs/binary --enable-checking=yes,glyphs
 'CFLAGS=-O0 -g3' LDFLAGS=-Lc:/Devel/emacs/lib
 CPPFLAGS=-Ic:/Devel/emacs/include'

bug#15214: 24.3.50; `Revert This Session's Customization' does not do what it says

[Mauro Aranda]

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

tags 15214 patch
thanks

Drew Adams <drew.adams@oracle.com> writes:

> 1. emacs -Q
>
> M-x customize-option backup-by-copying
>
> Click Toggle, then State and Set For Current Session.
> You have changed the value.
>
> Click State and Revert This Session's Customization.
>
> The current value does not change back to what it was before you changed
> the setting for the session.  This session's customization was not
> reverted.  However, even though the value was not changed, you see the
> message "CHANGED outside Customize".

I can reproduce this in latest master and emacs-27 branch.

> 2. The State choice of Revert This Session's Customization is not even
> documented at (emacs) `Changing a Variable'.  Other State actions are
> documented.  So it's hard to tell whether #1 above is really a bug: it's
> not even clear what this menu item is supposed to do.

Current (emacs) Changing a Variable says:
‘Revert This Session's Customizations’
     This restores the value of the variable to the last saved value, if
     there was one.  Otherwise it restores the standard value.  It
     updates the text accordingly.

The function that runs for that option is custom-variable-reset-saved,
and the doc string of custom-variable-reset-saved says something similar.

The attached patch fixes custom-variable-reset-saved to do what it says
it does when the variable has no previous saved value.

Best regards,
Mauro.

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

[-- Attachment #2: 0001-Reset-to-the-standard-value-when-reverting-session-s.patch --]
[-- Type: text/x-patch, Size: 1317 bytes --]

From 2092fb0b9a36775e22efc5fd967a05f6e4bffbeb Mon Sep 17 00:00:00 2001
From: Mauro Aranda <maurooaranda@gmail.com>
Date: Mon, 30 Dec 2019 18:10:28 -0300
Subject: [PATCH] Reset to the standard value when reverting session's
 customization

* lisp/cus-edit.el (custom-variable-reset-saved): When there is no
previous saved value, reset to the standard value.  (Bug#15214)
---
 lisp/cus-edit.el | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/lisp/cus-edit.el b/lisp/cus-edit.el
index 439667a..4fa6dfe 100644
--- a/lisp/cus-edit.el
+++ b/lisp/cus-edit.el
@@ -3038,8 +3038,11 @@ custom-variable-reset-saved
 	 (comment (get symbol 'saved-variable-comment)))
     (custom-variable-backup-value widget)
     (if (not (or saved-value comment))
-	;; If there is no saved value, remove the setting.
-	(custom-push-theme 'theme-value symbol 'user 'reset)
+        (progn
+	  ;; If there is no saved value, remove the setting.
+	  (custom-push-theme 'theme-value symbol 'user 'reset)
+          (funcall (or (get symbol 'custom-set) #'set-default) symbol
+                   (eval (car (get symbol 'standard-value)))))
       ;; Otherwise, apply the saved value.
       (put symbol 'variable-comment comment)
       (custom-push-theme 'theme-value symbol 'user 'set (car-safe saved-value))
-- 
2.7.4

bug#15214: 24.3.50; `Revert This Session's Customization' does not do what it says

[Drew Adams]

Thanks.

[(emacs) `Changing a Variable' was updated to mention 
this after the Emacs release for the bug report (24.3).]

bug#15214: 24.3.50; `Revert This Session's Customization' does not do what it says

[Eli Zaretskii]

> From: Mauro Aranda <maurooaranda@gmail.com>
> Date: Mon, 30 Dec 2019 18:30:31 -0300
> 
> > Click State and Revert This Session's Customization.

You should instead click "Set to Backup Value".  The Revert button is
in general for variables you have saved for future sessions.

> The function that runs for that option is custom-variable-reset-saved,
> and the doc string of custom-variable-reset-saved says something similar.

The doc string is ambiguous, and the code definitely does NOT intend
to reset the value, just to remove the recorded setting, so that it
won't be saved in the custom file.  That code was installed in
response to a very similar bug report (bug#9509, except that it
complained that Emacs signals an error for a variable that was never
saved).  I very much doubt that Chong, who made that change, omitted
setting the value by mistake.

In general, there's a feature creep here: this menu item was
originally only for customized options that were saved during this
session.  That was lifted back then, and now we want also to change
the value, although another menu item exists to do just that.

> The attached patch fixes custom-variable-reset-saved to do what it says
> it does when the variable has no previous saved value.

I won't object too much to such a change, if you still think it's TRT
here after reading the above, but I wonder whether it would be cleaner
and safer to set just the value, and move the funcall out of the
if-else form, so that we'd have only one funcall, and it will be
inside ignore-errors.

> -	;; If there is no saved value, remove the setting.
> -	(custom-push-theme 'theme-value symbol 'user 'reset)
> +        (progn
> +	  ;; If there is no saved value, remove the setting.

The comment needs to be updated, since we now don't merely remove the
setting.

Thanks.

bug#15214: 24.3.50; `Revert This Session's Customization' does not do what it says

[Mauro Aranda]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Mauro Aranda <maurooaranda@gmail.com>
>> Date: Mon, 30 Dec 2019 18:30:31 -0300
>>
>> > Click State and Revert This Session's Customization.
>
> You should instead click "Set to Backup Value".  The Revert button is
> in general for variables you have saved for future sessions.

For user options that are of boolean type, that works well.  But for
other kinds of user options, there is a risk it won't work:

1. emacs -Q
2. M-x customize-variable dired-kept-versions
3. Edit the field to 3 and then Set for current session.
4. Now edit it to 4, and then Set for current session.

Now "Set to Backup Value" won't ever take you back to the state before
your session's customizations.

>> The function that runs for that option is custom-variable-reset-saved,
>> and the doc string of custom-variable-reset-saved says something similar.
>
> The doc string is ambiguous, and the code definitely does NOT intend
> to reset the value, just to remove the recorded setting, so that it
> won't be saved in the custom file.  That code was installed in

What part of the doc string do you find ambiguous? Personally, I was
baffled about the (themed or standard) part, because if there is a
theme value (other than the user theme), that value counts as
the saved value too.

As for the intentions of the code, a symbol with no 'saved-value property
set would be enough for the customization to not be saved in the custom
file, AFAICS.  It's true that it does not intend to reset the value, but
at least my reading of the doc string makes me think it does, the Emacs
manual says it does, and even though the use case is rare, I think it's
better to do it.

> response to a very similar bug report (bug#9509, except that it
> complained that Emacs signals an error for a variable that was never
> saved).  I very much doubt that Chong, who made that change, omitted
> setting the value by mistake.

In Bug#9509 the suggestion was to revert those settings to the
un-customized value.  I can't tell whether Chong was inclined to
take that suggestion or not, but I do think that we should take it.

> In general, there's a feature creep here: this menu item was
> originally only for customized options that were saved during this
> session.  That was lifted back then, and now we want also to change
> the value, although another menu item exists to do just that.

See the first part of my reply.  I don't think there is another menu
item in an emacs -Q session able to take one back to the uncustomized
value, reliably.

>> The attached patch fixes custom-variable-reset-saved to do what it says
>> it does when the variable has no previous saved value.
>
> I won't object too much to such a change, if you still think it's TRT
> here after reading the above, but I wonder whether it would be cleaner
> and safer to set just the value, and move the funcall out of the
> if-else form, so that we'd have only one funcall, and it will be
> inside ignore-errors.

I still think it would be a good addition to the code.  In case you are
OK with it, I attach a patch with moves the funcall outside of the
if-else form.

Best regards,
Mauro.

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

[-- Attachment #2: 0001-Reset-to-the-standard-value-when-reverting-session-s.patch --]
[-- Type: text/x-patch, Size: 1959 bytes --]

From 0f1307239d29f4a06878c95840d9a43318f857b5 Mon Sep 17 00:00:00 2001
From: Mauro Aranda <maurooaranda@gmail.com>
Date: Mon, 30 Dec 2019 18:10:28 -0300
Subject: [PATCH] Reset to the standard value when reverting session's
 customizations

* lisp/cus-edit.el (custom-variable-reset-saved): When there is no
previous saved value, reset to the standard value.  (Bug#15214)
---
 lisp/cus-edit.el | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/lisp/cus-edit.el b/lisp/cus-edit.el
index 439667a..8a5fc8c 100644
--- a/lisp/cus-edit.el
+++ b/lisp/cus-edit.el
@@ -3035,17 +3035,18 @@ custom-variable-reset-saved
 before this operation becomes the backup value."
   (let* ((symbol (widget-value widget))
 	 (saved-value (get symbol 'saved-value))
-	 (comment (get symbol 'saved-variable-comment)))
+	 (comment (get symbol 'saved-variable-comment))
+         value)
     (custom-variable-backup-value widget)
     (if (not (or saved-value comment))
-	;; If there is no saved value, remove the setting.
-	(custom-push-theme 'theme-value symbol 'user 'reset)
-      ;; Otherwise, apply the saved value.
-      (put symbol 'variable-comment comment)
-      (custom-push-theme 'theme-value symbol 'user 'set (car-safe saved-value))
-      (ignore-errors
-	(funcall (or (get symbol 'custom-set) 'set-default)
-		 symbol (eval (car saved-value)))))
+        ;; If there is no saved value, remove the setting.
+        (custom-push-theme 'theme-value symbol 'user 'reset)
+      (setq value (car-safe saved-value))
+      (custom-push-theme 'theme-value symbol 'user 'set value)
+      (put symbol 'variable-comment comment))
+    (ignore-errors
+      (funcall (or (get symbol 'custom-set) #'set-default) symbol
+               (eval (or value (car (get symbol 'standard-value))))))
     (put symbol 'customized-value nil)
     (put symbol 'customized-variable-comment nil)
     (widget-put widget :custom-state 'unknown)
-- 
2.7.4

bug#15214: 24.3.50; `Revert This Session's Customization' does not do what it says

[Eli Zaretskii]

> From: Mauro Aranda <maurooaranda@gmail.com>
> Date: Wed, 1 Jan 2020 11:47:45 -0300
> Cc: 15214@debbugs.gnu.org
> 
> > You should instead click "Set to Backup Value".  The Revert button is
> > in general for variables you have saved for future sessions.
> 
> For user options that are of boolean type, that works well.  But for
> other kinds of user options, there is a risk it won't work:
> 
> 1. emacs -Q
> 2. M-x customize-variable dired-kept-versions
> 3. Edit the field to 3 and then Set for current session.
> 4. Now edit it to 4, and then Set for current session.
> 
> Now "Set to Backup Value" won't ever take you back to the state before
> your session's customizations.

"Set to Backup Value" reverts to the previous value, not to the
original one.  So this is working as intended.

> 
> >> The function that runs for that option is custom-variable-reset-saved,
> >> and the doc string of custom-variable-reset-saved says something similar.
> >
> > The doc string is ambiguous, and the code definitely does NOT intend
> > to reset the value, just to remove the recorded setting, so that it
> > won't be saved in the custom file.  That code was installed in
> 
> What part of the doc string do you find ambiguous?

The "reset" part.

> I still think it would be a good addition to the code.  In case you are
> OK with it, I attach a patch with moves the funcall outside of the
> if-else form.

It LGTM, thanks.

bug#15214: 24.3.50; `Revert This Session's Customization' does not do what it says

[Mauro Aranda]

[-- Attachment #1: Type: text/plain, Size: 517 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

>> What part of the doc string do you find ambiguous?
>
> The "reset" part.

The best I can come up with is just saying:
"...; otherwise, set the variable to the standard value."
(If the change is applied, of course).

>> I still think it would be a good addition to the code.  In case you are
>> OK with it, I attach a patch with moves the funcall outside of the
>> if-else form.
>
> It LGTM, thanks.

Great.  If there are no other objections, I'll wait for someone to push
it.

[-- Attachment #2: Type: text/html, Size: 692 bytes --]

bug#15214: 24.3.50; `Revert This Session's Customization' does not do what it says

[Eli Zaretskii]

> From: Mauro Aranda <maurooaranda@gmail.com>
> Date: Tue, 7 Jan 2020 18:58:13 -0300
> Cc: 15214@debbugs.gnu.org
> 
> >> I still think it would be a good addition to the code.  In case you are
> >> OK with it, I attach a patch with moves the funcall outside of the
> >> if-else form.
> >
> > It LGTM, thanks.
> 
> Great.  If there are no other objections, I'll wait for someone to push
> it.

Thanks, pushed to the emacs-27 branch, and closing the bug.