bug#63290: 30.0.50; Customize UI shows extra fields for (choice (const ...) (alist ...))

bug#63290: 30.0.50; Customize UI shows extra fields for (choice (const ...) (alist ...))

[Thomas Fitzsimmons]

This test case shows the issue:

(defcustom test-custom nil "" :type
  '(choice (alist
	    :key-type (string :tag "key")
	    :value-type (string :tag "value"))
           (const :tag "auto" nil)))
(customize-variable 'test-custom)

The UI first shows:

Hide Test Custom: Choice: Value Menu Alist:
INS
    State : STANDARD.

Then if I choose "Value Menu", and option 1 to choose the "auto" const
value, I get:

Hide Test Custom: Choice: Value Menu auto
    State : EDITED, shown value does not take effect until you set or save it.

which is fine.  Then if I choose "Value Menu" again and choose 0 for the
Alist, I get:

Hide Test Custom: Choice: Value Menu Alist:
INS DEL key: 
            value: 
INS
    State : EDITED, shown value does not take effect until you set or save it.

I wasn't expecting:

INS DEL key: 
            value: 

If I then save the customization, test-custom is ("" . "").  I think it
should instead be nil.

I noticed this on excorporate-configuration, which has:

(defcustom test-custom nil "" :type
  '(choice (const :tag "auto" nil)
           (alist
	    :key-type (string :tag "key")
	    :value-type (string :tag "value"))))

but where the alist is a large nested structure.  If the user
customizes, test-custom, selects the alist, and saves, the structure has
degenerate ("" . ""), or (nil . nil) entries in it.  To avoid this, the
user would have to hit "DEL" on the empty key/value entries, which is
not ideal.

It seems like after a const is shown, Customize considers the variable
"edited".  I don't know why it is adding those extra INS/DEL key/value
UI boxes though.

Thomas

bug#63290: 30.0.50; Customize UI shows extra fields for (choice (const ...) (alist ...))

[Mauro Aranda]

Thomas Fitzsimmons <fitzsim@fitzsim.org> writes:

 > This test case shows the issue:
 >
 > (defcustom test-custom nil "" :type
 >   '(choice (alist
 >         :key-type (string :tag "key")
 >         :value-type (string :tag "value"))
 >            (const :tag "auto" nil)))
 > (customize-variable 'test-custom)
 >
 > The UI first shows:
 >
 > Hide Test Custom: Choice: Value Menu Alist:
 > INS
 >     State : STANDARD.
 >
 > Then if I choose "Value Menu", and option 1 to choose the "auto" const
 > value, I get:
 >
 > Hide Test Custom: Choice: Value Menu auto
 >     State : EDITED, shown value does not take effect until you set or 
save it.
 >
 > which is fine.  Then if I choose "Value Menu" again and choose 0 for the
 > Alist, I get:
 >
 > Hide Test Custom: Choice: Value Menu Alist:
 > INS DEL key:
 >             value:
 > INS
 >     State : EDITED, shown value does not take effect until you set or 
save it.
 >
 > I wasn't expecting:
 >
 > INS DEL key:
 >             value:
 >
 > If I then save the customization, test-custom is ("" . "").  I think it
 > should instead be nil.
 >

Thanks for the bug report.

 >
 > It seems like after a const is shown, Customize considers the variable
 > "edited".  I don't know why it is adding those extra INS/DEL key/value
 > UI boxes though.

The code currently ignores the value if it's present but nil. That's
not good, obviously.  But it might be tricky to fix it because other
widgets depend on the code ignoring it...

I think it might be good to have a different property specify a default
value (defaulting to :value upon creation, if not provided), and let
:value be treated as of today, like the current value holder.

bug#63290: 30.0.50; Customize UI shows extra fields for (choice (const ...) (alist ...))

[Thomas Fitzsimmons]

Mauro Aranda <maurooaranda@gmail.com> writes:

> Thomas Fitzsimmons <fitzsim@fitzsim.org> writes: 
 
[...] 
 
>> It seems like after a const is shown, Customize considers the 
>> variable "edited".  I don't know why it is adding those extra 
>> INS/DEL key/value UI boxes though. 
> 
> The code currently ignores the value if it's present but 
> nil. That's not good, obviously.  But it might be tricky to fix 
> it because other widgets depend on the code ignoring it... 
> 
> I think it might be good to have a different property specify a 
> default value (defaulting to :value upon creation, if not 
> provided), and let :value be treated as of today, like the 
> current value holder. 

OK, thanks for taking a look; let me know if you want me to try a 
patch.  Sounds like a good way of maintaining backward 
compatibility with respect to other widgets.

Or you can test it against excorporate-configuration in GNU ELPA; 
just search for the FIXME that mentions this bug report.

Thanks,
Thomas

bug#63290: 30.0.50; Customize UI shows extra fields for (choice (const ...) (alist ...))

[Mauro Aranda]

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

tags 63290 patch
quit


I ended up adding a custom :default-get function for the list widget, to
make it respect a nil value as the :value.  This should be backward
compatible with other widgets, and should fix these "ghost" elements
insertions.

I also added a test for cus-edit-tests.


[-- Attachment #2: 0001-Respect-the-value-property-in-a-list-widget-Bug-6329.patch --]
[-- Type: text/x-patch, Size: 2838 bytes --]

From e64f9c55e1a1e57de3783fbbac1bf239ac30babb Mon Sep 17 00:00:00 2001
From: Mauro Aranda <maurooaranda@gmail.com>
Date: Wed, 9 Aug 2023 09:17:43 -0300
Subject: [PATCH] Respect the :value property in a list widget (Bug#63290)

* lisp/wid-edit.el (widget-list-default-get): New function.
* test/lisp/cus-edit-tests.el (cus-edit-test-bug-63290-option): New
test option.
(cus-edit-test-bug63290): New test.
---
 lisp/wid-edit.el            | 11 +++++++++++
 test/lisp/cus-edit-tests.el | 24 ++++++++++++++++++++++++
 2 files changed, 35 insertions(+)

diff --git a/lisp/wid-edit.el b/lisp/wid-edit.el
index 47531113ba8..c604225f10a 100644
--- a/lisp/wid-edit.el
+++ b/lisp/wid-edit.el
@@ -3812,8 +3812,19 @@ widget-character-notify
 (define-widget 'list 'group
   "A Lisp list."
   :tag "List"
+  :default-get #'widget-list-default-get
   :format "%{%t%}:\n%v")
 
+(defun widget-list-default-get (widget)
+  "Return the default external value for a list WIDGET.
+
+The default value is the one stored in the :value property, even if it is nil,
+or a list with the default value of each component of the list WIDGET."
+  (widget-apply widget :value-to-external
+                (if (widget-member widget :value)
+                    (widget-get widget :value)
+                  (widget-group-default-get widget))))
+
 (define-widget 'vector 'group
   "A Lisp vector."
   :tag "Vector"
diff --git a/test/lisp/cus-edit-tests.el b/test/lisp/cus-edit-tests.el
index eca35d7c96a..75d5f32c421 100644
--- a/test/lisp/cus-edit-tests.el
+++ b/test/lisp/cus-edit-tests.el
@@ -92,5 +92,29 @@ test-setopt
             (buffer-substring-no-properties (point-min) (point-max)))))
     (should (string-search "Value `:foo' does not match type number"
                            warn-txt))))
+
+(defcustom cus-edit-test-bug63290-option nil
+  "Choice option for testing Bug#63290."
+  :type '(choice (alist
+                  :key-type (string :tag "key")
+                  :value-type (string :tag "value"))
+                 (const :tag "auto" auto)))
+
+(ert-deftest cus-edit-test-bug63290 ()
+  "Test that changing a choice value back to an alist respects its nil value."
+  (customize-variable 'cus-edit-test-bug63290-option)
+  (search-forward "Value")
+  ;; Simulate changing the value.
+  (let* ((choice (widget-at))
+         (args (widget-get choice :args))
+         (list-opt (car (widget-get choice :children)))
+         (const-opt (nth 1 args)))
+    (widget-put choice :explicit-choice const-opt)
+    (widget-value-set choice (widget-default-get const-opt))
+    (widget-put choice :explicit-choice list-opt)
+    (widget-value-set choice (widget-default-get list-opt)))
+  ;; No empty key/value pairs should show up.
+  (should-not (search-forward "key" nil t)))
+
 (provide 'cus-edit-tests)
 ;;; cus-edit-tests.el ends here
-- 
2.34.1

bug#63290: 30.0.50; Customize UI shows extra fields for (choice (const ...) (alist ...))

[Eli Zaretskii]

> Date: Wed, 9 Aug 2023 09:19:37 -0300
> From: Mauro Aranda <maurooaranda@gmail.com>
> 
> I ended up adding a custom :default-get function for the list widget, to
> make it respect a nil value as the :value.  This should be backward
> compatible with other widgets, and should fix these "ghost" elements
> insertions.
> 
> I also added a test for cus-edit-tests.

Thanks.

bug#63290: 30.0.50; Customize UI shows extra fields for (choice (const ...) (alist ...))

[Thomas Fitzsimmons]

Hi Mauro,
 
Mauro Aranda <maurooaranda@gmail.com> writes:

> tags 63290 patch quit 
>  
> I ended up adding a custom :default-get function for the list 
> widget, to make it respect a nil value as the :value.  This 
> should be backward compatible with other widgets, and should fix 
> these "ghost" elements insertions. 
> 
> I also added a test for cus-edit-tests. 

Can you try this patch with:

M-x package-install RET excorporate RET

Then:

M-x customize-variable RET excorporate-configuration RET

then select "Value Menu" and 3, which is "EWS URL OAuth 2.0 
settings (no autodiscovery)".  With your wis-edit.el patch applied 
I still get empty values for:

    INS DEL Argument name: 
            Argument value:

and:
 
    INS DEL OAuth 2.0 setting name: 
            OAuth 2.0 setting value:

and when I apply the setting the value contains: 

  (... (... (#1# . #1#)) 
   (#1# . #1#))

Maybe this is a more complicated case than the test case I 
provided (which does now work for me with your patch)?

Thanks,
Thomas

bug#63290: 30.0.50; Customize UI shows extra fields for (choice (const ...) (alist ...))

[Mauro Aranda]

Thomas Fitzsimmons <fitzsim@fitzsim.org> writes:

 > Hi Mauro,
 >
 > Mauro Aranda <maurooaranda@gmail.com> writes:
 >
 >> tags 63290 patch quit  I ended up adding a custom :default-get
 >> function for the list widget, to make it respect a nil value as the
 >> :value.  This should be backward compatible with other widgets, and
 >> should fix these "ghost" elements insertions. I also added a test
 >> for cus-edit-tests.
 >
 > Can you try this patch with:
 >
 > M-x package-install RET excorporate RET
 >
 > Then:
 >
 > M-x customize-variable RET excorporate-configuration RET
 >
 > then select "Value Menu" and 3, which is "EWS URL OAuth 2.0 settings
 > (no autodiscovery)".  With your wis-edit.el patch applied I still get
 > empty values for:
 >
 >    INS DEL Argument name:             Argument value:
 >
 > and:
 >
 >    INS DEL OAuth 2.0 setting name:             OAuth 2.0 setting
 >   value:
 >
 > and when I apply the setting the value contains:   (... (... (#1#
 > . #1#))    (#1# . #1#))
 >
 > Maybe this is a more complicated case than the test case I provided
 > (which does now work for me with your patch)?
 >
 > Thanks,
 > Thomas

Oh, OK.  I'll take a look at it later today or tomorrow.

Thanks.

bug#63290: 30.0.50; Customize UI shows extra fields for (choice (const ...) (alist ...))

[Mauro Aranda]

Thomas Fitzsimmons <fitzsim@fitzsim.org> writes:

 > Hi Mauro,
 >
 > Mauro Aranda <maurooaranda@gmail.com> writes:
 >
 >> tags 63290 patch quit  I ended up adding a custom :default-get
 >> function for the list widget, to make it respect a nil value as the
 >> :value.  This should be backward compatible with other widgets, and
 >> should fix these "ghost" elements insertions. I also added a test
 >> for cus-edit-tests.
 >
 > Can you try this patch with:
 >
 > M-x package-install RET excorporate RET
 >
 > Then:
 >
 > M-x customize-variable RET excorporate-configuration RET
 >
 > then select "Value Menu" and 3, which is "EWS URL OAuth 2.0 settings
 > (no autodiscovery)".  With your wis-edit.el patch applied I still get
 > empty values for:
 >
 >    INS DEL Argument name:             Argument value:
 >
 > and:
 >
 >    INS DEL OAuth 2.0 setting name:             OAuth 2.0 setting
 >   value:
 >
 > and when I apply the setting the value contains:   (... (... (#1#
 > . #1#))    (#1# . #1#))
 >
 > Maybe this is a more complicated case than the test case I provided
 > (which does now work for me with your patch)?
 >
 > Thanks,
 > Thomas

I took a look and this seems to have something to do with how the alist
widget (and almost surely the plist widget) get created when they have
:options.  I'm not sure yet, and I don't totally understand what's
happening as of now, but I wanted to report back.

I'll try to investigate and see how this can be fixed, but it's going to
take me some time.

bug#63290: 30.0.50; Customize UI shows extra fields for (choice (const ...) (alist ...))

[Mauro Aranda]

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

Thomas Fitzsimmons <fitzsim@fitzsim.org> writes:

 > Hi Mauro,
 >
 > Mauro Aranda <maurooaranda@gmail.com> writes:
 >
 >> I ended up adding a custom :default-get
 >> function for the list widget, to make it respect a nil value as the
 >> :value.  This should be backward compatible with other widgets, and
 >> should fix these "ghost" elements insertions. I also added a test
 >> for cus-edit-tests.
 >
 > Can you try this patch with:
 >
 > M-x package-install RET excorporate RET
 >
 > Then:
 >
 > M-x customize-variable RET excorporate-configuration RET
 >
 > then select "Value Menu" and 3, which is "EWS URL OAuth 2.0 settings
 > (no autodiscovery)".  With your wis-edit.el patch applied I still get
 > empty values for:
 >
 >    INS DEL Argument name:             Argument value:
 >
 > and:
 >
 >    INS DEL OAuth 2.0 setting name:             OAuth 2.0 setting
 >   value:
 >
 > and when I apply the setting the value contains:   (... (... (#1#
 > . #1#))    (#1# . #1#))
 >
 > Maybe this is a more complicated case than the test case I provided
 > (which does now work for me with your patch)?

I think this ammended patch fixes it.  Since we want
widget-list-default-get to respect a nil :value property, the alist
widget needs to be modified so that its default value is nil.

[-- Attachment #2: 0001-Respect-the-value-property-in-a-list-widget-Bug-6329.patch --]
[-- Type: text/x-patch, Size: 3903 bytes --]

From 5ae17fcc45ea405f5cc2dc516efa1e9c05782021 Mon Sep 17 00:00:00 2001
From: Mauro Aranda <maurooaranda@gmail.com>
Date: Wed, 9 Aug 2023 09:17:43 -0300
Subject: [PATCH] Respect the :value property in a list widget (Bug#63290)

* lisp/wid-edit.el (widget-list-default-get): New function.
(alist): Define nil as a default value.
* test/lisp/cus-edit-tests.el (cus-edit-test-bug-63290-option)
(cus-edit-test-bug-63290-option2): New test options.
(cus-edit-test-bug63290): New test.
---
 lisp/wid-edit.el            | 12 +++++++++++
 test/lisp/cus-edit-tests.el | 43 +++++++++++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+)

diff --git a/lisp/wid-edit.el b/lisp/wid-edit.el
index 47531113ba8..d538dc42d53 100644
--- a/lisp/wid-edit.el
+++ b/lisp/wid-edit.el
@@ -3812,8 +3812,19 @@ widget-character-notify
 (define-widget 'list 'group
   "A Lisp list."
   :tag "List"
+  :default-get #'widget-list-default-get
   :format "%{%t%}:\n%v")
 
+(defun widget-list-default-get (widget)
+  "Return the default external value for a list WIDGET.
+
+The default value is the one stored in the :value property, even if it is nil,
+or a list with the default value of each component of the list WIDGET."
+  (widget-apply widget :value-to-external
+                (if (widget-member widget :value)
+                    (widget-get widget :value)
+                  (widget-group-default-get widget))))
+
 (define-widget 'vector 'group
   "A Lisp vector."
   :tag "Vector"
@@ -3952,6 +3963,7 @@ 'alist
   :key-type '(sexp :tag "Key")
   :value-type '(sexp :tag "Value")
   :convert-widget 'widget-alist-convert-widget
+  :value nil
   :tag "Alist")
 
 (defvar widget-alist-value-type)	;Dynamic variable
diff --git a/test/lisp/cus-edit-tests.el b/test/lisp/cus-edit-tests.el
index eca35d7c96a..3a788f19745 100644
--- a/test/lisp/cus-edit-tests.el
+++ b/test/lisp/cus-edit-tests.el
@@ -92,5 +92,48 @@ test-setopt
             (buffer-substring-no-properties (point-min) (point-max)))))
     (should (string-search "Value `:foo' does not match type number"
                            warn-txt))))
+
+(defcustom cus-edit-test-bug63290-option nil
+  "Choice option for testing Bug#63290."
+  :type '(choice (alist
+                  :key-type (string :tag "key")
+                  :value-type (string :tag "value"))
+                 (const :tag "auto" auto)))
+
+(defcustom cus-edit-test-bug63290-option2 'some
+  "Choice option for testing Bug#63290."
+  :type '(choice
+          (const :tag "some" some)
+          (alist
+           :key-type (string :tag "key")
+           :value-type (string :tag "value"))))
+
+(ert-deftest cus-edit-test-bug63290 ()
+  "Test that changing a choice value back to an alist respects its nil value."
+  (customize-variable 'cus-edit-test-bug63290-option)
+  (search-forward "Value")
+  ;; Simulate changing the value.
+  (let* ((choice (widget-at))
+         (args (widget-get choice :args))
+         (list-opt (car (widget-get choice :children)))
+         (const-opt (nth 1 args)))
+    (widget-put choice :explicit-choice const-opt)
+    (widget-value-set choice (widget-default-get const-opt))
+    (widget-put choice :explicit-choice list-opt)
+    (widget-value-set choice (widget-default-get list-opt)))
+  ;; No empty key/value pairs should show up.
+  (should-not (search-forward "key" nil t))
+  (customize-variable 'cus-edit-test-bug63290-option2)
+  (search-forward "Value")
+  ;; Simulate changing the value.
+  (let* ((choice (widget-at))
+         (args (widget-get choice :args))
+         (const-opt (car (widget-get choice :children)))
+         (list-opt (nth 1 args)))
+    (widget-put choice :explicit-choice list-opt)
+    (widget-value-set choice (widget-default-get list-opt)))
+  ;; No empty key/value pairs should show up.
+  (should-not (search-forward "key" nil t)))
+
 (provide 'cus-edit-tests)
 ;;; cus-edit-tests.el ends here
-- 
2.34.1

bug#63290: 30.0.50; Customize UI shows extra fields for (choice (const ...) (alist ...))

[Thomas Fitzsimmons]

Mauro Aranda <maurooaranda@gmail.com> writes:

> Thomas Fitzsimmons <fitzsim@fitzsim.org> writes: 
> 
>> Hi Mauro, 
>> 
>> Mauro Aranda <maurooaranda@gmail.com> writes: 
>> 
>>> I ended up adding a custom :default-get function for the list 
>>> widget, to make it respect a nil value as the :value.  This 
>>> should be backward compatible with other widgets, and should 
>>> fix these "ghost" elements insertions. I also added a test for 
>>> cus-edit-tests. 
>> 
>> Can you try this patch with: 
>> 
>> M-x package-install RET excorporate RET 
>> 
>> Then: 
>> 
>> M-x customize-variable RET excorporate-configuration RET 
>> 
>> then select "Value Menu" and 3, which is "EWS URL OAuth 2.0 
>> settings (no autodiscovery)".  With your wis-edit.el patch 
>> applied I still get empty values for: 
>>     INS DEL Argument name:             Argument value:  
>> and: 
>>     INS DEL OAuth 2.0 setting name:             OAuth 2.0 
>>setting    value:  
>> and when I apply the setting the value contains:   
>> (... (... (#1# . #1#))    (#1# . #1#)) 
>> 
>> Maybe this is a more complicated case than the test case I 
>> provided (which does now work for me with your patch)? 
> 
> I think this ammended patch fixes it.  Since we want 
> widget-list-default-get to respect a nil :value property, the 
> alist widget needs to be modified so that its default value is 
> nil. 

With the updated patch, when I select "EWS URL OAuth 2.0 settings 
(no autodiscovery)", all the widgets are disabled.  The blank 
values are no longer added though.  However, if I then set the 
value, without configuring anything, excorporate-configuration 
stays nil.  So I don't think the patch is correct yet.

Thomas

bug#63290: 30.0.50; Customize UI shows extra fields for (choice (const ...) (alist ...))

[Mauro Aranda]

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

Hi Thomas,

Thomas Fitzsimmons <fitzsim@fitzsim.org> writes:

 > Mauro Aranda <maurooaranda@gmail.com> writes:
 >>> Can you try this patch with: M-x package-install RET excorporate
 >>> RET Then: M-x customize-variable RET excorporate-configuration RET
 >>> then select "Value Menu" and 3, which is "EWS URL OAuth 2.0
 >>> settings (no autodiscovery)".  With your wis-edit.el patch applied
 >>> I still get empty values for:     INS DEL Argument
 >>> name:             Argument value:  and:     INS DEL OAuth 2.0
 >>> setting name:             OAuth 2.0 setting value:  and when I
 >>> apply the setting the value contains:   (... (... (#1# . #1#))
 >>> (#1# . #1#)) Maybe this is a more complicated case than the test
 >>> case I provided (which does now work for me with your patch)?
 >> I think this ammended patch fixes it.  Since we want
 >> widget-list-default-get to respect a nil :value property, the alist
 >> widget needs to be modified so that its default value is nil.
 >
 > With the updated patch, when I select "EWS URL OAuth 2.0 settings (no
 > autodiscovery)", all the widgets are disabled.  The blank values are
 > no longer added though.  However, if I then set the value, without
 > configuring anything, excorporate-configuration stays nil. So I don't
 > think the patch is correct yet.

So, in case the :value is missing for the alist widget, we want to
compute the default-value with the :options, and without including the
editable-list.

Hopefully the attached patch is 100% correct now.  I did try it with
excorporate-configuration and I think it works OK, but please give it
yourself a try.  And thank you for your patience.




[-- Attachment #2: 0001-Specialize-default-get-for-alist-widgets-Bug-63290.patch --]
[-- Type: text/x-patch, Size: 7430 bytes --]

From 228001795ab15936cd2e8d1fec38ec15b77c5b91 Mon Sep 17 00:00:00 2001
From: Mauro Aranda <maurooaranda@gmail.com>
Date: Tue, 15 Aug 2023 19:35:39 -0300
Subject: [PATCH] Specialize default-get for alist widgets (Bug#63290)

* lisp/wid-edit.el (widget-list-default-get)
(widget-alist-default-get): New functions.
(list, alist): Use it.
* test/lisp/cus-edit-tests.el (cus-edit-test-bug63290-option)
(cus-edit-test-bug63290-option-2): New test options.
(cus-edit-test-bug63290): New test.
* test/lisp/wid-edit-tests.el (widget-test-alist-default-value-1)
(widget-test-alist-default-value-2, widget-test-alist-default-value-3)
(widget-test-alist-default-value-4): New tests.
---
 lisp/wid-edit.el            | 32 ++++++++++++++++++++++++++-
 test/lisp/cus-edit-tests.el | 43 +++++++++++++++++++++++++++++++++++++
 test/lisp/wid-edit-tests.el | 31 ++++++++++++++++++++++++++
 3 files changed, 105 insertions(+), 1 deletion(-)

diff --git a/lisp/wid-edit.el b/lisp/wid-edit.el
index 47531113ba8..5dbdd7127aa 100644
--- a/lisp/wid-edit.el
+++ b/lisp/wid-edit.el
@@ -3812,8 +3812,19 @@ widget-character-notify
 (define-widget 'list 'group
   "A Lisp list."
   :tag "List"
+  :default-get #'widget-list-default-get
   :format "%{%t%}:\n%v")
 
+(defun widget-list-default-get (widget)
+  "Return the default external value for a list WIDGET.
+
+The default value is the one stored in the :value property, even if it is nil,
+or a list with the default value of each component of the list WIDGET."
+  (widget-apply widget :value-to-external
+                (if (widget-member widget :value)
+                    (widget-get widget :value)
+                  (widget-group-default-get widget))))
+
 (define-widget 'vector 'group
   "A Lisp vector."
   :tag "Vector"
@@ -3942,7 +3953,6 @@ widget-plist-convert-option
 	    value-type widget-plist-value-type))
     `(group :format "Key: %v" :inline t ,key-type ,value-type)))
 
-
 ;;; The `alist' Widget.
 ;;
 ;; Association lists.
@@ -3952,6 +3962,7 @@ 'alist
   :key-type '(sexp :tag "Key")
   :value-type '(sexp :tag "Value")
   :convert-widget 'widget-alist-convert-widget
+  :default-get #'widget-alist-default-get
   :tag "Alist")
 
 (defvar widget-alist-value-type)	;Dynamic variable
@@ -3986,6 +3997,25 @@ widget-alist-convert-option
       (setq key-type `(const ,option)
 	    value-type widget-alist-value-type))
     `(cons :format "Key: %v" ,key-type ,value-type)))
+
+(defun widget-alist-default-get (widget)
+  "Return the default value for WIDGET, an alist widget.
+
+The default value may be one of:
+- The one stored in the :value property, even if it is nil.
+- If WIDGET has options available, an alist consisting of the
+default values for each option.
+- nil, otherwise."
+  (widget-apply widget :value-to-external
+                (cond ((widget-member widget :value)
+                       (widget-get widget :value))
+                      ((widget-get widget :options)
+                       (mapcar #'widget-default-get
+                               ;; Last one is the editable-list part, and
+                               ;; we don't want those showing up as
+                               ;; part of the default value.  (Bug#63290)
+                               (butlast (widget-get widget :args))))
+                      (t nil))))
 \f
 (define-widget 'choice 'menu-choice
   "A union of several sexp types.
diff --git a/test/lisp/cus-edit-tests.el b/test/lisp/cus-edit-tests.el
index eca35d7c96a..3a788f19745 100644
--- a/test/lisp/cus-edit-tests.el
+++ b/test/lisp/cus-edit-tests.el
@@ -92,5 +92,48 @@ test-setopt
             (buffer-substring-no-properties (point-min) (point-max)))))
     (should (string-search "Value `:foo' does not match type number"
                            warn-txt))))
+
+(defcustom cus-edit-test-bug63290-option nil
+  "Choice option for testing Bug#63290."
+  :type '(choice (alist
+                  :key-type (string :tag "key")
+                  :value-type (string :tag "value"))
+                 (const :tag "auto" auto)))
+
+(defcustom cus-edit-test-bug63290-option2 'some
+  "Choice option for testing Bug#63290."
+  :type '(choice
+          (const :tag "some" some)
+          (alist
+           :key-type (string :tag "key")
+           :value-type (string :tag "value"))))
+
+(ert-deftest cus-edit-test-bug63290 ()
+  "Test that changing a choice value back to an alist respects its nil value."
+  (customize-variable 'cus-edit-test-bug63290-option)
+  (search-forward "Value")
+  ;; Simulate changing the value.
+  (let* ((choice (widget-at))
+         (args (widget-get choice :args))
+         (list-opt (car (widget-get choice :children)))
+         (const-opt (nth 1 args)))
+    (widget-put choice :explicit-choice const-opt)
+    (widget-value-set choice (widget-default-get const-opt))
+    (widget-put choice :explicit-choice list-opt)
+    (widget-value-set choice (widget-default-get list-opt)))
+  ;; No empty key/value pairs should show up.
+  (should-not (search-forward "key" nil t))
+  (customize-variable 'cus-edit-test-bug63290-option2)
+  (search-forward "Value")
+  ;; Simulate changing the value.
+  (let* ((choice (widget-at))
+         (args (widget-get choice :args))
+         (const-opt (car (widget-get choice :children)))
+         (list-opt (nth 1 args)))
+    (widget-put choice :explicit-choice list-opt)
+    (widget-value-set choice (widget-default-get list-opt)))
+  ;; No empty key/value pairs should show up.
+  (should-not (search-forward "key" nil t)))
+
 (provide 'cus-edit-tests)
 ;;; cus-edit-tests.el ends here
diff --git a/test/lisp/wid-edit-tests.el b/test/lisp/wid-edit-tests.el
index b379c7c91a8..ebfe729bc9a 100644
--- a/test/lisp/wid-edit-tests.el
+++ b/test/lisp/wid-edit-tests.el
@@ -349,4 +349,35 @@ widget-test-color-match
     (should-not (widget-apply widget :match "someundefinedcolorihope"))
     (should-not (widget-apply widget :match "#11223"))))
 
+(ert-deftest widget-test-alist-default-value-1 ()
+  "Test getting the default value for an alist widget with options."
+  (with-temp-buffer
+    (let ((w (widget-create '(alist :key-type string
+                                    :value-type integer
+                                    :options (("0" (integer)))))))
+      (should (equal '(("0" . 0)) (widget-default-get w))))))
+
+(ert-deftest widget-test-alist-default-value-2 ()
+  "Test getting the default value for an alist widget without :value."
+  (with-temp-buffer
+    (let ((w (widget-create '(alist :key-type string
+                                    :value-type integer))))
+      (should-not (widget-default-get w)))))
+
+(ert-deftest widget-test-alist-default-value-3 ()
+  "Test getting the default value for an alist widget with nil :value."
+  (with-temp-buffer
+    (let ((w (widget-create '(alist :key-type string
+                                    :value-type integer
+                                    :value nil))))
+      (should-not (widget-default-get w)))))
+
+(ert-deftest widget-test-alist-default-value-4 ()
+  "Test getting the default value for an alist widget with non-nil :value."
+  (with-temp-buffer
+    (let ((w (widget-create '(alist :key-type string
+                                    :value-type integer
+                                    :value (("1" . 1) ("2" . 2))))))
+      (should (equal '(("1" . 1) ("2" . 2)) (widget-default-get w))))))
+
 ;;; wid-edit-tests.el ends here
-- 
2.34.1

bug#63290: 30.0.50; Customize UI shows extra fields for (choice (const ...) (alist ...))

[Thomas Fitzsimmons]

Hi Mauro,
 
Mauro Aranda <maurooaranda@gmail.com> writes:

> Thomas Fitzsimmons <fitzsim@fitzsim.org> writes: 
> 
>> Mauro Aranda <maurooaranda@gmail.com> writes: 
>>>> Can you try this patch with: M-x package-install RET 
>>>> excorporate RET Then: M-x customize-variable RET 
>>>> excorporate-configuration RET then select "Value Menu" and 3, 
>>>> which is "EWS URL OAuth 2.0 settings (no autodiscovery)".  
>>>> With your wis-edit.el patch applied I still get empty values 
>>>> for:     INS DEL Argument name:             Argument value:  
>>>> and:     INS DEL OAuth 2.0 setting name:             OAuth 
>>>> 2.0 setting value:  and when I apply the setting the value 
>>>> contains:   (... (... (#1# . #1#)) (#1# . #1#)) Maybe this is 
>>>> a more complicated case than the test case I provided (which 
>>>> does now work for me with your patch)? 
>>> I think this ammended patch fixes it.  Since we want 
>>> widget-list-default-get to respect a nil :value property, the 
>>> alist widget needs to be modified so that its default value is 
>>> nil. 
>> 
>> With the updated patch, when I select "EWS URL OAuth 2.0 
>> settings (no autodiscovery)", all the widgets are disabled.  
>> The blank values are no longer added though.  However, if I 
>> then set the value, without configuring anything, 
>> excorporate-configuration stays nil. So I don't think the patch 
>> is correct yet. 
> 
> So, in case the :value is missing for the alist widget, we want 
> to compute the default-value with the :options, and without 
> including the editable-list. 
> 
> Hopefully the attached patch is 100% correct now.  I did try it 
> with excorporate-configuration and I think it works OK, but 
> please give it yourself a try.  And thank you for your patience. 

Yes, after applying this patch, I retried the test case and 
excorporate-configuration and both behave as I was originally 
expecting them to; no stray structures upon setting the value.

Thank you for all your work on this and the comprehensive test 
cases.  Feel free to close this bug report once the patch is 
pushed.

Thanks,
Thomas

bug#63290: 30.0.50; Customize UI shows extra fields for (choice (const ...) (alist ...))

[Eli Zaretskii]

> Cc: 63290@debbugs.gnu.org
> Date: Tue, 15 Aug 2023 19:46:35 -0300
> From: Mauro Aranda <maurooaranda@gmail.com>
> 
> So, in case the :value is missing for the alist widget, we want to
> compute the default-value with the :options, and without including the
> editable-list.
> 
> Hopefully the attached patch is 100% correct now.  I did try it with
> excorporate-configuration and I think it works OK, but please give it
> yourself a try.  And thank you for your patience.

Thanks, I've now installed this on the master branch, and I'm closing
the bug.

bug#63290: 30.0.50; Customize UI shows extra fields for (choice (const ...) (alist ...))

[Mattias Engdegård]

Mauro, the test has an unused variable binding, `const-opt`. Would you please confirm that it is an oversight and can be removed, or was your intention that it be used in the test?

bug#63290: 30.0.50; Customize UI shows extra fields for (choice (const ...) (alist ...))

[Mauro Aranda]

Mattias Engdegård <mattias.engdegard@gmail.com> writes:

 > Mauro, the test has an unused variable binding, `const-opt`. Would you
 > please confirm that it is an oversight and can be removed, or was your
 > intention that it be used in the test?

Hi Mattias,

It was a copy-pasta from the previous let.  It should be removed.

Thanks for catching it!

bug#63290: 30.0.50; Customize UI shows extra fields for (choice (const ...) (alist ...))

[Mattias Engdegård]

21 aug. 2023 kl. 16.43 skrev Mauro Aranda <maurooaranda@gmail.com>:

> It was a copy-pasta from the previous let.  It should be removed.

Now removed.

> Thanks for catching it!

Oh, thank the compiler. I'm just the messenger.