bug#61149: 29.0.60; keymap-local-set rejects key sequences

bug#61149: 29.0.60; keymap-local-set rejects key sequences

[Eli Zaretskii]

To reproduce:

  emacs -Q
  M-x keymap-local-set
  C-c .
  forward-line RET

Observe the error message:

  [3 46] is not a valid key definition; see ‘key-valid-p’

(The error message comes from keymap--check.)

To type the key sequence, I actually pressed Ctrl and typed 'c', then
released Ctrl and typed '.'.  Isn't that what one is supposed to use?

So the above basically makes keymap-local-set useless, or am I missing
something?  The following does work:

  M-: (keymap-local-set "C-c ." 'forward-line) RET

However, I could find no way of typing the string "C-c ." at the
prompt of keymap-local-set.


In GNU Emacs 29.0.60 (build 259, i686-pc-mingw32) of 2023-01-29 built on
 HOME-C4E4A596F7
Repository revision: b73539832d9c4e802925cb8f261a13473da383b3
Repository branch: emacs-29
Windowing system distributor 'Microsoft Corp.', version 5.1.2600
System Description: Microsoft Windows XP Service Pack 3 (v5.1.0.2600)

Configured using:
 'configure -C --prefix=/d/usr --with-wide-int
 --enable-checking=yes,glyphs 'CFLAGS=-O0 -gdwarf-4 -g3''

Configured features:
ACL GIF GMP GNUTLS HARFBUZZ JPEG JSON LCMS2 LIBXML2 MODULES NOTIFY
W32NOTIFY PDUMPER PNG RSVG SOUND SQLITE3 THREADS TIFF
TOOLKIT_SCROLL_BARS TREE_SITTER WEBP XPM ZLIB

Important settings:
  value of $LANG: ENU
  locale-coding-system: cp1255

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message mailcap yank-media puny dired
dired-loaddefs rfc822 mml mml-sec password-cache epa derived epg rfc6068
epg-config gnus-util text-property-search time-date subr-x mm-decode
mm-bodies mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader
cl-loaddefs cl-lib sendmail rfc2047 rfc2045 ietf-drums mm-util
mail-prsvr mail-utils rmc iso-transl tooltip cconv eldoc paren electric
uniquify ediff-hook vc-hooks lisp-float-type elisp-mode mwheel dos-w32
ls-lisp disp-table term/w32-win w32-win w32-vars term/common-win
tool-bar dnd fontset image regexp-opt fringe tabulated-list replace
newcomment text-mode lisp-mode prog-mode register page tab-bar menu-bar
rfn-eshadow isearch easymenu timer select scroll-bar mouse jit-lock
font-lock syntax font-core term/tty-colors frame minibuffer nadvice seq
simple cl-generic indonesian philippine cham georgian utf-8-lang
misc-lang vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms
cp51932 hebrew greek romanian slovak czech european ethiopic indian
cyrillic chinese composite emoji-zwj charscript charprop case-table
epa-hook jka-cmpr-hook help abbrev obarray oclosure cl-preloaded button
loaddefs theme-loaddefs faces cus-face macroexp files window
text-properties overlay sha1 md5 base64 format env code-pages mule
custom widget keymap hashtable-print-readable backquote threads
w32notify w32 lcms2 multi-tty make-network-process emacs)

Memory information:
((conses 16 42883 10204)
 (symbols 48 6278 0)
 (strings 16 16568 2962)
 (string-bytes 1 399068)
 (vectors 16 9325)
 (vector-slots 8 146723 13332)
 (floats 8 24 24)
 (intervals 40 293 123)
 (buffers 888 11))

bug#61149: 29.0.60; keymap-local-set rejects key sequences

[Stephen Berman]

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

On Sun, 29 Jan 2023 15:44:44 +0200 Eli Zaretskii <eliz@gnu.org> wrote:

> To reproduce:
>
>   emacs -Q
>   M-x keymap-local-set
>   C-c .
>   forward-line RET
>
> Observe the error message:
>
>   [3 46] is not a valid key definition; see ‘key-valid-p’
>
> (The error message comes from keymap--check.)
>
> To type the key sequence, I actually pressed Ctrl and typed 'c', then
> released Ctrl and typed '.'.  Isn't that what one is supposed to use?
>
> So the above basically makes keymap-local-set useless, or am I missing
> something?  The following does work:
>
>   M-: (keymap-local-set "C-c ." 'forward-line) RET
>
> However, I could find no way of typing the string "C-c ." at the
> prompt of keymap-local-set.

This patch seems to DTRT:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: keymap-local-set patch --]
[-- Type: text/x-patch, Size: 587 bytes --]

diff --git a/lisp/keymap.el b/lisp/keymap.el
index 791221f2459..9643dce5f9c 100644
--- a/lisp/keymap.el
+++ b/lisp/keymap.el
@@ -95,7 +95,8 @@ keymap-local-set
 cases is shared with all other buffers in the same major mode."
   (declare (compiler-macro (lambda (form) (keymap--compile-check key) form)))
   (interactive "KSet key locally: \nCSet key %s locally to command: ")
-  (let ((map (current-local-map)))
+  (let ((map (current-local-map))
+	(key (key-description key)))
     (unless map
       (use-local-map (setq map (make-sparse-keymap))))
     (keymap-set map key command)))

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


Steve Berman

bug#61149: 29.0.60; keymap-local-set rejects key sequences

[Stephen Berman]

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

On Sun, 29 Jan 2023 15:35:22 +0100 Stephen Berman <stephen.berman@gmx.net> wrote:

> On Sun, 29 Jan 2023 15:44:44 +0200 Eli Zaretskii <eliz@gnu.org> wrote:
>
>> To reproduce:
>>
>>   emacs -Q
>>   M-x keymap-local-set
>>   C-c .
>>   forward-line RET
>>
>> Observe the error message:
>>
>>   [3 46] is not a valid key definition; see ‘key-valid-p’
>>
>> (The error message comes from keymap--check.)
>>
>> To type the key sequence, I actually pressed Ctrl and typed 'c', then
>> released Ctrl and typed '.'.  Isn't that what one is supposed to use?
>>
>> So the above basically makes keymap-local-set useless, or am I missing
>> something?  The following does work:
>>
>>   M-: (keymap-local-set "C-c ." 'forward-line) RET
>>
>> However, I could find no way of typing the string "C-c ." at the
>> prompt of keymap-local-set.
>
> This patch seems to DTRT:
>
> diff --git a/lisp/keymap.el b/lisp/keymap.el
> index 791221f2459..9643dce5f9c 100644
> --- a/lisp/keymap.el
> +++ b/lisp/keymap.el
> @@ -95,7 +95,8 @@ keymap-local-set
>  cases is shared with all other buffers in the same major mode."
>    (declare (compiler-macro (lambda (form) (keymap--compile-check key) form)))
>    (interactive "KSet key locally: \nCSet key %s locally to command: ")
> -  (let ((map (current-local-map)))
> +  (let ((map (current-local-map))
> +	(key (key-description key)))
>      (unless map
>        (use-local-map (setq map (make-sparse-keymap))))
>      (keymap-set map key command)))

... but not in your non-interactive case; so maybe this instead:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: keymap-local-set patch --]
[-- Type: text/x-patch, Size: 438 bytes --]

diff --git a/lisp/keymap.el b/lisp/keymap.el
index 791221f2459..48ec91d03c8 100644
--- a/lisp/keymap.el
+++ b/lisp/keymap.el
@@ -98,6 +98,8 @@ keymap-local-set
   (let ((map (current-local-map)))
     (unless map
       (use-local-map (setq map (make-sparse-keymap))))
+    (unless (stringp key)
+      (setq key (key-description key)))
     (keymap-set map key command)))

 (defun keymap-global-unset (key &optional remove)

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


Steve Berman

bug#61149: 29.0.60; keymap-local-set rejects key sequences

[Robert Pluim]

>>>>> On Sun, 29 Jan 2023 16:28:44 +0100, Stephen Berman <stephen.berman@gmx.net> said:

    Stephen> ... but not in your non-interactive case; so maybe this instead:

    Stephen> diff --git a/lisp/keymap.el b/lisp/keymap.el
    Stephen> index 791221f2459..48ec91d03c8 100644
    Stephen> --- a/lisp/keymap.el
    Stephen> +++ b/lisp/keymap.el
    Stephen> @@ -98,6 +98,8 @@ keymap-local-set
    Stephen>    (let ((map (current-local-map)))
    Stephen>      (unless map
    Stephen>        (use-local-map (setq map (make-sparse-keymap))))
    Stephen> +    (unless (stringp key)
    Stephen> +      (setq key (key-description key)))
    Stephen>      (keymap-set map key command)))

`keymap-global-set' has the same type of issue. How about this
instead:

diff --git a/lisp/keymap.el b/lisp/keymap.el
index 791221f2459..1a339598f0c 100644
--- a/lisp/keymap.el
+++ b/lisp/keymap.el
@@ -79,7 +79,7 @@ keymap-global-set
   (interactive
    (let* ((menu-prompting nil)
           (key (read-key-sequence "Set key globally: " nil t)))
-     (list key
+     (list (key-description key)
            (read-command (format "Set key %s to command: "
                                  (key-description key))))))
   (keymap-set (current-global-map) key command))
@@ -94,7 +94,12 @@ keymap-local-set
 The binding goes in the current buffer's local map, which in most
 cases is shared with all other buffers in the same major mode."
   (declare (compiler-macro (lambda (form) (keymap--compile-check key) form)))
-  (interactive "KSet key locally: \nCSet key %s locally to command: ")
+  (interactive
+   (let* ((menu-prompting nil)
+          (key (read-key-sequence "Set key locally: " nil t)))
+     (list (key-description key)
+           (read-command (format "Set key %s to command: "
+                                 (key-description key))))))
   (let ((map (current-local-map)))
     (unless map
       (use-local-map (setq map (make-sparse-keymap))))

Robert
-- 

bug#61149: 29.0.60; keymap-local-set rejects key sequences

[Stephen Berman]

On Mon, 30 Jan 2023 08:59:03 +0100 Robert Pluim <rpluim@gmail.com> wrote:

>>>>>> On Sun, 29 Jan 2023 16:28:44 +0100, Stephen Berman
> <stephen.berman@gmx.net> said:
>
>     Stephen> ... but not in your non-interactive case; so maybe this instead:
>
>     Stephen> diff --git a/lisp/keymap.el b/lisp/keymap.el
>     Stephen> index 791221f2459..48ec91d03c8 100644
>     Stephen> --- a/lisp/keymap.el
>     Stephen> +++ b/lisp/keymap.el
>     Stephen> @@ -98,6 +98,8 @@ keymap-local-set
>     Stephen>    (let ((map (current-local-map)))
>     Stephen>      (unless map
>     Stephen>        (use-local-map (setq map (make-sparse-keymap))))
>     Stephen> +    (unless (stringp key)
>     Stephen> +      (setq key (key-description key)))
>     Stephen>      (keymap-set map key command)))
>
> `keymap-global-set' has the same type of issue.

Thanks, I failed to check that.

>                                                 How about this
> instead:
>
> diff --git a/lisp/keymap.el b/lisp/keymap.el
> index 791221f2459..1a339598f0c 100644
> --- a/lisp/keymap.el
> +++ b/lisp/keymap.el
> @@ -79,7 +79,7 @@ keymap-global-set
>    (interactive
>     (let* ((menu-prompting nil)
>            (key (read-key-sequence "Set key globally: " nil t)))
> -     (list key
> +     (list (key-description key)
>             (read-command (format "Set key %s to command: "
>                                   (key-description key))))))
>    (keymap-set (current-global-map) key command))
> @@ -94,7 +94,12 @@ keymap-local-set
>  The binding goes in the current buffer's local map, which in most
>  cases is shared with all other buffers in the same major mode."
>    (declare (compiler-macro (lambda (form) (keymap--compile-check key) form)))
> -  (interactive "KSet key locally: \nCSet key %s locally to command: ")
> +  (interactive
> +   (let* ((menu-prompting nil)
> +          (key (read-key-sequence "Set key locally: " nil t)))
> +     (list (key-description key)
> +           (read-command (format "Set key %s to command: "
> +                                 (key-description key))))))
>    (let ((map (current-local-map)))
>      (unless map
>        (use-local-map (setq map (make-sparse-keymap))))

This also DTRT, but it also gives keymap-local-set the same UI as that
of keymap-global-set, which is that at the first prompt the cursor jumps
out of the minibuffer (due to read-key-sequence), and then jumps back at
the second prompt.  I find this quite jarring (and the first time I was
quite surprised and thought I had either mistyped or there was a bug).
I would prefer it if both commands had keymap-local-set's current UI.

Steve Berman

bug#61149: 29.0.60; keymap-local-set rejects key sequences

[Robert Pluim]

>>>>> On Mon, 30 Jan 2023 09:46:49 +0100, Stephen Berman <stephen.berman@gmx.net> said:

    Stephen> This also DTRT, but it also gives keymap-local-set the same UI as that
    Stephen> of keymap-global-set, which is that at the first prompt the cursor jumps
    Stephen> out of the minibuffer (due to read-key-sequence), and then jumps back at
    Stephen> the second prompt.  I find this quite jarring (and the first time I was
    Stephen> quite surprised and thought I had either mistyped or there was a bug).
    Stephen> I would prefer it if both commands had keymap-local-set's current UI.

Yes, that is quite annoying. So letʼs do it your way (with an
incidental spacing fix :-) )

Eli, emacs-29, I think, if youʼre ok with the fix.

diff --git a/lisp/keymap.el b/lisp/keymap.el
index 791221f2459..caabedd5aec 100644
--- a/lisp/keymap.el
+++ b/lisp/keymap.el
@@ -76,12 +76,9 @@ keymap-global-set
 that local binding will continue to shadow any global binding
 that you make with this function."
   (declare (compiler-macro (lambda (form) (keymap--compile-check key) form)))
-  (interactive
-   (let* ((menu-prompting nil)
-          (key (read-key-sequence "Set key globally: " nil t)))
-     (list key
-           (read-command (format "Set key %s to command: "
-                                 (key-description key))))))
+  (interactive "KSet key globally:\nCSet key %s globally to command: ")
+  (unless (stringp key)
+    (setq key (key-description key)))
   (keymap-set (current-global-map) key command))
 
 (defun keymap-local-set (key command)
@@ -94,10 +91,12 @@ keymap-local-set
 The binding goes in the current buffer's local map, which in most
 cases is shared with all other buffers in the same major mode."
   (declare (compiler-macro (lambda (form) (keymap--compile-check key) form)))
-  (interactive "KSet key locally: \nCSet key %s locally to command: ")
+  (interactive "KSet key locally:\nCSet key %s locally to command: ")
   (let ((map (current-local-map)))
     (unless map
       (use-local-map (setq map (make-sparse-keymap))))
+    (unless (stringp key)
+      (setq key (key-description key)))
     (keymap-set map key command)))
 
 (defun keymap-global-unset (key &optional remove)

bug#61149: 29.0.60; keymap-local-set rejects key sequences

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  61149@debbugs.gnu.org
> Date: Mon, 30 Jan 2023 10:47:35 +0100
> 
> Yes, that is quite annoying. So letʼs do it your way (with an
> incidental spacing fix :-) )
> 
> Eli, emacs-29, I think, if youʼre ok with the fix.

Definitely emacs-29, since these commands are new in Emacs 29, and we
cannot possibly release broken commands.

Thanks.

bug#61149: 29.0.60; keymap-local-set rejects key sequences

[Robert Pluim]

tags 61149 fixed
close 61149 29.1
quit

>>>>> On Mon, 30 Jan 2023 15:14:24 +0200, Eli Zaretskii <eliz@gnu.org> said:

    >> From: Robert Pluim <rpluim@gmail.com>
    >> Cc: Eli Zaretskii <eliz@gnu.org>,  61149@debbugs.gnu.org
    >> Date: Mon, 30 Jan 2023 10:47:35 +0100
    >> 
    >> Yes, that is quite annoying. So letʼs do it your way (with an
    >> incidental spacing fix :-) )
    >> 
    >> Eli, emacs-29, I think, if youʼre ok with the fix.

    Eli> Definitely emacs-29, since these commands are new in Emacs 29, and we
    Eli> cannot possibly release broken commands.

    Eli> Thanks.

Closing, pushed as f67a9a12b7b

Robert
--