* bug#59937: 28.2; Bad defcustom behavior @ 2022-12-10 5:06 Drew Adams 2022-12-10 10:05 ` Mauro Aranda 0 siblings, 1 reply; 18+ messages in thread From: Drew Adams @ 2022-12-10 5:06 UTC (permalink / raw) To: 59937 [-- Attachment #1: Type: text/plain, Size: 4448 bytes --] These first four defcustoms behave as I expect. The first has a default value of nil - empty alist. The second has a default value of (("key1" :x ("a" "b") ("q"))). The third had a default value of (("key1" :x () :w ("q"))) - one of the plist-entry values is empty. The fourth has a default value of (("key1")) - empty plist. The option should be an alist with elements that have a string key and a plist value. Each plist element has a keyword key and a list of string values. The alist can be empty, any plist can be empty, and any plist-entry value can be empty. ;; OK (defcustom myvar () "..." :group 'emacs :type '(alist :key-type (string :tag "Alist key (string):") :value-type (plist :key-type symbol :options (:x :y :z) :value-type (repeat string)))) ;; OK (defcustom myvar '(("key1" :x ("a" "b") :w ("q"))) "..." :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)") :options (:x :y :z) :value-type (repeat string)))) ;; OK (defcustom myvar '(("key1" :x () :w ("q"))) "..." :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)") :options (:x :y :z) :value-type (repeat string)))) ;; OK (defcustom myvar '(("key1")) "..." :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)") :options (:x :y :z) :value-type (repeat string)))) The first OK example specifies the type of the plist key as just symbol, not as keywordp. But I want to constrain it to be a symbol that's a keyword. The problem occurs when I try to do that AND I use a default value of nil (see next). This one is no good - that's the bug: ;; When click initial INS get prompted for a Lisp expression, get warnings, ;; and Customize buffer contents are changed to just INS DEL while prompting. ;; (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)") :options (:x :y :z) :value-type (repeat string)))) In this case the default value is nil, but the defcustom also specifies the type of plist values as keywordp. I think this definition should work fine. But when you click the sole INS button at the outset you're prompted for a Lisp expression and you simultaneously get a warning that a plist key is not of the right type. If you enter a Lisp expression (e.g. just 42 or whatever) then the Customize buffer appears as it should. I think (1) you shouldn't be prompted for anything and (2) you shouln't get any warning of anything. Warning (widget-bad-default-value): A widget of type restricted-sexp has a bad default value. value: nil match function: widget-restricted-sexp-match match-alternatives: (keywordp) Disable showing Disable logging See attached screenshot, which shows the prompt as well as the Customize buffer during prompting and the *Warnings* buffer. In GNU Emacs 28.2 (build 2, x86_64-w64-mingw32) of 2022-09-13 built on AVALON Windowing system distributor 'Microsoft Corp.', version 10.0.19044 System Description: Microsoft Windows 10 Pro (v10.0.2009.19044.2251) Configured using: 'configure --with-modules --without-dbus --with-native-compilation --without-compress-install CFLAGS=-O2' Configured features: ACL GIF GMP GNUTLS HARFBUZZ JPEG JSON LCMS2 LIBXML2 MODULES NATIVE_COMP NOTIFY W32NOTIFY PDUMPER PNG RSVG SOUND THREADS TIFF TOOLKIT_SCROLL_BARS XPM ZLIB (NATIVE_COMP present but libgccjit not available) [-- Attachment #2: throw-emacs-defcustom-bug.png --] [-- Type: image/png, Size: 35101 bytes --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* bug#59937: 28.2; Bad defcustom behavior 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 0 siblings, 1 reply; 18+ messages in thread From: Mauro Aranda @ 2022-12-10 10:05 UTC (permalink / raw) To: Drew Adams; +Cc: 59937 Drew Adams <drew.adams@oracle.com> writes: > These first four defcustoms behave as I expect. > > The first has a default value of nil - empty alist. The second has a > default value of (("key1" :x ("a" "b") ("q"))). The third had a default > value of (("key1" :x () :w ("q"))) - one of the plist-entry values is > empty. The fourth has a default value of (("key1")) - empty plist. > > The option should be an alist with elements that have a string key and a > plist value. Each plist element has a keyword key and a list of string > values. The alist can be empty, any plist can be empty, and any > plist-entry value can be empty. > > ;; OK > (defcustom myvar () > "..." > :group 'emacs > :type '(alist :key-type (string :tag "Alist key (string):") > :value-type > (plist :key-type symbol > :options (:x :y :z) > :value-type (repeat string)))) > > ;; OK > (defcustom myvar '(("key1" :x ("a" "b") :w ("q"))) > "..." > :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)") > :options (:x :y :z) > :value-type (repeat string)))) > > ;; OK > (defcustom myvar '(("key1" :x () :w ("q"))) > "..." > :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)") > :options (:x :y :z) > :value-type (repeat string)))) > > ;; OK > (defcustom myvar '(("key1")) > "..." > :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)") > :options (:x :y :z) > :value-type (repeat string)))) > > The first OK example specifies the type of the plist key as just symbol, > not as keywordp. But I want to constrain it to be a symbol that's a > keyword. The problem occurs when I try to do that AND I use a default > value of nil (see next). The moment you add a match alternative that won't match the default value of a restricted-sexp widget (which is nil), then you should change the default value for the restricted-sexp widget. > This one is no good - that's the bug: > > ;; When click initial INS get prompted for a Lisp expression, get warnings, > ;; and Customize buffer contents are changed to just INS DEL while prompting. > ;; > (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)") > :options (:x :y :z) > :value-type (repeat string)))) > > In this case the default value is nil, but the defcustom also specifies > the type of plist values as keywordp. I think this definition should > work fine. You're looking at a different default value. The warning comes from Widget, and says that the default value for the restricted-sexp widget is not correct. It's not talking about the default value for the user option. > But when you click the sole INS button at the outset you're > prompted for a Lisp expression and you simultaneously get a warning that > a plist key is not of the right type. If you enter a Lisp expression > (e.g. just 42 or whatever) then the Customize buffer appears as it > should. I think (1) you shouldn't be prompted for anything and (2) you > shouln't get any warning of anything. Examples 2-4 get the same warning once the user clicks the INS button. If you specify a valid default value for the restricted-sexp widget, then the warning is gone. See also bugs #15689, #25152. ^ permalink raw reply [flat|nested] 18+ messages in thread
* bug#59937: 28.2; Bad defcustom behavior 2022-12-10 10:05 ` Mauro Aranda @ 2022-12-10 22:05 ` Drew Adams 2022-12-11 11:08 ` Mauro Aranda 0 siblings, 1 reply; 18+ messages in thread From: Drew Adams @ 2022-12-10 22:05 UTC (permalink / raw) To: Mauro Aranda; +Cc: 59937@debbugs.gnu.org I was actually hoping that you'd see and reply to this bug report, Mauro. You're our defcustom expert! Turns out that this is the 3rd time I've reported this bug, not realizing I'd already reported it! > The moment you add a match alternative that won't match the default > value of a restricted-sexp widget (which is nil), then you should change > the default value for the restricted-sexp widget. I don't even see how/where that widget gets a default value of nil. I see `:match-alternatives '(functionp)', and I see `:value 'ignore'. I admit that I don't really understand the code that implements `restricted-sexp'. (And I'd think that I shouldn't really need to understand it.) > > (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)") > > :options (:x :y :z) > > :value-type (repeat string)))) > > > > In this case the default value is nil, but the defcustom also > > specifies the type of plist values as keywordp. I think this > > definition should work fine. > > You're looking at a different default value. > The warning comes from Widget, and says that the default > value for the restricted-sexp widget is not > correct. It's not talking about the default value > for the user option. Note that in bug #25152 I expressed my disagreement with closing the bug - IMO, it's not fixed. I don't understand how a defcustom should be bothering with (working around) a default value for the widget that's defined for `restricted-sexp'. The default value of the option is the only default value that should matter, no? If the initial (i.e. default) value of the option is nil, then the alist is nil, which means it has no elements, which means there are no plists. So it's impossible to even speak about applying some condition to a plist element. Such a test (which is what `restricted-sexp' defines) is never - can never be, logically - applied to any plist element because no such elements exist in this default case. To me, this is just, well, a bug. A bug in the definition of widget `restricted-sexp', I guess (?). But a priori I'd think the bug is not in the definition of `restricted-sexp' but in the definition of anything that uses it. To me, widget `restricted-sexp' just shouldn't apply at all in a context such as described in this bug: there's _nothing to check_ with a predicate that's used to check each list element - there are no elements. To me, `null' as a predicate doesn't apply either. This isn't about testing whether some plist element is nil. Nothing about the plist should be checked, because there's no plist! There's certainly not a plist with nil elements - now should predicate `null' help here (but it does!)? Even in bug #25152, which is simpler, I can't see where a proper "fix" (workaround) by the defcustom definer is to add predicate `null'. Likewise, adding `:value ignore'. No predicate that tests a value in a `repeat' list can possibly be a way to validate or invalidate a `repeat' list that has no elements at all - an empty list. It's like saying that for (mapcar PRED ()) we need to have a PRED such as `null' - makes no sense to me. `mapcar' has to itself be defined so that any PRED you provide isn't invoked when the list arg is (). I guess I still don't get it. And I can't tell whether you think there is a bug or not. The duplicate bugs were closed (by Lars), after you tried to improve things by providing a warning. Though I suppose it was good to provide the warning, I don't see that the bug is fixed at all. And I'm (still) afraid that any user (including the person writing the defcustom and testing it) won't (1) understand what's going on and (2) be able to figure out how to fix the `restricted-sexp' to work around the problem. Does always adding `null' take care of it? I just now tried the _two_ alternative workarounds for `restricted-sexp' you showed in the context of bug #25152: (1) add `:value ignore' or (2) add predicate `null' to the list of predicates. In the case of the example in this bug (#59937) it looks like #1 doesn't work - you get the same warning etc. But #2 works. > Examples 2-4 get the same warning once the user clicks the INS button. > If you specify a valid default value for the restricted-sexp widget, > then the warning is gone. See also bugs #15689, #25152. 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. At my present, poor state of understanding this, about all I could tell someone is to add `null' as a predicate. I couldn't explain why or how that works. I can't see how/why any of the predicates would/should get called if the value of the option is (). Anyway, I'm _grateful_ to you for pointing this out (again), and for pointing to bugs #15689 and #25152. You're definitely the king of widgets, and we're very lucky to have you involved. Thx. 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* bug#59937: 28.2; Bad defcustom behavior 2022-12-10 22:05 ` Drew Adams @ 2022-12-11 11:08 ` Mauro Aranda 2022-12-13 22:10 ` Drew Adams 0 siblings, 1 reply; 18+ messages in thread From: Mauro Aranda @ 2022-12-11 11:08 UTC (permalink / raw) To: Drew Adams; +Cc: 59937@debbugs.gnu.org Drew Adams <drew.adams@oracle.com> writes: >> The moment you add a match alternative that won't match the default >> value of a restricted-sexp widget (which is nil), then you should change >> the default value for the restricted-sexp widget. > > I don't even see how/where that widget gets a default > value of nil. I see `:match-alternatives '(functionp)', > and I see `:value 'ignore'. I admit that I don't really > understand the code that implements `restricted-sexp'. I think you're looking at the function widget, not at the restricted-sexp widget. M-x widget-browse RET restricted-sexp shows some information about the restricted-sexp widget. Since there's no :value in that buffer, to find out what could it be you have to navigate to the parent widget, the sexp widget. That buffer will show: :value nil somewhere, saying the default value for this and derived widgets is nil, unless the code overrides it (by passing a different :value). > (And I'd think that I shouldn't really need to understand it.) I agree. Below I'll try to focus on the behavior and not on the implementation. >> > (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)") >> > :options (:x :y :z) >> > :value-type (repeat string)))) >> > >> > In this case the default value is nil, but the defcustom also >> > specifies the type of plist values as keywordp. I think this >> > definition should work fine. >> >> You're looking at a different default value. >> The warning comes from Widget, and says that the default >> value for the restricted-sexp widget is not >> correct. It's not talking about the default value >> for the user option. > > Note that in bug #25152 I expressed my disagreement > with closing the bug - IMO, it's not fixed. > > I don't understand how a defcustom should be bothering > with (working around) a default value for the widget > that's defined for `restricted-sexp'. > > The default value of the option is the only default > value that should matter, no? If the initial (i.e. > default) value of the option is nil, then the alist > is nil, which means it has no elements, which means > there are no plists. Once again, I think you're focusing on a different default value. The warning shows up when trying to insert a new element, right? At that moment, the user asked for a widget to edit, and like every other widget, it gets created with a default value. That value should be a valid one. If it helps, take these two examples: (defcustom foo nil "..." :type '(repeat (function))) Click INS and you'll see a new editable field with the value ignore. That's the default value for the function widget. You can check it with M-x widget-browse RET function Now take a look at this: (defcustom foo nil "..." :type '(repeat (restricted-sexp :match-alternatives (functionp)))) Click INS and you'll get the warning. The default value for the restricted-sexp widget is nil, as I said, and of course: (functionp nil) => nil > So it's impossible to even speak about applying some > condition to a plist element. Such a test (which is > what `restricted-sexp' defines) is never - can never > be, logically - applied to any plist element because > no such elements exist in this default case. > To me, this is just, well, a bug. A bug in the > definition of widget `restricted-sexp', I guess (?). > > But a priori I'd think the bug is not in the definition > of `restricted-sexp' but in the definition of anything > that uses it. To me, widget `restricted-sexp' just > shouldn't apply at all in a context such as described in > this bug: there's _nothing to check_ with a predicate > that's used to check each list element - there are no > elements. The elements begin to exist the moment the user asks to insert a new element with INS. I think this is as it should be. The restricted-sexp widget works fine given a valid default value according to the :match-alternatives or :match passed when defining it. As I said in Bug#25152, the ELisp manual says: ‘:value DEFAULT’ Provide a default value. If ‘nil’ is not a valid value for the alternative, then it is essential to specify a valid default with ‘:value’. If you're asking it to be valid only with keywords, then the definition should specify something that: (keywordp SOMETHING) => t > To me, `null' as a predicate doesn't apply either. > This isn't about testing whether some plist element > is nil. Nothing about the plist should be checked, > because there's no plist! There's certainly not a > plist with nil elements - now should predicate `null' > help here (but it does!)? As I said, when the user clicks INS, then there is a plist: the one being edited. > And I can't tell whether you think there is a bug > or not. The duplicate bugs were closed (by Lars), > after you tried to improve things by providing a > warning. Though I suppose it was good to provide > the warning, I don't see that the bug is fixed at all. I do think the warning should be there. The defcustom writer can see it when testing the defcustom, or a user can report it to the developer. With regards to the restricted-sexp widget implementation, I do think it is a bug, but I didn't see a safe way to fix it back then. I'll try to take another look, but I'm no smarter than I was :-(. > And I'm (still) afraid that any user (including the > person writing the defcustom and testing it) won't > (1) understand what's going on and (2) be able to > figure out how to fix the `restricted-sexp' to work > around the problem. Does always adding `null' take > care of it? If people think the warning message is not good enough, I'm certain someone can think of a way to improve it; I couldn't. > I just now tried the _two_ alternative workarounds > for `restricted-sexp' you showed in the context of > bug #25152: (1) add `:value ignore' or (2) add > predicate `null' to the list of predicates. > > In the case of the example in this bug (#59937) > it looks like #1 doesn't work - you get the same > warning etc. But #2 works. Try to pass a valid default value for the restricted-sexp widget. If you're asking it to only match keywords, then any keyword does the trick. >> Examples 2-4 get the same warning once the user clicks the INS button. >> If you specify a valid default value for the restricted-sexp widget, >> then the warning is gone. See also bugs #15689, #25152. > > 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. Maybe having some examples in the documentation could help here. I could write one if you and others think it could be helpful. > At my present, poor state of understanding this, > about all I could tell someone is to add `null' > as a predicate. I couldn't explain why or how > that works. I can't see how/why any of the > predicates would/should get called if the value > of the option is (). > > Anyway, I'm _grateful_ to you for pointing this > out (again), and for pointing to bugs #15689 and > #25152. You're definitely the king of widgets, > and we're very lucky to have you involved. Thx. Thanks, but I do not think I'm worthy of that title. > 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* bug#59937: 28.2; Bad defcustom behavior 2022-12-11 11:08 ` Mauro Aranda @ 2022-12-13 22:10 ` Drew Adams 2022-12-13 22:50 ` Mauro Aranda 0 siblings, 1 reply; 18+ messages in thread From: Drew Adams @ 2022-12-13 22:10 UTC (permalink / raw) To: Mauro Aranda; +Cc: 59937@debbugs.gnu.org > > 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. 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'? That would be too restrictive, IMO. It should still be possible to provide a default value of () for an alist or plist that makes use of `restricted-sexp' for some element keys or values. Or a default value that in some other way doesn't provide a value for parts that are defined by `restricted-sexp' but might not be needed in the option value. > 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. 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). 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). 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! 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. 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. In the case used in the bug example, the :tag is "Plist key (keyword):". With that as prompt there's little possible misunderstanding of what we're asking the user to enter. (And if the :tag isn't clear then it also isn't very clear when used as a field label, though someone might figure it out from UI context.) And if the user enters a value that doesn't satisfy the `restricted-sexp' predicates we can still raise an error. Better yet would be to put the sexp prompt in a loop till the input (after `read-from-string') satisfies the `restricted-sexp' predicates (or until C-g). IOW, don't just use `widget-sexp-prompt-value' (it just gets a string and then Lisp-reads that), but also apply the predicates as part of the expression reading. That is, provide the same behavior/support we provide for the UI input field. 3. Don't show any warning when prompting. Just try to have the inputting itself be clearer (#2). 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. Tell them that otherwise the prompt is just "Lisp expression: ", which makes no semantic connection to the type of data needed for the field. If a defcustom definer is defining a complex option then it behooves them to make clear what the parts are. And if a part is based on `restricted-sexp' then part of making that clear is to add a :tag. (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'. So if the default value does provide all such parts then there's no prompting (and the need for a :tag is reduced). > > 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). I may still be misunderstanding things. Let me know. But if so then I'm guessing others will also misunderstand. 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 Thanks for your efforts with this. ^ permalink raw reply [flat|nested] 18+ messages in thread
* bug#59937: 28.2; Bad defcustom behavior 2022-12-13 22:10 ` Drew Adams @ 2022-12-13 22:50 ` Mauro Aranda 2022-12-14 1:51 ` Drew Adams 0 siblings, 1 reply; 18+ messages in thread From: Mauro Aranda @ 2022-12-13 22:50 UTC (permalink / raw) To: Drew Adams; +Cc: 59937@debbugs.gnu.org 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* bug#59937: 28.2; Bad defcustom behavior 2022-12-13 22:50 ` Mauro Aranda @ 2022-12-14 1:51 ` Drew Adams 2022-12-14 12:40 ` Mauro Aranda 0 siblings, 1 reply; 18+ messages in thread From: Drew Adams @ 2022-12-14 1:51 UTC (permalink / raw) To: Mauro Aranda; +Cc: 59937@debbugs.gnu.org > > 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* bug#59937: 28.2; Bad defcustom behavior 2022-12-14 1:51 ` Drew Adams @ 2022-12-14 12:40 ` Mauro Aranda 2022-12-14 18:53 ` Drew Adams 0 siblings, 1 reply; 18+ messages in thread From: Mauro Aranda @ 2022-12-14 12:40 UTC (permalink / raw) To: Drew Adams; +Cc: 59937@debbugs.gnu.org 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* bug#59937: 28.2; Bad defcustom behavior 2022-12-14 12:40 ` Mauro Aranda @ 2022-12-14 18:53 ` Drew Adams 2022-12-14 22:20 ` Mauro Aranda 0 siblings, 1 reply; 18+ messages in thread From: Drew Adams @ 2022-12-14 18:53 UTC (permalink / raw) To: Mauro Aranda; +Cc: 59937@debbugs.gnu.org > 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* bug#59937: 28.2; Bad defcustom behavior 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 0 siblings, 2 replies; 18+ messages in thread From: Mauro Aranda @ 2022-12-14 22:20 UTC (permalink / raw) To: Drew Adams; +Cc: 59937@debbugs.gnu.org Drew Adams <drew.adams@oracle.com> writes: >> 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). Great! I'll work on it. I hope I don't find any surprises. >> 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. Yes, Customize checks at the time of creation (and inserts "(mismatch)" when the value isn't valid), and at the time of setting/saving. But the Widget code does not check when creating the widget. >> 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. :-) >> > 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? I think you're right on point. It's just that the code works that way, and makes assumptions that there is always going to be a value (default or edited). >> 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. I'll see what I can do in the next days. Thanks! ^ permalink raw reply [flat|nested] 18+ messages in thread
* bug#59937: 28.2; Bad defcustom behavior 2022-12-14 22:20 ` Mauro Aranda @ 2022-12-14 22:42 ` Drew Adams 2023-01-04 16:07 ` Drew Adams 1 sibling, 0 replies; 18+ messages in thread From: Drew Adams @ 2022-12-14 22:42 UTC (permalink / raw) To: Mauro Aranda; +Cc: 59937@debbugs.gnu.org > > 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? > > I think you're right on point. It's just that the code works that way, > and makes assumptions that there is always going to be a value (default > or edited). Thanks for confirming that you think that's the case. If using "" works then that will be good. ^ permalink raw reply [flat|nested] 18+ messages in thread
* bug#59937: 28.2; Bad defcustom behavior 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 1 sibling, 1 reply; 18+ messages in thread From: Drew Adams @ 2023-01-04 16:07 UTC (permalink / raw) To: Mauro Aranda; +Cc: 59937@debbugs.gnu.org > I'll see what I can do in the next days. Thanks! Hi Mauro. Any news on this? I know it's been a holiday season; just checking. Thx. ^ permalink raw reply [flat|nested] 18+ messages in thread
* bug#59937: 28.2; Bad defcustom behavior 2023-01-04 16:07 ` Drew Adams @ 2023-01-04 22:31 ` Mauro Aranda 2023-01-04 22:47 ` Drew Adams 0 siblings, 1 reply; 18+ messages in thread From: Mauro Aranda @ 2023-01-04 22:31 UTC (permalink / raw) To: Drew Adams; +Cc: 59937@debbugs.gnu.org Drew Adams <drew.adams@oracle.com> writes: >> I'll see what I can do in the next days. Thanks! > > Hi Mauro. Any news on this? I know it's been > a holiday season; just checking. Thx. Hi Drew. Sorry, I'll try to make time to look at this, but I'm swamped with work right now. ^ permalink raw reply [flat|nested] 18+ messages in thread
* bug#59937: 28.2; Bad defcustom behavior 2023-01-04 22:31 ` Mauro Aranda @ 2023-01-04 22:47 ` Drew Adams 2023-07-16 23:00 ` Mauro Aranda 0 siblings, 1 reply; 18+ messages in thread From: Drew Adams @ 2023-01-04 22:47 UTC (permalink / raw) To: Mauro Aranda; +Cc: 59937@debbugs.gnu.org > >> I'll see what I can do in the next days. Thanks! > > > > Hi Mauro. Any news on this? I know it's been > > a holiday season; just checking. Thx. > > Hi Drew. Sorry, I'll try to make time to look at this, > but I'm swamped with work right now. No problem and no hurry. Was just wondering if you were still planning on taking a look. Thanks for hanging in there. ^ permalink raw reply [flat|nested] 18+ messages in thread
* bug#59937: 28.2; Bad defcustom behavior 2023-01-04 22:47 ` Drew Adams @ 2023-07-16 23:00 ` Mauro Aranda 2023-07-17 15:30 ` Drew Adams 2023-07-22 12:56 ` Eli Zaretskii 0 siblings, 2 replies; 18+ messages in thread From: Mauro Aranda @ 2023-07-16 23:00 UTC (permalink / raw) To: 59937@debbugs.gnu.org; +Cc: Drew Adams [-- Attachment #1: Type: text/plain, Size: 713 bytes --] Hi Drew, I'm sorry it took me so long to actually take a look at this. I had to reread the conversation to put myself back into topic. As I said in https://debbugs.gnu.org/cgi/bugreport.cgi?bug=59937#26 I was looking for a way to: 1. Being able to create the restricted-sexp widget, even if the default value for it isn't valid. 2. Being able to do it without prompting the user. That's what the attached patch does. AFAIU, we both agreed that that approach was a good enough fix for this bug, so that the behavior of certain defcustoms is less confusing. We didn't really agree back then about the warning, and I'm guessing that we still disagree on that, so I didn't change anything about the warning. [-- Attachment #2: 0001-Avoid-prompting-when-creating-a-restricted-sexp-widg.patch --] [-- Type: text/x-patch, Size: 1494 bytes --] From 12969e3b8b40c5af39209993c0a31e6b3bcb4a10 Mon Sep 17 00:00:00 2001 From: Mauro Aranda <maurooaranda@gmail.com> Date: Sun, 16 Jul 2023 19:46:54 -0300 Subject: [PATCH] Avoid prompting when creating a restricted-sexp widget * lisp/wid-edit.el (restricted-sexp): Turn value into a string before reading. (widget-field-value-create): Guard against value being nil, so the field can be created even if the widget has a bad default value. (Bug#59937) --- lisp/wid-edit.el | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/lisp/wid-edit.el b/lisp/wid-edit.el index 234f3d9b74d..aaec2360db8 100644 --- a/lisp/wid-edit.el +++ b/lisp/wid-edit.el @@ -2127,7 +2127,8 @@ widget-field-value-create ;; `widget-setup' is called. (overlay (cons (make-marker) (make-marker)))) (widget-put widget :field-overlay overlay) - (insert value) + (when value + (insert value)) (and size (< (length value) size) (insert-char ?\s (- size (length value)))) @@ -3655,7 +3656,9 @@ 'restricted-sexp value (widget-get widget :match) (widget-get widget :match-alternatives)) - :warning)) + :warning) + ;; Make sure we will `read' a string. + (setq value (prin1-to-string value))) (read value))) (defun widget-restricted-sexp-match (widget value) -- 2.34.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* bug#59937: 28.2; Bad defcustom behavior 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 1 sibling, 1 reply; 18+ messages in thread From: Drew Adams @ 2023-07-17 15:30 UTC (permalink / raw) To: Mauro Aranda, 59937@debbugs.gnu.org > I'm sorry it took me so long to actually take a look at this. I had to > reread the conversation to put myself back into topic. I had to reread it just now, to remind myself what this is all about. ;-) > As I said in > https://debbugs.gnu.org/cgi/bugreport.cgi?bug=59937*26 > I was looking for a way to: > 1. Being able to create the restricted-sexp widget, even if the default > value for it isn't valid. > 2. Being able to do it without prompting the user. > > That's what the attached patch does. AFAIU, we both agreed that that > approach was a good enough fix for this bug, so that the behavior of > certain defcustoms is less confusing. We didn't really agree back then > about the warning, and I'm guessing that we still disagree on that, so I > didn't change anything about the warning. Thanks, Mauro. I don't have the time now to check the result, but I have no problem at all with whatever fix you implemented. If later (probably much later) I find something I think could be improved more in this regard, I'll file a new enhancement/bug request. Thx! ^ permalink raw reply [flat|nested] 18+ messages in thread
* bug#59937: 28.2; Bad defcustom behavior 2023-07-17 15:30 ` Drew Adams @ 2023-07-17 16:21 ` Mauro Aranda 0 siblings, 0 replies; 18+ messages in thread From: Mauro Aranda @ 2023-07-17 16:21 UTC (permalink / raw) To: Drew Adams; +Cc: 59937@debbugs.gnu.org Drew Adams <drew.adams@oracle.com> writes: >> As I said in >> https://debbugs.gnu.org/cgi/bugreport.cgi?bug=59937*26 >> I was looking for a way to: >> 1. Being able to create the restricted-sexp widget, even if the default >> value for it isn't valid. >> 2. Being able to do it without prompting the user. >> >> That's what the attached patch does. AFAIU, we both agreed that that >> approach was a good enough fix for this bug, so that the behavior of >> certain defcustoms is less confusing. We didn't really agree back then >> about the warning, and I'm guessing that we still disagree on that, so I >> didn't change anything about the warning. > > Thanks, Mauro. I don't have the time now to > check the result, but I have no problem at all > with whatever fix you implemented. If later > (probably much later) I find something I think > could be improved more in this regard, I'll > file a new enhancement/bug request. > > Thx! Ok, thank you. I'll wait for a few days in case someone wants to chime in, and then I'll install the fix if there are no objections. ^ permalink raw reply [flat|nested] 18+ messages in thread
* bug#59937: 28.2; Bad defcustom behavior 2023-07-16 23:00 ` Mauro Aranda 2023-07-17 15:30 ` Drew Adams @ 2023-07-22 12:56 ` Eli Zaretskii 1 sibling, 0 replies; 18+ messages in thread From: Eli Zaretskii @ 2023-07-22 12:56 UTC (permalink / raw) To: Mauro Aranda; +Cc: drew.adams, 59937-done > Cc: Drew Adams <drew.adams@oracle.com> > Date: Sun, 16 Jul 2023 20:00:40 -0300 > From: Mauro Aranda <maurooaranda@gmail.com> > > I'm sorry it took me so long to actually take a look at this. I had to > reread the conversation to put myself back into topic. > > As I said in https://debbugs.gnu.org/cgi/bugreport.cgi?bug=59937#26 > I was looking for a way to: > 1. Being able to create the restricted-sexp widget, even if the default > value for it isn't valid. > 2. Being able to do it without prompting the user. > > That's what the attached patch does. AFAIU, we both agreed that that > approach was a good enough fix for this bug, so that the behavior of > certain defcustoms is less confusing. We didn't really agree back then > about the warning, and I'm guessing that we still disagree on that, so I > didn't change anything about the warning. Thanks, installed on the master branch, and closing the bug. ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2023-07-22 12:56 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 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 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
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).