bug#59937: 28.2; Bad defcustom behavior

[Drew Adams]

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

Fabulous!

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

100%.  I hope you can do it without too much trouble.
It will make a big difference, I think, including
perhaps in how much people make use of `restricted-sexp'.

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

I was seeing prompting only as a necessity as long as the code requires a value 
before it can create the UI field for the `restricted-sexp'.  If you can 
dispense with that need then great!  Certainly it would be much better not to 
have any prompting (especially not with just the default prompt).

> I thought I was doing an improvement by giving the warning, since
> providing invalid default values is somewhat common.

I know you did.  I'm afraid that the warnings are too
difficult to understand.  They were for me, as one user.

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

I thought it already coped with invalid input.
I guess I was mistaken.  It definitely should.

Generally, all Customize UI fields (including
buttons, checkboxes, etc.) do check the input
for validity, I think.  Not necessarily at the
time you edit but at least when you try to set
the value to what your editing resulted in.

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

Yes.  I think the code essentially reads a Lisp
expression.  IOW, I think that it just does what
`read--expression' does, but in a roundabout way.
(I haven't looked at the wid-edit.el code before
saying this; I could be wrong.)

;; From `simple.el':
(defun read--expression (prompt &optional initial-contents)
  (let ((minibuffer-completing-symbol t))
    (minibuffer-with-setup-hook
        (lambda ()
          ;; FIXME: call emacs-lisp-mode?
          (add-function :before-until (local 'eldoc-documentation-function)
                        #'elisp-eldoc-documentation-function)
          (eldoc-mode 1)
          (add-hook 'completion-at-point-functions
                    #'elisp-completion-at-point nil t)
          (run-hooks 'eval-expression-minibuffer-setup-hook))
      (read-from-minibuffer prompt initial-contents
                            read-expression-map t
                            'read-expression-history))))

FWIW, I define `pp-read--expression' in pp+.el
to have the same code, except that it uses
`pp-read-expression-map', which provides Elisp
key bindings:

(defvar pp-read-expression-map nil
  "`read-expression-map' with some Emacs-Lisp key bindings.")
(unless pp-read-expression-map
  (let ((map  (make-sparse-keymap)))
    (define-key map "\M-\t" 'lisp-indent-line)
    (define-key map "\t" 'lisp-complete-symbol)
    (define-key map "\e\C-q" 'indent-sexp)
    (define-key map "\e\t" 'lisp-indent-line)
    (define-key map "\e\C-x" 'eval-defun)
    (define-key map "\e\C-q" 'indent-pp-sexp)
    ;;(define-key map "\177" 'backward-delete-char-untabify)
    (set-keymap-parent map minibuffer-local-map)
    (setq pp-read-expression-map  map)))

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

Really glad I could contribute something to this,
by my incessant arguing/questioning. ;-)
I appreciate your working on this.  I doubt that
anyone else would try to tackle it.

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

Sounds great to me.  Do what you can, and we'll
see how far we get.

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

Yeah, no doubt there are still things that could
be ironed out.  The insertion of additional (empty)
pairs of INS DEL when you click INS is one.

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

Ah.  Maybe we do disagree, in the sense that I still
don't understand.

Is there a _logical_ requirement that there be a
value, in order to create the editable field for
the `restricted-sexp'?  I don't think there should
be.

That's different from the need for a value because
the current code works that way.

But I really don't see why a value is needed.  All
the code needs to do is create an editable field
that expects text that satisfies the predicates,
no?  Of what (logical) use is the ("default") value?

Anyway, if you can get empty fields by just using
"" as the value then perhaps all will be well.  My
feeling was/is that there should be no need for any
value, just to create the field.  But if my logic
is wrong, or if it's too complex to alter the code
not to need a default value (in order to create the
field), and if using "" works, then great!

And yes, if the definer provides a default value
then that should be respected instead of "".  But
I was thinking/expecting that the default value
(and any user-supplied value by editing) would be
checked by the predicates.  Now I guess that's not
the case at the time of field creation, or even at
the time of editing.  (I expect it's the case at
time of setting, however.)

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

Sounds good to me.  As I say above, I still don't
see why any value would be needed, just to create
the field.  But (1) I could be wrong about that
not being a logical necessity or (2) it could be
a pain to try to modify the code not to need that.

I really have no idea how the code currently depends
on having a value in order to create the 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.

Terrific.  To me, `restricted-sexp' is super
powerful/general, but it's not used much and
it seems there are some ~bugs wrt its use (by
definers) and the use of its fields (by users).

If you can fix this then I'm hoping more people
will take advantage of `restricted-sexp'.  In a
way, it's a poor-man's substitute for defining UI
widgets for custom types.  Only a poor substitute,
but still useful.

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

That's what I've been guessing.  But even if that's
true, it doesn't follow that it's worth trying to
rewrite the code not to depend on that.  Widget &
custom code is complicated.

> But one can say that the default value at least shows an example
> of what's expected.

Absolutely.  When you can provide a (useful, correct)
default value, you should.

I'm embarrassed that I didn't understand that you
can use :value nearly everywhere.  I think the doc
could maybe be improved a bit...

> I'm not too convinced of that point of view, so
> don't take it too seriously.

Not to worry.

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

In case it's missing, definitely.

In case it's not valid?  I guess so, but in that
case it would be good to signal an error (somehow),
or a message saying that it's invalid and so will
be ignored (create the field without any value).

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

It all sounds good to me.  Looking forward to
whatever you come up with.  Thx.