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

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

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

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

Hi,


I'm using the package pdf-tools to view PDF files. However the popu-menu does't work in recent emacs snapshot. I find it is due the automaticly lowering the menu-bar by easy-menu-deine. Is it a feature or bug?


Of pdf-tools (from melpa or from http://github.com/vedang/pdf-tools/), in the pdf-misc.el it defines
```
(easy-menu-define nil pdf-misc-menu-bar-minor-mode-map
  "Menu for PDF Tools."
  `("PDF Tools"
    ["Go Backward" pdf-history-backward
     :visible (bound-and-true-p pdf-history-minor-mode)
     :active (and (bound-and-true-p pdf-history-minor-mode)
                  (not (pdf-history-end-of-history-p)))]
```
And the responding function
```
(defun pdf-misc-popup-context-menu (event)
  "Popup a context menu at position determined by EVENT."
  (interactive "@e")
  (popup-menu
   (cons 'keymap
         (cddr (lookup-key pdf-misc-menu-bar-minor-mode-map
                           [menu-bar PDF\ Tools])))))
```

However, it can't find the key of "PDF\ Tools". Instead, I find in the `pdf-misc-menu-bar-minor-mode-map`, that,
```
(keymap
 (menu-bar keymap
           (pdf\ tools menu-item #1="PDF Tools"
                       (keymap #1#
                               (Go\ Backward menu-item "Go Backward" pdf-history-backward :enable
```

It becomes lower case (`pdf\ tools`).


If I change the case, the function works again:
```(defun pdf-misc-popup-context-menu (event)
  "Popup a context menu at position determined by EVENT."
  (interactive "@e")
  (popup-menu
   (cons 'keymap
         (cddr (lookup-key pdf-misc-menu-bar-minor-mode-map
                           [menu-bar pdf\ tools])))))
```


Best Regards,
Shuguang Sun





In GNU Emacs 28.0.50 (build 1, x86_64-w64-mingw32)
 of 2021-09-21 built on YJ190169-SSG
Repository revision: 5b962a7ad8d0acfe40a41ce139059b9c8e46f666
Repository branch: master
Windowing system distributor 'Microsoft Corp.', version 10.0.19043
System Description: Microsoft Windows 10 Pro (v10.0.2009.19043.1237)

Configured using:
 'configure --without-pop --with-native-image-api
 --with-native-compilation --without-compress-install
 '--program-transform-name=s/^ctags$/ctags.emacs/''

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

[-- Attachment #2: Type: text/html, Size: 3664 bytes --]

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

[Juri Linkov]

> However, it can't find the key of "PDF\ Tools". Instead, I find in the
> `pdf-misc-menu-bar-minor-mode-map`, that,
>
> It becomes lower case (`pdf\ tools`).

It seems this problem is caused by the recent commit
e5392d38ac27c4cf1674997ab38a453877e65109.

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

[Lars Ingebrigtsen]

"Shuguang Sun" <shuguang79@qq.com> writes:

> If I change the case, the function works again:
> ```(defun pdf-misc-popup-context-menu (event)
>   "Popup a context menu at position determined by EVENT."
>   (interactive "@e")
>   (popup-menu
>    (cons 'keymap
>          (cddr (lookup-key pdf-misc-menu-bar-minor-mode-map
>                            [menu-bar pdf\ tools])))))

Yes, the names have changed in Emacs 28, so external packages that alter
the menus like this have to be adjusted.  But I wonder whether we could
make `lookup-key' be case-insensitive in this case...

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

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

[Glenn Morris]

Previous discussion: https://lists.gnu.org/r/emacs-devel/2021-03/msg00031.html

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

[Stefan Kangas]

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

tags 50752 + patch
thanks

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Yes, the names have changed in Emacs 28, so external packages that alter
> the menus like this have to be adjusted.  But I wonder whether we could
> make `lookup-key' be case-insensitive in this case...

As Glenn points out, we discussed this here:
https://lists.gnu.org/r/emacs-devel/2021-03/msg00031.html

We also discussed it here:
https://lists.gnu.org/r/emacs-devel/2021-03/msg00014.html

I had an incomplete patch in the works, but I dropped the ball here.

Please find attached an updated and complete patch that should fix the
above issue.  In addition to the new tests, I have tested it locally,
and without the patch, I can reproduce the issue reported here by
Shuguang, i.e. I get this error:

    popup-menu: Empty menu

With the patch, the menu is correctly displayed, as is expected.  This
is tested with pdf-tools-20211004.514 installed from MELPA, which should
be equivalent to the latest version in the git repository at:

    http://github.com/vedang/pdf-tools

Review and additional testing is very welcome.

[-- Attachment #2: 0001-Be-more-allowing-when-looking-for-menu-bar-items.patch --]
[-- Type: text/x-diff, Size: 6222 bytes --]

From 0ee56b91506b445161bf2e839f4caa5ed78972bf Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefan@marxist.se>
Date: Wed, 13 Oct 2021 00:04:23 +0200
Subject: [PATCH] Be more allowing when looking for menu-bar items

* src/keymap.c (lookup_key_1): Factor out function from
Flookup_key.
(Flookup_key): Be case insensitive, and treat spaces as dashes,
when looking for Qmenu_bar items.  (Bug#50752)
* test/src/keymap-tests.el (keymap-lookup-key/mixed-case)
(subr-test-lookup-keymap/with-spaces): New tests.
---
 src/keymap.c             | 110 +++++++++++++++++++++++++++++++--------
 test/src/keymap-tests.el |  11 ++++
 2 files changed, 100 insertions(+), 21 deletions(-)

diff --git a/src/keymap.c b/src/keymap.c
index be45d2be1e..2fb5d8f41e 100644
--- a/src/keymap.c
+++ b/src/keymap.c
@@ -1180,27 +1180,8 @@ DEFUN ("command-remapping", Fcommand_remapping, Scommand_remapping, 1, 3, 0,
   return FIXNUMP (command) ? Qnil : command;
 }
 
-/* Value is number if KEY is too long; nil if valid but has no definition.  */
-/* GC is possible in this function.  */
-
-DEFUN ("lookup-key", Flookup_key, Slookup_key, 2, 3, 0,
-       doc: /* Look up key sequence KEY in KEYMAP.  Return the definition.
-A value of nil means undefined.  See doc of `define-key'
-for kinds of definitions.
-
-A number as value means KEY is "too long";
-that is, characters or symbols in it except for the last one
-fail to be a valid sequence of prefix characters in KEYMAP.
-The number is how many characters at the front of KEY
-it takes to reach a non-prefix key.
-KEYMAP can also be a list of keymaps.
-
-Normally, `lookup-key' ignores bindings for t, which act as default
-bindings, used when nothing else in the keymap applies; this makes it
-usable as a general function for probing keymaps.  However, if the
-third optional argument ACCEPT-DEFAULT is non-nil, `lookup-key' will
-recognize the default bindings, just as `read-key-sequence' does.  */)
-  (Lisp_Object keymap, Lisp_Object key, Lisp_Object accept_default)
+static Lisp_Object
+lookup_key_1 (Lisp_Object keymap, Lisp_Object key, Lisp_Object accept_default)
 {
   bool t_ok = !NILP (accept_default);
 
@@ -1240,6 +1221,93 @@ DEFUN ("lookup-key", Flookup_key, Slookup_key, 2, 3, 0,
     }
 }
 
+/* Value is number if KEY is too long; nil if valid but has no definition.  */
+/* GC is possible in this function.  */
+
+DEFUN ("lookup-key", Flookup_key, Slookup_key, 2, 3, 0,
+       doc: /* Look up key sequence KEY in KEYMAP.  Return the definition.
+A value of nil means undefined.  See doc of `define-key'
+for kinds of definitions.
+
+A number as value means KEY is "too long";
+that is, characters or symbols in it except for the last one
+fail to be a valid sequence of prefix characters in KEYMAP.
+The number is how many characters at the front of KEY
+it takes to reach a non-prefix key.
+KEYMAP can also be a list of keymaps.
+
+Normally, `lookup-key' ignores bindings for t, which act as default
+bindings, used when nothing else in the keymap applies; this makes it
+usable as a general function for probing keymaps.  However, if the
+third optional argument ACCEPT-DEFAULT is non-nil, `lookup-key' will
+recognize the default bindings, just as `read-key-sequence' does.  */)
+  (Lisp_Object keymap, Lisp_Object key, Lisp_Object accept_default)
+{
+  Lisp_Object found = lookup_key_1 (keymap, key, accept_default);
+
+  if (!NILP (found) && !NUMBERP (found))
+    goto end;
+
+  /* Menu definitions might use mixed case symbols (notably in old
+     versions of `easy-menu-define'), or use " " instead of "-".  */
+  if (VECTORP (key) && EQ (AREF (key, 0), Qmenu_bar))
+    {
+      ptrdiff_t key_len = ASIZE (key);
+      Lisp_Object new_key = make_vector (key_len, Qnil);
+
+      /* First, let's try converting all symbols like "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)));
+	  ASET (new_key, i, Fintern (lc_key, Qnil));
+	}
+      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)));
+
+	  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++;
+
+	      int len;
+	      string_char_and_length (o, &len);
+	      o += len;
+	    }
+	  eassert (o <= dst + n);
+	  Lisp_Object
+	    new_it = make_multibyte_string ((char *) dst, chars, o - dst);
+	  ASET (new_key, i, Fintern (new_it, Qnil));
+	  SAFE_FREE ();
+	}
+      found = lookup_key_1 (keymap, new_key, accept_default);
+    }
+
+ end:
+  return found;
+}
+
 /* Make KEYMAP define event C as a keymap (i.e., as a prefix).
    Assume that currently it does not define C at all.
    Return the keymap.  */
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"
-- 
2.30.2

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

[Lars Ingebrigtsen]

Stefan Kangas <stefan@marxist.se> writes:

> Review and additional testing is very welcome.

Looks good to me.  I've tested it now in Emacs 28, and I don't see any
regressions.

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

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

[Eli Zaretskii]

> 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.

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

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

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

An upper case I in Turkish would be translated to ı, which would not be
what we wanted, I think.  

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

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

[Stefan Kangas]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> An upper case I in Turkish would be translated to ı, which would not be
> what we wanted, I think.

Could you explain why you think that?

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

[Lars Ingebrigtsen]

Stefan Kangas <stefan@marxist.se> writes:

> Lars Ingebrigtsen <larsi@gnus.org> writes:
>
>> An upper case I in Turkish would be translated to ı, which would not be
>> what we wanted, I think.
>
> Could you explain why you think that?

If the menu name the package tried to change was

"PIF Tools"

it's now `pif tools' after being changed to use easy-menu-define, I
think?  So the downcase would work fine for everybody -- except people
that have a Turkish locale, where the downcase would yield `pıf tools',
and we'd have no match after all.

I think.

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

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

[Stefan Kangas]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> it's now `pif tools' after being changed to use easy-menu-define, I
> think?  So the downcase would work fine for everybody -- except people
> that have a Turkish locale, where the downcase would yield `pıf tools',
> and we'd have no match after all.

I didn't see that we were discussing a regular "I" character (bad font
in this client), but yes that is obviously incorrect.

Do we have an alternative to downcase, or should we just ensure that
it uses a standard case-table?  Could that lead to any other problems?

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

[Lars Ingebrigtsen]

Stefan Kangas <stefan@marxist.se> writes:

> Do we have an alternative to downcase, or should we just ensure that
> it uses a standard case-table?  Could that lead to any other problems?

But then non-ASCII characters wouldn't downcase correctly.  :-)

Since we're just trying to be backwards compatible, perhaps it would
make sense to try downcase twice -- once with the current case-table and
once with the standard one and see whether either matches?

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

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

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Wed, 13 Oct 2021 17:26:33 +0200
> Cc: Eli Zaretskii <eliz@gnu.org>, Shuguang Sun <shuguang79@qq.com>, 50752@debbugs.gnu.org
> 
> Do we have an alternative to downcase, or should we just ensure that
> it uses a standard case-table?  Could that lead to any other problems?
 
We could use the equivalent of

  (get-char-code-property ?I 'lowercase)

If the above returns nil, it means the lower-case variant is the
character itself.

In C, this means to use uniprop_table, like bidi.c and casefiddle.c
do.  This accesses the database generated from UnicodeData.txt.

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

[Eli Zaretskii]

> Date: Wed, 13 Oct 2021 14:59:03 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: shuguang79@qq.com, larsi@gnus.org, 50752@debbugs.gnu.org
> 
> > +	  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.

Thinking more about this, you don't need all these complications with
fetch_string_char_advance and CHAR_STRING.  Since all you need is
replace ' ' with '-', you can walk the string data byte by byte,
because UTF-8 encoding makes sure no other byte of any multibyte
sequence will ever include a 7-bit byte equal to an ASCII single-byte
character.  So just checking the bytes for equality to ' ' is enough.
Thus, you could make a copy of the symbol's name, then walk that copy
byte by byte looking for space characters and replacing them.

Moreover, you could check up front, using 'strstr', whether the
symbol's name includes any space characters, and if not, short-circuit
the entire second attempt.

These two measures should make the code faster and easier to program
and understand.

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

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

> These two measures should make the code faster and easier to program
> and understand.

Thanks, this really helps!

I plan to continue looking into the issue this weekend.

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

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

> 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?

AFAICT, they need to go through the added processing, as the items could
be extended menu items in all cases.

(I will send separately a new patch that should fix the rest of your
comments.)

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

[Stefan Kangas]

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

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Stefan Kangas <stefan@marxist.se> writes:
>
>> Do we have an alternative to downcase, or should we just ensure that
>> it uses a standard case-table?  Could that lead to any other problems?
>
> But then non-ASCII characters wouldn't downcase correctly.  :-)
>
> Since we're just trying to be backwards compatible, perhaps it would
> make sense to try downcase twice -- once with the current case-table and
> once with the standard one and see whether either matches?

So I've tried this approach in the attached patch, but I couldn't get it
to work.  I'm probably doing something wrong, given that I've never so
much as glanced at language environments and case tables before this.

Eli Zaretskii <eliz@gnu.org> writes:

> We could use the equivalent of
>
>   (get-char-code-property ?I 'lowercase)
>
> If the above returns nil, it means the lower-case variant is the
> character itself.
>
> In C, this means to use uniprop_table, like bidi.c and casefiddle.c
> do.  This accesses the database generated from UnicodeData.txt.

I didn't try this approach, mostly because it sounds more difficult to
implement than what Lars said.  I think?  Wouldn't it amount to
basically re-implementing Fdowncase?  Sorry, I didn't look too closely
at this.  Perhaps this would be the better approach.

If anyone has any preferences or further ideas here, that would be much
appreciated, otherwise I'll keep investigating.

The attached patch is what I have so far.  It's obviously not yet
finished, but all tests pass except for the one for "I->i" conversion in
the Turkish language environment.

[-- Attachment #2: 0001-Be-more-allowing-when-looking-for-menu-bar-items.patch --]
[-- Type: text/x-diff, Size: 9529 bytes --]

From 7060128663ebb74ccf659bd1ee6dc38f7c66ba9e Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefan@marxist.se>
Date: Wed, 13 Oct 2021 00:04:23 +0200
Subject: [PATCH] Be more allowing when looking for menu-bar items

* src/keymap.c (lookup_key_1): Factor out function from
Flookup_key.
(Flookup_key): Be case insensitive, and treat spaces as dashes,
when looking for Qmenu_bar items.  (Bug#50752)

* test/src/keymap-tests.el
(keymap-lookup-key/mixed-case)
(keymap-lookup-key/mixed-case-multibyte)
(keymap-lookup-keymap/with-spaces)
(keymap-lookup-keymap/with-spaces-multibyte)
(keymap-lookup-keymap/with-spaces-multibyte-lang-env): New tests.
---
 etc/NEWS                 |   8 +++
 src/keymap.c             | 125 ++++++++++++++++++++++++++++++++-------
 test/src/keymap-tests.el |  43 ++++++++++++++
 3 files changed, 155 insertions(+), 21 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index b7c4346db9..cb3a0c3ec4 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -4319,6 +4319,14 @@ The new optional "," parameter has been added, and
 ** 'parse-time-string' can now parse ISO 8601 format strings.
 These have a format like "2020-01-15T16:12:21-08:00".
 
+---
+** 'lookup-key' now downcases symbols in extended menu items.
+If looking for a key like '[menu-bar Foo-Bar]', attempt to find
+'[menu-bar foo-bar]' as well.  If looking for a key like '[menu-bar
+Foo\ Bar]', attempt to find both '[menu-bar foo\ bar]' and '[menu-bar
+foo-bar]'.  This improves backwards compatibility when menus are
+converted to use 'easy-menu-define'.
+
 ---
 ** 'make-network-process', 'make-serial-process' ':coding' behavior change.
 Previously, passing ':coding nil' to either of these functions would
diff --git a/src/keymap.c b/src/keymap.c
index be45d2be1e..4b3d50f53e 100644
--- a/src/keymap.c
+++ b/src/keymap.c
@@ -1180,27 +1180,8 @@ DEFUN ("command-remapping", Fcommand_remapping, Scommand_remapping, 1, 3, 0,
   return FIXNUMP (command) ? Qnil : command;
 }
 
-/* Value is number if KEY is too long; nil if valid but has no definition.  */
-/* GC is possible in this function.  */
-
-DEFUN ("lookup-key", Flookup_key, Slookup_key, 2, 3, 0,
-       doc: /* Look up key sequence KEY in KEYMAP.  Return the definition.
-A value of nil means undefined.  See doc of `define-key'
-for kinds of definitions.
-
-A number as value means KEY is "too long";
-that is, characters or symbols in it except for the last one
-fail to be a valid sequence of prefix characters in KEYMAP.
-The number is how many characters at the front of KEY
-it takes to reach a non-prefix key.
-KEYMAP can also be a list of keymaps.
-
-Normally, `lookup-key' ignores bindings for t, which act as default
-bindings, used when nothing else in the keymap applies; this makes it
-usable as a general function for probing keymaps.  However, if the
-third optional argument ACCEPT-DEFAULT is non-nil, `lookup-key' will
-recognize the default bindings, just as `read-key-sequence' does.  */)
-  (Lisp_Object keymap, Lisp_Object key, Lisp_Object accept_default)
+static Lisp_Object
+lookup_key_1 (Lisp_Object keymap, Lisp_Object key, Lisp_Object accept_default)
 {
   bool t_ok = !NILP (accept_default);
 
@@ -1240,6 +1221,108 @@ DEFUN ("lookup-key", Flookup_key, Slookup_key, 2, 3, 0,
     }
 }
 
+/* Value is number if KEY is too long; nil if valid but has no definition.  */
+/* GC is possible in this function.  */
+
+DEFUN ("lookup-key", Flookup_key, Slookup_key, 2, 3, 0,
+       doc: /* Look up key sequence KEY in KEYMAP.  Return the definition.
+A value of nil means undefined.  See doc of `define-key'
+for kinds of definitions.
+
+A number as value means KEY is "too long";
+that is, characters or symbols in it except for the last one
+fail to be a valid sequence of prefix characters in KEYMAP.
+The number is how many characters at the front of KEY
+it takes to reach a non-prefix key.
+KEYMAP can also be a list of keymaps.
+
+Normally, `lookup-key' ignores bindings for t, which act as default
+bindings, used when nothing else in the keymap applies; this makes it
+usable as a general function for probing keymaps.  However, if the
+third optional argument ACCEPT-DEFAULT is non-nil, `lookup-key' will
+recognize the default bindings, just as `read-key-sequence' does.  */)
+  (Lisp_Object keymap, Lisp_Object key, Lisp_Object accept_default)
+{
+  Lisp_Object found = lookup_key_1 (keymap, key, accept_default);
+
+  if (!NILP (found) && !NUMBERP (found))
+    return found;
+
+  /* Menu definitions might use mixed case symbols (notably in old
+     versions of `easy-menu-define'), or use " " instead of "-".
+     We accept these variations for backwards-compatibility.
+     (Bug#50752)  */
+  if (VECTORP (key) && ASIZE (key) > 0 && EQ (AREF (key, 0), Qmenu_bar))
+    {
+      ptrdiff_t key_len = ASIZE (key);
+      Lisp_Object new_key = make_vector (key_len, Qnil);
+
+      /* Try both the default ASCII case table, and the buffer local
+	 one.  Otherwise, we will fail for e.g. the "Turkish" language
+	 environment where 'I' does not downcase to 'i'.  */
+      Lisp_Object old_case_table = Fcurrent_case_table ();
+      Lisp_Object tables[2] = {Vascii_downcase_table, old_case_table};
+      for (int i = 0; i < 2; i++)
+	{
+	  Fset_case_table (tables[i]);
+
+	  /* First, let's try converting all symbols like "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)));
+	      ASET (new_key, i, Fintern (lc_key, Qnil));
+	    }
+	  found = lookup_key_1 (keymap, new_key, accept_default);
+
+	  if (!NILP (found) && !NUMBERP (found))
+	    break;
+
+	  /* 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 = Fsymbol_name (AREF (new_key, i));
+
+	      /* If there are no spaces in this symbol, just skip it.  */
+	      if (!strstr (SSDATA (lc_key), " "))
+		continue;
+
+	      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);
+
+	      /* We can walk the string data byte by byte, because UTF-8
+		 encoding ensures that no other byte of any multibyte
+		 sequence will ever include a 7-bit byte equal to an ASCII
+		 single-byte character.  */
+	      memcpy (dst, SSDATA (lc_key), SBYTES (lc_key));
+	      for (int i = 0; i < SBYTES (lc_key); ++i)
+		{
+		  if (*(dst + i) == ' ')
+		    *(dst + i) = '-';
+		}
+
+	      Lisp_Object
+		new_it = make_multibyte_string ((char *) dst, SCHARS (lc_key), SBYTES (lc_key));
+	      ASET (new_key, i, Fintern (new_it, Qnil));
+	      SAFE_FREE ();
+	    }
+	  found = lookup_key_1 (keymap, new_key, accept_default);
+
+	  if (!NILP (found) && !NUMBERP (found))
+	    break;
+	}
+      /* Restore the previous case table before returning.  */
+      Fset_case_table (old_case_table);
+    }
+
+  return found;
+}
+
 /* Make KEYMAP define event C as a keymap (i.e., as a prefix).
    Assume that currently it does not define C at all.
    Return the keymap.  */
diff --git a/test/src/keymap-tests.el b/test/src/keymap-tests.el
index 68b42c346c..a7480fe5cc 100644
--- a/test/src/keymap-tests.el
+++ b/test/src/keymap-tests.el
@@ -124,6 +124,49 @@ keymap-lookup-key/too-long
 ;; (ert-deftest keymap-lookup-key/accept-default ()
 ;;   ...)
 
+(ert-deftest keymap-lookup-key/mixed-case ()
+  "Backwards compatibility behaviour (Bug#50752)."
+  (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)))
+  (let ((map (make-keymap)))
+    (define-key map [menu-bar i-bar] 'foo)
+    (should (eq (lookup-key map [menu-bar I-bar]) 'foo))))
+
+(ert-deftest keymap-lookup-key/mixed-case-multibyte ()
+  "Backwards compatibility behaviour (Bug#50752)."
+  (let ((map (make-keymap)))
+    ;; (downcase "Åäö") => "åäö"
+    (define-key map [menu-bar åäö bar] 'foo)
+    (should (eq (lookup-key map [menu-bar åäö bar]) 'foo))
+    (should (eq (lookup-key map [menu-bar Åäö Bar]) 'foo))
+    ;; (downcase "Γ") => "γ"
+    (define-key map [menu-bar γ bar] 'baz)
+    (should (eq (lookup-key map [menu-bar γ bar]) 'baz))
+    (should (eq (lookup-key map [menu-bar Γ Bar]) 'baz))))
+
+(ert-deftest keymap-lookup-keymap/with-spaces ()
+  "Backwards compatibility behaviour (Bug#50752)."
+  (let ((map (make-keymap)))
+    (define-key map [menu-bar foo-bar] 'foo)
+    (should (eq (lookup-key map [menu-bar Foo\ Bar]) 'foo))))
+
+(ert-deftest keymap-lookup-keymap/with-spaces-multibyte ()
+  "Backwards compatibility behaviour (Bug#50752)."
+  (let ((map (make-keymap)))
+    (define-key map [menu-bar åäö-bar] 'foo)
+    (should (eq (lookup-key map [menu-bar Åäö\ Bar]) 'foo))))
+
+(ert-deftest keymap-lookup-keymap/with-spaces-multibyte-lang-env ()
+  "Backwards compatibility behaviour (Bug#50752)."
+  (let ((lang-env current-language-environment))
+    (set-language-environment "Turkish")
+    (let ((map (make-keymap)))
+      (define-key map [menu-bar i-bar] 'foo)
+      (should (eq (lookup-key map [menu-bar I-bar]) 'foo)))
+    (set-language-environment lang-env)))
+
 (ert-deftest describe-buffer-bindings/header-in-current-buffer ()
   "Header should be inserted into the current buffer.
 https://debbugs.gnu.org/39149#31"
-- 
2.30.2

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

[Lars Ingebrigtsen]

Stefan Kangas <stefan@marxist.se> writes:

> The attached patch is what I have so far.  It's obviously not yet
> finished, but all tests pass except for the one for "I->i" conversion in
> the Turkish language environment.

I set up a Turkish lang environment to test, and I'm just getting very
odd results all over from downcase.

After generating the locale, I said:

LANG=tr_TR.UTF-8 ./src/emacs -Q

and

(downcase "İ i I ı")
=> "i̇ i ı ı"

(upcase "İ i I ı")
=> "İ İ I I"

So everything here looks totally fine.  I is downcased to ı as supposed.
But look!

(downcase "I")
=> "1"

Here it's downcasing

  name: LATIN CAPITAL LETTER I

to

  name: DIGIT ONE

!?

But if I put something else that's non-ASCII into the string, then it
downcases correctly:

(downcase "I é")
=> "ı é"

So if the string is multibyte, it downcases correctly, but if not, it...
turns a capital I into the digit 1?

(downcase "ABCDEFGHIJKLMNOPQRSTUVWXYZ")
=> "abcdefgh1jklmnopqrstuvwxyz"

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

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

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> So if the string is multibyte, it downcases correctly, but if not, it...
> turns a capital I into the digit 1?
>
> (downcase "ABCDEFGHIJKLMNOPQRSTUVWXYZ")
> => "abcdefgh1jklmnopqrstuvwxyz"

I guess it's from this:

static Lisp_Object
do_casify_unibyte_string (struct casing_context *ctx, Lisp_Object obj)
{
  ptrdiff_t i, size = SCHARS (obj);
  int ch, cased;

  obj = Fcopy_sequence (obj);
  for (i = 0; i < size; i++)
    {
      ch = make_char_multibyte (SREF (obj, i));
      cased = case_single_character (ctx, ch);
      if (ch == cased)
	continue;
      cased = make_char_unibyte (cased);
      /* If the char can't be converted to a valid byte, just don't
	 change it.  */
      if (SINGLE_BYTE_CHAR_P (cased))
	SSET (obj, i, cased);
    }
  return obj;
}

Which just looks wrong to me.  The logic in Fdowncase seems to assume
that a unibyte string can never be downcased to a multibyte one, but
that make_char_unibyte is picking out the lowest eight bits of the char
we've downcased to and puts that into the downcased string.

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

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

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Mon, 18 Oct 2021 20:22:18 -0700
> Cc: Eli Zaretskii <eliz@gnu.org>, Shuguang Sun <shuguang79@qq.com>, 50752@debbugs.gnu.org
> 
> If anyone has any preferences or further ideas here, that would be much
> appreciated, otherwise I'll keep investigating.
> 
> The attached patch is what I have so far.  It's obviously not yet
> finished, but all tests pass except for the one for "I->i" conversion in
> the Turkish language environment.

Don't give up, you are close.

> +	      memcpy (dst, SSDATA (lc_key), SBYTES (lc_key));
> +	      for (int i = 0; i < SBYTES (lc_key); ++i)
> +		{
> +		  if (*(dst + i) == ' ')
> +		    *(dst + i) = '-';
> +		}

If you want to use an index to walk the string data, as you did above,
please use dst[i] instead of *(dst + i); the latter is correct, but
ugly and un-C-ish.  Or you could use a pointer to walk, like this:

        unsigned char *p = dst, *dst_end = dst + SBYTES (lc_key);
	for ( ; p < dst_end; p++)
	  {
	    if (*p == ' ')
	      *p = '-';
	  }

> > We could use the equivalent of
> >
> >   (get-char-code-property ?I 'lowercase)
> >
> > If the above returns nil, it means the lower-case variant is the
> > character itself.
> >
> > In C, this means to use uniprop_table, like bidi.c and casefiddle.c
> > do.  This accesses the database generated from UnicodeData.txt.
> 
> I didn't try this approach, mostly because it sounds more difficult to
> implement than what Lars said.  I think?  Wouldn't it amount to
> basically re-implementing Fdowncase?  Sorry, I didn't look too closely
> at this.  Perhaps this would be the better approach.

It shouldn't be hard.  You need to call uniprop_table to get a
char-table:

   Lisp_Object unicode_case_table = uniprop_table (intern ("lowercase"));

which you then reference with

   int low_ch = XFIXNUM (CHAR_TABLE_REF (unicode_case_table, ch));

to get the codepoint of the lower-case character that corresponds to
the (possibly upper-case) character whose codepoint is CH.  Then
downcasing a string boils down to a loop that fetches characters one
by one with fetch_string_char_advance and then stores the lower-case
characters, obtained as above, with CHAR_STRING.  (It's a bit more
complicated than that, because CHAR_TABLE_REF can return nil for the
characters that are either already lower-case or don't have case
variants.  And the uniprop_table call should be done once, at startup
time, or upon first usage, and stored in a staticpro'd variable, see
bidi_initialize for an example.)

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

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Tue, 19 Oct 2021 05:52:15 +0200
> Cc: Shuguang Sun <shuguang79@qq.com>, 50752@debbugs.gnu.org
> 
> Which just looks wrong to me.  The logic in Fdowncase seems to assume
> that a unibyte string can never be downcased to a multibyte one, but
> that make_char_unibyte is picking out the lowest eight bits of the char
> we've downcased to and puts that into the downcased string.

What would you do instead?  It's a conundrum with no easy solutions.
The original string could include non-ASCII bytes.

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

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> What would you do instead?  It's a conundrum with no easy solutions.
> The original string could include non-ASCII bytes.

First of all, I think the code is just plain buggy:

  for (i = 0; i < size; i++)
    {
      ch = make_char_multibyte (SREF (obj, i));
      cased = case_single_character (ctx, ch);
      if (ch == cased)
	continue;
      cased = make_char_unibyte (cased);
      /* If the char can't be converted to a valid byte, just don't
	 change it.  */
      if (SINGLE_BYTE_CHAR_P (cased))
	SSET (obj, i, cased);
    }

That make_char_unibyte makes SINGLE_BYTE_CHAR_P always return true,
doesn't it?  Uhm...  No, what happens to raw bytes...  Oh, yeah,
case_single_character doesn't work for raw bytes, so the "continue" is
always taken for those.

So the code is all kinds of confused, I think.

Second of all -- to handle this specific, very unusual case (i.e.,
downcasing a ascii character gets us a non-ascii character), when we
detect this issue, we should just abort this loop and call
do_casify_multibyte_string instead.

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

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

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

>       /* If the char can't be converted to a valid byte, just don't
> 	 change it.  */
>       if (SINGLE_BYTE_CHAR_P (cased))
> 	SSET (obj, i, cased);

[...]

> So the code is all kinds of confused, I think.

(I.e., I think the probable intention here is that when downcasing
something, and you get something that's multibyte, you shouldn't alter
the string -- but it does alter the string: It makes the "I" into the
digit "1".)

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

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

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: stefan@marxist.se,  shuguang79@qq.com,  50752@debbugs.gnu.org
> Date: Tue, 19 Oct 2021 14:07:59 +0200
> 
> Second of all -- to handle this specific, very unusual case (i.e.,
> downcasing a ascii character gets us a non-ascii character), when we
> detect this issue, we should just abort this loop and call
> do_casify_multibyte_string instead.

You mean, you want to special-case Turkish (and a couple of similar)
locale?  That might be possible, but the more general problem will
still be left more or less intact.  And then we need to consider how
many places don't expect to get multibyte string when they downcase a
unibyte one.

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

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> You mean, you want to special-case Turkish (and a couple of similar)
> locale?

Nope.  The loop can just check whether we get a multibyte result when
downcasing unibyte (ASCII), and then defer to the multibyte version to
do the computation.

This will basically only happen when using a Turkish (and a couple
similar) locales, though, so the general performance impact will be
approximately nil.

> And then we need to consider how many places don't expect to get
> multibyte string when they downcase a unibyte one.

That's a consideration, but I don't think we'll find out until we try.
(My guess is that nothing will be affected.)

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

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

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Nope.  The loop can just check whether we get a multibyte result when
> downcasing unibyte (ASCII), and then defer to the multibyte version to
> do the computation.

I.e., this change, which even passes all the case tests:

diff --git a/src/casefiddle.c b/src/casefiddle.c
index a7a2541490..b46d42f5e2 100644
--- a/src/casefiddle.c
+++ b/src/casefiddle.c
@@ -310,11 +310,12 @@ do_casify_unibyte_string (struct casing_context *ctx, Lisp_Object obj)
       cased = case_single_character (ctx, ch);
       if (ch == cased)
 	continue;
-      cased = make_char_unibyte (cased);
-      /* If the char can't be converted to a valid byte, just don't
-	 change it.  */
-      if (SINGLE_BYTE_CHAR_P (cased))
-	SSET (obj, i, cased);
+      /* If downcasing changed an ASCII character into a non-ASCII
+	 character (this can happen in some locales, like the Turkish
+	 "I"), use the multibyte algorithm.  */
+      if (SINGLE_BYTE_CHAR_P (ch) && !SINGLE_BYTE_CHAR_P (cased))
+	return do_casify_multibyte_string (ctx, obj);
+      SSET (obj, i, make_char_unibyte (cased));
     }
   return obj;
 }

It's a wonder that Emacs works at all in a Turkish environment -- I
instrumented the function and made it spit out every time it translated
"I" to "1" during startup, and it was a huge stream of

Got multi: jISX0208.1983-0
Got multi: bIG5-0
Got multi: jISX0208.1983-0
Got multi: jISX0208.1983-0
Got multi: bIG5-0
Got multi: jISX0208.1983-0
Got multi: bIG5-0
Got multi: jISX0208.1983-0

etc.  But I guess it doesn't trip up anything really vital.

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

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

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: stefan@marxist.se,  shuguang79@qq.com,  50752@debbugs.gnu.org
> Date: Tue, 19 Oct 2021 14:45:23 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > You mean, you want to special-case Turkish (and a couple of similar)
> > locale?
> 
> Nope.  The loop can just check whether we get a multibyte result when
> downcasing unibyte (ASCII), and then defer to the multibyte version to
> do the computation.
> 
> This will basically only happen when using a Turkish (and a couple
> similar) locales, though, so the general performance impact will be
> approximately nil.

I'd actually prefer to ignore any locale-specific behavior that take
an ASCII character and downcases (or upcases) it to a non-ASCII one,
in the unibyte case.  IOW, if changing a case of an ASCII character
from a unibyte string produces a non-ASCII multibyte character,
disregard any locale-specific case-conversion and use US ASCII rules.
That at least has the chance of not adversely affecting anything,
whereas producing non-ASCII multibyte string instead is IMO much
worse.

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

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> I'd actually prefer to ignore any locale-specific behavior that take
> an ASCII character and downcases (or upcases) it to a non-ASCII one,
> in the unibyte case.  IOW, if changing a case of an ASCII character
> from a unibyte string produces a non-ASCII multibyte character,
> disregard any locale-specific case-conversion and use US ASCII rules.
> That at least has the chance of not adversely affecting anything,
> whereas producing non-ASCII multibyte string instead is IMO much
> worse.

For the vast majority of cases, that would indeed produce the result
that's probably expected.  (I mean, somebody doing (downcase "LATIN-1")
in code doesn't want the locale, anyway.)

But it means that downcasing a string in a buffer and saying (downcase
"THAT SAME STRING") gives us different results.

But perhaps that's the least bad solution?

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

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

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: stefan@marxist.se,  shuguang79@qq.com,  50752@debbugs.gnu.org
> Date: Tue, 19 Oct 2021 15:24:12 +0200
> 
> +      /* If downcasing changed an ASCII character into a non-ASCII
> +	 character (this can happen in some locales, like the Turkish
> +	 "I"), use the multibyte algorithm.  */
> +      if (SINGLE_BYTE_CHAR_P (ch) && !SINGLE_BYTE_CHAR_P (cased))
> +	return do_casify_multibyte_string (ctx, obj);
> +      SSET (obj, i, make_char_unibyte (cased));

Like I said elsewhere, I'd prefer to use ASCII rules in that case.
It's safer, and definitely less surprising.  Whoever wants Turkish
rules will need to submit multibyte strings, even for ASCII
characters.

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

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: stefan@marxist.se,  shuguang79@qq.com,  50752@debbugs.gnu.org
> Date: Tue, 19 Oct 2021 17:57:09 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I'd actually prefer to ignore any locale-specific behavior that take
> > an ASCII character and downcases (or upcases) it to a non-ASCII one,
> > in the unibyte case.  IOW, if changing a case of an ASCII character
> > from a unibyte string produces a non-ASCII multibyte character,
> > disregard any locale-specific case-conversion and use US ASCII rules.
> > That at least has the chance of not adversely affecting anything,
> > whereas producing non-ASCII multibyte string instead is IMO much
> > worse.
> 
> For the vast majority of cases, that would indeed produce the result
> that's probably expected.  (I mean, somebody doing (downcase "LATIN-1")
> in code doesn't want the locale, anyway.)
> 
> But it means that downcasing a string in a buffer and saying (downcase
> "THAT SAME STRING") gives us different results.

If the buffer is unibyte, the results will be the same.

> But perhaps that's the least bad solution?

I think so, yes.  _If_ we are going to change the current behavior,
that is.

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

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> I think so, yes.  _If_ we are going to change the current behavior,
> that is.

Well, the current behaviour is clearly a bug.  So it should be fixed in
one way or another.

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

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

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> But perhaps that's the least bad solution?
>
> I think so, yes.

As for the implementation...  do you happen to know whether there are
any locales that makes up/downcase behave differently from the "C"
locale on ASCII characters, but does produce ASCII characters?

I'm just wondering whether the unibyte version of down/upcase should
just explicitly be documented to only do "C" locale stuff on unibyte
strings?  That's easy to explain, at least, but I don't know what the
practical repercussions would be.

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

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

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: stefan@marxist.se,  shuguang79@qq.com,  50752@debbugs.gnu.org
> Date: Tue, 19 Oct 2021 18:21:35 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> But perhaps that's the least bad solution?
> >
> > I think so, yes.
> 
> As for the implementation...  do you happen to know whether there are
> any locales that makes up/downcase behave differently from the "C"
> locale on ASCII characters, but does produce ASCII characters?

Not off the top of my head, no.

> I'm just wondering whether the unibyte version of down/upcase should
> just explicitly be documented to only do "C" locale stuff on unibyte
> strings?  That's easy to explain, at least, but I don't know what the
> practical repercussions would be.

This would be an unnecessary loss of functionality.  Right now, one
can arrange a case-conversion table for raw bytes, and it will work
with the current code.  We don't have to lose that, although the
utility of it is questionable.

I think we can leave the behavior with raw bytes as it is, and still
document this use case as being meant for ASCII conversions, leaving
the subtle case of raw bytes ... subtle.

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

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> I think we can leave the behavior with raw bytes as it is, and still
> document this use case as being meant for ASCII conversions, leaving
> the subtle case of raw bytes ... subtle.

Yup.

The following patch seems to do the trick, and should be conservative
enough for emacs-28, I think.

diff --git a/src/casefiddle.c b/src/casefiddle.c
index a7a2541490..edc9dec2d7 100644
--- a/src/casefiddle.c
+++ b/src/casefiddle.c
@@ -297,6 +297,16 @@ do_casify_multibyte_string (struct casing_context *ctx, Lisp_Object obj)
   return obj;
 }
 
+static int
+ascii_casify_character (bool downcase, int c)
+{
+  Lisp_Object cased = CHAR_TABLE_REF (downcase?
+				      uniprop_table (Qlowercase) :
+				      uniprop_table (Quppercase),
+				      c);
+  return FIXNATP (cased) ? XFIXNAT (cased) : c;
+}
+
 static Lisp_Object
 do_casify_unibyte_string (struct casing_context *ctx, Lisp_Object obj)
 {
@@ -310,11 +320,12 @@ do_casify_unibyte_string (struct casing_context *ctx, Lisp_Object obj)
       cased = case_single_character (ctx, ch);
       if (ch == cased)
 	continue;
-      cased = make_char_unibyte (cased);
-      /* If the char can't be converted to a valid byte, just don't
-	 change it.  */
-      if (SINGLE_BYTE_CHAR_P (cased))
-	SSET (obj, i, cased);
+      /* If down/upcasing changed an ASCII character into a non-ASCII
+	 character (this can happen in some locales, like the Turkish
+	 "I"), downcase using the ASCII char table.  */
+      if (SINGLE_BYTE_CHAR_P (ch) && !SINGLE_BYTE_CHAR_P (cased))
+	cased = ascii_casify_character (ctx->flag == CASE_DOWN, ch);
+      SSET (obj, i, make_char_unibyte (cased));
     }
   return obj;
 }
@@ -651,6 +662,8 @@ syms_of_casefiddle (void)
   DEFSYM (Qbounds, "bounds");
   DEFSYM (Qidentity, "identity");
   DEFSYM (Qtitlecase, "titlecase");
+  DEFSYM (Qlowercase, "lowercase");
+  DEFSYM (Quppercase, "uppercase");
   DEFSYM (Qspecial_uppercase, "special-uppercase");
   DEFSYM (Qspecial_lowercase, "special-lowercase");
   DEFSYM (Qspecial_titlecase, "special-titlecase");

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

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

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: stefan@marxist.se,  shuguang79@qq.com,  50752@debbugs.gnu.org
> Date: Tue, 19 Oct 2021 19:12:20 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I think we can leave the behavior with raw bytes as it is, and still
> > document this use case as being meant for ASCII conversions, leaving
> > the subtle case of raw bytes ... subtle.
> 
> Yup.
> 
> The following patch seems to do the trick, and should be conservative
> enough for emacs-28, I think.

I wouldn't start such experiments on the release branch.  It's not an
urgent problem.

> +static int
> +ascii_casify_character (bool downcase, int c)
> +{
> +  Lisp_Object cased = CHAR_TABLE_REF (downcase?
> +				      uniprop_table (Qlowercase) :
> +				      uniprop_table (Quppercase),
> +				      c);
> +  return FIXNATP (cased) ? XFIXNAT (cased) : c;
> +}

uniprop_table is somewhat expensive, and is definitely an overkill for
converting pure ASCII characters.  Why not just set/reset the 0x20
bit?

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

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> The following patch seems to do the trick, and should be conservative
>> enough for emacs-28, I think.
>
> I wouldn't start such experiments on the release branch.  It's not an
> urgent problem.

Yeah, it's pretty obscure.

> uniprop_table is somewhat expensive, and is definitely an overkill for
> converting pure ASCII characters.  Why not just set/reset the 0x20
> bit?

I thought it made sense to be conservative here and do it "properly",
instead of open-coding an A-Za-z upcase/downcase ourselves.  And doesn't
uniprop_table just return the table from char-code-property-alist here?
So it's an assq from a short list.

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

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

[Stefan Kangas]

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

Eli Zaretskii <eliz@gnu.org> writes:

> Don't give up, you are close.

Thank you!  Your feedback so far has been extremely useful and much
appreciated.

Based on your comments, I have been able to come up with the attached
patch.  It bootstraps and all tests pass.

Please let me know what you think.

[-- Attachment #2: 0001-Be-more-allowing-when-looking-for-menu-bar-items.patch --]
[-- Type: text/x-diff, Size: 11250 bytes --]

From 67d08470b9a07da2053a562879e222a456098e2b Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefan@marxist.se>
Date: Wed, 13 Oct 2021 00:04:23 +0200
Subject: [PATCH] Be more allowing when looking for menu-bar items

* src/keymap.c (lookup_key_1): Factor out function from
Flookup_key.
(Flookup_key): Be case insensitive, and treat spaces as dashes,
when looking for Qmenu_bar items.  (Bug#50752)

* test/src/keymap-tests.el
(keymap-lookup-key/mixed-case)
(keymap-lookup-key/mixed-case-multibyte)
(keymap-lookup-keymap/with-spaces)
(keymap-lookup-keymap/with-spaces-multibyte)
(keymap-lookup-keymap/with-spaces-multibyte-lang-env): New tests.
---
 etc/NEWS                 |   8 ++
 src/keymap.c             | 162 ++++++++++++++++++++++++++++++++++-----
 test/src/keymap-tests.el |  43 +++++++++++
 3 files changed, 192 insertions(+), 21 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 7031be311e..b47939305f 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -4320,6 +4320,14 @@ The new optional "," parameter has been added, and
 ** 'parse-time-string' can now parse ISO 8601 format strings.
 These have a format like "2020-01-15T16:12:21-08:00".
 
+---
+** 'lookup-key' is more allowing when searching for extended menu items.
+When looking for a menu item '[menu-bar Foo-Bar]', first try to find
+an exact match, then look for the lowercased '[menu-bar foo-bar]'.
+When looking for a menu item with a symbol containing spaces, as in
+'[menu-bar Foo\ Bar]', look for an exact match , then look for both
+'[menu-bar foo\ bar]' and '[menu-bar foo-bar]'.
+
 ---
 ** 'make-network-process', 'make-serial-process' ':coding' behavior change.
 Previously, passing ':coding nil' to either of these functions would
diff --git a/src/keymap.c b/src/keymap.c
index be45d2be1e..75422caf48 100644
--- a/src/keymap.c
+++ b/src/keymap.c
@@ -65,6 +65,9 @@
 /* Pre-allocated 2-element vector for Fcommand_remapping to use.  */
 static Lisp_Object command_remapping_vector;
 
+/* Char table for the backwards-compatibility part in Flookup_key.  */
+Lisp_Object unicode_case_table;
+
 /* Hash table used to cache a reverse-map to speed up calls to where-is.  */
 static Lisp_Object where_is_cache;
 /* Which keymaps are reverse-stored in the cache.  */
@@ -1180,27 +1183,8 @@ DEFUN ("command-remapping", Fcommand_remapping, Scommand_remapping, 1, 3, 0,
   return FIXNUMP (command) ? Qnil : command;
 }
 
-/* Value is number if KEY is too long; nil if valid but has no definition.  */
-/* GC is possible in this function.  */
-
-DEFUN ("lookup-key", Flookup_key, Slookup_key, 2, 3, 0,
-       doc: /* Look up key sequence KEY in KEYMAP.  Return the definition.
-A value of nil means undefined.  See doc of `define-key'
-for kinds of definitions.
-
-A number as value means KEY is "too long";
-that is, characters or symbols in it except for the last one
-fail to be a valid sequence of prefix characters in KEYMAP.
-The number is how many characters at the front of KEY
-it takes to reach a non-prefix key.
-KEYMAP can also be a list of keymaps.
-
-Normally, `lookup-key' ignores bindings for t, which act as default
-bindings, used when nothing else in the keymap applies; this makes it
-usable as a general function for probing keymaps.  However, if the
-third optional argument ACCEPT-DEFAULT is non-nil, `lookup-key' will
-recognize the default bindings, just as `read-key-sequence' does.  */)
-  (Lisp_Object keymap, Lisp_Object key, Lisp_Object accept_default)
+static Lisp_Object
+lookup_key_1 (Lisp_Object keymap, Lisp_Object key, Lisp_Object accept_default)
 {
   bool t_ok = !NILP (accept_default);
 
@@ -1240,6 +1224,140 @@ DEFUN ("lookup-key", Flookup_key, Slookup_key, 2, 3, 0,
     }
 }
 
+/* Value is number if KEY is too long; nil if valid but has no definition.  */
+/* GC is possible in this function.  */
+
+DEFUN ("lookup-key", Flookup_key, Slookup_key, 2, 3, 0,
+       doc: /* Look up key sequence KEY in KEYMAP.  Return the definition.
+A value of nil means undefined.  See doc of `define-key'
+for kinds of definitions.
+
+A number as value means KEY is "too long";
+that is, characters or symbols in it except for the last one
+fail to be a valid sequence of prefix characters in KEYMAP.
+The number is how many characters at the front of KEY
+it takes to reach a non-prefix key.
+KEYMAP can also be a list of keymaps.
+
+Normally, `lookup-key' ignores bindings for t, which act as default
+bindings, used when nothing else in the keymap applies; this makes it
+usable as a general function for probing keymaps.  However, if the
+third optional argument ACCEPT-DEFAULT is non-nil, `lookup-key' will
+recognize the default bindings, just as `read-key-sequence' does.  */)
+  (Lisp_Object keymap, Lisp_Object key, Lisp_Object accept_default)
+{
+  Lisp_Object found = lookup_key_1 (keymap, key, accept_default);
+  if (!NILP (found) && !NUMBERP (found))
+    return found;
+
+  /* Menu definitions might use mixed case symbols (notably in old
+     versions of `easy-menu-define'), or use " " instead of "-".
+     The rest of this function is about accepting these variations for
+     backwards-compatibility.  (Bug#50752) */
+
+  /* Just skip everything below unless this is a menu item.  */
+  if (!VECTORP (key) || !(ASIZE (key) > 0)
+      || !EQ (AREF (key, 0), Qmenu_bar))
+    return found;
+
+  /* Initialize the unicode case table, if it wasn't already.  */
+  if (NILP (unicode_case_table))
+    unicode_case_table = uniprop_table (intern ("lowercase"));
+
+  ptrdiff_t key_len = ASIZE (key);
+  Lisp_Object new_key = make_vector (key_len, Qnil);
+
+  /* Try both the Unicode case table, and the buffer local one.
+     Otherwise, we will fail for e.g. the "Turkish" language
+     environment where 'I' does not downcase to 'i'.  */
+  Lisp_Object tables[2] = {unicode_case_table, Fcurrent_case_table ()};
+  for (int tbl_num = 0; tbl_num < 2; tbl_num++)
+    {
+      /* First, let's try converting all symbols like "Foo-Bar-Baz" to
+	 "foo-bar-baz".  */
+      for (int i = 0; i < key_len; i++)
+	{
+	  Lisp_Object key_item = Fsymbol_name (AREF (key, i));
+	  Lisp_Object new_item;
+	  if (!STRING_MULTIBYTE (key_item))
+	    {
+	      new_item = Fdowncase (key_item);
+	    }
+	  else
+	    {
+	      USE_SAFE_ALLOCA;
+	      ptrdiff_t size = SCHARS (key_item), n;
+	      if (INT_MULTIPLY_WRAPV (size, MAX_MULTIBYTE_LENGTH, &n))
+		n = PTRDIFF_MAX;
+	      unsigned char *dst = SAFE_ALLOCA (n);
+	      unsigned char *p = dst;
+	      ptrdiff_t j_char = 0, j_byte = 0;
+
+	      while (j_char < size)
+		{
+		  int ch = fetch_string_char_advance (key_item, &j_char, &j_byte);
+		  Lisp_Object ch_conv = CHAR_TABLE_REF (tables[tbl_num], ch);
+		  if (!NILP (ch_conv))
+		    CHAR_STRING (XFIXNUM (ch_conv), p);
+		  else
+		    CHAR_STRING (ch, p);
+		  p = dst + j_byte;
+		}
+	      new_item = make_multibyte_string ((char *) dst,
+						SCHARS (key_item),
+						SBYTES (key_item));
+	      SAFE_FREE ();
+	    }
+	  ASET (new_key, i, Fintern (new_item, Qnil));
+	}
+
+      /* Check for match.  */
+      found = lookup_key_1 (keymap, new_key, accept_default);
+      if (!NILP (found) && !NUMBERP (found))
+	break;
+
+      /* 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 = Fsymbol_name (AREF (new_key, i));
+
+	  /* If there are no spaces in this symbol, just skip it.  */
+	  if (!strstr (SSDATA (lc_key), " "))
+	    continue;
+
+	  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);
+
+	  /* We can walk the string data byte by byte, because UTF-8
+	     encoding ensures that no other byte of any multibyte
+	     sequence will ever include a 7-bit byte equal to an ASCII
+	     single-byte character.  */
+	  memcpy (dst, SSDATA (lc_key), SBYTES (lc_key));
+	  for (int i = 0; i < SBYTES (lc_key); ++i)
+	    {
+	      if (dst[i] == ' ')
+		dst[i] = '-';
+	    }
+	  Lisp_Object
+	    new_it = make_multibyte_string ((char *) dst, SCHARS (lc_key), SBYTES (lc_key));
+	  ASET (new_key, i, Fintern (new_it, Qnil));
+	  SAFE_FREE ();
+	}
+
+      /* Check for match.  */
+      found = lookup_key_1 (keymap, new_key, accept_default);
+      if (!NILP (found) && !NUMBERP (found))
+	break;
+    }
+
+  return found;
+}
+
 /* Make KEYMAP define event C as a keymap (i.e., as a prefix).
    Assume that currently it does not define C at all.
    Return the keymap.  */
@@ -3210,6 +3328,8 @@ syms_of_keymap (void)
 			     intern_c_string ("mouse-4"),
 			     intern_c_string ("mouse-5"));
 
+  staticpro (&unicode_case_table);
+
   /* Keymap used for minibuffers when doing completion.  */
   /* Keymap used for minibuffers when doing completion and require a match.  */
   DEFSYM (Qkeymapp, "keymapp");
diff --git a/test/src/keymap-tests.el b/test/src/keymap-tests.el
index 68b42c346c..a7480fe5cc 100644
--- a/test/src/keymap-tests.el
+++ b/test/src/keymap-tests.el
@@ -124,6 +124,49 @@ keymap-lookup-key/too-long
 ;; (ert-deftest keymap-lookup-key/accept-default ()
 ;;   ...)
 
+(ert-deftest keymap-lookup-key/mixed-case ()
+  "Backwards compatibility behaviour (Bug#50752)."
+  (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)))
+  (let ((map (make-keymap)))
+    (define-key map [menu-bar i-bar] 'foo)
+    (should (eq (lookup-key map [menu-bar I-bar]) 'foo))))
+
+(ert-deftest keymap-lookup-key/mixed-case-multibyte ()
+  "Backwards compatibility behaviour (Bug#50752)."
+  (let ((map (make-keymap)))
+    ;; (downcase "Åäö") => "åäö"
+    (define-key map [menu-bar åäö bar] 'foo)
+    (should (eq (lookup-key map [menu-bar åäö bar]) 'foo))
+    (should (eq (lookup-key map [menu-bar Åäö Bar]) 'foo))
+    ;; (downcase "Γ") => "γ"
+    (define-key map [menu-bar γ bar] 'baz)
+    (should (eq (lookup-key map [menu-bar γ bar]) 'baz))
+    (should (eq (lookup-key map [menu-bar Γ Bar]) 'baz))))
+
+(ert-deftest keymap-lookup-keymap/with-spaces ()
+  "Backwards compatibility behaviour (Bug#50752)."
+  (let ((map (make-keymap)))
+    (define-key map [menu-bar foo-bar] 'foo)
+    (should (eq (lookup-key map [menu-bar Foo\ Bar]) 'foo))))
+
+(ert-deftest keymap-lookup-keymap/with-spaces-multibyte ()
+  "Backwards compatibility behaviour (Bug#50752)."
+  (let ((map (make-keymap)))
+    (define-key map [menu-bar åäö-bar] 'foo)
+    (should (eq (lookup-key map [menu-bar Åäö\ Bar]) 'foo))))
+
+(ert-deftest keymap-lookup-keymap/with-spaces-multibyte-lang-env ()
+  "Backwards compatibility behaviour (Bug#50752)."
+  (let ((lang-env current-language-environment))
+    (set-language-environment "Turkish")
+    (let ((map (make-keymap)))
+      (define-key map [menu-bar i-bar] 'foo)
+      (should (eq (lookup-key map [menu-bar I-bar]) 'foo)))
+    (set-language-environment lang-env)))
+
 (ert-deftest describe-buffer-bindings/header-in-current-buffer ()
   "Header should be inserted into the current buffer.
 https://debbugs.gnu.org/39149#31"
-- 
2.30.2

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

[Lars Ingebrigtsen]

I'm still seeing some oddities in the case stuff tr_TR.UTF-8.

If you have

india
ındıa

in a (multibyte) buffer and hit `M-u M-u', I get

İNDİA
INDIA

which is expected.  However, `M-c M-c' gets me

India
Indıa

I tried googling whether dotted lower-case i should be upcased
differently when capitalising, but I couldn't find anything...

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

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

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: stefan@marxist.se,  shuguang79@qq.com,  50752@debbugs.gnu.org
> Date: Tue, 19 Oct 2021 20:21:42 +0200
> 
> > uniprop_table is somewhat expensive, and is definitely an overkill for
> > converting pure ASCII characters.  Why not just set/reset the 0x20
> > bit?
> 
> I thought it made sense to be conservative here and do it "properly",
> instead of open-coding an A-Za-z upcase/downcase ourselves.

Why is it a problem to do it ourselves?  It's a simple and very
efficient operation, and it will always be correct.

> And doesn't
> uniprop_table just return the table from char-code-property-alist here?
> So it's an assq from a short list.

It returns a char-table (so not really a short list), and it loads a
file when first called.  It isn't incorrect, just too heavy for such a
simple job.

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

[Glenn Morris]

This all seems rather complicated to me.

It's very late to comment, but was the approach of adding an optional
argument to easy-menu-define that specifies the precise string to intern
(in place of "(downcase (car menu))"), and using that argument where
needed in those Emacs files that were converted to use easymenu,
rejected?

Basically the last paragraph from
https://lists.gnu.org/r/emacs-devel/2021-03/msg00046.html

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

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: stefan@marxist.se,  shuguang79@qq.com,  50752@debbugs.gnu.org
> Date: Wed, 20 Oct 2021 09:45:44 +0200
> 
> I'm still seeing some oddities in the case stuff tr_TR.UTF-8.
> 
> If you have
> 
> india
> ındıa
> 
> in a (multibyte) buffer and hit `M-u M-u', I get
> 
> İNDİA
> INDIA
> 
> which is expected.  However, `M-c M-c' gets me
> 
> India
> Indıa
> 
> I tried googling whether dotted lower-case i should be upcased
> differently when capitalising, but I couldn't find anything...

Michal, could you perhaps help us out here?

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

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Tue, 19 Oct 2021 14:54:59 -0700
> Cc: larsi@gnus.org, shuguang79@qq.com, 50752@debbugs.gnu.org
> 
> +/* Char table for the backwards-compatibility part in Flookup_key.  */
> +Lisp_Object unicode_case_table;

Should be static, I guess.

> +  /* Initialize the unicode case table, if it wasn't already.  */
> +  if (NILP (unicode_case_table))
> +    unicode_case_table = uniprop_table (intern ("lowercase"));

This is okay, but the call to staticpro should be immediately after
you call uniprop_table, so that you protect the value of the table as
it was created.  It won't work to staticpro it in syms_of_keymap,
which runs in temacs at build time.

> +	  if (!STRING_MULTIBYTE (key_item))
> +	    {
> +	      new_item = Fdowncase (key_item);
> +	    }

Style: a single statement in a block doesn't need braces.

And I guess the above assumes we resolve the issue with Turkish
locales when unibyte strings are downcased?

Otherwise, LGTM (but I didn't try applying and running the code).

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

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> I thought it made sense to be conservative here and do it "properly",
>> instead of open-coding an A-Za-z upcase/downcase ourselves.
>
> Why is it a problem to do it ourselves?  It's a simple and very
> efficient operation, and it will always be correct.

Hopefully.

>> And doesn't
>> uniprop_table just return the table from char-code-property-alist here?
>> So it's an assq from a short list.
>
> It returns a char-table (so not really a short list), and it loads a
> file when first called.  It isn't incorrect, just too heavy for such a
> simple job.

These case tables are predefined (they're in `char-code-property-alist'
on startup with -Q), so calling uniprop_table here doesn't load
anything, and it doesn't cons anything -- it just does an assq on
char-code-property-alist and returns the char table.

Unless I'm misreading the code completely.

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

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

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: stefan@marxist.se,  shuguang79@qq.com,  50752@debbugs.gnu.org
> Date: Thu, 21 Oct 2021 04:45:36 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> And doesn't
> >> uniprop_table just return the table from char-code-property-alist here?
> >> So it's an assq from a short list.
> >
> > It returns a char-table (so not really a short list), and it loads a
> > file when first called.  It isn't incorrect, just too heavy for such a
> > simple job.
> 
> These case tables are predefined (they're in `char-code-property-alist'
> on startup with -Q)

Right, we do that when dumping (loading charprop.el does it).

Still, I think using Unicode properties is overkill for this.

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

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Still, I think using Unicode properties is overkill for this.

Sure.  And having a fast and efficient ASCII-only up/downcaser may be
generally useful.

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

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

[Stefan Kangas]

Glenn Morris <rgm@gnu.org> writes:

> It's very late to comment, but was the approach of adding an optional
> argument to easy-menu-define that specifies the precise string to intern
> (in place of "(downcase (car menu))"), and using that argument where
> needed in those Emacs files that were converted to use easymenu,
> rejected?

I think at this point, the main counter-argument is that we have one of
the fixes ready, whereas the other one isn't.  I have been mulling your
message over the past couple of days, and I can't really see that your
proposed solution is significantly better.  The chosen solution allows
us to side-step the issue with a small and localized change.

Furthermore, the approach we have chosen here is IMO not that
complicated, at least not on a basic level.  Written in Lisp, it would
be very straight-forward indeed, but it turned out that with the
structure of our code it was easier to just write it in C.

So I would suggest pushing my fix to emacs-28, so that we can give it as
much testing as possible.  At this stage, it feels important to make
sure there is some fix on the emacs-28 branch so that we can start
testing it and gather feedback.

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

[Lars Ingebrigtsen]

Stefan Kangas <stefan@marxist.se> writes:

> So I would suggest pushing my fix to emacs-28, so that we can give it as
> much testing as possible.  At this stage, it feels important to make
> sure there is some fix on the emacs-28 branch so that we can start
> testing it and gather feedback.

Yup.  (And I think your fix is the right one.)

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

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

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: Glenn Morris <rgm@gnu.org>,  Eli Zaretskii <eliz@gnu.org>,
>   50752@debbugs.gnu.org,  shuguang79@qq.com
> Date: Mon, 25 Oct 2021 15:06:15 +0200
> 
> Stefan Kangas <stefan@marxist.se> writes:
> 
> > So I would suggest pushing my fix to emacs-28, so that we can give it as
> > much testing as possible.  At this stage, it feels important to make
> > sure there is some fix on the emacs-28 branch so that we can start
> > testing it and gather feedback.
> 
> Yup.  (And I think your fix is the right one.)

Sorry, I see no reason to put this on the release branch.

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

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Sorry, I see no reason to put this on the release branch.

It fixes real interoperability problems -- there are packages out there
that break with the current emacs-28 because of the previous
easy-menu-define changes.  Stefan's patch unbreaks those, but should
otherwise not affect operation.

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

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

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: stefan@marxist.se,  rgm@gnu.org,  50752@debbugs.gnu.org,  shuguang79@qq.com
> Date: Mon, 25 Oct 2021 15:21:56 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Sorry, I see no reason to put this on the release branch.
> 
> It fixes real interoperability problems -- there are packages out there
> that break with the current emacs-28 because of the previous
> easy-menu-define changes.  Stefan's patch unbreaks those, but should
> otherwise not affect operation.

Can we come up with a much simpler and safer variant that could
perhaps solve 85% of the problem? like perhaps only the letter-case,
and disregarding the Turkish issue somehow?  Then I could agree to
have that on the release branch.  The code on master is complex and
non-trivial, so having that on the release branch scares me.

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

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Can we come up with a much simpler and safer variant that could
> perhaps solve 85% of the problem? like perhaps only the letter-case,
> and disregarding the Turkish issue somehow?  Then I could agree to
> have that on the release branch.

Sure, I think that's what Stefan went for at the start (and which I was
thinking of, too), but it sounded like you wanted this to be more
ambitious.

I think just downcasing ASCII would get use more than 99%, really.

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

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

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: stefan@marxist.se,  rgm@gnu.org,  50752@debbugs.gnu.org,  shuguang79@qq.com
> Date: Mon, 25 Oct 2021 15:55:03 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Can we come up with a much simpler and safer variant that could
> > perhaps solve 85% of the problem? like perhaps only the letter-case,
> > and disregarding the Turkish issue somehow?  Then I could agree to
> > have that on the release branch.
> 
> Sure, I think that's what Stefan went for at the start (and which I was
> thinking of, too), but it sounded like you wanted this to be more
> ambitious.
> 
> I think just downcasing ASCII would get use more than 99%, really.

Then let's go with this variant on emacs-28, and leave the full
solution for master.

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

[Stefan Kangas]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> I think just downcasing ASCII would get use more than 99%, really.
>
> Then let's go with this variant on emacs-28, and leave the full
> solution for master.

OK.  I'm not sure if you just mean my previous patch, but without Lars'
fixes for the Turkish language environment, or if you mean a stripped
down version of my patch.

So I have attached two versions of the patch, one minimal one and the
one you've already seen with the additional fixes for language
environments.

Please let me know which of these is acceptable for emacs-28, and I will
push it as soon as possible.

Thanks in advance.

[-- Attachment #2: minimal-0001-Be-more-allowing-when-looking-for-menu-bar-items.patch --]
[-- Type: text/x-diff, Size: 8647 bytes --]

From f4a8cff7ee0ef09f5b95496dccc5fbcfc2a99d18 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefan@marxist.se>
Date: Wed, 13 Oct 2021 00:04:23 +0200
Subject: [PATCH] Be more allowing when looking for menu-bar items

* src/keymap.c (lookup_key_1): Factor out function from
Flookup_key.
(Flookup_key): Be case insensitive, and treat spaces as dashes,
when looking for Qmenu_bar items.  (Bug#50752)

* test/src/keymap-tests.el
(keymap-lookup-key/mixed-case)
(keymap-lookup-key/mixed-case-multibyte)
(keymap-lookup-keymap/with-spaces)
(keymap-lookup-keymap/with-spaces-multibyte): New tests.
---
 etc/NEWS                 |   8 +++
 src/keymap.c             | 112 +++++++++++++++++++++++++++++++--------
 test/src/keymap-tests.el |  34 ++++++++++++
 3 files changed, 133 insertions(+), 21 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 7f9797e1fa..60dfff60ab 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -4348,6 +4348,14 @@ The new optional "," parameter has been added, and
 ** 'parse-time-string' can now parse ISO 8601 format strings.
 These have a format like "2020-01-15T16:12:21-08:00".
 
+---
+** 'lookup-key' is more allowing when searching for extended menu items.
+When looking for a menu item '[menu-bar Foo-Bar]', first try to find
+an exact match, then look for the lowercased '[menu-bar foo-bar]'.
+When looking for a menu item with a symbol containing spaces, as in
+'[menu-bar Foo\ Bar]', look for an exact match , then look for both
+'[menu-bar foo\ bar]' and '[menu-bar foo-bar]'.
+
 ---
 ** 'make-network-process', 'make-serial-process' ':coding' behavior change.
 Previously, passing ':coding nil' to either of these functions would
diff --git a/src/keymap.c b/src/keymap.c
index 940a6f492e..5955b73c63 100644
--- a/src/keymap.c
+++ b/src/keymap.c
@@ -1183,27 +1183,8 @@ DEFUN ("command-remapping", Fcommand_remapping, Scommand_remapping, 1, 3, 0,
   return FIXNUMP (command) ? Qnil : command;
 }
 
-/* Value is number if KEY is too long; nil if valid but has no definition.  */
-/* GC is possible in this function.  */
-
-DEFUN ("lookup-key", Flookup_key, Slookup_key, 2, 3, 0,
-       doc: /* Look up key sequence KEY in KEYMAP.  Return the definition.
-A value of nil means undefined.  See doc of `define-key'
-for kinds of definitions.
-
-A number as value means KEY is "too long";
-that is, characters or symbols in it except for the last one
-fail to be a valid sequence of prefix characters in KEYMAP.
-The number is how many characters at the front of KEY
-it takes to reach a non-prefix key.
-KEYMAP can also be a list of keymaps.
-
-Normally, `lookup-key' ignores bindings for t, which act as default
-bindings, used when nothing else in the keymap applies; this makes it
-usable as a general function for probing keymaps.  However, if the
-third optional argument ACCEPT-DEFAULT is non-nil, `lookup-key' will
-recognize the default bindings, just as `read-key-sequence' does.  */)
-  (Lisp_Object keymap, Lisp_Object key, Lisp_Object accept_default)
+static Lisp_Object
+lookup_key_1 (Lisp_Object keymap, Lisp_Object key, Lisp_Object accept_default)
 {
   bool t_ok = !NILP (accept_default);
 
@@ -1243,6 +1224,95 @@ DEFUN ("lookup-key", Flookup_key, Slookup_key, 2, 3, 0,
     }
 }
 
+/* Value is number if KEY is too long; nil if valid but has no definition.  */
+/* GC is possible in this function.  */
+
+DEFUN ("lookup-key", Flookup_key, Slookup_key, 2, 3, 0,
+       doc: /* Look up key sequence KEY in KEYMAP.  Return the definition.
+A value of nil means undefined.  See doc of `define-key'
+for kinds of definitions.
+
+A number as value means KEY is "too long";
+that is, characters or symbols in it except for the last one
+fail to be a valid sequence of prefix characters in KEYMAP.
+The number is how many characters at the front of KEY
+it takes to reach a non-prefix key.
+KEYMAP can also be a list of keymaps.
+
+Normally, `lookup-key' ignores bindings for t, which act as default
+bindings, used when nothing else in the keymap applies; this makes it
+usable as a general function for probing keymaps.  However, if the
+third optional argument ACCEPT-DEFAULT is non-nil, `lookup-key' will
+recognize the default bindings, just as `read-key-sequence' does.  */)
+  (Lisp_Object keymap, Lisp_Object key, Lisp_Object accept_default)
+{
+  Lisp_Object found = lookup_key_1 (keymap, key, accept_default);
+  if (!NILP (found) && !NUMBERP (found))
+    return found;
+
+  /* Menu definitions might use mixed case symbols (notably in old
+     versions of `easy-menu-define'), or use " " instead of "-".
+     The rest of this function is about accepting these variations for
+     backwards-compatibility.  (Bug#50752) */
+
+  /* Just skip everything below unless this is a menu item.  */
+  if (!VECTORP (key) || !(ASIZE (key) > 0)
+      || !EQ (AREF (key, 0), Qmenu_bar))
+    return found;
+
+  ptrdiff_t key_len = ASIZE (key);
+  Lisp_Object new_key = make_vector (key_len, Qnil);
+
+  /* First, let's try converting all symbols like "Foo-Bar-Baz" to
+     "foo-bar-baz".  */
+  for (int i = 0; i < key_len; i++)
+    {
+      Lisp_Object new_item = Fdowncase (Fsymbol_name (AREF (key, i)));
+      ASET (new_key, i, Fintern (new_item, Qnil));
+    }
+
+  /* Check for match.  */
+  found = lookup_key_1 (keymap, new_key, accept_default);
+  if (!NILP (found) && !NUMBERP (found))
+    return found;
+
+  /* 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 = Fsymbol_name (AREF (new_key, i));
+
+      /* If there are no spaces in this symbol, just skip it.  */
+      if (!strstr (SSDATA (lc_key), " "))
+	continue;
+
+      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);
+
+      /* We can walk the string data byte by byte, because UTF-8
+	 encoding ensures that no other byte of any multibyte
+	 sequence will ever include a 7-bit byte equal to an ASCII
+	 single-byte character.  */
+      memcpy (dst, SSDATA (lc_key), SBYTES (lc_key));
+      for (int i = 0; i < SBYTES (lc_key); ++i)
+	{
+	  if (dst[i] == ' ')
+	    dst[i] = '-';
+	}
+      Lisp_Object
+	new_it = make_multibyte_string ((char *) dst, SCHARS (lc_key), SBYTES (lc_key));
+      ASET (new_key, i, Fintern (new_it, Qnil));
+      SAFE_FREE ();
+    }
+
+  /* Check for match.  */
+  return lookup_key_1 (keymap, new_key, accept_default);
+}
+
 /* Make KEYMAP define event C as a keymap (i.e., as a prefix).
    Assume that currently it does not define C at all.
    Return the keymap.  */
diff --git a/test/src/keymap-tests.el b/test/src/keymap-tests.el
index 68b42c346c..e835af4890 100644
--- a/test/src/keymap-tests.el
+++ b/test/src/keymap-tests.el
@@ -124,6 +124,40 @@ keymap-lookup-key/too-long
 ;; (ert-deftest keymap-lookup-key/accept-default ()
 ;;   ...)
 
+(ert-deftest keymap-lookup-key/mixed-case ()
+  "Backwards compatibility behaviour (Bug#50752)."
+  (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)))
+  (let ((map (make-keymap)))
+    (define-key map [menu-bar i-bar] 'foo)
+    (should (eq (lookup-key map [menu-bar I-bar]) 'foo))))
+
+(ert-deftest keymap-lookup-key/mixed-case-multibyte ()
+  "Backwards compatibility behaviour (Bug#50752)."
+  (let ((map (make-keymap)))
+    ;; (downcase "Åäö") => "åäö"
+    (define-key map [menu-bar åäö bar] 'foo)
+    (should (eq (lookup-key map [menu-bar åäö bar]) 'foo))
+    (should (eq (lookup-key map [menu-bar Åäö Bar]) 'foo))
+    ;; (downcase "Γ") => "γ"
+    (define-key map [menu-bar γ bar] 'baz)
+    (should (eq (lookup-key map [menu-bar γ bar]) 'baz))
+    (should (eq (lookup-key map [menu-bar Γ Bar]) 'baz))))
+
+(ert-deftest keymap-lookup-keymap/with-spaces ()
+  "Backwards compatibility behaviour (Bug#50752)."
+  (let ((map (make-keymap)))
+    (define-key map [menu-bar foo-bar] 'foo)
+    (should (eq (lookup-key map [menu-bar Foo\ Bar]) 'foo))))
+
+(ert-deftest keymap-lookup-keymap/with-spaces-multibyte ()
+  "Backwards compatibility behaviour (Bug#50752)."
+  (let ((map (make-keymap)))
+    (define-key map [menu-bar åäö-bar] 'foo)
+    (should (eq (lookup-key map [menu-bar Åäö\ 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"
-- 
2.30.2


[-- Attachment #3: full-0001-Be-more-allowing-when-looking-for-menu-bar-items.patch --]
[-- Type: text/x-diff, Size: 10959 bytes --]

From 7a3d5eacf20a2ee1cf732a850c8e0283e591aaa7 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefan@marxist.se>
Date: Wed, 13 Oct 2021 00:04:23 +0200
Subject: [PATCH] Be more allowing when looking for menu-bar items

* src/keymap.c (lookup_key_1): Factor out function from
Flookup_key.
(Flookup_key): Be case insensitive, and treat spaces as dashes,
when looking for Qmenu_bar items.  (Bug#50752)

* test/src/keymap-tests.el
(keymap-lookup-key/mixed-case)
(keymap-lookup-key/mixed-case-multibyte)
(keymap-lookup-keymap/with-spaces)
(keymap-lookup-keymap/with-spaces-multibyte)
(keymap-lookup-keymap/with-spaces-multibyte-lang-env): New tests.
---
 etc/NEWS                 |   8 ++
 src/keymap.c             | 161 ++++++++++++++++++++++++++++++++++-----
 test/src/keymap-tests.el |  43 +++++++++++
 3 files changed, 191 insertions(+), 21 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index 7f9797e1fa..60dfff60ab 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -4348,6 +4348,14 @@ The new optional "," parameter has been added, and
 ** 'parse-time-string' can now parse ISO 8601 format strings.
 These have a format like "2020-01-15T16:12:21-08:00".
 
+---
+** 'lookup-key' is more allowing when searching for extended menu items.
+When looking for a menu item '[menu-bar Foo-Bar]', first try to find
+an exact match, then look for the lowercased '[menu-bar foo-bar]'.
+When looking for a menu item with a symbol containing spaces, as in
+'[menu-bar Foo\ Bar]', look for an exact match , then look for both
+'[menu-bar foo\ bar]' and '[menu-bar foo-bar]'.
+
 ---
 ** 'make-network-process', 'make-serial-process' ':coding' behavior change.
 Previously, passing ':coding nil' to either of these functions would
diff --git a/src/keymap.c b/src/keymap.c
index 940a6f492e..e4e5065f26 100644
--- a/src/keymap.c
+++ b/src/keymap.c
@@ -65,6 +65,9 @@
 /* Pre-allocated 2-element vector for Fcommand_remapping to use.  */
 static Lisp_Object command_remapping_vector;
 
+/* Char table for the backwards-compatibility part in Flookup_key.  */
+static Lisp_Object unicode_case_table;
+
 /* Hash table used to cache a reverse-map to speed up calls to where-is.  */
 static Lisp_Object where_is_cache;
 /* Which keymaps are reverse-stored in the cache.  */
@@ -1183,27 +1186,8 @@ DEFUN ("command-remapping", Fcommand_remapping, Scommand_remapping, 1, 3, 0,
   return FIXNUMP (command) ? Qnil : command;
 }
 
-/* Value is number if KEY is too long; nil if valid but has no definition.  */
-/* GC is possible in this function.  */
-
-DEFUN ("lookup-key", Flookup_key, Slookup_key, 2, 3, 0,
-       doc: /* Look up key sequence KEY in KEYMAP.  Return the definition.
-A value of nil means undefined.  See doc of `define-key'
-for kinds of definitions.
-
-A number as value means KEY is "too long";
-that is, characters or symbols in it except for the last one
-fail to be a valid sequence of prefix characters in KEYMAP.
-The number is how many characters at the front of KEY
-it takes to reach a non-prefix key.
-KEYMAP can also be a list of keymaps.
-
-Normally, `lookup-key' ignores bindings for t, which act as default
-bindings, used when nothing else in the keymap applies; this makes it
-usable as a general function for probing keymaps.  However, if the
-third optional argument ACCEPT-DEFAULT is non-nil, `lookup-key' will
-recognize the default bindings, just as `read-key-sequence' does.  */)
-  (Lisp_Object keymap, Lisp_Object key, Lisp_Object accept_default)
+static Lisp_Object
+lookup_key_1 (Lisp_Object keymap, Lisp_Object key, Lisp_Object accept_default)
 {
   bool t_ok = !NILP (accept_default);
 
@@ -1243,6 +1227,141 @@ DEFUN ("lookup-key", Flookup_key, Slookup_key, 2, 3, 0,
     }
 }
 
+/* Value is number if KEY is too long; nil if valid but has no definition.  */
+/* GC is possible in this function.  */
+
+DEFUN ("lookup-key", Flookup_key, Slookup_key, 2, 3, 0,
+       doc: /* Look up key sequence KEY in KEYMAP.  Return the definition.
+A value of nil means undefined.  See doc of `define-key'
+for kinds of definitions.
+
+A number as value means KEY is "too long";
+that is, characters or symbols in it except for the last one
+fail to be a valid sequence of prefix characters in KEYMAP.
+The number is how many characters at the front of KEY
+it takes to reach a non-prefix key.
+KEYMAP can also be a list of keymaps.
+
+Normally, `lookup-key' ignores bindings for t, which act as default
+bindings, used when nothing else in the keymap applies; this makes it
+usable as a general function for probing keymaps.  However, if the
+third optional argument ACCEPT-DEFAULT is non-nil, `lookup-key' will
+recognize the default bindings, just as `read-key-sequence' does.  */)
+  (Lisp_Object keymap, Lisp_Object key, Lisp_Object accept_default)
+{
+  Lisp_Object found = lookup_key_1 (keymap, key, accept_default);
+  if (!NILP (found) && !NUMBERP (found))
+    return found;
+
+  /* Menu definitions might use mixed case symbols (notably in old
+     versions of `easy-menu-define'), or use " " instead of "-".
+     The rest of this function is about accepting these variations for
+     backwards-compatibility.  (Bug#50752) */
+
+  /* Just skip everything below unless this is a menu item.  */
+  if (!VECTORP (key) || !(ASIZE (key) > 0)
+      || !EQ (AREF (key, 0), Qmenu_bar))
+    return found;
+
+  /* Initialize the unicode case table, if it wasn't already.  */
+  if (NILP (unicode_case_table))
+    {
+      unicode_case_table = uniprop_table (intern ("lowercase"));
+      staticpro (&unicode_case_table);
+    }
+
+  ptrdiff_t key_len = ASIZE (key);
+  Lisp_Object new_key = make_vector (key_len, Qnil);
+
+  /* Try both the Unicode case table, and the buffer local one.
+     Otherwise, we will fail for e.g. the "Turkish" language
+     environment where 'I' does not downcase to 'i'.  */
+  Lisp_Object tables[2] = {unicode_case_table, Fcurrent_case_table ()};
+  for (int tbl_num = 0; tbl_num < 2; tbl_num++)
+    {
+      /* First, let's try converting all symbols like "Foo-Bar-Baz" to
+	 "foo-bar-baz".  */
+      for (int i = 0; i < key_len; i++)
+	{
+	  Lisp_Object key_item = Fsymbol_name (AREF (key, i));
+	  Lisp_Object new_item;
+	  if (!STRING_MULTIBYTE (key_item))
+	    new_item = Fdowncase (key_item);
+	  else
+	    {
+	      USE_SAFE_ALLOCA;
+	      ptrdiff_t size = SCHARS (key_item), n;
+	      if (INT_MULTIPLY_WRAPV (size, MAX_MULTIBYTE_LENGTH, &n))
+		n = PTRDIFF_MAX;
+	      unsigned char *dst = SAFE_ALLOCA (n);
+	      unsigned char *p = dst;
+	      ptrdiff_t j_char = 0, j_byte = 0;
+
+	      while (j_char < size)
+		{
+		  int ch = fetch_string_char_advance (key_item, &j_char, &j_byte);
+		  Lisp_Object ch_conv = CHAR_TABLE_REF (tables[tbl_num], ch);
+		  if (!NILP (ch_conv))
+		    CHAR_STRING (XFIXNUM (ch_conv), p);
+		  else
+		    CHAR_STRING (ch, p);
+		  p = dst + j_byte;
+		}
+	      new_item = make_multibyte_string ((char *) dst,
+						SCHARS (key_item),
+						SBYTES (key_item));
+	      SAFE_FREE ();
+	    }
+	  ASET (new_key, i, Fintern (new_item, Qnil));
+	}
+
+      /* Check for match.  */
+      found = lookup_key_1 (keymap, new_key, accept_default);
+      if (!NILP (found) && !NUMBERP (found))
+	break;
+
+      /* 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 = Fsymbol_name (AREF (new_key, i));
+
+	  /* If there are no spaces in this symbol, just skip it.  */
+	  if (!strstr (SSDATA (lc_key), " "))
+	    continue;
+
+	  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);
+
+	  /* We can walk the string data byte by byte, because UTF-8
+	     encoding ensures that no other byte of any multibyte
+	     sequence will ever include a 7-bit byte equal to an ASCII
+	     single-byte character.  */
+	  memcpy (dst, SSDATA (lc_key), SBYTES (lc_key));
+	  for (int i = 0; i < SBYTES (lc_key); ++i)
+	    {
+	      if (dst[i] == ' ')
+		dst[i] = '-';
+	    }
+	  Lisp_Object
+	    new_it = make_multibyte_string ((char *) dst, SCHARS (lc_key), SBYTES (lc_key));
+	  ASET (new_key, i, Fintern (new_it, Qnil));
+	  SAFE_FREE ();
+	}
+
+      /* Check for match.  */
+      found = lookup_key_1 (keymap, new_key, accept_default);
+      if (!NILP (found) && !NUMBERP (found))
+	break;
+    }
+
+  return found;
+}
+
 /* Make KEYMAP define event C as a keymap (i.e., as a prefix).
    Assume that currently it does not define C at all.
    Return the keymap.  */
diff --git a/test/src/keymap-tests.el b/test/src/keymap-tests.el
index 68b42c346c..a7480fe5cc 100644
--- a/test/src/keymap-tests.el
+++ b/test/src/keymap-tests.el
@@ -124,6 +124,49 @@ keymap-lookup-key/too-long
 ;; (ert-deftest keymap-lookup-key/accept-default ()
 ;;   ...)
 
+(ert-deftest keymap-lookup-key/mixed-case ()
+  "Backwards compatibility behaviour (Bug#50752)."
+  (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)))
+  (let ((map (make-keymap)))
+    (define-key map [menu-bar i-bar] 'foo)
+    (should (eq (lookup-key map [menu-bar I-bar]) 'foo))))
+
+(ert-deftest keymap-lookup-key/mixed-case-multibyte ()
+  "Backwards compatibility behaviour (Bug#50752)."
+  (let ((map (make-keymap)))
+    ;; (downcase "Åäö") => "åäö"
+    (define-key map [menu-bar åäö bar] 'foo)
+    (should (eq (lookup-key map [menu-bar åäö bar]) 'foo))
+    (should (eq (lookup-key map [menu-bar Åäö Bar]) 'foo))
+    ;; (downcase "Γ") => "γ"
+    (define-key map [menu-bar γ bar] 'baz)
+    (should (eq (lookup-key map [menu-bar γ bar]) 'baz))
+    (should (eq (lookup-key map [menu-bar Γ Bar]) 'baz))))
+
+(ert-deftest keymap-lookup-keymap/with-spaces ()
+  "Backwards compatibility behaviour (Bug#50752)."
+  (let ((map (make-keymap)))
+    (define-key map [menu-bar foo-bar] 'foo)
+    (should (eq (lookup-key map [menu-bar Foo\ Bar]) 'foo))))
+
+(ert-deftest keymap-lookup-keymap/with-spaces-multibyte ()
+  "Backwards compatibility behaviour (Bug#50752)."
+  (let ((map (make-keymap)))
+    (define-key map [menu-bar åäö-bar] 'foo)
+    (should (eq (lookup-key map [menu-bar Åäö\ Bar]) 'foo))))
+
+(ert-deftest keymap-lookup-keymap/with-spaces-multibyte-lang-env ()
+  "Backwards compatibility behaviour (Bug#50752)."
+  (let ((lang-env current-language-environment))
+    (set-language-environment "Turkish")
+    (let ((map (make-keymap)))
+      (define-key map [menu-bar i-bar] 'foo)
+      (should (eq (lookup-key map [menu-bar I-bar]) 'foo)))
+    (set-language-environment lang-env)))
+
 (ert-deftest describe-buffer-bindings/header-in-current-buffer ()
   "Header should be inserted into the current buffer.
 https://debbugs.gnu.org/39149#31"
-- 
2.30.2

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

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Tue, 26 Oct 2021 01:38:32 -0700
> Cc: rgm@gnu.org, 50752@debbugs.gnu.org, shuguang79@qq.com
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> I think just downcasing ASCII would get use more than 99%, really.
> >
> > Then let's go with this variant on emacs-28, and leave the full
> > solution for master.
> 
> OK.  I'm not sure if you just mean my previous patch, but without Lars'
> fixes for the Turkish language environment, or if you mean a stripped
> down version of my patch.

Unfortunately, I meant neither of these two, because they both are
quite non-trivial.  I meant a much simpler patch which only downcases
ASCII letters, and that's all.  Such a change doesn't need to call
Fdowncase, and definitely doesn't need to futz with multibyte
characters.  It should be a simple copy and a single loop downcasing
the characters.  (The NEWS entry for emacs-28 should thus say that we
only handle this simple class of problems.)

The log message should say "don't merge to master".

Sorry for not expressing myself clearly enough.

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

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

> Unfortunately, I meant neither of these two, because they both are
> quite non-trivial.  I meant a much simpler patch which only downcases
> ASCII letters, and that's all.  Such a change doesn't need to call
> Fdowncase, and definitely doesn't need to futz with multibyte
> characters.  It should be a simple copy and a single loop downcasing
> the characters.  (The NEWS entry for emacs-28 should thus say that we
> only handle this simple class of problems.)

OK, let me just double check that you mean to memcpy and then just loop
over the memory byte by bite and update them like so:

    int new_ch = XFIXNUM (CHAR_TABLE_REF (Vascii_downcase_table, ch))

Or do you mean something even simpler like this?

    if (ch >= 'A' && ch <= 'Z')
        new_ch = c + ('A' - 'a');

> The log message should say "don't merge to master".

Hmm.  If we can live with a simpler fix on emacs-28, can we not live
with it on master as well?

Or at least live with it until we can evaluate if the simpler fix did
the job well enough?

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

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Tue, 26 Oct 2021 22:24:10 +0200
> Cc: larsi@gnus.org, rgm@gnu.org, 50752@debbugs.gnu.org, shuguang79@qq.com
> 
> OK, let me just double check that you mean to memcpy and then just loop
> over the memory byte by bite and update them like so:
> 
>     int new_ch = XFIXNUM (CHAR_TABLE_REF (Vascii_downcase_table, ch))
> 
> Or do you mean something even simpler like this?
> 
>     if (ch >= 'A' && ch <= 'Z')
>         new_ch = c + ('A' - 'a');

The latter.

> > The log message should say "don't merge to master".
> 
> Hmm.  If we can live with a simpler fix on emacs-28, can we not live
> with it on master as well?

On master, I'd prefer to have the full solution, which you already
coded.  There's no need to give up a complete solution there.  It just
is too complex to safely backport it to the release branch, so Emacs
28 will have to do with a 99% solution, not 100%.

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

[Stefan Kangas]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> Or do you mean something even simpler like this?
>>
>>     if (ch >= 'A' && ch <= 'Z')
>>         new_ch = c + ('A' - 'a');
>
> The latter.

OK, I guess that should be the attached.  I hope I got it right.

[-- Attachment #2: 0001-Be-more-allowing-when-looking-for-menu-bar-items.patch --]
[-- Type: text/x-diff, Size: 6503 bytes --]

From 669efa1b066f15a4550ab00e1020d23e2581fda5 Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefan@marxist.se>
Date: Wed, 13 Oct 2021 00:04:23 +0200
Subject: [PATCH] Be more allowing when looking for menu-bar items

* src/keymap.c (lookup_key_1): Factor out function from
Flookup_key.
(Flookup_key): Be case insensitive when looking for Qmenu_bar
items.  (Bug#50752)

* test/src/keymap-tests.el
(keymap-lookup-key/mixed-case)
(keymap-lookup-key/mixed-case-multibyte): New tests.

Don't merge to master.
---
 etc/NEWS                 |  8 ++++
 src/keymap.c             | 80 +++++++++++++++++++++++++++++-----------
 test/src/keymap-tests.el | 10 +++++
 3 files changed, 77 insertions(+), 21 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index a2b7baf1ad..9f1a00134d 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -4343,6 +4343,14 @@ The new optional "," parameter has been added, and
 ** 'parse-time-string' can now parse ISO 8601 format strings.
 These have a format like "2020-01-15T16:12:21-08:00".
 
+---
+** 'lookup-key' is more allowing when searching for extended menu items.
+When looking for a menu item '[menu-bar Foo-Bar]', first try to find
+an exact match, then look for the lowercased '[menu-bar foo-bar]'.
+It will only try to downcase ASCII characters in the range "A-Z".
+This improves backwards-compatibility when converting menus to use
+'easy-menu-define'.
+
 ---
 ** 'make-network-process', 'make-serial-process' ':coding' behavior change.
 Previously, passing ':coding nil' to either of these functions would
diff --git a/src/keymap.c b/src/keymap.c
index 940a6f492e..f7529f808b 100644
--- a/src/keymap.c
+++ b/src/keymap.c
@@ -1183,27 +1183,8 @@ DEFUN ("command-remapping", Fcommand_remapping, Scommand_remapping, 1, 3, 0,
   return FIXNUMP (command) ? Qnil : command;
 }
 
-/* Value is number if KEY is too long; nil if valid but has no definition.  */
-/* GC is possible in this function.  */
-
-DEFUN ("lookup-key", Flookup_key, Slookup_key, 2, 3, 0,
-       doc: /* Look up key sequence KEY in KEYMAP.  Return the definition.
-A value of nil means undefined.  See doc of `define-key'
-for kinds of definitions.
-
-A number as value means KEY is "too long";
-that is, characters or symbols in it except for the last one
-fail to be a valid sequence of prefix characters in KEYMAP.
-The number is how many characters at the front of KEY
-it takes to reach a non-prefix key.
-KEYMAP can also be a list of keymaps.
-
-Normally, `lookup-key' ignores bindings for t, which act as default
-bindings, used when nothing else in the keymap applies; this makes it
-usable as a general function for probing keymaps.  However, if the
-third optional argument ACCEPT-DEFAULT is non-nil, `lookup-key' will
-recognize the default bindings, just as `read-key-sequence' does.  */)
-  (Lisp_Object keymap, Lisp_Object key, Lisp_Object accept_default)
+static Lisp_Object
+lookup_key_1 (Lisp_Object keymap, Lisp_Object key, Lisp_Object accept_default)
 {
   bool t_ok = !NILP (accept_default);
 
@@ -1243,6 +1224,63 @@ DEFUN ("lookup-key", Flookup_key, Slookup_key, 2, 3, 0,
     }
 }
 
+/* Value is number if KEY is too long; nil if valid but has no definition.  */
+/* GC is possible in this function.  */
+
+DEFUN ("lookup-key", Flookup_key, Slookup_key, 2, 3, 0,
+       doc: /* Look up key sequence KEY in KEYMAP.  Return the definition.
+A value of nil means undefined.  See doc of `define-key'
+for kinds of definitions.
+
+A number as value means KEY is "too long";
+that is, characters or symbols in it except for the last one
+fail to be a valid sequence of prefix characters in KEYMAP.
+The number is how many characters at the front of KEY
+it takes to reach a non-prefix key.
+KEYMAP can also be a list of keymaps.
+
+Normally, `lookup-key' ignores bindings for t, which act as default
+bindings, used when nothing else in the keymap applies; this makes it
+usable as a general function for probing keymaps.  However, if the
+third optional argument ACCEPT-DEFAULT is non-nil, `lookup-key' will
+recognize the default bindings, just as `read-key-sequence' does.  */)
+  (Lisp_Object keymap, Lisp_Object key, Lisp_Object accept_default)
+{
+  Lisp_Object found = lookup_key_1 (keymap, key, accept_default);
+  if (!NILP (found) && !NUMBERP (found))
+    return found;
+
+  /* Menu definitions might use mixed case symbols (notably in old
+     versions of `easy-menu-define').  We accept this variation for
+     backwards-compatibility.  (Bug#50752)  */
+  ptrdiff_t key_len = ASIZE (key);
+  if (VECTORP (key) && key_len > 0 && EQ (AREF (key, 0), Qmenu_bar))
+    {
+      Lisp_Object new_key = make_vector (key_len, Qnil);
+      for (int i = 0; i < key_len; ++i)
+	{
+	  Lisp_Object sym = Fsymbol_name (AREF (key, i));
+	  USE_SAFE_ALLOCA;
+	  unsigned char *dst = SAFE_ALLOCA (SBYTES (sym) + 1);
+	  memcpy (dst, SSDATA (sym), SBYTES (sym));
+	  /* We can walk the string data byte by byte, because UTF-8
+	     encoding ensures that no other byte of any multibyte
+	     sequence will ever include a 7-bit byte equal to an ASCII
+	     single-byte character.  */
+	  for (int j = 0; j < SBYTES (sym); ++j)
+	    if (dst[j] >= 'A' && dst[j] <= 'Z')
+	      dst[j] += 'a' - 'A';  /* Convert to lower case.  */
+	  ASET (new_key, i, Fintern (make_multibyte_string ((char *) dst,
+							    SCHARS (sym),
+							    SBYTES (sym)),
+				     Qnil));
+	  SAFE_FREE ();
+	}
+      found = lookup_key_1 (keymap, new_key, accept_default);
+    }
+  return found;
+}
+
 /* Make KEYMAP define event C as a keymap (i.e., as a prefix).
    Assume that currently it does not define C at all.
    Return the keymap.  */
diff --git a/test/src/keymap-tests.el b/test/src/keymap-tests.el
index 68b42c346c..1943e719ab 100644
--- a/test/src/keymap-tests.el
+++ b/test/src/keymap-tests.el
@@ -124,6 +124,16 @@ keymap-lookup-key/too-long
 ;; (ert-deftest keymap-lookup-key/accept-default ()
 ;;   ...)
 
+(ert-deftest keymap-lookup-key/mixed-case ()
+  "Backwards compatibility behaviour (Bug#50752)."
+  (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)))
+  (let ((map (make-keymap)))
+    (define-key map [menu-bar i-bar] 'foo)
+    (should (eq (lookup-key map [menu-bar I-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"
-- 
2.30.2

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

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Wed, 27 Oct 2021 22:29:48 -0700
> Cc: larsi@gnus.org, rgm@gnu.org, 50752@debbugs.gnu.org, shuguang79@qq.com
> 
> >>     if (ch >= 'A' && ch <= 'Z')
> >>         new_ch = c + ('A' - 'a');
> >
> > The latter.
> 
> OK, I guess that should be the attached.  I hope I got it right.

Yes, thanks.  But you still didn't say "don't merge to master", for
some reason.

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

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

> Yes, thanks.  But you still didn't say "don't merge to master", for
> some reason.

AFAICT, the commit message in the patch I sent does say so, right at the end.

Is it not in the correct format?

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

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Thu, 28 Oct 2021 01:06:45 -0700
> Cc: larsi@gnus.org, rgm@gnu.org, 50752@debbugs.gnu.org, shuguang79@qq.com
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Yes, thanks.  But you still didn't say "don't merge to master", for
> > some reason.
> 
> AFAICT, the commit message in the patch I sent does say so, right at the end.

It does indeed, sorry for being blind.

> Is it not in the correct format?

Not wrong, just not easy to read.

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

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

> Not wrong, just not easy to read.

Indeed, it's sort of easy to miss.  Noam seems to have used this more
visible format before that I chose to copy:

    Don't merge to master.  This is a safe-for-release fix for Bug#40727.

(I also moved it to the third line for good measure.)

With that change, this fix is now pushed to emacs-28 (commit
0f8417d597).  Should I go ahead and push the patch for master as well?

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

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Thu, 28 Oct 2021 06:49:12 -0400
> Cc: larsi@gnus.org, rgm@gnu.org, 50752@debbugs.gnu.org, shuguang79@qq.com
> 
> With that change, this fix is now pushed to emacs-28 (commit
> 0f8417d597).  Should I go ahead and push the patch for master as well?

The more thorough one, which also supported spaces instead of dashes?
Yes, I think so.

Thanks.

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

[Stefan Kangas]

close 50752 28.1
thanks

Eli Zaretskii <eliz@gnu.org> writes:

> The more thorough one, which also supported spaces instead of dashes?
> Yes, I think so.

OK, now done (commit 2671ea0de8).  I'm consequently closing this bug
report, and marking it as done in "28.1" (as the recipe in the original
bug report was fixed in that version).

BTW, thanks for the assertion fix on emacs-28.  I had somehow forgotten
to re-enable the assertion flags after disabling them to try something
out the other day.  I'll try to remember to double check that next time.