Subject: bug#76156: 31.0.50; Wrong STATE when customizing allout-command-prefix

From: Mauro Aranda <[email-masked]>
Date: Sat, 15 Feb 2025 09:37:46 -0300

Mauro Aranda <[email-masked]> writes:

 > "Basil L. Contovounesios" <[email-masked]> writes:
 >
 >> Mauro Aranda [2025-02-13 21:22 -0300] wrote:
 >>
 >>> "Basil L. Contovounesios" <[email-masked]> writes:
 >>>> Mauro Aranda [2025-02-09 08:57 -0300] wrote:
 >>>>> Mauro Aranda <[email-masked]> writes:
 >>>>>
 >>>>>> After emacs -Q:
 >>>>>> (require 'allout)
 >>>>>> M-x customize-option RET allout-command-prefix
 >>>>>>
 >>>>>> The Customize buffer shows up, with STATE being:
 >>>>>> EDITED, shown value does not take effect until you set or save it.
 >>>>>>
 >>>>>> That's wrong, it should say STANDARD.
 >>>>
 >>>> Agreed, but this must be common to all key-sequence user options,
 >     right?
 >>>>
 >>>> I see the same state with:
 >>>>
 >>>> - cua-rectangle-mark-key
 >>>> - flyspell-auto-correct-binding
 >>>> - footnote-prefix
 >>>> - gud-key-prefix
 >>>> - hide-ifdef-mode-prefix-key
 >>>> - outline-minor-mode-prefix
 >>>> - viper-toggle-key
 >>>
 >>> Right.  Thanks for finding those.
 >>>
 >>>> So I think this was a regression somewhere in Emacs 27;
 >>>> the startup state looks as expected in Emacs versions 24 through 26.
 >>>
 >>> I can't try Emacs 26, sadly.  But I take your word. Problem is, with
 >>> the widget being obsolete, there's no much incentive to go chasing the
 >>> cause, at least for me.
 >>
 >> I asked Git, so you can give me my word back ;).
 >>
 >>   283fd5f2f6f3fa1f650c5a77f9e3587faddd6881 is the first bad commit
 >>   Author: Mauro Aranda <[email-masked]>
 >>   Date:   2019-09-27 18:06:36 +0200
 >>
 >>   Don't discard customizations in progress when adding comments
 >  (Bug#5358)
 >>
 >> The bisect covered emacs-26.3..emacs-27.2.  In every step I checked the
 >> state of viper-toggle-key, which has been a vector since 2005, and a
 >> key-sequence since Emacs 25.
 >>
 >
 > Thanks! I was looking into it, and wasn't sure if that was the culprit
 > commit.
 >
 >> Starting with the 'bad' commit, the user option shows up as EDITED in
 >> emacs -Q.  This is because default-value returns [(control ?z)], whereas
 >> widget-value returns "^Z" in the function custom-variable-modified-p.
 >
 > Yes.  I'm trying to see if calling the :value-to-external on value
 > is enough, but I think I'm running into the issue you mentioned:
 > key-parse is dropping the trailing space, so no luck so far.

I came up with a fix for custom-variable-modified-p, in the attached
patch.

With the patch, all of these options show its state as STANDARD, which
is what should happen:
- allout-command-prefix
- cua-rectangle-mark-key
- flyspell-auto-correct-binding
- footnote-prefix
- gud-key-prefix
- hide-ifdef-mode-prefix-key
- outline-minor-mode-prefix
- viper-toggle-key

I've found that converting those to use the key widget instead is
potentially not backward compatible, and introduces too many
complications that I concluded it's not worth it, so I didn't pursue it
anymore.

I've done extensive testing on other types to avoid regressions, and
didn't find any.

From d0f08f7b3f4bb15fc49f262314677789c51bb055 Mon Sep 17 00:00:00 2001
From: Mauro Aranda <[email-masked]>
Date: Sat, 15 Feb 2025 09:26:46 -0300
Subject: [PATCH] Fix comparison of current values for the key-sequence :type

* lisp/cus-edit.el (custom-variable-modified-p): Round-trip
the option value before comparing it against the widget's value.
This mostly fixes comparison against the obsolete key-sequence
widget, but could fix other corner cases, when the widget
accepts different types as values.  (Bug#76156)

* test/lisp/cus-edit-tests.el (cus-edit-test-bug76156)
(cus-edit-test-bug76156-2): New test options.
(cus-edit-test-unedited-option): New test.
---
 lisp/cus-edit.el            | 17 ++++++++++++-----
 test/lisp/cus-edit-tests.el | 18 ++++++++++++++++++
 2 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/lisp/cus-edit.el b/lisp/cus-edit.el
index febbc8d1b8b..21e744bae5d 100644
--- a/lisp/cus-edit.el
+++ b/lisp/cus-edit.el
@@ -3054,11 +3054,18 @@ custom-variable-modified-p
     (let* ((form (widget-get widget :custom-form))
            (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)))
+           (value-widget (car (widget-get widget :children)))
+           ;; Round-trip the value, for the sake of widgets that accept
+           ;; values of different types (e.g., the obsolete key-sequence widget
+           ;; which takes either strings or vectors.  (Bug#76156)
+           (value
+            (widget-apply value-widget :value-to-external
+                          (widget-apply value-widget :value-to-internal
+                                        (if (default-boundp symbol)
+                                            (condition-case nil
+                                                (funcall get symbol)
+                                              (error (throw 'get-error t)))
+                                          (symbol-value symbol)))))
            (orig-value (widget-value (car (widget-get widget :children)))))
       (not (equal (if (memq form '(lisp mismatch))
                       ;; Mimic `custom-variable-value-create'.
diff --git a/test/lisp/cus-edit-tests.el b/test/lisp/cus-edit-tests.el
index 8c7697ac635..1662a7ee32e 100644
--- a/test/lisp/cus-edit-tests.el
+++ b/test/lisp/cus-edit-tests.el
@@ -134,5 +134,23 @@ cus-edit-test-bug63290
   ;; No empty key/value pairs should show up.
   (should-not (search-forward "key" nil t)))
 
+(defcustom cus-edit-test-bug76156 "\C-c "
+  "Key-sequence option that might show up as EDITED even though it's not."
+  :type 'key-sequence)
+
+(defcustom cus-edit-test-bug76156-2 [(control ?z)]
+  "Key-sequence option that might show up as EDITED even though it's not."
+  :type 'key-sequence)
+
+(ert-deftest cus-edit-test-unedited-option ()
+  "Test that customizing unedited options doesn't show up as EDITED."
+  (dolist (option '(cus-edit-test-bug76156
+                    cus-edit-test-bug76156-2
+                    cus-edit-test-foo1))
+    (customize-option option)
+    (let ((widget (car custom-options)))
+      (should (eq (widget-get widget :custom-state) 'standard)))
+    (kill-buffer)))
+
 (provide 'cus-edit-tests)
 ;;; cus-edit-tests.el ends here
-- 
2.34.1