widget-choice-action

widget-choice-action

[Luc Teirlinck]

Is there a reason for the "as long as the value is the same" in the
following comment in widget-choice-action?

      ;; If this was an explicit user choice,
      ;; record the choice, and the record the value it was made for.
      ;; widget-choice-value-create will respect this choice,
      ;; as long as the value is the same.

In other words, is there a reason not to make the changes to
`widget-choice-action and `widget-choice-value-create' in the patch
below?  Why should the user's explicit choice _not_ be respected if
the value changes?  This seems very counterintuitive and I had to struggle
with this in the new indicate-buffer-boundaries defcustom, although I
finally found an acceptable workaround (but that was a coincidence and
I would rather not have used the workaround, although it is not too
bad).  If the user selects a choice from a value menu, he expects to
get that choice, regardless of whether the default :value of that
choice is identical with the previous value or not.

===File ~/wid-edit-diff=====================================
*** wid-edit.el	21 Jul 2005 06:54:49 -0500	1.143
--- wid-edit.el	01 Aug 2005 23:00:23 -0500	
***************
*** 1955,1964 ****
  	(args (widget-get widget :args))
  	(explicit (widget-get widget :explicit-choice))
  	current)
!     (if (and explicit (equal value (widget-get widget :explicit-choice-value)))
  	(progn
  	  ;; If the user specified the choice for this value,
! 	  ;; respect that choice as long as the value is the same.
  	  (widget-put widget :children (list (widget-create-child-value
  					      widget explicit value)))
  	  (widget-put widget :choice explicit))
--- 1955,1964 ----
  	(args (widget-get widget :args))
  	(explicit (widget-get widget :explicit-choice))
  	current)
!     (if explicit
  	(progn
  	  ;; If the user specified the choice for this value,
! 	  ;; respect that choice.
  	  (widget-put widget :children (list (widget-create-child-value
  					      widget explicit value)))
  	  (widget-put widget :choice explicit))
***************
*** 2047,2059 ****
  		 (setq this-explicit t)
  		 (widget-choose tag (reverse choices) event))))
      (when current
!       ;; If this was an explicit user choice,
!       ;; record the choice, and the record the value it was made for.
!       ;; widget-choice-value-create will respect this choice,
!       ;; as long as the value is the same.
        (when this-explicit
! 	(widget-put widget :explicit-choice current)
! 	(widget-put widget :explicit-choice-value (widget-get widget :value)))
        (widget-value-set widget (widget-default-get current))
        (widget-setup)
        (widget-apply widget :notify widget event)))
--- 2047,2056 ----
  		 (setq this-explicit t)
  		 (widget-choose tag (reverse choices) event))))
      (when current
!       ;; If this was an explicit user choice, record the choice,
!       ;; so that widget-choice-value-create will respect it.
        (when this-explicit
! 	(widget-put widget :explicit-choice current))
        (widget-value-set widget (widget-default-get current))
        (widget-setup)
        (widget-apply widget :notify widget event)))
============================================================

Re: widget-choice-action

[Per Abrahamsen]

It was RMS who wrote code, but since I'm CC'ed, I'll contribute with
my recollection of it (which may be entirely wrong).

Luc Teirlinck <teirllm@dms.auburn.edu> writes:

> This seems very counterintuitive and I had to struggle
> with this in the new indicate-buffer-boundaries defcustom, although I
> finally found an acceptable workaround (but that was a coincidence and
> I would rather not have used the workaround, although it is not too
> bad). 

What did you first try to do, what did you end up with?

> If the user selects a choice from a value menu, he expects to
> get that choice, regardless of whether the default :value of that
> choice is identical with the previous value or not.

The "previous value" is not used.  It compares the value of the choice
the user selected, with the *new* value of the widget, which are
supposed to be the same.  

I can imagine some cases where they are different, and for those cases
there are no guarantee that the new value of the widget will actually
fit the type of the explicit chosen widget.  So these should be
ignored.  I.e., please don't apply your patch.

...

I'm in general very suspicious of any customize type that need the
:explicit-choice functionality in order to work.

A choice widget will normally select the first choice that match the
value.  :explicit-choice overrides that, so an later choice can be
selected even if a prior choice matches.  However, if you save the
option and enter customize again, the first match will be used, thus
the choice you see will not be the one you saved.

Thus :explicit-choice often just hides a problem in a custom type long
enough for the developer not to discover it, but not long enough for
the user to avoid being bitten by it.

The real solution is that when you have overlapping types in a choice
widget, the most specific *must* be listed first.  This

   (choice string sexp); OK

and not

   (choice sexp string); BAD: "string" will never be selected
                       ; implicit, as any string is also a sexp

Re: widget-choice-action

[Luc Teirlinck]

Per Abrahamsen wrote:

   The real solution is that when you have overlapping types in a choice
   widget, the most specific *must* be listed first.

That is exactly what my defcustom below does.  The duplicated value is
nil (as it most often is, in practice).  The most explicit is the
const nil.  It is listed first.

   The "previous value" is not used.  It compares the value of the choice
   the user selected, with the *new* value of the widget, which are
   supposed to be the same.  

Are you sure about that?  It might have been what the code was
intended to do, but is that really what it actually does?  Maybe my
fix is not the correct one, but something needs to be fixed.

   A choice widget will normally select the first choice that match the
   value.  :explicit-choice overrides that, so an later choice can be
   selected even if a prior choice matches.  However, if you save the
   option and enter customize again, the first match will be used, thus
   the choice you see will not be the one you saved.

That is actually completely OK.  In many cases, as in the example
below, the only reason to select a less explicit choice is to change
the value.  But there is often no natural explicit :value to select,
so one wants to leave the :value nil, even though saving that value is
equivalent to selecting the explicit nil and, if that is done and the
Custom is revisited, the explicit nil _should_ be shown.

Things will probably be clearer on the concrete example.  We are talking
about the new indicate-buffer-boundaries defcustom, which in today's
CVS is in the fringe group.  (There are unrelated bugs hat still need
to be fixed.  Loading fringe.el will override any values you saved for
`indicate-buffer-boundaries' and `indicate-empty-lines' and I would
like to get rid of the (redundant) `fringe-indicators' defcustom.

(indicate-buffer-boundaries
              fringe
              (choice
               (const :tag "No indicators" nil)
               (const :tag "On left, with arrows" left)
               (const :tag "On right, with arrows" right)
               (set :tag "Pick your own design"
                    :value ((t . nil))
                    :format "%{%t%}:\n%v\n%d"
                    :doc "You can specify a default and then override it \
for individual indicators.
Leaving \"Default\" unchecked is equivalent with specifying a default of
\"Do not show\"."
                    (choice :tag "Default"
                            :value (t . nil)
                            (const :tag "Do not show" (t . nil))
                            (const :tag "On the left" (t . left))
                            (const :tag "On the right" (t . right)))
                    (choice :tag "Top"
                            :value (top . left)
                            (const :tag "Do not show" (top . nil))
                            (const :tag "On the left" (top . left))
                            (const :tag "On the right" (top . right)))
                    (choice :tag "Bottom"
                            :value (bottom . left)
                            (const :tag "Do not show" (bottom . nil))
                            (const :tag "On the left" (bottom . left))
                            (const :tag "On the right" (bottom . right)))
                    (choice :tag "Up arrow"
                            :value (up . left)
                            (const :tag "Do not show" (up . nil))
                            (const :tag "On the left" (up . left))
                            (const :tag "On the right" (up . right)))
                    (choice :tag "Down arrow"
                            :value (down . left)
                            (const :tag "Do not show" (down . nil))
                            (const :tag "On the left" (down . left))
                            (const :tag "On the right" (down . right))))
               (other :tag "On left, no arrows" t))
              "22.1")

With this (the current code) everything works fine.  But originally, I
did not have the `:value (t . nil)', so the actual :value was nil,
which was intended.  (t .  nil) happens to be equivalent in its effects
with nil, but is a formally different value, which is why I can use
this workaround.  But such a workaround will not always be available.

Without the `:value (t . nil), if the current value shown is the
explicit nil and you select "Pick your own design", it works.  But if
any other value is currently shown (whether it is the actual value of
the option, or whether it is just edited but not set does not appear
to matter) and you try to select "Pick your own design", the explicit
nil actually gets selected.  It is irrelevant that if I would
immediately save the option, the explicit nil and the unedited "Pick
your own design" interface would save the same value.  Clearly, I
selected "Pick your own design" to edit that widget, not to save the
blank interface.

Sincerely,

Luc.

Re: widget-choice-action

[Luc Teirlinck]

>From my previous reply:

   With this (the current code) everything works fine.  But originally, I
   did not have the `:value (t . nil)', so the actual :value was nil,
   which was intended.

Sorry, I forgot that I have plenty of :value's in my defcustom and I
forgot to double up my parentheses around ((t . nil)).  The
`:value ((t . nil))' I was talking about was the one of the `set':

               (set :tag "Pick your own design"
                    :value ((t . nil))

That was the one I had to put in to work around the problem.  I had
:value's in my choices, because there nil was not an acceptable default.

Sincerely,

Luc.

Re: widget-choice-action

[Luc Teirlinck]

Apart from the `indicate-buffer-boundaries' problem I described before
my patch actually also seems to correct a bug that I seem to remember
was reported a while ago by somebody else, but is still present.  The
example is easier and the problem is more obvious.  Take the following
defcustom:

(defcustom testvar 0 "Great Doc"
  :type '(choice (const nil) (number :value 0) alist))

C-M-x that and do `M-x customize-option testvar'.  Now click on value
menu and select `Alist'.  Result:  invalid ((nil)).  Should not
happen, because nil is a valid alternative for alist.  Using an
explicit (alist :value nil) does not solve the problem.

My patch completely solves this problem.  _Maybe_ there is a better
solution to these two (related) problems, but these two problems need
to be solved one way or the other.

Sincerely,

Luc.

Re: widget-choice-action

[Per Abrahamsen]

Ok, I think the problem is that the original code try to extract the
value of the choosen widget before it has been instantiated in the
buffer.  When a widget is instantiated, lots of stuff (mostly
regarding quoting) can happen which can change its intrinsic value.

The reason the the choosen value has to be remembered, is that the
widget can also have its value set programmatically (with
widget-value-set), not just from the user pressing the [value menu]
button.  But if the user first change the widget with the [value menu]
button, and then something else set the value with (with
widget-value-set), the :explicit-choice property will still be there,
overwriting the match based choise.

I don't remember if Customize call widget-value-set, maybe it does
when you do some kind of reset.  If so, your patch should break
Customize when you first do an explicit choise, then a reset.  If not,
it will break other widget applications.

However, if this is the only reason for :explicit-choice-value, it
seems it would be much cleaner to simply erase the :explicit-choice
attribute after use.

Re: widget-choice-action

[Luc Teirlinck]

Per Abrahamsen wrote:

   However, if this is the only reason for :explicit-choice-value, it
   seems it would be much cleaner to simply erase the :explicit-choice
   attribute after use.

I believe that it should be erased.  It seems very bad to leave a
property hanging around forever when it is no longer valid.  If I
understood correctly, the patch below which erases the
:explicit-choice after use, should take care of the problems you
mentioned.

===File ~/wid-edit-diff=====================================
*** wid-edit.el	21 Jul 2005 06:54:49 -0500	1.143
--- wid-edit.el	03 Aug 2005 07:56:36 -0500	
***************
*** 1955,1967 ****
  	(args (widget-get widget :args))
  	(explicit (widget-get widget :explicit-choice))
  	current)
!     (if (and explicit (equal value (widget-get widget :explicit-choice-value)))
  	(progn
  	  ;; If the user specified the choice for this value,
! 	  ;; respect that choice as long as the value is the same.
  	  (widget-put widget :children (list (widget-create-child-value
  					      widget explicit value)))
! 	  (widget-put widget :choice explicit))
        (while args
  	(setq current (car args)
  	      args (cdr args))
--- 1955,1968 ----
  	(args (widget-get widget :args))
  	(explicit (widget-get widget :explicit-choice))
  	current)
!     (if explicit
  	(progn
  	  ;; If the user specified the choice for this value,
! 	  ;; respect that choice.
  	  (widget-put widget :children (list (widget-create-child-value
  					      widget explicit value)))
! 	  (widget-put widget :choice explicit)
! 	  (widget-put widget :explicit-choice nil))
        (while args
  	(setq current (car args)
  	      args (cdr args))
***************
*** 2047,2059 ****
  		 (setq this-explicit t)
  		 (widget-choose tag (reverse choices) event))))
      (when current
!       ;; If this was an explicit user choice,
!       ;; record the choice, and the record the value it was made for.
!       ;; widget-choice-value-create will respect this choice,
!       ;; as long as the value is the same.
        (when this-explicit
! 	(widget-put widget :explicit-choice current)
! 	(widget-put widget :explicit-choice-value (widget-get widget :value)))
        (widget-value-set widget (widget-default-get current))
        (widget-setup)
        (widget-apply widget :notify widget event)))
--- 2048,2057 ----
  		 (setq this-explicit t)
  		 (widget-choose tag (reverse choices) event))))
      (when current
!       ;; If this was an explicit user choice, record the choice,
!       ;; so that widget-choice-value-create will respect it.
        (when this-explicit
! 	(widget-put widget :explicit-choice current))
        (widget-value-set widget (widget-default-get current))
        (widget-setup)
        (widget-apply widget :notify widget event)))
============================================================