bug#59937: 28.2; Bad defcustom behavior

[Mauro Aranda]

Drew Adams <drew.adams@oracle.com> writes:

>>  > 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?

Oh, I think I see a way around that now.  I think the following will
take care of:
1.  Being able to create the restricted-sexp (sub)widget even if the
default value isn't valid.

(Which I think it's one of your main points throughout the bug report)

2.  Being able to do it without prompting or whatsoever.

(Which is one of my main points in this conversation).


When the restricted-sexp widget has to be created, if there's a valid
default value we create it with that one (like I showed in my previous
message), but if there's not we create it empty.

Let me know if you agree with that.

> 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?

As I've said, I don't think we need to (nor want to) prompt.  I think
the prompt there is just an accident, and I would like to avoid it.
Sorry if I sound stubborn about this, but I'm convinced that prompting
at that time of the widget's creation can be really harmful.

>>  > 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.

I thought I was doing an improvement by giving the warning, since
providing invalid default values is somewhat common.  I've seen things
like:

(defcustom foo nil
  "..."
  :type '(repeat (function :value t)))

And I would like to make more evident these kind of errors.  But if we
find a way to cope with an invalid default value for the restricted-sexp
widget, then it might be fine to remove it (I'm not so sure yet).

>>  > 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.

Because my understanding is that in
(read var)
it was always expected that var holds a string, whatever that is.

> 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?

Yes, thanks to your response I was able to see a way to create the
editable field (with value ""), when there's no valid default value.

I really hope we are in agreement here that that approach is a good one
to follow.

>> 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?

I was trying to make the point that prompting at that moment can result
in bad things: we are not ready to process a quit, to catch an error or
whatever, so the whole UI breaks.

>>  > 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).

Hopefully we can do without the prompt; see my idea above.

> 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.

I'm not sure if I understand what you say here.  I don't think it's
possible to figure out a good value to use as a default from the
predicates: that's why my idea is about creating it with the empty
string.

>>  > 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?

Then maybe you agree with me that creating it with the empty string is a
good enough solution.  I'll wait for your confirmation.

> 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.

Oh, don't worry.  It is a pleasure for me to contribute to Emacs with
the few bug reports I can, and this is one of them.

>> 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.

Maybe it's not very useful, and it is just a current limitation of the
code.  But one can say that the default value at least shows an example
of what's expected.  I'm not too convinced of that point of view, so
don't take it too seriously.

>> 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!

So, would you agree to creating the restricted-sexp widget with an empty
editable field, in case the default value is not valid?

Then the need to provide a valid default value is not so strong anymore
(but still should be encouraged, I think), and Customize can work better
and more intuitively when there isn't a valid default value.