bug#50752: 28.0.50; easy-menu-define lowers the menu-bar key

[Eli Zaretskii <eliz@gnu.org>]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Tue, 12 Oct 2021 15:22:59 -0700
> Cc: Shuguang Sun <shuguang79@qq.com>, 50752@debbugs.gnu.org
> 
> +      for (int i = 0; i < key_len; i++)
> +	{
> +	  Lisp_Object lc_key = Fdowncase (Fsymbol_name (AREF (key, i)));
> +	  ASET (new_key, i, Fintern (lc_key, Qnil));
> +	}

Beware: downcase uses the current buffer's case-table.  Is that
something we want here, or could it be undesirable in some cases?

> +      found = lookup_key_1 (keymap, new_key, accept_default);
> +
> +      if (!NILP (found) && !NUMBERP (found))
> +	goto end;
> +
> +      /* If we still don't have a match, let's convert any spaces in
> +	 our lowercased string into dashes, e.g. "foo bar baz" to
> +	 "foo-bar-baz". */
> +      for (int i = 0; i < key_len; i++)
> +	{
> +	  Lisp_Object lc_key = Fdowncase (Fsymbol_name (AREF (key, i)));

Can't we reuse the results of the original downcasing, instead of
doing that again?

> +	  USE_SAFE_ALLOCA;
> +	  ptrdiff_t size = SCHARS (lc_key), n;
> +	  if (INT_MULTIPLY_WRAPV (size, MAX_MULTIBYTE_LENGTH, &n))
> +	    n = PTRDIFF_MAX;
> +	  unsigned char *dst = SAFE_ALLOCA (n);
> +	  unsigned char *o = dst;
> +	  ptrdiff_t j = 0, j_byte = 0, chars = 0;
> +
> +	  while (j < SCHARS (lc_key))
> +	    {
> +	      int ch = fetch_string_char_advance (lc_key, &j, &j_byte);
> +	      if (ch == ' ')
> +		*o = '-';
> +	      else
> +		*o = ch;
> +	      chars++;

This will only work with plain-ASCII characters in lc_key (but then
you don't need fetch_string_char_advance, you can access the bytes one
by one).  You need to use CHAR_STRING instead.

> +	      int len;
> +	      string_char_and_length (o, &len);
> +	      o += len;

This is overhead.  You already know the length of the multibyte
string, because fetch_string_char_advance reports it back to you via
j_byte.  So just use that.

> diff --git a/test/src/keymap-tests.el b/test/src/keymap-tests.el
> index 68b42c346c..8f3dff2acb 100644
> --- a/test/src/keymap-tests.el
> +++ b/test/src/keymap-tests.el
> @@ -124,6 +124,17 @@ keymap-lookup-key/too-long
>  ;; (ert-deftest keymap-lookup-key/accept-default ()
>  ;;   ...)
>  
> +(ert-deftest keymap-lookup-key/mixed-case ()
> +  (let ((map (make-keymap)))
> +    (define-key map [menu-bar foo bar] 'foo)
> +    (should (eq (lookup-key map [menu-bar foo bar]) 'foo))
> +    (should (eq (lookup-key map [menu-bar Foo Bar]) 'foo))))
> +
> +(ert-deftest subr-test-lookup-keymap/with-spaces ()
> +  (let ((map (make-keymap)))
> +    (define-key map [menu-bar foo-bar] 'foo)
> +    (should (eq (lookup-key map [menu-bar Foo\ Bar]) 'foo))))
> +
>  (ert-deftest describe-buffer-bindings/header-in-current-buffer ()
>    "Header should be inserted into the current buffer.
>  https://debbugs.gnu.org/39149#31"

Please add tests where the symbols use non-ASCII characters.

Also, what about the existing calls to Flookup_key from C: do they all
need to go through the added processing, or could some of them be
satisfied by calling lookup_key_1?

This change needs a NEWS entry.

Thanks.