bug#54161: 27.2; `define-minor-mode' with alist of key bindings

bug#54161: 27.2; `define-minor-mode' with alist of key bindings

[Drew Adams]

Apologies for this bug report, as I imagine it must be a duplicate, but
I couldn't find the dup.

And I imagine that there's no bug in behavior, and I'm just
misunderstanding the doc.  (The behavior is longstanding across Emacs
releases.)

In that case, maybe the doc could benefit from some rewording?  I've
reread it a few times now, and I haven't figured out what I'm
misreading.

emacs -Q

These, and similar variants (e.g. using keyword :keymap) produce a
keymap that defines a binding for command `forward-char' with prefix key
`C', followed by `-', followed by `o'.  I would expect them to instead
bind the command to key `C-o'.

(define-minor-mode tata-mode
  "TATA MODE" nil nil '(("\\C-o" . forward-char)))

(define-minor-mode titi-mode
  "TITI MODE" nil nil '(((kbd "C-o") . forward-char)))

Digging into the expansion of `define-minor-mode' I see that those sexps
expand these sexps to produce the keymaps:

(easy-mmode-define-keymap '(("\\C-o" . forward-char)))
(easy-mmode-define-keymap '(((kbd "C-o") . forward-char)))

And those produce this keymap:

(keymap (67 keymap (45 keymap (111 . forward-char))))

That is,

(keymap (?C keymap (?- keymap (?o . forward-char))))

The doc (both Elisp manual and doc string) says this:

 The optional argument KEYMAP specifies the keymap for the minor
 mode.  If non-'nil', it should be a variable name (whose value is a
 keymap), a keymap, or an alist of the form

      (KEY-SEQUENCE . DEFINITION)

 where each KEY-SEQUENCE and DEFINITION are arguments suitable for
 passing to 'define-key'.

I think that's the case in these examples, no?  Both (kbd "C-o") and
"\C-o" are suitable args for `define-key'.

What am I missing?

I searched the Elisp sources and noticed only one occurrence of using an
explicit alist, in refill.el:

 :keymap '(("\177" . backward-delete-char-untabify))

And indeed, if I use this, which has a literal Control-O character,
there's no problem (key `C-o' is bound to `forward-char):

(define-minor-mode toto-mode
  "TOTO MODE" nil nil '(("^O" . forward-char)))

(The Control-O char won't pass through email, so I've substituted the
string "^O", but it is actually a string with just a Control-O char.)

In GNU Emacs 27.2 (build 1, x86_64-w64-mingw32)
 of 2021-03-26 built on CIRROCUMULUS
Repository revision: deef5efafb70f4b171265b896505b92b6eef24e6
Repository branch: HEAD
Windowing system distributor 'Microsoft Corp.', version 10.0.19043
System Description: Microsoft Windows 10 Pro (v10.0.2009.19043.1526)

bug#54161: 27.2; `define-minor-mode' with alist of key bindings

[Gilles]

Hi Drew,

I don't understand the documentation in the same way. I don't think
the documentation is wrong, but it could always use a few examples.

> > where each KEY-SEQUENCE and DEFINITION are arguments suitable for
> > passing to 'define-key'.
> I think that's the case in these examples, no?  Both (kbd "C-o") and
> "\C-o" are suitable args for `define-key'.

The Lisp object (kbd "C-o") (a two-element list) is not a suitable
argument for define-key. The Lisp *expression* (kbd "C-o") *returns* a
suitable argument for define-key.

(define-minor-mode titi-mode
  "TITI MODE" nil nil '(((kbd "C-o") . forward-char)))
(define-minor-mode tata-mode
  "TATA MODE" nil nil '(("\\C-o" . forward-char)))

I would in fact expect titi to result in an error, since a list whose
car is the symbol kbd is not a valid key. But this seems to work, as I
would expect it to:

(define-minor-mode tutu-mode
  "TUTU MODE" nil nil `((,(kbd "C-o") . forward-char)))

As for tata, it binds the four-character sequence {backslash, capital
C, dash, lowercase o}. To bind C-o, you need to pass the one-character
string "\C-o".

-- 
Gilles

bug#54161: 27.2; `define-minor-mode' with alist of key bindings

[Drew Adams]

> The doc (both Elisp manual and doc string) says this:
> 
>  The optional argument KEYMAP specifies the keymap for the minor
>  mode.  If non-'nil', it should be a variable name (whose value is a
>  keymap), a keymap, or an alist of the form
> 
>       (KEY-SEQUENCE . DEFINITION)
> 
>  where each KEY-SEQUENCE and DEFINITION are arguments suitable for
>  passing to 'define-key'.
> 
> I think that's the case in these examples, no?  Both (kbd "C-o") and
> "\C-o" are suitable args for `define-key'.
> 
> What am I missing?

I guess I understand it now.  I think the language
about something "suitable for passing" to `define-key'
is what's misleading, and confused me.  Usually we
speak instead of an argument that is "acceptable to
function ____".  "Passing" suggests an unevaluated
value you're passing, which is then evaluated as the
actual argument for the function.

But even that wouldn't be as clear as this could be.
I'd suggest making it very clear somehow, that neither
KEY-SEQUENCE nor DEFINITION gets evaluated.  Maybe add
a short example.

In any case, you can close this bug, if you like.
Consider it user feedback of something I found confusing.

bug#54161: 27.2; `define-minor-mode' with alist of key bindings

[Drew Adams]

BTW, note that this is not a problem:


(define-minor-mode tata-mode
  "TATA MODE" nil nil '(((kbd ">") . forward-char)))

The key `>' gets defined, no problem. Even though
`(kbd ">")' is actually a list, not a key sequence.

But this is a problem (as expected, once it's
understood that the `(kbd "C->")' doesn't get
evaluated):

(define-minor-mode tata-mode
  "TATA MODE" nil nil '(((kbd "C->") . forward-char)))

Why isn't there the same problem with `(kbd ">")'?

The behavior is a bit confusing.  All the more reason
for making sure the doc can't mislead.

bug#54161: [External] : bug#54161: 27.2; `define-minor-mode' with alist of key bindings

[Drew Adams]

> > > where each KEY-SEQUENCE and DEFINITION are arguments suitable for
> > > passing to 'define-key'.

As a starter, it would be better to say "arguments suitable
for `define-key', and not speak of what you "pass" to it.

> > I think that's the case in these examples, no?  Both (kbd "C-o") and
> > "\C-o" are suitable args for `define-key'.
> 
> The Lisp object (kbd "C-o") (a two-element list) is not a suitable
> argument for define-key. The Lisp *expression* (kbd "C-o") *returns*
> a suitable argument for define-key.

Sure.  It depends on how one reads "passing" an arg.  And
notice that you wrote "suitable argument for define-key,
which already shifts the focus to what the function accepts
and not to what you "pass" it.

A user writes an argument expression in a function-application
expression "(define-key...)".  Lisp evals each argument
expression and applies the function that's the car of the
overall expression to the evaluated arg expressions.

The function receives Lisp objects. The user writes an
expression - especially in the typical case of using
`define-key' or `:keymap'.

It's easy to read "passing" as being about the expressions
you write, even if that's not ultimately all that's involved.
And especially for something like `define-key' and arg
expressions like (kbd...).

The odd "tolerance" (if that's what it is) of (kbd "<")
confuses things further wrt the behavior.  As agreed in
the source Q&A in emacs.SE, neither of us understands why
the same error is NOT raised for (kbd "<") as is raised
for (kbd "C-<").  In both cases if the alist arg to
:keymap is quoted then what gets passed is a 2-element
list with car `kbd'.  Why does (kbd "<") work?  That can
lead (did lead) to the confusion about (kbd "C-<").

"Something you can pass to define-key" can mislead, even
if correct when viewed right.  This should be worded in
some less ambiguous way in the doc.  It's possible to think
of "passing" (kbd "C->") to define-key - it all depends on
how one interprets "passing" something to a function.

An example in the doc would help, along with speaking of
something "acceptable to `define-key' as an arg - something
such as what `kbd returns."  Putting the emphasis on what
the function accepts rather than on what you "pass" can tend
to shift the focus from an expression you write to the result
of its evaluation, which is what the function receives.

Yes, the function is passed the result of evaluating the
sexp.  But it's easy to think of what you write as being
what you "pass" to the function.  Especially if you try
"passing" (writing) "(kbd ">")" and no error is raised.
It's the inconsistency that misled, and made the user
think that there was a particular problem with (kbd "C->"). 

bug#54161: 27.2; `define-minor-mode' with alist of key bindings

[Lars Ingebrigtsen]

Gilles <gilles.usenet@gmail.com> writes:

> I would in fact expect titi to result in an error, since a list whose
> car is the symbol kbd is not a valid key. But this seems to work, as I
> would expect it to:
>
> (define-minor-mode tutu-mode
>   "TUTU MODE" nil nil `((,(kbd "C-o") . forward-char)))
>
> As for tata, it binds the four-character sequence {backslash, capital
> C, dash, lowercase o}. To bind C-o, you need to pass the one-character
> string "\C-o".

So I don't think there's anything to fix here, and I'm closing this bug
report.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#54161: 27.2; `define-minor-mode' with alist of key bindings

[Michael Heerdegen]

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

Gilles <gilles.usenet@gmail.com> writes:

> The Lisp *expression* (kbd "C-o") *returns* a suitable argument for
> define-key.

I think the small source of confusion is whether these arguments are
evaluated.  How about making that clearer, e.g.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-WIP-Fix-54161.patch --]
[-- Type: text/x-diff, Size: 1366 bytes --]

From 73283a3a37a7df41a0a1ab9d88bcf31704e9e841 Mon Sep 17 00:00:00 2001
From: Michael Heerdegen <michael_heerdegen@web.de>
Date: Tue, 1 Mar 2022 02:13:14 +0100
Subject: [PATCH] WIP: Fix #54161

---
 lisp/emacs-lisp/easy-mmode.el | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lisp/emacs-lisp/easy-mmode.el b/lisp/emacs-lisp/easy-mmode.el
index 688c76e0c5..b35be58b3a 100644
--- a/lisp/emacs-lisp/easy-mmode.el
+++ b/lisp/emacs-lisp/easy-mmode.el
@@ -168,9 +168,9 @@ define-minor-mode
                 If non-nil, it should be an unquoted variable name (whose value
                 is a keymap), or an expression that returns either a keymap or
 		a list of (KEY . BINDING) pairs where KEY and BINDING are
-		suitable for `define-key'.  If you supply a KEYMAP argument
-		that is not a symbol, this macro defines the variable MODE-map
-		and gives it the value that KEYMAP specifies.
+		suitable values for `define-key'.  If you supply a KEYMAP
+		argument that is not a symbol, this macro defines the
+		variable MODE-map and gives it the value that KEYMAP specifies.
 :interactive VAL  Whether this mode should be a command or not.  The default
                 is to make it one; use nil to avoid that.  If VAL is a list,
                 it's interpreted as a list of major modes this minor mode
--
2.30.2


[-- Attachment #3: Type: text/plain, Size: 93 bytes --]


BTW, should we fix the mixed indentation style in these lines (tabs
vs. spaces)?

Michael.

bug#54161: 27.2; `define-minor-mode' with alist of key bindings

[Lars Ingebrigtsen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> I think the small source of confusion is whether these arguments are
> evaluated.  How about making that clearer, e.g.

Looks OK to me.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#54161: [External] : bug#54161: 27.2; `define-minor-mode' with alist of key bindings

[Drew Adams]

> I think the small source of confusion is whether these arguments are
> evaluated.  

Yes, exactly.

> How about making that clearer, e.g.

That helps quite a bit, yes.

It kind of relies on a careful reading - the only
difference is that you've added "values" in "where
KEY and BINDING are suitable values for `define-key'."
                             ^^^^^^
But that's probably enough.  I had suggested this:

 > An example in the doc would help, along with
 > speaking of something "acceptable to `define-key'
 > as an arg - something such as what `kbd returns."  
___

The other problem reported in this bug is that an
(unquoted) `(kbd ">")' does NOT raise an error
(unlike, say, `(kbd "C->")').

Neither I nor Gilles understands why no error is
raised in the case of unquoted `(kbd ">")'.  And it
was that that introduced confusion in the case at
hand.  The user had written this:

(define-minor-mode narrative-mode
  "..."
  :lighter " narr"
  :keymap
  '(((kbd "<") . electric-left-angle)  ; <== No error
    ((kbd ">") . electric-right-angle) ; <== No error
    ((kbd "C-<") . quoted-insert-left-angle-bracket)
    ((kbd "C->") . quoted-insert-right-angle-bracket)))

https://emacs.stackexchange.com/q/70714/105

The fact that no error is raised for the first
two of those bindings led to mistakenly thinking
that there was something wrong with the latter
two `kbd' sexps, rather than seeing that the
problem was not evaluating any of the `kbd' sexps.

I would have liked to get some explanation of
this in this bug thread.  Plus a fix for the
behavior in the case of the first two bindings,
if that behavior is in fact bugged.

Alas, no consideration of this problem at all
(whether it represents an Emacs behavior bug or
just our lack of understanding).

bug#54161: [External] : bug#54161: 27.2; `define-minor-mode' with alist of key bindings

[Drew Adams]

> > I think the small source of confusion is whether these arguments are
> > evaluated.  How about making that clearer, e.g.
> 
> Looks OK to me.

You said there was no bug - nothing to fix.
Guess it depends on who reports there's a
problem to fix.  Anyway, thanks to Michael
for persisting.

bug#54161: [External] : bug#54161: 27.2; `define-minor-mode' with alist of key bindings

[Michael Heerdegen]

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

> > > I think the small source of confusion is whether these arguments are
> > > evaluated.  How about making that clearer, e.g.
> >
> > Looks OK to me.
>
> You said there was no bug - nothing to fix.  Guess it depends on who
> reports there's a problem to fix.

Luckily, even if that's maybe not always absolutely wrong, it's not a
big problem since there are always more eyes to look at the reports.

But yes Lars, would be good if people who normally read emacs-bug daily
have a chance to look at new reports before they are already closed.
The amount of work you are doing is absolutely impressive and I will at
no single day come close to that - but you are not totally alone with
that work either.

Michael.

bug#54161: [External] : bug#54161: 27.2; `define-minor-mode' with alist of key bindings

[Lars Ingebrigtsen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

>> You said there was no bug - nothing to fix.  Guess it depends on who
>> reports there's a problem to fix.

The text was fine before, and it's fine after the change, too.

> But yes Lars, would be good if people who normally read emacs-bug daily
> have a chance to look at new reports before they are already closed.

I close bugs that I don't think needs further work.  If you disagree
with that, you can still work on the bugs, of course -- whether the bug
is "closed" or not doesn't make a difference.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#54161: [External] : bug#54161: 27.2; `define-minor-mode' with alist of key bindings

[Michael Heerdegen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> I close bugs that I don't think needs further work.  If you disagree
> with that, you can still work on the bugs, of course -- whether the bug
> is "closed" or not doesn't make a difference.

Yes, and I could continue to work on it if it was removed completely
from the bug tracker as well.  This is the same problem I suffered from
in the discussion with Eli: I simply feel hasted.

It somewhat has become a task to "rescue" single bug reports that didn't
get enough attention.  Why the haste?  It is probably more exhausting
for me to look through the closed bug reports to find important stuff
than it is for you to look through all of them at first.

In the discussion with Eli, I "rescued" Bug#14582.  Really, I don't have
any knowlegde about that matter.  I just intervened because I found it is
important to be handled.

You said it should be closed.  Eli then said it should be closed,
several times, for different reasons every time.  It was my task to
withstand the urge to close, and I had not really arguments because I
don't have enough knowlegde in that field.  In such cases I feel urged
to learn about that stuff and find arguments, in a short period of time,
in a field I don't have knowlegde in.  This distracts me from doing
other, more useful work for Emacs, and is really frustrating and
demotivating.  I can't be productive that way.  I end up having dozens
of half baked commits in my pipeline, but I get distracted by the next
report to be closed, I forget about the older things, and it becomes
harder to finish something.

So, please; I totally understand you want to get things done.  But not
everybody can keep up with your working speed.


Michael.

bug#54161: [External] : bug#54161: 27.2; `define-minor-mode' with alist of key bindings

[Michael Heerdegen]

Michael Heerdegen <michael_heerdegen@web.de> writes:

> So, please; I totally understand you want to get things done.  But not
> everybody can keep up with your working speed.

And I'll try to commit my patch tomorrow.  Sorry if my reply was
emotional, I barely slept last night.  No offense intended.

Michael.