bug#59937: 28.2; Bad defcustom behavior

[Drew Adams <drew.adams@oracle.com>]

>  > How does s?he define/provide a default value for the
>  > `restricted-sexp' widget?  I see no way to do that.
> 
> It's the very first keyword mentioned under the Type Keywords node in
> the ELisp manual.  Usage would be like this:
> 
> (defcustom foo nil
>    "..."
>    :type '(repeat (restricted-sexp
>                    :value :key
>                    :tag "Keyword"
>                    :match-alternatives (keywordp))))

Thank you!

For some reason, I didn't realize that :value was
available for `restricted-sexp'.  I guess I had
the impression it was usable only with `choice'.

(I also wondered if, by "the restricted-sexp widget"
you maybe somehow meant the definition of the widget
itself, i.e., its `define-widget' definition.)

>  > And that's the case whether or not the _option's_ default
>  > value has a part that corresponds to a plist key.  (If yes,
>  > the default value must match the `restricted-sexp', if no,
>  > you're prompted for the sexp type, so it knows what kind
>  > of field to make.)
>  >
>  > And yes, a simple example with `restricted-sexp' would
>  > help (maybe 2 examples: bad & good).
> 
> I'll work on a patch some day this week, then.

Thank you again.  There's no hurry - it's been
the way it is now for a long time.

>  > The problem really stems, I guess, from the fact that
>  > `restricted-sexp' can involve any kinds of predicates,
>  > and depending on what those do, the UI field can be
>  > different.  Put differently, the UI field takes into
>  > account the `restricted-sexp' predicates.  But the
>  > prompting does not take them into account!
> 
> I'd say don't focus too much on the prompt.  It really shouldn't be
> there, and I consider it a bug in the Widget code, but it's really an
> implementation detail.  Without going into a lot of details, we want to
> READ a string like this: (read var) where VAR is a string, the
> representation of the value of the widget, whatever that is, but because
> the widget's value didn't match, it is not a string that we read and VAR
> is nil (Note: not "nil", but nil), so we end up calling (read nil) and
> that's the unintended prompt.

To me (so far):

I'd say don't focus too much on how the value is
currently read (whether the code gives nil etc.) ;-).

defcustom definers _will_ sometimes (maybe usually)
not use :value for a `restricted-sexp'.  Without
prompting (regardless of how its read, though it
should be read in a good way), Customize apparently
can't create the (sub)widget that corresponds to
the `restricted-sexp'.  So I think the user needs
to be prompted - no way around that, no?

And in the case where we need to prompt, why not
read taking the predicates into account?  IOW, why
shouldn't we do the same thing we do when you enter
text in the editable field: require the predicates
to be satisfied?

I repeat that question, as you didn't speak to it.
If we need to prompt, and we want to get reasonable
input at the prompt, why not apply the predicates
that we know the value must satisfy?  Why instead
let a user enter any old string, providing invalid
input?

>  > My thoughts about this - let me know what you think:
>  >
>  > 1. The warning(s) are not very helpful.  They will
>  > mainly confuse, I think.
>  >
>  > First, end users _will_ see them, as the defcustom
>  > author may not have tested every possibility well.
>  >
>  > Second, many defcustom authors also won't understand
>  > them.
> 
> I don't know if you're suggesting to improve it or to get rid of it.
> I'd like to make it more helpful, as I think it should be there.

I think it's just a liability and should be removed.
But clearly you understand all of this much better
than I.

My overall feeling is:

1. We should never need to point out that there's
such a problem (I think accepting only valid input
at a prompt should obviate the problem).

2. Warnings are in general not a good idea.  Quite
different from messages (echo area) and errors.
(Just a personal opinion, no doubt.)

3. If we can't avoid the problem (see #1) then end
users will see the warnings, and that will only
confuse them.  The warnings will confuse defcustom
definers as well, no matter how well they're worded.

>  > 2. I think a big improvement could be to make use of
>  > any :tag that the defcustom author provides for the
>  > `restricted-sexp' field - using the :tag also as the
>  > prompt, instead of "Lisp expression: ".  When you see
>  > that generic prompt you have _no clue_ what it wants,
>  > or why.  The :tag should tell you what to enter.
> 
> I don't think that's an improvement because of what I said above. No
> prompt should be there for starters.  At least that's what I understand
> about the code.

I don't think you've said why/how you think
there's no need for prompting.  Is this about
the returning-nil-instead-of-a-string thing?
If so, sure, if you can remove the need to
prompt altogether, great.

My understanding about why/when the prompt is
needed is stated above.  What do you do if no
:value is provided for the `restricted-sexp'
and the default option value doesn't fill in
a default for that field either?

Maybe you're saying that you can fix the code
so it creates the right kind of editable field,
based only on the `restricted-sexp' predicates,
i.e., without needing any default value for
that field.

If so, great!  That's what I was saying from
the beginning: wid-edit knows how to create
the field UI, so why does it need a default
value for the field to do so?  Of what (real)
use is the default value?

> Note that in Bug#25152 you ended up with a weird buffer state after
> hitting C-g at that prompt.  That's because the Widget library is not
> ready to take user input at that moment.

I'm not sure what that's about, e.g. whether
it's something else or related to the same
problem we've been talking about here.  Is
that about the fact that if you fall into the
problem (e.g., you get the warnings) then the
UI keeps adding another INS DEL pair?

>  > 3. Don't show any warning when prompting.  Just try
>  > to have the inputting itself be clearer (#2).
> 
> Because of my response, I don't think #3 applies.  I hope you agree with
> me after reading my response.

See above.  Clearly I still don't understand
this well.  If you understand what I'm saying,
then that's enough.  I trust your judgment
(and your knowledge of the problem).

From my point of view, neither users nor
defcustom definers should be bothered with
warnings nor incomprehensible prompts (nor
any prompts at all, if Emacs can do without
them).

And definers ideally shouldn't need to specify
default values for such fields - the set of
predicates should be able to define what kind
of UI field is needed.

>  > With those changes, the manual could also be improved:
>  >
>  > (1) Tell defcustom definers that if they use
>  > `restricted-sexp' then good practice is to provide a
>  > :tag for the field.  And tell them that the :tag will
>  > also be used as a prompt for creating the appropriate
>  > editable field.
> 
> Here again, I don't think this is the path we want to follow.  And the
> manual already emphasizes that providing a valid default value is
> essential, when nil isn't it.

Why is a default value essential?  Why can't the
right editable field (UI) be created based on
the `restrictive-sexp' definition, i.e., its
predicates?  Why is a default value needed?

I create an option using :value, as you suggested:

(defcustom myvar ()
  "..."
  :group 'emacs
  :type '(alist :key-type (string :tag "Alist key (string):")
	        :value-type
		(plist :key-type
                       (restricted-sexp :match-alternatives (keywordp)
                                        :tag "Plist key (keyword)"
					:value :x) ; <==============
                       :options (:x :y :z)
                       :value-type (repeat string))))

I click INS to get the fields for the alist key
and the options (checkboxes).

I click the INS (after the checkboxes), to get
the fields for the plist:

INS DEL :
          Plist key (keyword): :x
          Repeat:
          INS

Now suppose I _remove_ that :x in the editable
field.  That's the state I'd like to get without
having to specify :value.  Is it doable?

All fields are present (for one alist entry with
one plist entry).  None of them are filled in
(beyond the initiative of starting to create an
alist entry and a plist entry).

I don't think any default values were really
needed to get there - or at least none should be
needed, I'm guessing.

I think we agree that Customize can't do that
today: it can't build the UI for a field unless
it knows what its default value is.  (Correct?)

But is that a necessary (logical) restriction?
Does the default value actually help with the
definition of the kind of field needed?

I don't get that.  I'd think that all that's
needed to define the plist key field is the set
of predicates in the `restricted-sexp'.  Of what
use/need is the default value, for creating that
UI field?

Sorry this is taking so much of your time.  If
you feel you understand what I'm missing, and
it doesn't matter, please just do whatever you
think is right.  I do hope that we can somehow
do away with the warnings - and the prompt as
well, if possible.

>  > (2) Explain that such prompting happens whenever the
>  > default value of the option doesn't provide a value
>  > for each of its parts that corresponds to the use of
>  > a `restricted-sexp'.
> 
> As I said, I consider the prompt a bug in the Widget code, so I don't
> think we should mention it in docs.

OK.  See above for my understanding.  Hopefully
you can see what it is that I'm missing.
 
> We'll have to disagree here, about two things.
> 
> 1.  I still think there's a bug in the defcustoms.  Any widget needs a
> valid default value, and it's up to the person that's defining the
> widget to provide it.  If he/she doesn't, then that's a bug.

I've understood that now.  I don't understand
why it's the case, however.  What's the reason
we can't create the plist field shown above
(which requires the plist key field value to be
`keywordp') without providing a default value?

Of what real use is the default value?  That I
don't get.

> I think you understand now, 

I thought I did.  Now I think I must not. ;-)

> but the prompt being there is really confusing

I agree with that.  I feel doubly so about the
warnings, however.  I made a suggestion to help
get better prompting.  (But I agree that if you
could do away with the prompt altogether that
would be good.)

> (it was to me when I first read your bug report in
> Bug#25152).  Hopefully I clarified a little more with my response.

Yes and no, I'm afraid.  I don't get why a
default is needed, to construct the UI field.
(I understand that it _is_ needed; I don't
understand why that is.)

And I don't understand why, if a value is
needed and missing, we wouldn't need to prompt
for it.  IOW: if we need a value, then I think
we need to prompt for it.

If we don't need a value then great!

But in that case, I think we shouldn't need any
warnings either.  In that case, we'd build the
field based on the predicates, and the field
itself would - as it does now anyway - allow
only valid inputs/edits.

The field, and only the field, would handle
erroneous input to it.  The field would be
defined only by the predicates.  (I think
that's the case logically - I don't see why
the default value's needed.)

I'm hoping my misunderstanding is clear to
you now, and you can set me straight.  I'm
sure you're right; I just don't understand yet.

>  > Thanks for your efforts with this.
> 
> You're welcome, I'm happy to help.

We're very lucky we have your expertise and
interest with Customize.  Per A. was the expert,
but he's no longer available to help.