bug#5358: 23.1; Adding comment loses customizations in progress

bug#5358: 23.1; Adding comment loses customizations in progress

[David Abrahams]

Please write in English if possible, because the Emacs maintainers
usually do not have translators to read other languages for them.

Your bug report will be posted to the bug-gnu-emacs@gnu.org mailing list,
and to the gnu.emacs.bug news group.

Please describe exactly what actions triggered the bug
and the precise symptoms of the bug:

Using the customization interface, I can make a change to a variable,
then try to add a comment before saving.  When I do that, the changes
I've already made are lost.

If Emacs crashed, and you have the Emacs process in the gdb debugger,
please include the output from the following gdb commands:
    `bt full' and `xbacktrace'.
If you would like to further debug the crash, please read the file
/Applications/Emacs.app/Contents/Resources/etc/DEBUG for instructions.


In GNU Emacs 23.1.1 (i386-apple-darwin9.8.0, NS apple-appkit-949.54)
 of 2009-08-16 on black.local
Windowing system distributor `Apple', version 10.3.1038
configured using `configure  '--with-ns''

Important settings:
  value of $LC_ALL: nil
  value of $LC_COLLATE: nil
  value of $LC_CTYPE: nil
  value of $LC_MESSAGES: nil
  value of $LC_MONETARY: nil
  value of $LC_NUMERIC: nil
  value of $LC_TIME: nil
  value of $LANG: nil
  value of $XMODIFIERS: nil
  locale-coding-system: nil
  default-enable-multibyte-characters: t

Major mode: Custom

Minor modes in effect:
  diff-auto-refine-mode: t
  shell-dirtrack-mode: t
  show-paren-mode: t
  server-mode: t
  global-auto-revert-mode: t
  delete-selection-mode: t
  tooltip-mode: t
  mouse-wheel-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  global-auto-composition-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Recent input:
C-n C-n C-p C-p C-. C-. C-. C-b C-b <return> C-n C-n 
C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n 
C-e <backspace> 0 C-n C-n C-n C-n C-n C-n C-n C-n C-n 
C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n 
C-p C-p C-p C-p C-p C-p C-p C-p C-p C-p C-p C-p C-p 
C-p C-p C-p C-n C-n C-n C-n C-n C-n C-n C-n C-n C-p 
C-p C-p C-p C-p C-p C-p C-p C-p C-p C-p C-p C-p C-p 
C-p C-p C-n C-n C-n C-n C-n C-n C-p C-p C-p C-p C-n 
C-n C-n C-n C-n C-n C-n C-n C-n C-p C-n C-n C-n C-n 
C-n C-n C-n C-n C-n C-n C-n C-a C-p C-. C-. C-, <return> 
7 C-n C-n C-n C-e M-< C-n C-n C-n C-n C-n C-n C-n C-n 
C-n C-n C-n C-n C-n C-n C-n C-n C-e C-p C-e <backspace> 
0 C-n C-n C-n C-n C-n C-n C-n C-n C-n C-p C-p C-p C-p 
C-p C-p C-p C-p C-p C-p C-p C-x C-s C-n C-n C-n C-n 
C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n 
C-n C-n C-n C-n C-n C-n C-n C-n C-n C-p <M-return> 
<C-return> C-q C-j I SPC r e a l l y SPC d o n ' t 
SPC w a n t SPC t o SPC s e t e <backspace> <backspace> 
e SPC H T M L S-SPC u n l e s s SPC i <backspace> t 
h a t ' s SPC a l l SPC t h e r e SPC i s . <backspace> 
C-c C-c C-x C-s C-p C-p C-p C-p C-p C-p C-p C-p C-p 
C-p C-p M-x r e p o <tab> r <tab> b <tab> <return>

Recent messages:
Mark set [2 times]
Saving file /Users/dave/src/elisp.repo/elisp/custom.el...
Wrote /Users/dave/src/elisp.repo/elisp/custom.el
Compiling /Users/dave/src/elisp.repo/elisp/custom.el...done
Wrote /Users/dave/src/elisp.repo/elisp/custom.elc
Saving file /Users/dave/src/elisp.repo/elisp/custom.el...
Wrote /Users/dave/src/elisp.repo/elisp/custom.el
Compiling /Users/dave/src/elisp.repo/elisp/custom.el...done
Wrote /Users/dave/src/elisp.repo/elisp/custom.elc
Making completion list...
-- 
Dave Abrahams           Meet me at BoostCon: http://www.boostcon.com
BoostPro Computing
http://www.boostpro.com

bug#5358: 23.1; Adding comment loses customizations in progress

[Andrew Hyatt]

Sorry, I don't understand this bug report. What do you mean by adding
comments?

David Abrahams <dave@boostpro.com> writes:

> Please write in English if possible, because the Emacs maintainers
> usually do not have translators to read other languages for them.
>
> Your bug report will be posted to the bug-gnu-emacs@gnu.org mailing list,
> and to the gnu.emacs.bug news group.
>
> Please describe exactly what actions triggered the bug
> and the precise symptoms of the bug:
>
> Using the customization interface, I can make a change to a variable,
> then try to add a comment before saving.  When I do that, the changes
> I've already made are lost.
>
> If Emacs crashed, and you have the Emacs process in the gdb debugger,
> please include the output from the following gdb commands:
>     `bt full' and `xbacktrace'.
> If you would like to further debug the crash, please read the file
> /Applications/Emacs.app/Contents/Resources/etc/DEBUG for instructions.
>
>
> In GNU Emacs 23.1.1 (i386-apple-darwin9.8.0, NS apple-appkit-949.54)
>  of 2009-08-16 on black.local
> Windowing system distributor `Apple', version 10.3.1038
> configured using `configure  '--with-ns''
>
> Important settings:
>   value of $LC_ALL: nil
>   value of $LC_COLLATE: nil
>   value of $LC_CTYPE: nil
>   value of $LC_MESSAGES: nil
>   value of $LC_MONETARY: nil
>   value of $LC_NUMERIC: nil
>   value of $LC_TIME: nil
>   value of $LANG: nil
>   value of $XMODIFIERS: nil
>   locale-coding-system: nil
>   default-enable-multibyte-characters: t
>
> Major mode: Custom
>
> Minor modes in effect:
>   diff-auto-refine-mode: t
>   shell-dirtrack-mode: t
>   show-paren-mode: t
>   server-mode: t
>   global-auto-revert-mode: t
>   delete-selection-mode: t
>   tooltip-mode: t
>   mouse-wheel-mode: t
>   menu-bar-mode: t
>   file-name-shadow-mode: t
>   global-font-lock-mode: t
>   font-lock-mode: t
>   global-auto-composition-mode: t
>   auto-composition-mode: t
>   auto-encryption-mode: t
>   auto-compression-mode: t
>   line-number-mode: t
>   transient-mark-mode: t
>
> Recent input:
> C-n C-n C-p C-p C-. C-. C-. C-b C-b <return> C-n C-n 
> C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n 
> C-e <backspace> 0 C-n C-n C-n C-n C-n C-n C-n C-n C-n 
> C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n 
> C-p C-p C-p C-p C-p C-p C-p C-p C-p C-p C-p C-p C-p 
> C-p C-p C-p C-n C-n C-n C-n C-n C-n C-n C-n C-n C-p 
> C-p C-p C-p C-p C-p C-p C-p C-p C-p C-p C-p C-p C-p 
> C-p C-p C-n C-n C-n C-n C-n C-n C-p C-p C-p C-p C-n 
> C-n C-n C-n C-n C-n C-n C-n C-n C-p C-n C-n C-n C-n 
> C-n C-n C-n C-n C-n C-n C-n C-a C-p C-. C-. C-, <return> 
> 7 C-n C-n C-n C-e M-< C-n C-n C-n C-n C-n C-n C-n C-n 
> C-n C-n C-n C-n C-n C-n C-n C-n C-e C-p C-e <backspace> 
> 0 C-n C-n C-n C-n C-n C-n C-n C-n C-n C-p C-p C-p C-p 
> C-p C-p C-p C-p C-p C-p C-p C-x C-s C-n C-n C-n C-n 
> C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n 
> C-n C-n C-n C-n C-n C-n C-n C-n C-n C-p <M-return> 
> <C-return> C-q C-j I SPC r e a l l y SPC d o n ' t 
> SPC w a n t SPC t o SPC s e t e <backspace> <backspace> 
> e SPC H T M L S-SPC u n l e s s SPC i <backspace> t 
> h a t ' s SPC a l l SPC t h e r e SPC i s . <backspace> 
> C-c C-c C-x C-s C-p C-p C-p C-p C-p C-p C-p C-p C-p 
> C-p C-p M-x r e p o <tab> r <tab> b <tab> <return>
>
> Recent messages:
> Mark set [2 times]
> Saving file /Users/dave/src/elisp.repo/elisp/custom.el...
> Wrote /Users/dave/src/elisp.repo/elisp/custom.el
> Compiling /Users/dave/src/elisp.repo/elisp/custom.el...done
> Wrote /Users/dave/src/elisp.repo/elisp/custom.elc
> Saving file /Users/dave/src/elisp.repo/elisp/custom.el...
> Wrote /Users/dave/src/elisp.repo/elisp/custom.el
> Compiling /Users/dave/src/elisp.repo/elisp/custom.el...done
> Wrote /Users/dave/src/elisp.repo/elisp/custom.elc
> Making completion list...

bug#5358: 23.1; Adding comment loses customizations in progress

[Dave Abrahams]

There's an choice in the customization UI on emacsen < 24.5 that lets you add
a comment to any customization, but now I don't see it.  Apparently that
feature was dropped :(

on Tue Feb 16 2016, Andrew Hyatt <ahyatt-AT-gmail.com> wrote:

> Sorry, I don't understand this bug report. What do you mean by adding
> comments?
>
> David Abrahams <dave@boostpro.com> writes:
>
>> Please write in English if possible, because the Emacs maintainers
>> usually do not have translators to read other languages for them.
>>
>> Your bug report will be posted to the bug-gnu-emacs@gnu.org mailing list,
>> and to the gnu.emacs.bug news group.
>>
>> Please describe exactly what actions triggered the bug
>> and the precise symptoms of the bug:
>>
>> Using the customization interface, I can make a change to a variable,
>> then try to add a comment before saving.  When I do that, the changes
>> I've already made are lost.
>>
>> If Emacs crashed, and you have the Emacs process in the gdb debugger,
>> please include the output from the following gdb commands:
>>     `bt full' and `xbacktrace'.
>> If you would like to further debug the crash, please read the file
>> /Applications/Emacs.app/Contents/Resources/etc/DEBUG for instructions.
>>
>>
>> In GNU Emacs 23.1.1 (i386-apple-darwin9.8.0, NS apple-appkit-949.54)
>>  of 2009-08-16 on black.local
>> Windowing system distributor `Apple', version 10.3.1038
>> configured using `configure  '--with-ns''
>>
>> Important settings:
>>   value of $LC_ALL: nil
>>   value of $LC_COLLATE: nil
>>   value of $LC_CTYPE: nil
>>   value of $LC_MESSAGES: nil
>>   value of $LC_MONETARY: nil
>>   value of $LC_NUMERIC: nil
>>   value of $LC_TIME: nil
>>   value of $LANG: nil
>>   value of $XMODIFIERS: nil
>>   locale-coding-system: nil
>>   default-enable-multibyte-characters: t
>>
>> Major mode: Custom
>>
>> Minor modes in effect:
>>   diff-auto-refine-mode: t
>>   shell-dirtrack-mode: t
>>   show-paren-mode: t
>>   server-mode: t
>>   global-auto-revert-mode: t
>>   delete-selection-mode: t
>>   tooltip-mode: t
>>   mouse-wheel-mode: t
>>   menu-bar-mode: t
>>   file-name-shadow-mode: t
>>   global-font-lock-mode: t
>>   font-lock-mode: t
>>   global-auto-composition-mode: t
>>   auto-composition-mode: t
>>   auto-encryption-mode: t
>>   auto-compression-mode: t
>>   line-number-mode: t
>>   transient-mark-mode: t
>>
>> Recent input:
>> C-n C-n C-p C-p C-. C-. C-. C-b C-b <return> C-n C-n 
>> C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n 
>> C-e <backspace> 0 C-n C-n C-n C-n C-n C-n C-n C-n C-n 
>> C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n 
>> C-p C-p C-p C-p C-p C-p C-p C-p C-p C-p C-p C-p C-p 
>> C-p C-p C-p C-n C-n C-n C-n C-n C-n C-n C-n C-n C-p 
>> C-p C-p C-p C-p C-p C-p C-p C-p C-p C-p C-p C-p C-p 
>> C-p C-p C-n C-n C-n C-n C-n C-n C-p C-p C-p C-p C-n 
>> C-n C-n C-n C-n C-n C-n C-n C-n C-p C-n C-n C-n C-n 
>> C-n C-n C-n C-n C-n C-n C-n C-a C-p C-. C-. C-, <return> 
>> 7 C-n C-n C-n C-e M-< C-n C-n C-n C-n C-n C-n C-n C-n 
>> C-n C-n C-n C-n C-n C-n C-n C-n C-e C-p C-e <backspace> 
>> 0 C-n C-n C-n C-n C-n C-n C-n C-n C-n C-p C-p C-p C-p 
>> C-p C-p C-p C-p C-p C-p C-p C-x C-s C-n C-n C-n C-n 
>> C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n C-n 
>> C-n C-n C-n C-n C-n C-n C-n C-n C-n C-p <M-return> 
>> <C-return> C-q C-j I SPC r e a l l y SPC d o n ' t 
>> SPC w a n t SPC t o SPC s e t e <backspace> <backspace> 
>> e SPC H T M L S-SPC u n l e s s SPC i <backspace> t 
>> h a t ' s SPC a l l SPC t h e r e SPC i s . <backspace> 
>> C-c C-c C-x C-s C-p C-p C-p C-p C-p C-p C-p C-p C-p 
>> C-p C-p M-x r e p o <tab> r <tab> b <tab> <return>
>>
>> Recent messages:
>> Mark set [2 times]
>> Saving file /Users/dave/src/elisp.repo/elisp/custom.el...
>> Wrote /Users/dave/src/elisp.repo/elisp/custom.el
>> Compiling /Users/dave/src/elisp.repo/elisp/custom.el...done
>> Wrote /Users/dave/src/elisp.repo/elisp/custom.elc
>> Saving file /Users/dave/src/elisp.repo/elisp/custom.el...
>> Wrote /Users/dave/src/elisp.repo/elisp/custom.el
>> Compiling /Users/dave/src/elisp.repo/elisp/custom.el...done
>> Wrote /Users/dave/src/elisp.repo/elisp/custom.elc
>> Making completion list...

-- 
-Dave

bug#5358: 23.1; Adding comment loses customizations in progress

[Glenn Morris]

?

M-x customize-variable RET fill-column RET
Click "State", see "Add Comment".

(Current master.)

bug#5358: 23.1; Adding comment loses customizations in progress

[Andrew Hyatt]

Thanks, now I see it.

Now that I see it, I was able to reproduce this bug on Emacs 25.

Glenn Morris <rgm@gnu.org> writes:

> ?
>
> M-x customize-variable RET fill-column RET
> Click "State", see "Add Comment".
>
> (Current master.)

bug#5358: 23.1; Adding comment loses customizations in progress

[Mauro Aranda]

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

The possibility to preserve the customizations in progress is already
present in the customize machinery, but for some reason it wasn't used
in this case.

I propose a patch that makes use of the :shown-value property of the
custom-variable and custom-face widgets, thus allowing to preserve the
customizations in progress when redrawing a widget.

Since after redrawing customize recomputes the state of the widget, it
is necessary to check if the widget was modified.  That isn't
possible with `custom-variable-state' or `custom-face-state',
because they are pretty "widget unaware".  So I added a check in
`custom-variable-state-set' and `custom-face-state-set' to catch this
situation.

Finally, to be able to use the value of the widget in
`custom-face-state-set', it is necessary to rearrange the final part of
`custom-face-value-create'.  Namely, add the children to the :children
property, before calling `custom-face-state-set'.

Best regards,
Mauro.

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

[-- Attachment #2: 0001-Don-t-discard-customizations-in-progress-when-adding.patch --]
[-- Type: text/x-patch, Size: 5155 bytes --]

From d2656c7da97acf689ec12f1fde496e608d8e90fb Mon Sep 17 00:00:00 2001
From: Mauro Aranda <maurooaranda@gmail.com>
Date: Fri, 6 Sep 2019 09:55:39 -0300
Subject: [PATCH] Don't discard customizations in progress when adding comments
 (Bug#5358)

* lisp/cus-edit.el (custom-comment-show): Add docstring.  Save the
widget value in the :shown-value property, before redrawing.
(custom-variable-modified-p): New function, to complement the return
values of custom-variable-state.
(custom-variable-state-set): Use it.
(custom-face-value-create): Add children to the custom-face widget
before setting the state, to be able to check for user edits.
(custom-face-state-set): Check for user edits before calling
custom-face-state.
---
 lisp/cus-edit.el | 63 +++++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 51 insertions(+), 12 deletions(-)

diff --git a/lisp/cus-edit.el b/lisp/cus-edit.el
index 8a8bad9..60e6996 100644
--- a/lisp/cus-edit.el
+++ b/lisp/cus-edit.el
@@ -2411,9 +2411,21 @@ custom-comment-hide
 ;; Those functions are for the menu. WIDGET is NOT the comment widget. It's
 ;; the global custom one
 (defun custom-comment-show (widget)
-  (widget-put widget :comment-shown t)
-  (custom-redraw widget)
-  (widget-setup))
+  "Show the comment editable field that belongs to WIDGET."
+  (let ((child (car (widget-get widget :children)))
+        ;; Just to be safe, we will restore this value after redrawing.
+        (old-shown-value (widget-get widget :shown-value)))
+    (widget-put widget :comment-shown t)
+    ;; Save the changes made by the user before redrawing, to avoid
+    ;; losing customizations in progress.  (Bug#5358)
+    (if (eq (widget-type widget) 'custom-face)
+        (if (eq (widget-type child) 'custom-face-edit)
+            (widget-put widget :shown-value `((t ,(widget-value child))))
+          (widget-put widget :shown-value (widget-value child)))
+      (widget-put widget :shown-value (list (widget-value child))))
+    (custom-redraw widget)
+    (widget-put widget :shown-value old-shown-value)
+    (widget-setup)))
 
 (defun custom-comment-invisible-p (widget)
   (let ((val (widget-value (widget-get widget :comment-widget))))
@@ -2805,12 +2817,34 @@ custom-variable-state
 	     'changed))
 	  (t 'rogue))))
 
+(defun custom-variable-modified-p (widget)
+  "Non-nil if the variable value of WIDGET has been modified.
+WIDGET should be a custom-variable widget, whose first child is the widget
+that holds the value.
+Modified means that the widget that holds the value has been edited by the user
+in a customize buffer.
+To check for other states, call `custom-variable-state'."
+  (condition-case nil
+      (let* ((symbol (widget-get widget :value))
+             (get (or (get symbol 'custom-get) 'default-value))
+             (value (if (default-boundp symbol)
+                        (funcall get symbol)
+                      (symbol-value symbol))))
+        (not (equal value (widget-value (car (widget-get widget :children))))))
+    (error t)))
+
 (defun custom-variable-state-set (widget &optional state)
   "Set the state of WIDGET to STATE.
-If STATE is nil, the value is computed by `custom-variable-state'."
+If STATE is nil, the new state is computed by `custom-variable-modified-p' if
+WIDGET has been edited in the Custom buffer, or by `custom-variable-state'
+otherwise."
   (widget-put widget :custom-state
-	      (or state (custom-variable-state (widget-value widget)
-					       (widget-get widget :value)))))
+	      (or state
+                  (and (custom-variable-modified-p widget) 'modified)
+                  (custom-variable-state (widget-value widget)
+                                         (widget-value
+                                          (car
+                                           (widget-get widget :children)))))))
 
 (defun custom-variable-standard-value (widget)
   (get (widget-value widget) 'standard-value))
@@ -3630,9 +3664,9 @@ custom-face-value-create
 		    (insert-char ?\s indent))
 		  (widget-create-child-and-convert
 		   widget 'sexp :value spec))))
-	  (custom-face-state-set widget)
-	  (push editor children)
-	  (widget-put widget :children children))))))
+          (push editor children)
+          (widget-put widget :children children)
+	  (custom-face-state-set widget))))))
 
 (defvar custom-face-menu
   `(("Set for Current Session" custom-face-set)
@@ -3718,9 +3752,14 @@ custom-face-state
       state)))
 
 (defun custom-face-state-set (widget)
-  "Set the state of WIDGET."
-  (widget-put widget :custom-state
-	      (custom-face-state (widget-value widget))))
+  "Set the state of WIDGET, a custom-face widget.
+If the user edited the widget, set the state to modified.  If not, the new
+state is one of the return values of `custom-face-state'."
+  (let ((face (widget-value widget)))
+    (widget-put widget :custom-state
+                (if (face-spec-match-p face (custom-face-widget-to-spec widget))
+                    (custom-face-state face)
+                  'modified))))
 
 (defun custom-face-action (widget &optional event)
   "Show the menu for `custom-face' WIDGET.
-- 
2.7.4

bug#5358: 23.1; Adding comment loses customizations in progress

[Mauro Aranda]

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

I added a test for this change.

Best regards,
Mauro.

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

[-- Attachment #2: 0001-Don-t-discard-customizations-in-progress-when-adding.patch --]
[-- Type: text/x-patch, Size: 7124 bytes --]

From 69ec406a67b11d2398ce795b31db7a0ae4cb479c Mon Sep 17 00:00:00 2001
From: Mauro Aranda <maurooaranda@gmail.com>
Date: Fri, 6 Sep 2019 09:55:39 -0300
Subject: [PATCH] Don't discard customizations in progress when adding comments
 (Bug#5358)

* lisp/cus-edit.el (custom-comment-show): Add docstring.  Save the
widget value in the :shown-value property, before redrawing.
(custom-variable-modified-p): New function, to complement the return
values of custom-variable-state.
(custom-variable-state-set): Use it.
(custom-face-value-create): Add children to the custom-face widget
before setting the state, to be able to check for user edits.
(custom-face-state-set): Check for user edits before calling
custom-face-state.

* test/lisp/custom-tests.el (custom-test-show-comment-preserves-changes):
New test.
---
 lisp/cus-edit.el          | 63 ++++++++++++++++++++++++++++++++++++++---------
 test/lisp/custom-tests.el | 28 +++++++++++++++++++++
 2 files changed, 79 insertions(+), 12 deletions(-)

diff --git a/lisp/cus-edit.el b/lisp/cus-edit.el
index 2496963..f3445c6 100644
--- a/lisp/cus-edit.el
+++ b/lisp/cus-edit.el
@@ -2416,9 +2416,21 @@ custom-comment-hide
 ;; Those functions are for the menu. WIDGET is NOT the comment widget. It's
 ;; the global custom one
 (defun custom-comment-show (widget)
-  (widget-put widget :comment-shown t)
-  (custom-redraw widget)
-  (widget-setup))
+  "Show the comment editable field that belongs to WIDGET."
+  (let ((child (car (widget-get widget :children)))
+        ;; Just to be safe, we will restore this value after redrawing.
+        (old-shown-value (widget-get widget :shown-value)))
+    (widget-put widget :comment-shown t)
+    ;; Save the changes made by the user before redrawing, to avoid
+    ;; losing customizations in progress.  (Bug#5358)
+    (if (eq (widget-type widget) 'custom-face)
+        (if (eq (widget-type child) 'custom-face-edit)
+            (widget-put widget :shown-value `((t ,(widget-value child))))
+          (widget-put widget :shown-value (widget-value child)))
+      (widget-put widget :shown-value (list (widget-value child))))
+    (custom-redraw widget)
+    (widget-put widget :shown-value old-shown-value)
+    (widget-setup)))
 
 (defun custom-comment-invisible-p (widget)
   (let ((val (widget-value (widget-get widget :comment-widget))))
@@ -2810,12 +2822,34 @@ custom-variable-state
 	     'changed))
 	  (t 'rogue))))
 
+(defun custom-variable-modified-p (widget)
+  "Non-nil if the variable value of WIDGET has been modified.
+WIDGET should be a custom-variable widget, whose first child is the widget
+that holds the value.
+Modified means that the widget that holds the value has been edited by the user
+in a customize buffer.
+To check for other states, call `custom-variable-state'."
+  (condition-case nil
+      (let* ((symbol (widget-get widget :value))
+             (get (or (get symbol 'custom-get) 'default-value))
+             (value (if (default-boundp symbol)
+                        (funcall get symbol)
+                      (symbol-value symbol))))
+        (not (equal value (widget-value (car (widget-get widget :children))))))
+    (error t)))
+
 (defun custom-variable-state-set (widget &optional state)
   "Set the state of WIDGET to STATE.
-If STATE is nil, the value is computed by `custom-variable-state'."
+If STATE is nil, the new state is computed by `custom-variable-modified-p' if
+WIDGET has been edited in the Custom buffer, or by `custom-variable-state'
+otherwise."
   (widget-put widget :custom-state
-	      (or state (custom-variable-state (widget-value widget)
-					       (widget-get widget :value)))))
+	      (or state
+                  (and (custom-variable-modified-p widget) 'modified)
+                  (custom-variable-state (widget-value widget)
+                                         (widget-value
+                                          (car
+                                           (widget-get widget :children)))))))
 
 (defun custom-variable-standard-value (widget)
   (get (widget-value widget) 'standard-value))
@@ -3635,9 +3669,9 @@ custom-face-value-create
 		    (insert-char ?\s indent))
 		  (widget-create-child-and-convert
 		   widget 'sexp :value spec))))
-	  (custom-face-state-set widget)
-	  (push editor children)
-	  (widget-put widget :children children))))))
+          (push editor children)
+          (widget-put widget :children children)
+	  (custom-face-state-set widget))))))
 
 (defvar custom-face-menu
   `(("Set for Current Session" custom-face-set)
@@ -3723,9 +3757,14 @@ custom-face-state
       state)))
 
 (defun custom-face-state-set (widget)
-  "Set the state of WIDGET."
-  (widget-put widget :custom-state
-	      (custom-face-state (widget-value widget))))
+  "Set the state of WIDGET, a custom-face widget.
+If the user edited the widget, set the state to modified.  If not, the new
+state is one of the return values of `custom-face-state'."
+  (let ((face (widget-value widget)))
+    (widget-put widget :custom-state
+                (if (face-spec-match-p face (custom-face-widget-to-spec widget))
+                    (custom-face-state face)
+                  'modified))))
 
 (defun custom-face-action (widget &optional event)
   "Show the menu for `custom-face' WIDGET.
diff --git a/test/lisp/custom-tests.el b/test/lisp/custom-tests.el
index 0c49db6..270acda 100644
--- a/test/lisp/custom-tests.el
+++ b/test/lisp/custom-tests.el
@@ -21,6 +21,10 @@
 
 (require 'ert)
 
+(require 'wid-edit)
+(require 'cus-edit)
+(require 'seq) ; For `seq-find'.
+
 (ert-deftest custom-theme--load-path ()
   "Test `custom-theme--load-path' behavior."
   (let ((tmpdir (file-name-as-directory (make-temp-file "custom-tests-" t))))
@@ -123,4 +127,28 @@ custom--test-variable
     (should (equal custom--test-user-option 'baz))
     (should (equal custom--test-variable 'baz))))
 
+;; This tests Bug#5358.
+(ert-deftest custom-test-show-comment-preserves-changes ()
+  "Test that adding a comment doesn't discard modifications in progress."
+  (customize-option 'custom--test-user-option)
+  (let* ((field (seq-find (lambda (widget)
+                            (eq custom--test-user-option (widget-value widget)))
+                          widget-field-list))
+         (parent (widget-get field :parent))
+	 (origvalue (widget-value field)))
+    ;; Move to the end of the text of the widget, and modify it.  This
+    ;; modification should be preserved after showing the comment field.
+    (goto-char (widget-field-text-end field))
+    (insert "bar")
+    (custom-comment-show parent)
+    ;; From now on, must use `widget-at' to get the value of the widget.
+    (should-not (eq origvalue (widget-value (widget-at))))
+    (should (eq (widget-get parent :custom-state) 'modified))
+    (should (eq (widget-value (widget-at))
+                (widget-apply field
+                              :value-to-external
+                              (concat
+                               (widget-apply field :value-to-internal origvalue)
+                               "bar"))))))
+
 ;;; custom-tests.el ends here
-- 
2.7.4

bug#5358: 23.1; Adding comment loses customizations in progress

[Lars Ingebrigtsen]

Mauro Aranda <maurooaranda@gmail.com> writes:

> Subject: [PATCH] Don't discard customizations in progress when adding comments
>  (Bug#5358)

Looks good to me.  Just one tiny comment:

> +  (condition-case nil
> +      (let* ((symbol (widget-get widget :value))
> +             (get (or (get symbol 'custom-get) 'default-value))
> +             (value (if (default-boundp symbol)
> +                        (funcall get symbol)
> +                      (symbol-value symbol))))
> +        (not (equal value (widget-value (car (widget-get widget :children))))))
> +    (error t)))

If it's just the funcall you expect that might fail, then moving the
condition-case down there might be a better choice.  Having a
condition-case around code that shouldn't fail can hide errors you don't
want to hide.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#5358: 23.1; Adding comment loses customizations in progress

[Mauro Aranda]

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

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Mauro Aranda <maurooaranda@gmail.com> writes:
>
>> Subject: [PATCH] Don't discard customizations in progress when adding
comments
>>  (Bug#5358)
>
> Looks good to me.  Just one tiny comment:
>
>> +  (condition-case nil
>> +      (let* ((symbol (widget-get widget :value))
>> +             (get (or (get symbol 'custom-get) 'default-value))
>> +             (value (if (default-boundp symbol)
>> +                        (funcall get symbol)
>> +                      (symbol-value symbol))))
>> +        (not (equal value (widget-value (car (widget-get widget
:children))))))
>> +    (error t)))
>
> If it's just the funcall you expect that might fail, then moving the
> condition-case down there might be a better choice.  Having a
> condition-case around code that shouldn't fail can hide errors you don't
> want to hide.

Right, thanks for noticing that.

I've moved down the condition-case, and I've added an outer catch, to
avoid the comparison in case of an error.  I think that's OK, but if you
see something wrong, please let me know.

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

[-- Attachment #2: 0001-Don-t-discard-customizations-in-progress-when-adding.patch --]
[-- Type: text/x-patch, Size: 7197 bytes --]

From 39334b56d71ae309e9deba1568c2b21347c519e0 Mon Sep 17 00:00:00 2001
From: Mauro Aranda <maurooaranda@gmail.com>
Date: Fri, 6 Sep 2019 09:55:39 -0300
Subject: [PATCH] Don't discard customizations in progress when adding comments
 (Bug#5358)

* lisp/cus-edit.el (custom-comment-show): Add docstring.  Save the
widget value in the :shown-value property, before redrawing.
(custom-variable-modified-p): New function, to complement the return
values of custom-variable-state.
(custom-variable-state-set): Use it.
(custom-face-value-create): Add children to the custom-face widget
before setting the state, to be able to check for user edits.
(custom-face-state-set): Check for user edits before calling
custom-face-state.

* test/lisp/custom-tests.el (custom-test-show-comment-preserves-changes):
New test.
---
 lisp/cus-edit.el          | 64 ++++++++++++++++++++++++++++++++++++++---------
 test/lisp/custom-tests.el | 28 +++++++++++++++++++++
 2 files changed, 80 insertions(+), 12 deletions(-)

diff --git a/lisp/cus-edit.el b/lisp/cus-edit.el
index 2496963..b9fd3e0 100644
--- a/lisp/cus-edit.el
+++ b/lisp/cus-edit.el
@@ -2416,9 +2416,21 @@ custom-comment-hide
 ;; Those functions are for the menu. WIDGET is NOT the comment widget. It's
 ;; the global custom one
 (defun custom-comment-show (widget)
-  (widget-put widget :comment-shown t)
-  (custom-redraw widget)
-  (widget-setup))
+  "Show the comment editable field that belongs to WIDGET."
+  (let ((child (car (widget-get widget :children)))
+        ;; Just to be safe, we will restore this value after redrawing.
+        (old-shown-value (widget-get widget :shown-value)))
+    (widget-put widget :comment-shown t)
+    ;; Save the changes made by the user before redrawing, to avoid
+    ;; losing customizations in progress.  (Bug#5358)
+    (if (eq (widget-type widget) 'custom-face)
+        (if (eq (widget-type child) 'custom-face-edit)
+            (widget-put widget :shown-value `((t ,(widget-value child))))
+          (widget-put widget :shown-value (widget-value child)))
+      (widget-put widget :shown-value (list (widget-value child))))
+    (custom-redraw widget)
+    (widget-put widget :shown-value old-shown-value)
+    (widget-setup)))
 
 (defun custom-comment-invisible-p (widget)
   (let ((val (widget-value (widget-get widget :comment-widget))))
@@ -2810,12 +2822,35 @@ custom-variable-state
 	     'changed))
 	  (t 'rogue))))
 
+(defun custom-variable-modified-p (widget)
+  "Non-nil if the variable value of WIDGET has been modified.
+WIDGET should be a custom-variable widget, whose first child is the widget
+that holds the value.
+Modified means that the widget that holds the value has been edited by the user
+in a customize buffer.
+To check for other states, call `custom-variable-state'."
+  (catch 'get-error
+    (let* ((symbol (widget-get widget :value))
+           (get (or (get symbol 'custom-get) 'default-value))
+           (value (if (default-boundp symbol)
+                      (condition-case nil
+                          (funcall get symbol)
+                        (error (throw 'get-error t)))
+                    (symbol-value symbol))))
+      (not (equal value (widget-value (car (widget-get widget :children))))))))
+
 (defun custom-variable-state-set (widget &optional state)
   "Set the state of WIDGET to STATE.
-If STATE is nil, the value is computed by `custom-variable-state'."
+If STATE is nil, the new state is computed by `custom-variable-modified-p' if
+WIDGET has been edited in the Custom buffer, or by `custom-variable-state'
+otherwise."
   (widget-put widget :custom-state
-	      (or state (custom-variable-state (widget-value widget)
-					       (widget-get widget :value)))))
+	      (or state
+                  (and (custom-variable-modified-p widget) 'modified)
+                  (custom-variable-state (widget-value widget)
+                                         (widget-value
+                                          (car
+                                           (widget-get widget :children)))))))
 
 (defun custom-variable-standard-value (widget)
   (get (widget-value widget) 'standard-value))
@@ -3635,9 +3670,9 @@ custom-face-value-create
 		    (insert-char ?\s indent))
 		  (widget-create-child-and-convert
 		   widget 'sexp :value spec))))
-	  (custom-face-state-set widget)
-	  (push editor children)
-	  (widget-put widget :children children))))))
+          (push editor children)
+          (widget-put widget :children children)
+	  (custom-face-state-set widget))))))
 
 (defvar custom-face-menu
   `(("Set for Current Session" custom-face-set)
@@ -3723,9 +3758,14 @@ custom-face-state
       state)))
 
 (defun custom-face-state-set (widget)
-  "Set the state of WIDGET."
-  (widget-put widget :custom-state
-	      (custom-face-state (widget-value widget))))
+  "Set the state of WIDGET, a custom-face widget.
+If the user edited the widget, set the state to modified.  If not, the new
+state is one of the return values of `custom-face-state'."
+  (let ((face (widget-value widget)))
+    (widget-put widget :custom-state
+                (if (face-spec-match-p face (custom-face-widget-to-spec widget))
+                    (custom-face-state face)
+                  'modified))))
 
 (defun custom-face-action (widget &optional event)
   "Show the menu for `custom-face' WIDGET.
diff --git a/test/lisp/custom-tests.el b/test/lisp/custom-tests.el
index 0c49db6..270acda 100644
--- a/test/lisp/custom-tests.el
+++ b/test/lisp/custom-tests.el
@@ -21,6 +21,10 @@
 
 (require 'ert)
 
+(require 'wid-edit)
+(require 'cus-edit)
+(require 'seq) ; For `seq-find'.
+
 (ert-deftest custom-theme--load-path ()
   "Test `custom-theme--load-path' behavior."
   (let ((tmpdir (file-name-as-directory (make-temp-file "custom-tests-" t))))
@@ -123,4 +127,28 @@ custom--test-variable
     (should (equal custom--test-user-option 'baz))
     (should (equal custom--test-variable 'baz))))
 
+;; This tests Bug#5358.
+(ert-deftest custom-test-show-comment-preserves-changes ()
+  "Test that adding a comment doesn't discard modifications in progress."
+  (customize-option 'custom--test-user-option)
+  (let* ((field (seq-find (lambda (widget)
+                            (eq custom--test-user-option (widget-value widget)))
+                          widget-field-list))
+         (parent (widget-get field :parent))
+	 (origvalue (widget-value field)))
+    ;; Move to the end of the text of the widget, and modify it.  This
+    ;; modification should be preserved after showing the comment field.
+    (goto-char (widget-field-text-end field))
+    (insert "bar")
+    (custom-comment-show parent)
+    ;; From now on, must use `widget-at' to get the value of the widget.
+    (should-not (eq origvalue (widget-value (widget-at))))
+    (should (eq (widget-get parent :custom-state) 'modified))
+    (should (eq (widget-value (widget-at))
+                (widget-apply field
+                              :value-to-external
+                              (concat
+                               (widget-apply field :value-to-internal origvalue)
+                               "bar"))))))
+
 ;;; custom-tests.el ends here
-- 
2.7.4

bug#5358: 23.1; Adding comment loses customizations in progress

[Lars Ingebrigtsen]

Mauro Aranda <maurooaranda@gmail.com> writes:

> I've moved down the condition-case, and I've added an outer catch, to
> avoid the comparison in case of an error.  I think that's OK, but if you
> see something wrong, please let me know.

Looks good to me, so I've applied it to the trunk now.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no