unofficial mirror of bug-gnu-emacs@gnu.org 
 help / color / mirror / code / Atom feed
From: Mauro Aranda <maurooaranda@gmail.com>
To: Drew Adams <drew.adams@oracle.com>
Cc: "59937@debbugs.gnu.org" <59937@debbugs.gnu.org>
Subject: bug#59937: 28.2; Bad defcustom behavior
Date: Tue, 13 Dec 2022 19:50:11 -0300	[thread overview]
Message-ID: <533dba58-e543-f356-664f-5dfa0b85467c@gmail.com> (raw)
In-Reply-To: <SJ0PR10MB54888D121294945B9741EEA3F3E39@SJ0PR10MB5488.namprd10.prod.outlook.com>

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

 >>  > Expecting a defcustom definer to understand this
 >>  > and figure out what a "valid default value for
 >>  > the restricted-sexp widget" might be, is a bridge
 >>  > too far, IMO.
 >>
 >> I don't think so.  The defcustom definer is specifying the matching
 >> alternatives, he/she should be able to think of a valid default
 >> value.
 >
 > 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))))

Clicking INS creates a restricted-sexp without trouble.

 > Or did you mean that s?he should always provide, within
 > the _option's_ default value, a value for each part of
 > it that corresponds to a `restricted-sexp'?

I didn't mean that.  I meant something like the code above.

 >> Maybe having some examples in the documentation could help here. I
 >> could write one if you and others think it could be helpful.
 >
 > I _think_ I understand this now.  The problem is that for
 > the Customize UI to present a field for inputting/defining
 > the part of the option value that corresponds to a plist
 > key (which is defined by a `restricted-sexp'), it needs to
 > know just what kind of input/edit widget to build.  It
 > needs to build an editable-field that also demands respect
 > of the `restricted-sexp' predicates.

Yes.

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

 > The idea/problem isn't limited to `restricted-sexp', IIUC.
 > But in other cases it's much less likely to be a gotcha,
 > because the parts of the defcustom value that correspond
 > to each field in the Customize UI will have types that
 > correspond to existing widgets (they don't require
 > additional input/prompting to know what kind of UI field
 > to create).

Yes.

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

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

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

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.

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

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

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

 >>  > I am curious whether you think there's actually
 >>  > a bug or not.  It's hard for me to believe that
 >>  > we should expect _anyone_ defining a defcustom
 >>  > (let alone anyone using Customize) to understand
 >>  > the `restricted-sexp' widget, what it requires
 >>  > wrt its "default value", and how to adjust a
 >>  > defcustom to give it what it needs, to DTRT.
 >>
 >> I think a better behavior would be to avoid the prompting altogether
 >> (there should be no prompt at that moment, for starters).  But again,
 >> this situation arises when there is a bug on the defcustom :type, so I'd
 >> be happier if people can help with improving the warning message.
 >
 > See above.
 >
 > I don't think there's a bug in the defcustoms in
 > the examples shown.  And I do think users should be
 > prompted if Emacs needs to know what kind of input
 > (UI) field to create.  And I don't think we should
 > show any warnings.  We can raise an error if the
 > user input is, in the end, invalid (and I think we
 > already take care of that).

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.

2.  I do think a warning (or an error) is in place.

 > I may still be misunderstanding things.  Let me
 > know.  But if so then I'm guessing others will also
 > misunderstand.

I think you understand now, but the prompt being there is really
confusing (it was to me when I first read your bug report in
Bug#25152).  Hopefully I clarified a little more with my response.

 > The current state is, I think, poor
 > support for the flexible, powerful feature that is
 > `restricted-sexp'.  Understandable, but we should
 > somehow try to do better.
 >
 > I, for one, wish more definers of defcustoms spent
 > more time defining tighter types.  And often that
 > could mean using `restricted-sexp'.
 >
 > FWIW, this bug report came directly from a user
 > question on emacs.SE, here:
 >

 > https://emacs.stackexchange.com/q/74913

I missed that.  I usually lurk on SE for questions with custom or widget
tags, but you do a better job than me :-).

 > Thanks for your efforts with this.

You're welcome, I'm happy to help.






  reply	other threads:[~2022-12-13 22:50 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-12-10  5:06 bug#59937: 28.2; Bad defcustom behavior Drew Adams
2022-12-10 10:05 ` Mauro Aranda
2022-12-10 22:05   ` Drew Adams
2022-12-11 11:08     ` Mauro Aranda
2022-12-13 22:10       ` Drew Adams
2022-12-13 22:50         ` Mauro Aranda [this message]
2022-12-14  1:51           ` Drew Adams
2022-12-14 12:40             ` Mauro Aranda
2022-12-14 18:53               ` Drew Adams
2022-12-14 22:20                 ` Mauro Aranda
2022-12-14 22:42                   ` Drew Adams
2023-01-04 16:07                   ` Drew Adams
2023-01-04 22:31                     ` Mauro Aranda
2023-01-04 22:47                       ` Drew Adams
2023-07-16 23:00                         ` Mauro Aranda
2023-07-17 15:30                           ` Drew Adams
2023-07-17 16:21                             ` Mauro Aranda
2023-07-22 12:56                           ` Eli Zaretskii

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://www.gnu.org/software/emacs/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=533dba58-e543-f356-664f-5dfa0b85467c@gmail.com \
    --to=maurooaranda@gmail.com \
    --cc=59937@debbugs.gnu.org \
    --cc=drew.adams@oracle.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).