all messages for Emacs-related lists mirrored at yhetil.org
 help / color / mirror / code / Atom feed
* bug#44579: Unintended behaviour with defcustom’s ‘choice’ widgets and ":inline t" & Wrong documentation on "(elisp) Splicing into Lists"
@ 2020-11-11 15:28 Leo Vivier
  2020-11-16 23:48 ` Mauro Aranda
  0 siblings, 1 reply; 5+ messages in thread
From: Leo Vivier @ 2020-11-11 15:28 UTC (permalink / raw)
  To: 44579

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

Hi there,

There seems to be a problem in `defcustom' forms with the way the
`choice' widget handles `:inline t'.

I’ve written an .el file to walk you through it: I’ve documented the
bug, the explanation, and a tentative solution.

I recommended downloading the attachment and eval’ing the forms as they
appear.  Make sure that you use C-M-x with the `defcustom' forms, lest
their STANDARD not be reset.

HTH,

-- 
Leo Vivier
Freelance Software Engineer
Website: www.leovivier.com | Blog: www.zaeph.net

[-- Attachment #2: defcustom-inline-debug.el --]
[-- Type: text/emacs-lisp, Size: 13476 bytes --]

;;; DESCRIPTION

;; Incriminated section of the manual:
(info "(elisp) Splicing into Lists")

;; >    When the element-type is a ‘choice’, you use ‘:inline’ not in the
;; > ‘choice’ itself, but in (some of) the alternatives of the ‘choice’.  For
;; > example, to match a list which must start with a file name, followed
;; > either by the symbol ‘t’ or two strings, use this customization type:
;; >
;; >      (list file
;; >            (choice (const t)
;; >                    (list :inline t string string)))
;; >
;; > If the user chooses the first alternative in the choice, then the
;; > overall list has two elements and the second element is ‘t’.  If the
;; > user chooses the second alternative, then the overall list has three
;; > elements and the second and third must be strings.

;; The first option in ‘choice’ works.
(defcustom zp/testing '("foo" t)
  "Testing variable."
  :type
  '(list file
         (choice (const t)
                 (list :inline t
                   string
                   string))))

(customize-variable 'zp/testing)
;; => The form is recognised.

;; The second option in ‘choice’ doesn’t work.
(defcustom zp/testing '("foo" "bar" "baz")
  "Testing variable."
  :type
  '(list file
         (choice (const t)
                 (list :inline t
                   string
                   string))))

(customize-variable 'zp/testing)
;; => The form is *not* recognised.

;; Proof that ‘choice’ implies a list in this context by enclosing the result
;; of the ‘choice’ in a list in STANDARD.
(defcustom zp/testing '("foo" ("bar" "baz")) ; <---
  "Testing variable."
  :type
  '(list file
         (choice
          (const t)
          (list :inline t
            string
            string))))

(customize-variable 'zp/testing)
;; => The form is recognised.



;;; DEBUGGING

;; Adding another ":inline t" for `choice' allows us to make progress.
(defcustom zp/testing '("foo" "bar" "baz")
  "Testing variable."
  :type
  '(list file
         (choice :inline t            ; <---
          (const t)
          (list :inline t
            string
            string))))

(customize-variable 'zp/testing)
;; => The form is recognised.

;; …But this makes the first option not recognised t as a legitimate value.
(defcustom zp/testing '("foo" t)
  "Testing variable."
  :type
  '(list file
         (choice :inline t            ; <---
          (const t)
          (list :inline t
            string
            string))))

(customize-variable 'zp/testing)
;; => The form is recognised, but t is not recognised as a valid value.
;; However, manually selecting t in the "Value menu" produces a form which you
;; can apply, producing exactly the same form as STANDARD.  Closing and
;; reopening the customise window after applying the value produces the same
;; invalid value, as expected.

;; Enclosing t in a list resolves the issue.
(defcustom zp/testing '("foo" "bar" "baz")
  "Testing variable."
  :type
  '(list file
         (choice :inline t
           (list :inline t              ; <---
             (const t))
           (list :inline t
             string
             string))))

(customize-variable 'zp/testing)
;; => The form is recognised, but at what cost?
;; Also, it is slightly misleading to present the first choice as a "List" in
;; the menu, considering that the list is exploded into its single element.

;; …Let’s dive deeper, shall we?



;;; EXPLANATION (for the courageous)

;; The issue can be more easily grokked if we consider a close alternative to
;; the ‘choice’ widget: ‘set’.  Instead of the XOR presented in the previous
;; choices, we’re going to turn it into an OR and enclose it into a list.
(defcustom zp/testing '("foo" (t "bar" "baz")) ; <---
  "Testing variable."
  :type
  '(list file
         (set                       ;No ":inline t" to keep the the list as is
          (const t)
          (list :inline t
            string
            string))))

(customize-variable 'zp/testing)
;; => The form is recognised.
;; As we can see, the ‘set’ widget always implies a list, which is why all the
;; options are enclosed in a list.

;; Now, let’s explode that list by adding ":inline t" to the ‘set’ widget:
(defcustom zp/testing '("foo" t "bar" "baz") ; <---
  "Testing variable."
  :type
  '(list file
         (set :inline t                 ; <---
           (const t)
           (list :inline t
             string
             string))))

(customize-variable 'zp/testing)
;; => The form is recognised.

;; If we focus on the second option in ‘set’, we can see that ‘set’ receives
;; the constant t alongside two strings which are the elements of an exploded
;; list.  Therefore, ‘set’ is handed three elements which it packages into
;; a list.  This list is then exploded via ":inline t".
;; (Please note that my usage of "handed over" is used to describe what is
;; *seemingly* happening, which is not how the code actually implements it, as
;; we’ll see later.)
;;
;; The difference between the ‘set’ widget and the ‘choice’ widget is that the
;; later does not always imply a list.
;;
;; Let’s return to the previous non-functioning examples with ‘choice’ and
;; ":inline t".

;; Scenario 1: 2nd option for ‘choice’
(defcustom zp/testing '("foo" "bar" "baz") ; <---
  "Testing variable."
  :type
  '(list file
         (choice :inline t
           (const t)
           (list :inline t         ; <--- MATCH
             string
             string))))

(customize-variable 'zp/testing)
;; => The form is recognised.
;; Here, we have a very similar scenario to the one which was just described
;; with ‘set’: ‘choice’ is handed an exploded list which it packages into
;; a list.  This list is then exploded via ":inline t".

;; Scenario 2: 1st option for ‘choice’ *without* a wrapping list
(defcustom zp/testing '("foo" t)
  "Testing variable."
  :type
  '(list file
         (choice :inline t
           (const t)               ; <--- MATCH?
           (list :inline t
             string
             string))))

(customize-variable 'zp/testing)
;; => The form is recognised, but t is not recognised as a valid value.
;; In this scenario, ‘choice’ receives a single element, t, and is instructed
;; to explode it via ":inline t".  This results in an existential panic
;; (« Sacrebleu, ceci n’est pas une liste ?! ») which ultimately causes
;; ‘customize’ to not parse the form properly.

;; Scenario 3: 1st option for ‘choice’ *with* a wrapping list
(defcustom zp/testing '("foo" t)
  "Testing variable."
  :type
  '(list file
         (choice :inline t
           (list :inline t         ;Wrapping list
             (const t))            ; <--- MATCH
           (list :inline t
             string
             string))))

(customize-variable 'zp/testing)
;; => The form is recognised, but at what cost?

;; If we pause here for a moment, this is the reason why we need to revise the
;; "handed over" model I described above.  If ‘choice’ and ‘set’ exploded
;; their elements in the order I detailed, elements shouldn’t be discriminated
;; upon their provenance, i.e., whether they were atomic elements or if they
;; came from an exploded list.

;; Let’s essentialise the form a little bit to clarify my point:
;; (All the examples below produce well-formed results for ‘customize’.)
(defcustom zp/testing t            ;Atomic element
  "Testing variable."
  :type
  '(choice
    (list :inline t
      (const t))
    (const t)))                    ; <--- MATCH

(customize-variable 'zp/testing)
;; Based on the order of the options, if the 1st option had been exploded
;; before considering STANDARD, it should have matched.  Instead, it’s the 2nd
;; option which matches.

(defcustom zp/testing '(t)         ;List with an atomic element
  "Testing variable."
  :type
  '(choice
    (list :inline t                ; <--- MATCH
      (const t))
    (const t)))

(customize-variable 'zp/testing)
;; When STANDARD is a list, the 1st option matches in spite of the exploded
;; list, which goes against what is described in the manual.

;; My interpretation is that ‘choice’ was not designed with ":inline t" in
;; mind, or at least not as ‘set’ was.  I haven’t investigated how the parsing
;; is done internally, but I assume that ‘choice’ fails to understand ":inline
;; t" in composite widgets.

;; Just to be clear, ":inline t" implies that we want the elements in the
;; composite widget to be merged inside the super-element, i.e., the one which
;; *includes* the composite widget with ":inline t".

;; Let’s refer back to the examples with ‘set’ to illustrate this:
(defcustom zp/testing '("foo" (t "bar" "baz")) ; <---
  "Testing variable."
  :type
  '(list file
         (set              ;No ":inline t" to keep the the list as is
          (const t)
          (list :inline t
            string
            string))))

(customize-variable 'zp/testing)
;; => The form is recognised.

(defcustom zp/testing '("foo" t "bar" "baz") ; <---
  "Testing variable."
  :type
  '(list file
         (set :inline t    ;Now with ":inline t"
          (const t)
          (list :inline t
            string
            string))))

(customize-variable 'zp/testing)
;; => The form is recognised.

;; Now, the fundamental problem with ‘choice’---and mind the quotes, this
;; isn’t Philosophy 101---is that it is a composite widget which must
;; *sometimes* return non-composite widgets:
;;
;; 1. (choice symbol symbol)
;;    always returns a non-composite widget
;;
;; 2. (choice (list symbol) (list symbol))
;;    always returns a composite widget
;;
;; 3. (choice (list symbol) symbol)
;;    *sometimes* returns a non-composite widget
;;    *sometimes* returns a composite widget
;;
;; Because of this, the current implementation of ‘choice’ always tries to
;; return a single well-formed widget.  When we use ":inline t" on a composite
;; widget, we explicity tell that widget that we don’t care about its opinion
;; and we explode it.  If it only had one element, Mazel-fucking-tov, we’ve
;; got a well-formed non-composite widget!  But if it had more than one
;; element, all we’re left with is a a group of elements which aren’t in
;; a list, but which would have to be coerced into one for ‘choice’ to
;; consider them as a well-formed option.
;;
;; So, since all widgets are created equals, rather than discriminating upon
;; well-formedness---society abhors originals, after all---we coerce this
;; rebelious group into a list to set the world aright.
;;
;; But there is a way, and I’ll explain it in the next section.



;;; SUGGESTED FIX

;; ‘choice’ needs to be made aware of the ":inline t" in its options.
;;
;; Since ‘choice’ is intended to receed into the background once the
;; appropriate option has been pattern-matched, it doesn’t make sense to have
;; it carry the ":inline t".  Instead, it should respect the ":inline t" that
;; is present in its option when said option is matched.

;; Let’s walk through an example to clarify:
(defcustom zp/testing t    ;Atomic element
  "Testing variable."
  :type
  '(choice
    (list :inline t        ;Option 1
      (const t))
    (const t)))            ;Option 2

(customize-variable 'zp/testing)
;; (For those of you following at home, this was the first essentialised
;; example.  If you didn’t read the previous section and you’re feeling left
;; out, that’s what you get for skipping my lecture, you ingrate.)

;; With the *current implementation*, Option 2 is matched.  Option 1 is
;; exploded as instructed, but ‘choice’ coerces it into a list in order to
;; return it as a well-formed widget.
;;
;; Instead, when ‘choice’ pattern-matches an option which has ":inline t" in
;; it (notwithstanding those which might be nested if the option happens to be
;; a composite widget), it should *distribute* that ":inline t" to the
;; ‘choice’ widget itself so as to return the rebellious group of elements
;; (that’s another reference you might have missed from the previous section;
;; sucks to be you, eh?).
;;
;; So, in short: when ‘choice’ has an option with ":inline t", explode the
;; option for pattern-matching, and if it matches, distribute the ":inline t"
;; to the ‘choice’ widget.
;;
;;                                     ***
;;
;; Now, I know what you’re thinking: why spend so long documenting a problem
;; rather than *actually* trying to fix it?  Well, firstly, congratulations,
;; you’ve described academia, but I also happen to be an English major, and
;; I’ll jump on every occasion I get to offset my technical-illiterateness
;; with my actual-literateness. Secondly I couldn’t believe the doc of Emacs
;; to be any less than perfect: claiming otherwise would have been hubris, and
;; I had no idea what pandemonium awaited me if I dared to refactor the
;; scriptures.
;;
;; Thank you for listening to my TED Talk.^A^K
;; Remember that the real treasure is the friends we made along the way.^A^K
;;
;; …Thank you.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* bug#44579: Unintended behaviour with defcustom’s ‘choice’ widgets and ":inline t" & Wrong documentation on "(elisp) Splicing into Lists"
  2020-11-11 15:28 bug#44579: Unintended behaviour with defcustom’s ‘choice’ widgets and ":inline t" & Wrong documentation on "(elisp) Splicing into Lists" Leo Vivier
@ 2020-11-16 23:48 ` Mauro Aranda
  2020-11-17 17:56   ` Leo Vivier
  2020-11-24  6:36   ` Lars Ingebrigtsen
  0 siblings, 2 replies; 5+ messages in thread
From: Mauro Aranda @ 2020-11-16 23:48 UTC (permalink / raw)
  To: Leo Vivier; +Cc: 44579


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

tags 44579 patch
quit

Leo Vivier <zaeph@zaeph.net> writes:

> Hi there,
>
> There seems to be a problem in `defcustom' forms with the way the
> `choice' widget handles `:inline t'.
>
> I’ve written an .el file to walk you through it: I’ve documented the
> bug, the explanation, and a tentative solution.
>

[...]

> ;; >    When the element-type is a ‘choice’, you use ‘:inline’ not in the
> ;; > ‘choice’ itself, but in (some of) the alternatives of the ‘choice’.
For
> ;; > example, to match a list which must start with a file name, followed
> ;; > either by the symbol ‘t’ or two strings, use this customization type:
> ;; >
> ;; >      (list file
> ;; >            (choice (const t)
> ;; >                    (list :inline t string string)))
> ;; >
> ;; > If the user chooses the first alternative in the choice, then the
> ;; > overall list has two elements and the second element is ‘t’.  If the
> ;; > user chooses the second alternative, then the overall list has three
> ;; > elements and the second and third must be strings.
>
> ;; The first option in ‘choice’ works.
> (defcustom zp/testing '("foo" t)
>   "Testing variable."
>   :type
>   '(list file
>          (choice (const t)
>                  (list :inline t
>                    string
>                    string))))
>
> (customize-variable 'zp/testing)
> ;; => The form is recognised.
>
> ;; The second option in ‘choice’ doesn’t work.
> (defcustom zp/testing '("foo" "bar" "baz")
>   "Testing variable."
>   :type
>   '(list file
>          (choice (const t)
>                  (list :inline t
>                    string
>                    string))))
>
> (customize-variable 'zp/testing)
> ;; => The form is *not* recognised.

Confirmed.

> ;;; SUGGESTED FIX
>
> ;; ‘choice’ needs to be made aware of the ":inline t" in its options.
> ;;
> ;; Since ‘choice’ is intended to receed into the background once the
> ;; appropriate option has been pattern-matched, it doesn’t make sense to
have
> ;; it carry the ":inline t".  Instead, it should respect the ":inline t"
that
> ;; is present in its option when said option is matched.

Yes, something like that.  This bug happens because the choice widget is
unable to tell to widget-match-inline that it wants to try to match more
than one member of a list, when one of its choices is inline.  So
widget-match-inline only passes it one element, in this case a string,
and one string won't match a list of two strings.

So the choice widget needs to be able to tell widget-match-inline about
that.  To avoid a large impact of tweaking the code to fix this, I made
a change to affect only choice widgets with inline choices, which are
the ones that suffer exhibit this bug.

The patch to wid-edit.el is a little larger, because once the choice
widget can match inline values, then it has to be able to create them.

I added tests for both matching choice widgets and creating the choice
widgets as a part of other grouping widgets.  In current master, the
following tests should fail, exposing the bug:
widget-test-choice-match-all-inline
widget-test-choice-match-some-inline

And without the changes to the create functions, the following tests
would fail:
widget-test-repeat-can-handle-inlinable-choice
widget-test-list-can-handle-inlinable-choice
widget-test-option-can-handle-inlinable-choice

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

[-- Attachment #2: 0001-Fix-matching-of-inline-choices-for-the-choice-widget.patch --]
[-- Type: text/x-patch, Size: 14906 bytes --]

From afce8db9e42ab20bf0e27fb087b9c17a41aeb70a Mon Sep 17 00:00:00 2001
From: Mauro Aranda <maurooaranda@gmail.com>
Date: Mon, 16 Nov 2020 20:05:04 -0300
Subject: [PATCH] Fix matching of inline choices for the choice widget

A choice widget should be able to match either no inline values or
inline values, upon request.  (Bug#44579)

* lisp/wid-edit.el (choice): New property, :inline-bubbles-p.  A
predicate that returns non-nil if the choice widget can act as an
inline widget.  Document it.
(widget-choice-inline-bubbles-p): New function, for the
:inline-bubbles-p property of the choice widget.
(widget-inline-p): New function.  Use the :inline-bubbles-p property
of the widget, if any.
(widget-match-inline): Use the above to see if the widget can act like
an inline widget.  Document it.
(widget-choice-value-create): Account for the case of a choice widget
that has inline members.
(widget-checklist-add-item, widget-editable-list-value-create)
(widget-group-value-create): Use widget-inline-p rather than just
checking for a non-nil :inline property, allowing these functions to
pass the complete information to widgets like the choice widget to
create their values.

* test/lisp/wid-edit-tests.el (widget-test-choice-match-no-inline)
(widget-test-choice-match-all-inline)
widget-test-choice-match-some-inline): New tests, to check that choice
widgets can match its choices, inline or not.
(widget-test-inline-p): New test, for the new function
widget-inline-p.
(widget-test-repeat-can-handle-choice)
(widget-test-repeat-can-handle-inlinable-choice)
(widget-test-list-can-handle-choice)
(widget-test-list-can-handle-inlinable-choice)
(widget-test-option-can-handle-choice)
(widget-test-option-can-handle-inlinable-choice): New tests.  This
grouping widgets need to be able to create a choice widget regardless
if it has inline choices or not.
---
 lisp/wid-edit.el            |  72 +++++++++++++----
 test/lisp/wid-edit-tests.el | 153 ++++++++++++++++++++++++++++++++++++
 2 files changed, 211 insertions(+), 14 deletions(-)

diff --git a/lisp/wid-edit.el b/lisp/wid-edit.el
index 4e2cf7416d..8250316bcc 100644
--- a/lisp/wid-edit.el
+++ b/lisp/wid-edit.el
@@ -591,9 +591,25 @@ widget-default-get
 			  (widget-put widget :args args)))
 		      (widget-apply widget :default-get)))))
 
+(defun widget-inline-p (widget &optional bubblep)
+  "Non-nil if the widget WIDGET is inline.
+
+With BUBBLEP non-nil, check also if WIDGET has a member that bubbles its inline
+property (if any), up to WIDGET, so that WIDGET can act as an inline widget."
+  (or (widget-get widget :inline)
+      (and bubblep
+           (widget-get widget :inline-bubbles-p)
+           (widget-apply widget :inline-bubbles-p))))
+
 (defun widget-match-inline (widget vals)
-  "In WIDGET, match the start of VALS."
-  (cond ((widget-get widget :inline)
+  "In WIDGET, match the start of VALS.
+
+For an inline widget or for a widget that acts like one (see `widget-inline-p'),
+try to match elements in VALS as far as possible.  Otherwise, match the first
+element of the list VALS.
+
+Return a list whose car contains all members of VALS that matched WIDGET."
+  (cond ((widget-inline-p widget t)
 	 (widget-apply widget :match-inline vals))
 	((and (listp vals)
 	      (widget-apply widget :match (car vals)))
@@ -2198,7 +2214,7 @@ widget-choice-value-create
   (let ((value (widget-get widget :value))
 	(args (widget-get widget :args))
 	(explicit (widget-get widget :explicit-choice))
-	current)
+        current val inline-p fun)
     (if explicit
 	(progn
 	  ;; If the user specified the choice for this value,
@@ -2207,15 +2223,24 @@ widget-choice-value-create
 					      widget explicit value)))
 	  (widget-put widget :choice explicit)
 	  (widget-put widget :explicit-choice nil))
+      (setq inline-p (widget-inline-p widget t))
       (while args
 	(setq current (car args)
 	      args (cdr args))
-	(when (widget-apply current :match value)
-	  (widget-put widget :children (list (widget-create-child-value
-					      widget current value)))
-	  (widget-put widget :choice current)
-	  (setq args nil
-		current nil)))
+        (if inline-p
+            (if (widget-get current :inline)
+                (setq val value
+                      fun :match-inline)
+              (setq val (car value)
+                    fun :match))
+          (setq val value
+                fun :match))
+        (when (widget-apply current fun val)
+          (widget-put widget :children (list (widget-create-child-value
+                                              widget current val)))
+          (widget-put widget :choice current)
+          (setq args nil
+                current nil)))
       (when current
 	(let ((void (widget-get widget :void)))
 	  (widget-put widget :children (list (widget-create-child-and-convert
@@ -2438,7 +2463,7 @@ widget-checklist-add-item
 			     (let ((child (widget-create-child widget type)))
 			       (widget-apply child :deactivate)
 			       child))
-			    ((widget-get type :inline)
+                            ((widget-inline-p type t)
 			     (widget-create-child-value
 			      widget type (cdr chosen)))
 			    (t
@@ -2795,7 +2820,7 @@ widget-editable-list-value-create
 	(if answer
 	    (setq children (cons (widget-editable-list-entry-create
 				  widget
-				  (if (widget-get type :inline)
+                                  (if (widget-inline-p type t)
 				      (car answer)
 				    (car (car answer)))
 				  t)
@@ -2979,7 +3004,7 @@ widget-group-value-create
 	   (insert-char ?\s (widget-get widget :indent)))
       (push (cond ((null answer)
 		   (widget-create-child widget arg))
-		  ((widget-get arg :inline)
+                  ((widget-inline-p arg t)
 		   (widget-create-child-value widget arg (car answer)))
 		  (t
 		   (widget-create-child-value widget arg (car (car answer)))))
@@ -3900,12 +3925,17 @@ widget-alist-convert-option
     `(cons :format "Key: %v" ,key-type ,value-type)))
 \f
 (define-widget 'choice 'menu-choice
-  "A union of several sexp types."
+  "A union of several sexp types.
+
+If one of the choices of a choice widget has an :inline t property,
+then the choice widget can act as an inline widget on its own if the
+current choice is inline."
   :tag "Choice"
   :format "%{%t%}: %[Value Menu%] %v"
   :button-prefix 'widget-push-button-prefix
   :button-suffix 'widget-push-button-suffix
-  :prompt-value 'widget-choice-prompt-value)
+  :prompt-value 'widget-choice-prompt-value
+  :inline-bubbles-p #'widget-choice-inline-bubbles-p)
 
 (defun widget-choice-prompt-value (widget prompt value _unbound)
   "Make a choice."
@@ -3948,6 +3978,20 @@ widget-choice-prompt-value
     (if current
 	(widget-prompt-value current prompt nil t)
       value)))
+
+(defun widget-choice-inline-bubbles-p (widget)
+  "Non-nil if the choice WIDGET has at least one choice that is inline.
+This is used when matching values, because a choice widget needs to
+match a value inline rather than just match it if at least one of its choices
+is inline."
+  (let ((args (widget-get widget :args))
+        cur found)
+    (while (and args (not found))
+      (setq cur (car args)
+            args (cdr args)
+            found (widget-get cur :inline)))
+    found))
+
 \f
 (define-widget 'radio 'radio-button-choice
   "A union of several sexp types."
diff --git a/test/lisp/wid-edit-tests.el b/test/lisp/wid-edit-tests.el
index 4508b68023..1bd429736e 100644
--- a/test/lisp/wid-edit-tests.el
+++ b/test/lisp/wid-edit-tests.el
@@ -148,4 +148,157 @@ widget-test-moving-editable-list-item
       ;; Check that we effectively moved the item to the last position.
       (should (equal (widget-value lst) '("beg" "middle" "end"))))))
 
+(ert-deftest widget-test-choice-match-no-inline ()
+  "Test that a no-inline choice widget can match its values."
+  (let* ((choice '(choice (const nil) (const t) string function))
+         (widget (widget-convert choice)))
+    (should (widget-apply widget :match nil))
+    (should (widget-apply widget :match t))
+    (should (widget-apply widget :match ""))
+    (should (widget-apply widget :match 'ignore))))
+
+(ert-deftest widget-test-choice-match-all-inline ()
+  "Test that a choice widget with all inline members can match its values."
+  (let* ((lst '(list (choice (list :inline t symbol number)
+                             (list :inline t symbol regexp))))
+         (widget (widget-convert lst)))
+    (should-not (widget-apply widget :match nil))
+    (should (widget-apply widget :match '(:test 2)))
+    (should (widget-apply widget :match '(:test ".*")))
+    (should-not (widget-apply widget :match '(:test ignore)))))
+
+(ert-deftest widget-test-choice-match-some-inline ()
+  "Test that a choice widget with some inline members can match its values."
+  (let* ((lst '(list string
+                     (choice (const t)
+                             (list :inline t symbol number)
+                             (list :inline t symbol regexp))))
+         (widget (widget-convert lst)))
+    (should-not (widget-apply widget :match nil))
+    (should (widget-apply widget :match '("" t)))
+    (should (widget-apply widget :match '("" :test 2)))
+    (should (widget-apply widget :match '("" :test ".*")))
+    (should-not (widget-apply widget :match '(:test ignore)))))
+
+(ert-deftest widget-test-inline-p ()
+  "Test `widget-inline-p'.
+For widgets without an :inline t property, `widget-inline-p' has to return nil.
+But if the widget is a choice widget, it has to return nil if passed nil as
+the bubblep argument, or non-nil if one of the members of the choice widget has
+an :inline t property and we pass a non-nil bubblep argument.  If no members of
+the choice widget have an :inline t property, then `widget-inline-p' has to
+return nil, even with a non-nil bubblep argument."
+  (with-temp-buffer
+    (widget-insert "Testing.\n\n")
+    (let* ((widget (widget-create 'repeat
+                                  :value '(nil)
+                                  '(choice (const nil) (const t)
+                                           (list :inline t symbol number))
+                                  '(choice (const nil) (const t)
+                                           (list function string))))
+           (children (widget-get widget :children))
+           (child-1 (car children))
+           (child-2 (cadr children)))
+      (should-not (widget-inline-p widget))
+      (should-not (widget-inline-p child-1))
+      (should (widget-inline-p child-1 'bubble))
+      (should-not (widget-inline-p child-2))
+      (should-not (widget-inline-p child-2 'bubble)))))
+
+(ert-deftest widget-test-repeat-can-handle-choice ()
+  "Test that we can create a repeat widget with a choice correctly."
+  (with-temp-buffer
+    (widget-insert "Testing.\n\n")
+    (let* ((widget (widget-create 'repeat
+                                  :entry-format "%i %d %v"
+                                  :value '((:test 2))
+                                  '(choice (const nil) (const t)
+                                           (list symbol number))))
+           (child (car (widget-get widget :children))))
+      (widget-insert "\n")
+      (use-local-map widget-keymap)
+      (widget-setup)
+      (should child)
+      (should (equal (widget-value widget) '((:test 2)))))))
+
+(ert-deftest widget-test-repeat-can-handle-inlinable-choice ()
+  "Test that we can create a repeat widget with an inlinable choice correctly."
+  (with-temp-buffer
+    (widget-insert "Testing.\n\n")
+    (let* ((widget (widget-create 'repeat
+                                  :entry-format "%i %d %v"
+                                  :value '(:test 2)
+                                  '(choice (const nil) (const t)
+                                           (list :inline t symbol number))))
+           (child (widget-get widget :children)))
+      (widget-insert "\n")
+      (use-local-map widget-keymap)
+      (widget-setup)
+      (should child)
+      (should (equal (widget-value widget) '(:test 2))))))
+
+(ert-deftest widget-test-list-can-handle-choice ()
+  "Test that we can create a list widget with a choice correctly."
+  (with-temp-buffer
+    (widget-insert "Testing.\n\n")
+    (let* ((widget (widget-create 'list
+                                  :value '((1 "One"))
+                                  '(choice string
+                                           (list number string))))
+           (child (car (widget-get widget :children))))
+      (widget-insert "\n")
+      (use-local-map widget-keymap)
+      (widget-setup)
+      (should child)
+      (should (equal (widget-value widget) '((1 "One")))))))
+
+(ert-deftest widget-test-list-can-handle-inlinable-choice ()
+  "Test that we can create a list widget with an inlinable choice correctly."
+  (with-temp-buffer
+    (widget-insert "Testing.\n\n")
+    (let* ((widget (widget-create 'list
+                                  :value '(1 "One")
+                                  '(choice string
+                                           (list :inline t number string))))
+           (child (car (widget-get widget :children))))
+      (widget-insert "\n")
+      (use-local-map widget-keymap)
+      (widget-setup)
+      (should child)
+      (should (equal (widget-value widget) '(1 "One"))))))
+
+(ert-deftest widget-test-option-can-handle-choice ()
+  "Test that we can create a option widget with a choice correctly."
+  (with-temp-buffer
+    (widget-insert "Testing.\n\n")
+    (let* ((widget (widget-create 'repeat
+                                  :value '(("foo"))
+                                  '(list (option
+                                          (choice string
+                                                  (list :inline t
+                                                        number string))))))
+           (child (car (widget-get widget :children))))
+      (widget-insert "\n")
+      (use-local-map widget-keymap)
+      (widget-setup)
+      (should child)
+      (should (equal (widget-value widget) '(("foo")))))))
+
+(ert-deftest widget-test-option-can-handle-inlinable-choice ()
+  "Test that we can create a option widget with an inlinable choice correctly."
+  (with-temp-buffer
+    (widget-insert "Testing.\n\n")
+    (let* ((widget (widget-create 'repeat
+                                  :value '((1 "One"))
+                                  '(list (option
+                                          (choice string
+                                                  (list :inline t
+                                                        number string))))))
+           (child (car (widget-get widget :children))))
+      (widget-insert "\n")
+      (use-local-map widget-keymap)
+      (widget-setup)
+      (should child)
+      (should (equal (widget-value widget) '((1 "One")))))))
+
 ;;; wid-edit-tests.el ends here
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* bug#44579: Unintended behaviour with defcustom’s ‘choice’ widgets and ":inline t" & Wrong documentation on "(elisp) Splicing into Lists"
  2020-11-16 23:48 ` Mauro Aranda
@ 2020-11-17 17:56   ` Leo Vivier
  2020-11-24  6:36   ` Lars Ingebrigtsen
  1 sibling, 0 replies; 5+ messages in thread
From: Leo Vivier @ 2020-11-17 17:56 UTC (permalink / raw)
  To: Mauro Aranda; +Cc: 44579

Hi there,

Mauro Aranda <maurooaranda@gmail.com> writes:

> * lisp/wid-edit.el (choice): New property, :inline-bubbles-p.  A
> predicate that returns non-nil if the choice widget can act as an
> inline widget.  Document it.
> (widget-choice-inline-bubbles-p): New function, for the
> :inline-bubbles-p property of the choice widget.
> (widget-inline-p): New function.  Use the :inline-bubbles-p property
> of the widget, if any.
> (widget-match-inline): Use the above to see if the widget can act like
> an inline widget.  Document it.
> (widget-choice-value-create): Account for the case of a choice widget
> that has inline members.
> (widget-checklist-add-item, widget-editable-list-value-create)
> (widget-group-value-create): Use widget-inline-p rather than just
> checking for a non-nil :inline property, allowing these functions to
> pass the complete information to widgets like the choice widget to
> create their values.
>
> * test/lisp/wid-edit-tests.el (widget-test-choice-match-no-inline)
> (widget-test-choice-match-all-inline)
> widget-test-choice-match-some-inline): New tests, to check that choice
> widgets can match its choices, inline or not.
> (widget-test-inline-p): New test, for the new function
> widget-inline-p.
> (widget-test-repeat-can-handle-choice)
> (widget-test-repeat-can-handle-inlinable-choice)
> (widget-test-list-can-handle-choice)
> (widget-test-list-can-handle-inlinable-choice)
> (widget-test-option-can-handle-choice)
> (widget-test-option-can-handle-inlinable-choice): New tests.  This
> grouping widgets need to be able to create a choice widget regardless
> if it has inline choices or not.

Thank you for your work and for the patch.

Best,

-- 
Leo Vivier
Freelance Software Engineer
Website: www.leovivier.com | Blog: www.zaeph.net





^ permalink raw reply	[flat|nested] 5+ messages in thread

* bug#44579: Unintended behaviour with defcustom’s ‘choice’ widgets and ":inline t" & Wrong documentation on "(elisp) Splicing into Lists"
  2020-11-16 23:48 ` Mauro Aranda
  2020-11-17 17:56   ` Leo Vivier
@ 2020-11-24  6:36   ` Lars Ingebrigtsen
  2020-11-24 11:33     ` Mauro Aranda
  1 sibling, 1 reply; 5+ messages in thread
From: Lars Ingebrigtsen @ 2020-11-24  6:36 UTC (permalink / raw)
  To: Mauro Aranda; +Cc: Leo Vivier, 44579

Mauro Aranda <maurooaranda@gmail.com> writes:

> The patch to wid-edit.el is a little larger, because once the choice
> widget can match inline values, then it has to be able to create them.

Looks good to me; go ahead and push.

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





^ permalink raw reply	[flat|nested] 5+ messages in thread

* bug#44579: Unintended behaviour with defcustom’s ‘choice’ widgets and ":inline t" & Wrong documentation on "(elisp) Splicing into Lists"
  2020-11-24  6:36   ` Lars Ingebrigtsen
@ 2020-11-24 11:33     ` Mauro Aranda
  0 siblings, 0 replies; 5+ messages in thread
From: Mauro Aranda @ 2020-11-24 11:33 UTC (permalink / raw)
  To: Lars Ingebrigtsen; +Cc: zaeph, 44579

tags 44579 fixed
close 44579 28.1
quit


Lars Ingebrigtsen <larsi@gnus.org> writes:

> Mauro Aranda <maurooaranda@gmail.com> writes:
>
>> The patch to wid-edit.el is a little larger, because once the choice
>> widget can match inline values, then it has to be able to create them.
>
> Looks good to me; go ahead and push.

Thanks, done.





^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-11-24 11:33 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-11-11 15:28 bug#44579: Unintended behaviour with defcustom’s ‘choice’ widgets and ":inline t" & Wrong documentation on "(elisp) Splicing into Lists" Leo Vivier
2020-11-16 23:48 ` Mauro Aranda
2020-11-17 17:56   ` Leo Vivier
2020-11-24  6:36   ` Lars Ingebrigtsen
2020-11-24 11:33     ` Mauro Aranda

Code repositories for project(s) associated with this external index

	https://git.savannah.gnu.org/cgit/emacs.git
	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.