Re: master f3aa648: Make `lookup-key' understand the new key sequence syntax

Re: master f3aa648: Make `lookup-key' understand the new key sequence syntax

[Stefan Monnier]

I think your `possibly_translate_key_sequence` should be extended to
cover the XEmacs format as well, And it should be exported to ELisp.

And I'd argue it should also convert strings to vectors.
Oh and also convert [M-C-next] to [C-M-next].

WDYT?


        Stefan


> +static Lisp_Object
> +possibly_translate_key_sequence (Lisp_Object key, ptrdiff_t *length)
> +{
> +  if (VECTORP (key) && ASIZE (key) == 1 && STRINGP (AREF (key, 0)))
> +    {
> +      /* KEY is on the ["C-c"] format, so translate to internal
> +	 format.  */
> +      if (NILP (Ffboundp (Qkbd_valid_p)))
> +	xsignal2 (Qerror,
> +		  build_string ("`kbd-valid-p' is not defined, so this syntax can't be used: %s"),
> +		  key);
> +      if (NILP (call1 (Qkbd_valid_p, AREF (key, 0))))
> +	xsignal2 (Qerror, build_string ("Invalid `kbd' syntax: %S"), key);
> +      key = call1 (Qkbd, AREF (key, 0));
> +      *length = CHECK_VECTOR_OR_STRING (key);
> +      if (*length == 0)
> +	xsignal2 (Qerror, build_string ("Invalid `kbd' syntax: %S"), key);
> +    }
> +
> +  return key;
> +}
> +
>  /* GC is possible in this function if it autoloads a keymap.  */
>  
>  DEFUN ("define-key", Fdefine_key, Sdefine_key, 3, 3, 0,
> @@ -1084,21 +1106,7 @@ binding KEY to DEF is added at the front of KEYMAP.  */)
>        def = tmp;
>      }
>  
> -  if (VECTORP (key) && ASIZE (key) == 1 && STRINGP (AREF (key, 0)))
> -    {
> -      /* KEY is on the ["C-c"] format, so translate to internal
> -	 format.  */
> -      if (NILP (Ffboundp (Qkbd_valid_p)))
> -	xsignal2 (Qerror,
> -		  build_string ("`kbd-valid-p' is not defined, so this syntax can't be used: %s"),
> -		  key);
> -      if (NILP (call1 (Qkbd_valid_p, AREF (key, 0))))
> -	xsignal2 (Qerror, build_string ("Invalid `kbd' syntax: %S"), key);
> -      key = call1 (Qkbd, AREF (key, 0));
> -      length = CHECK_VECTOR_OR_STRING (key);
> -      if (length == 0)
> -	xsignal2 (Qerror, build_string ("Invalid `kbd' syntax: %S"), key);
> -    }
> +  key = possibly_translate_key_sequence (key, &length);
>  
>    ptrdiff_t idx = 0;
>    while (1)
> @@ -1229,6 +1237,8 @@ recognize the default bindings, just as `read-key-sequence' does.  */)
>    if (length == 0)
>      return keymap;
>  
> +  key = possibly_translate_key_sequence (key, &length);
> +
>    ptrdiff_t idx = 0;
>    while (1)
>      {
> diff --git a/test/src/keymap-tests.el b/test/src/keymap-tests.el
> index 68b42c3..13f47b4 100644
> --- a/test/src/keymap-tests.el
> +++ b/test/src/keymap-tests.el
> @@ -317,6 +317,13 @@ g .. h		foo
>    (should (equal (single-key-description 'C-s-home)
>                   "C-s-<home>")))
>  
> +(ert-deftest keymap-test-lookups ()
> +  (should (eq (lookup-key (current-global-map) "\C-x\C-f") 'find-file))
> +  (should (eq (lookup-key (current-global-map) [(control x) (control f)])
> +              'find-file))
> +  (should (eq (lookup-key (current-global-map) ["C-x C-f"]) 'find-file))
> +  (should (eq (lookup-key (current-global-map) [?\C-x ?\C-f]) 'find-file)))
> +
>  (provide 'keymap-tests)
>  
>  ;;; keymap-tests.el ends here

Re: master f3aa648: Make `lookup-key' understand the new key sequence syntax

[Lars Ingebrigtsen]

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

> I think your `possibly_translate_key_sequence` should be extended to
> cover the XEmacs format as well, And it should be exported to ELisp.

lookup-key/define-key already understands that format, but mixes the
interpretation into the code itself.

(lookup-key (current-global-map) [(control x) (control f)])
=> find-file

Pulling it out of the code and putting it into
possibly_translate_key_sequence would be fine, I think.

> And I'd argue it should also convert strings to vectors.

Sure.

> Oh and also convert [M-C-next] to [C-M-next].

Uhm...  I'm not sure what the impact would be...

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

Re: master f3aa648: Make `lookup-key' understand the new key sequence syntax

[Stefan Monnier]

Lars Ingebrigtsen [2021-10-19 15:40:57] wrote:
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> I think your `possibly_translate_key_sequence` should be extended to
>> cover the XEmacs format as well, And it should be exported to ELisp.
>
> lookup-key/define-key already understands that format, but mixes the
> interpretation into the code itself.
>
> (lookup-key (current-global-map) [(control x) (control f)])
> => find-file
>
> Pulling it out of the code and putting it into
> possibly_translate_key_sequence would be fine, I think.
>
>> And I'd argue it should also convert strings to vectors.
>
> Sure.
>
>> Oh and also convert [M-C-next] to [C-M-next].
>
> Uhm...  I'm not sure what the impact would be...

In terms of behavior, no impact:

    ELISP> (let ((map (make-sparse-keymap))) (define-key map [C-M-next] 'dummy) map)
    (keymap (C-M-next . dummy))
    
    ELISP> (let ((map (make-sparse-keymap))) (define-key map [M-C-next] 'dummy) map)
    (keymap (C-M-next . dummy))
    
    ELISP> 


-- Stefan

Re: master f3aa648: Make `lookup-key' understand the new key sequence syntax

[Lars Ingebrigtsen]

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

> In terms of behavior, no impact:
>
>     ELISP> (let ((map (make-sparse-keymap))) (define-key map
>     ELISP> [C-M-next] 'dummy) map)
>     (keymap (C-M-next . dummy))
>
>     ELISP> (let ((map (make-sparse-keymap))) (define-key map
>     ELISP> [M-C-next] 'dummy) map)
>     (keymap (C-M-next . dummy))

Would this just be a simplification of the innards of `define-key', then?

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

Re: master f3aa648: Make `lookup-key' understand the new key sequence syntax

[Stefan Monnier]

Lars Ingebrigtsen [2021-10-19 16:17:50] wrote:
> Stefan Monnier <monnier@iro.umontreal.ca> writes:
>> In terms of behavior, no impact:
>>
>>     ELISP> (let ((map (make-sparse-keymap))) (define-key map
>>     ELISP> [C-M-next] 'dummy) map)
>>     (keymap (C-M-next . dummy))
>>
>>     ELISP> (let ((map (make-sparse-keymap))) (define-key map
>>     ELISP> [M-C-next] 'dummy) map)
>>     (keymap (C-M-next . dummy))
>
> Would this just be a simplification of the innards of `define-key', then?

It would move all the canonicalization to a separate function, yes.


        Stefan