bug#70589: [PATCH] Refine the Custom type of generated '*-modes' options

bug#70589: [PATCH] Refine the Custom type of generated '*-modes' options

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 156 bytes --]

Tags: patch

This makes it easier for users to customize generated '*-modes' options,
such as 'global-completion-preview-modes', via the Custom interface.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Refine-the-Custom-type-of-generated-modes-options.patch --]
[-- Type: text/patch, Size: 1873 bytes --]

From f81f30895dd1e54684ed3bd66a49ea76dd9e5cf7 Mon Sep 17 00:00:00 2001
From: Eshel Yaron <me@eshelyaron.com>
Date: Fri, 26 Apr 2024 11:43:29 +0200
Subject: [PATCH] Refine the Custom type of generated '*-modes' options

* lisp/emacs-lisp/easy-mmode.el (define-globalized-minor-mode):
Refine the Custom type of the '*-modes' option, generated when
this macro is given a ':predicate' argument.
---
 lisp/emacs-lisp/easy-mmode.el | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/lisp/emacs-lisp/easy-mmode.el b/lisp/emacs-lisp/easy-mmode.el
index eaad9646985..72509a24241 100644
--- a/lisp/emacs-lisp/easy-mmode.el
+++ b/lisp/emacs-lisp/easy-mmode.el
@@ -580,7 +580,19 @@ define-globalized-minor-mode
 and nil means \"don't use\".  There's an implicit nil at the end of the
 list."
                       mode)
-             :type '(repeat sexp)
+             :type '(choice (const :tag "Enable in all major modes" t)
+                            (const :tag "Don't enable in any major mode" nil)
+                            (repeat :tag "Only enable in"
+                                    (choice
+                                     (const :tag "All major modes" t)
+                                     (const :tag "No major mode" nil)
+                                     (symbol :value fundamental-mode
+                                             :tag "Specific major mode")
+                                     (cons :tag "Exclude modes"
+                                           (const :tag "Exclude..." not)
+                                           (repeat
+                                            (symbol :value fundamental-mode
+                                                    :tag "Major mode"))))))
              ,@group))
 
        ;; Autoloading define-globalized-minor-mode autoloads everything
-- 
2.44.0

bug#70589: [PATCH] Refine the Custom type of generated '*-modes' options

[Eli Zaretskii]

> Date: Fri, 26 Apr 2024 11:49:19 +0200
> From:  Eshel Yaron via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> This makes it easier for users to customize generated '*-modes' options,
> such as 'global-completion-preview-modes', via the Custom interface.

This kind of discussion is not useful with Stefan (CC'ed) on board.

bug#70589: [PATCH] Refine the Custom type of generated '*-modes' options

[Eli Zaretskii]

> Date: Fri, 26 Apr 2024 11:49:19 +0200
> From:  Eshel Yaron via "Bug reports for GNU Emacs,
>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> 
> --- a/lisp/emacs-lisp/easy-mmode.el
> +++ b/lisp/emacs-lisp/easy-mmode.el
> @@ -580,7 +580,19 @@ define-globalized-minor-mode
>  and nil means \"don't use\".  There's an implicit nil at the end of the
>  list."
>                        mode)
> -             :type '(repeat sexp)
> +             :type '(choice (const :tag "Enable in all major modes" t)
> +                            (const :tag "Don't enable in any major mode" nil)
> +                            (repeat :tag "Only enable in"
> +                                    (choice
> +                                     (const :tag "All major modes" t)
> +                                     (const :tag "No major mode" nil)
> +                                     (symbol :value fundamental-mode
> +                                             :tag "Specific major mode")
> +                                     (cons :tag "Exclude modes"
> +                                           (const :tag "Exclude..." not)
> +                                           (repeat
> +                                            (symbol :value fundamental-mode
> +                                                    :tag "Major mode"))))))

Why does it make sense to have the "all" and "none" alternatives
twice?

bug#70589: [PATCH] Refine the Custom type of generated '*-modes' options

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Hi,

Eli Zaretskii <eliz@gnu.org> writes:

>> Date: Fri, 26 Apr 2024 11:49:19 +0200
>> From:  Eshel Yaron via "Bug reports for GNU Emacs,
>>  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
>>
>> --- a/lisp/emacs-lisp/easy-mmode.el
>> +++ b/lisp/emacs-lisp/easy-mmode.el
>> @@ -580,7 +580,19 @@ define-globalized-minor-mode
>>  and nil means \"don't use\".  There's an implicit nil at the end of the
>>  list."
>>                        mode)
>> -             :type '(repeat sexp)
>> +             :type '(choice (const :tag "Enable in all major modes" t)
>> +                            (const :tag "Don't enable in any major mode" nil)
>> +                            (repeat :tag "Only enable in"
>> +                                    (choice
>> +                                     (const :tag "All major modes" t)
>> +                                     (const :tag "No major mode" nil)
>> +                                     (symbol :value fundamental-mode
>> +                                             :tag "Specific major mode")
>> +                                     (cons :tag "Exclude modes"
>> +                                           (const :tag "Exclude..." not)
>> +                                           (repeat
>> +                                            (symbol :value fundamental-mode
>> +                                                    :tag "Major mode"))))))
>
> Why does it make sense to have the "all" and "none" alternatives
> twice?

These are slightly different: a value of t ("Enable in all major modes")
says to enable everywhere, while a t inside a list ("All major modes")
says to enable in modes that were not mentioned earlier in the list (so
putting t in the end of the list means "otherwise, enable").  Similarly
for the "none" alternatives: a value of nil ("Don't enable...") means we
never enable the minor mode, while the effect of a nil inside the list
is relative to the previous elements, and means "otherwise, don't".
Does that make sense?

bug#70589: [PATCH] Refine the Custom type of generated '*-modes' options

[Eli Zaretskii]

> From: Eshel Yaron <me@eshelyaron.com>
> Cc: 70589@debbugs.gnu.org,  Stefan Monnier <monnier@iro.umontreal.ca>
> Date: Fri, 26 Apr 2024 13:13:37 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Why does it make sense to have the "all" and "none" alternatives
> > twice?
> 
> These are slightly different: a value of t ("Enable in all major modes")
> says to enable everywhere, while a t inside a list ("All major modes")
> says to enable in modes that were not mentioned earlier in the list (so
> putting t in the end of the list means "otherwise, enable").  Similarly
> for the "none" alternatives: a value of nil ("Don't enable...") means we
> never enable the minor mode, while the effect of a nil inside the list
> is relative to the previous elements, and means "otherwise, don't".
> Does that make sense?

It might make sense, if we change the labels to say what you tell
above.

Thanks.

bug#70589: [PATCH] Refine the Custom type of generated '*-modes' options

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

[-- Attachment #1: Type: text/plain, Size: 1015 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Eshel Yaron <me@eshelyaron.com>
>> Cc: 70589@debbugs.gnu.org,  Stefan Monnier <monnier@iro.umontreal.ca>
>> Date: Fri, 26 Apr 2024 13:13:37 +0200
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > Why does it make sense to have the "all" and "none" alternatives
>> > twice?
>> 
>> These are slightly different: a value of t ("Enable in all major modes")
>> says to enable everywhere, while a t inside a list ("All major modes")
>> says to enable in modes that were not mentioned earlier in the list (so
>> putting t in the end of the list means "otherwise, enable").  Similarly
>> for the "none" alternatives: a value of nil ("Don't enable...") means we
>> never enable the minor mode, while the effect of a nil inside the list
>> is relative to the previous elements, and means "otherwise, don't".
>> Does that make sense?
>
> It might make sense, if we change the labels to say what you tell
> above.
>
> Thanks.

OK, here's an updated patch with revised labels:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: v2-0001-Refine-the-Custom-type-of-generated-modes-options.patch --]
[-- Type: text/x-patch, Size: 1947 bytes --]

From 88ad06894cba8cfaf7b7c1916449f9f10e1cb6e7 Mon Sep 17 00:00:00 2001
From: Eshel Yaron <me@eshelyaron.com>
Date: Fri, 26 Apr 2024 11:43:29 +0200
Subject: [PATCH v2] Refine the Custom type of generated '*-modes' options

* lisp/emacs-lisp/easy-mmode.el (define-globalized-minor-mode):
Refine the Custom type of the '*-modes' option, generated when
this macro is given a ':predicate' argument.  (Bug#70589)
---
 lisp/emacs-lisp/easy-mmode.el | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/lisp/emacs-lisp/easy-mmode.el b/lisp/emacs-lisp/easy-mmode.el
index eaad9646985..081619002c8 100644
--- a/lisp/emacs-lisp/easy-mmode.el
+++ b/lisp/emacs-lisp/easy-mmode.el
@@ -580,7 +580,19 @@ define-globalized-minor-mode
 and nil means \"don't use\".  There's an implicit nil at the end of the
 list."
                       mode)
-             :type '(repeat sexp)
+             :type '(choice (const :tag "Enable in all major modes" t)
+                            (const :tag "Don't enable in any major mode" nil)
+                            (repeat :tag "Rules (earlier takes precedence)..."
+                                    (choice
+                                     (const :tag "By default, enable" t)
+                                     (const :tag "By default, don't enable" nil)
+                                     (symbol :value fundamental-mode
+                                             :tag "Enable in major mode")
+                                     (cons :tag "Don't enable in major modes"
+                                           (const :tag "Don't enable in..." not)
+                                           (repeat
+                                            (symbol :value fundamental-mode
+                                                    :tag "Major mode"))))))
              ,@group))
 
        ;; Autoloading define-globalized-minor-mode autoloads everything
-- 
2.44.0

bug#70589: [PATCH] Refine the Custom type of generated '*-modes' options

[Eli Zaretskii]

> From: Eshel Yaron <me@eshelyaron.com>
> Cc: 70589@debbugs.gnu.org,  monnier@iro.umontreal.ca
> Date: Fri, 26 Apr 2024 14:24:42 +0200
> 
> >> These are slightly different: a value of t ("Enable in all major modes")
> >> says to enable everywhere, while a t inside a list ("All major modes")
> >> says to enable in modes that were not mentioned earlier in the list (so
> >> putting t in the end of the list means "otherwise, enable").  Similarly
> >> for the "none" alternatives: a value of nil ("Don't enable...") means we
> >> never enable the minor mode, while the effect of a nil inside the list
> >> is relative to the previous elements, and means "otherwise, don't".
> >> Does that make sense?
> >
> > It might make sense, if we change the labels to say what you tell
> > above.
> >
> > Thanks.
> 
> OK, here's an updated patch with revised labels:

Thanks, but I find the likes of "By default, enable" not clear enough.
Would it be accurate to say this instead:

                        (const :tag "Enable in any other mode" t)
                        (const :tag "Don't enable in the other modes" nil)

bug#70589: [PATCH] Refine the Custom type of generated '*-modes' options

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Eshel Yaron <me@eshelyaron.com>
>> Cc: 70589@debbugs.gnu.org,  monnier@iro.umontreal.ca
>> Date: Fri, 26 Apr 2024 14:24:42 +0200
>>
>> >> These are slightly different: a value of t ("Enable in all major modes")
>> >> says to enable everywhere, while a t inside a list ("All major modes")
>> >> says to enable in modes that were not mentioned earlier in the list (so
>> >> putting t in the end of the list means "otherwise, enable").  Similarly
>> >> for the "none" alternatives: a value of nil ("Don't enable...") means we
>> >> never enable the minor mode, while the effect of a nil inside the list
>> >> is relative to the previous elements, and means "otherwise, don't".
>> >> Does that make sense?
>> >
>> > It might make sense, if we change the labels to say what you tell
>> > above.
>> >
>> > Thanks.
>>
>> OK, here's an updated patch with revised labels:
>
> Thanks, but I find the likes of "By default, enable" not clear enough.
> Would it be accurate to say this instead:
>
>                         (const :tag "Enable in any other mode" t)
>                         (const :tag "Don't enable in the other modes" nil)

That could also work, but I think this wording is a bit confusing when
entering the first list element (so there are no "other mode/s" yet).
If you think it's nevertheless preferable, I don't object.

bug#70589: [PATCH] Refine the Custom type of generated '*-modes' options

[Eli Zaretskii]

> From: Eshel Yaron <me@eshelyaron.com>
> Cc: 70589@debbugs.gnu.org,  monnier@iro.umontreal.ca
> Date: Fri, 26 Apr 2024 18:34:20 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Eshel Yaron <me@eshelyaron.com>
> >> Cc: 70589@debbugs.gnu.org,  monnier@iro.umontreal.ca
> >> Date: Fri, 26 Apr 2024 14:24:42 +0200
> >>
> >> >> These are slightly different: a value of t ("Enable in all major modes")
> >> >> says to enable everywhere, while a t inside a list ("All major modes")
> >> >> says to enable in modes that were not mentioned earlier in the list (so
> >> >> putting t in the end of the list means "otherwise, enable").  Similarly
> >> >> for the "none" alternatives: a value of nil ("Don't enable...") means we
> >> >> never enable the minor mode, while the effect of a nil inside the list
> >> >> is relative to the previous elements, and means "otherwise, don't".
> >> >> Does that make sense?
> >> >
> >> > It might make sense, if we change the labels to say what you tell
> >> > above.
> >> >
> >> > Thanks.
> >>
> >> OK, here's an updated patch with revised labels:
> >
> > Thanks, but I find the likes of "By default, enable" not clear enough.
> > Would it be accurate to say this instead:
> >
> >                         (const :tag "Enable in any other mode" t)
> >                         (const :tag "Don't enable in the other modes" nil)
> 
> That could also work, but I think this wording is a bit confusing when
> entering the first list element (so there are no "other mode/s" yet).
> If you think it's nevertheless preferable, I don't object.

Feel free to suggest a better wording, that caters also to that
special situation.

bug#70589: [PATCH] Refine the Custom type of generated '*-modes' options

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

Eli Zaretskii <eliz@gnu.org> writes:

>> > Thanks, but I find the likes of "By default, enable" not clear enough.
>> > Would it be accurate to say this instead:
>> >
>> >                         (const :tag "Enable in any other mode" t)
>> >                         (const :tag "Don't enable in the other modes" nil)
>>
>> That could also work, but I think this wording is a bit confusing when
>> entering the first list element (so there are no "other mode/s" yet).
>> If you think it's nevertheless preferable, I don't object.
>
> Feel free to suggest a better wording, that caters also to that
> special situation.

Hmm, how about "Enable in all (other) modes" and "Don't enable in any
(other) mode"?

bug#70589: [PATCH] Refine the Custom type of generated '*-modes' options

[Eli Zaretskii]

> From: Eshel Yaron <me@eshelyaron.com>
> Cc: 70589@debbugs.gnu.org,  monnier@iro.umontreal.ca
> Date: Fri, 26 Apr 2024 19:55:05 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> > Thanks, but I find the likes of "By default, enable" not clear enough.
> >> > Would it be accurate to say this instead:
> >> >
> >> >                         (const :tag "Enable in any other mode" t)
> >> >                         (const :tag "Don't enable in the other modes" nil)
> >>
> >> That could also work, but I think this wording is a bit confusing when
> >> entering the first list element (so there are no "other mode/s" yet).
> >> If you think it's nevertheless preferable, I don't object.
> >
> > Feel free to suggest a better wording, that caters also to that
> > special situation.
> 
> Hmm, how about "Enable in all (other) modes" and "Don't enable in any
> (other) mode"?

Fine with me, thanks.

bug#70589: [PATCH] Refine the Custom type of generated '*-modes' options

[Eli Zaretskii]

> Cc: 70589@debbugs.gnu.org
> Date: Fri, 26 Apr 2024 13:56:50 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> 
> > Date: Fri, 26 Apr 2024 11:49:19 +0200
> > From:  Eshel Yaron via "Bug reports for GNU Emacs,
> >  the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org>
> > 
> > This makes it easier for users to customize generated '*-modes' options,
> > such as 'global-completion-preview-modes', via the Custom interface.
> 
> This kind of discussion is not useful with Stefan (CC'ed) on board.
                                        ^^^^
Oops, I meant "without", of course.  Sorry.

bug#70589: [PATCH] Refine the Custom type of generated '*-modes' options

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> > This makes it easier for users to customize generated '*-modes' options,
>> > such as 'global-completion-preview-modes', via the Custom interface.
>> 
>> This kind of discussion is not useful with Stefan (CC'ed) on board.
>                                         ^^^^
> Oops, I meant "without", of course.  Sorry.

🙂

FWIW, the latest patch looks fine to me, but really this is Lars's code,
so you don't need me "on board", really.

Also, while I'm here: I think part of the "problem" is in the semantics:
anything after a t or nil is ignored, which is hard to explain via
a "single small line" of text.

We could imagine extending the semantics so that you can use `((not a)
. t)` instead, in which case this meaning of `t` can be shared with the
"main one" (instead of having two `t` cases and two `nil` cases) in the
sense that the syntax would be:

    PRED can be:
    t                        Always accept
    nil                      Always decline
    ((not . MODES) . PRED)   Decline if in MODES, otherwise like PRED
    (MODE . PRED)            Accept if in MODE, otherwise like PRED

but then we couldn't use the `repeat` Custom type and instead we'd have
to implement an ad-hoc type for it, I believe.


        Stefan

bug#70589: [PATCH] Refine the Custom type of generated '*-modes' options

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

close 70589 30.1
quit

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> > This makes it easier for users to customize generated '*-modes' options,
>>> > such as 'global-completion-preview-modes', via the Custom interface.
>>>
>>> This kind of discussion is not useful with Stefan (CC'ed) on board.
>>                                         ^^^^
>> Oops, I meant "without", of course.  Sorry.
>
> 🙂
>
> FWIW, the latest patch looks fine to me,

Thanks, so I've now pushed it to master.  I'm not closing the bug yet,
in favor of your additional suggestion below.

> but really this is Lars's code, so you don't need me "on board", really.
>
> Also, while I'm here: I think part of the "problem" is in the semantics:
> anything after a t or nil is ignored, which is hard to explain via
> a "single small line" of text.

Agreed.  Although it's not such a big problem IMO, since the Customize
buffer also shows the docstring of the option, which explains the
semantics pretty clearly.

> We could imagine extending the semantics so that you can use `((not a)
> . t)` instead, in which case this meaning of `t` can be shared with the
> "main one" (instead of having two `t` cases and two `nil` cases) in the
> sense that the syntax would be:
>
>     PRED can be:
>     t                        Always accept
>     nil                      Always decline
>     ((not . MODES) . PRED)   Decline if in MODES, otherwise like PRED
>     (MODE . PRED)            Accept if in MODE, otherwise like PRED
>

That sounds neat.

> but then we couldn't use the `repeat` Custom type and instead we'd have
> to implement an ad-hoc type for it, I believe.

I think that's right.

bug#70589: [PATCH] Refine the Custom type of generated '*-modes' options

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

reopen 70589
quit

Eshel Yaron writes:

> Thanks, so I've now pushed it to master.  I'm not closing the bug yet,
> in favor of your additional suggestion below.

Sorry, I mistakenly closed the bug nonetheless.  Reopening.

>> but really this is Lars's code, so you don't need me "on board", really.
>>
>> Also, while I'm here: I think part of the "problem" is in the semantics:
>> anything after a t or nil is ignored, which is hard to explain via
>> a "single small line" of text.
>
> Agreed.  Although it's not such a big problem IMO, since the Customize
> buffer also shows the docstring of the option, which explains the
> semantics pretty clearly.
>
>> We could imagine extending the semantics so that you can use `((not a)
>> . t)` instead, in which case this meaning of `t` can be shared with the
>> "main one" (instead of having two `t` cases and two `nil` cases) in the
>> sense that the syntax would be:
>>
>>     PRED can be:
>>     t                        Always accept
>>     nil                      Always decline
>>     ((not . MODES) . PRED)   Decline if in MODES, otherwise like PRED
>>     (MODE . PRED)            Accept if in MODE, otherwise like PRED
>>
>
> That sounds neat.
>
>> but then we couldn't use the `repeat` Custom type and instead we'd have
>> to implement an ad-hoc type for it, I believe.
>
> I think that's right.

bug#70589: [PATCH] Refine the Custom type of generated '*-modes' options

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> Thanks, so I've now pushed it to master.  I'm not closing the bug yet,
>> in favor of your additional suggestion below.
> Sorry, I mistakenly closed the bug nonetheless.  Reopening.

FWIW, I think we can close it: the bug was about improving the Custom
type and that's done.


        Stefan

bug#70589: [PATCH] Refine the Custom type of generated '*-modes' options

[Eshel Yaron via Bug reports for GNU Emacs, the Swiss army knife of text editors]

close 70589 30.1
quit

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>>> Thanks, so I've now pushed it to master.  I'm not closing the bug yet,
>>> in favor of your additional suggestion below.
>> Sorry, I mistakenly closed the bug nonetheless.  Reopening.
>
> FWIW, I think we can close it: the bug was about improving the Custom
> type and that's done.

All right then, closing again :)