woman.el broken?

woman.el broken?

[T.V Raman]

Trying to load woman.el
produces:
Wrong type argument: keymapp, nil

Appears to come from the call to woman-dired-define-keys  suspect some
kind of incorrect load order during compile.

-- 

Thanks,

--Raman
♉ Id: kg:/m/0285kf1  🦮

-- 

Thanks,

--Raman
♉ Id: kg:/m/0285kf1  🦮

Re: woman.el broken?

[Lars Ingebrigtsen]

"T.V Raman" <raman@google.com> writes:

> Trying to load woman.el
> produces:
> Wrong type argument: keymapp, nil

Yup.  This should now be fixed.

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

Re: woman.el broken?

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Mon, 01 Mar 2021 20:48:38 +0100
> Cc: emacs-devel@gnu.org
> 
> "T.V Raman" <raman@google.com> writes:
> 
> > Trying to load woman.el
> > produces:
> > Wrong type argument: keymapp, nil
> 
> Yup.  This should now be fixed.

So this means that converting any Lisp file to use easymenu will
potentially break all the Lisp programs out there that reference its
menus, due to letter-case issues?  That problem alone is IMO enough to
stop this conversion, until we fix look up-key to not break other
packages, or find some other way of preventing such problems.

Re: woman.el broken?

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> So this means that converting any Lisp file to use easymenu will
> potentially break all the Lisp programs out there that reference its
> menus, due to letter-case issues?  That problem alone is IMO enough to
> stop this conversion, until we fix look up-key to not break other
> packages, or find some other way of preventing such problems.

Yes, fixing the lookup to be more forgiving here would be nice.

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

Re: woman.el broken?

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Mon, 01 Mar 2021 21:07:00 +0100
> Cc: emacs-devel@gnu.org
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > So this means that converting any Lisp file to use easymenu will
> > potentially break all the Lisp programs out there that reference its
> > menus, due to letter-case issues?  That problem alone is IMO enough to
> > stop this conversion, until we fix look up-key to not break other
> > packages, or find some other way of preventing such problems.
> 
> Yes, fixing the lookup to be more forgiving here would be nice.

Stefan, would you please make that happen?  Without that, the switch
to easymenu is an incompatible change, ironic as it may be.

Re: woman.el broken?

[Eli Zaretskii]

> Date: Mon, 01 Mar 2021 22:19:14 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: emacs-devel@gnu.org
> 
> > Yes, fixing the lookup to be more forgiving here would be nice.
> 
> Stefan, would you please make that happen?  Without that, the switch
> to easymenu is an incompatible change, ironic as it may be.

Btw, it isn't just letter-case, but also "Foo Bar Baz" as opposed to
"foo-bar-baz", right?

Re: woman.el broken?

[Stefan Monnier]

> Btw, it isn't just letter-case, but also "Foo Bar Baz" as opposed to
> "foo-bar-baz", right?

It can be anything, really: the pseudo-events used in the easymenu code
is auto-generated from the menu entry's text, but in the "old-style"
code, it's generated by hand by the programmer, so it's in general
impossible to find one from the other.  We can at best add heuristics.


        Stefan

Re: woman.el broken?

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: stefan@marxist.se,  larsi@gnus.org,  emacs-devel@gnu.org
> Date: Mon, 01 Mar 2021 15:42:09 -0500
> 
> > Btw, it isn't just letter-case, but also "Foo Bar Baz" as opposed to
> > "foo-bar-baz", right?
> 
> It can be anything, really: the pseudo-events used in the easymenu code
> is auto-generated from the menu entry's text, but in the "old-style"
> code, it's generated by hand by the programmer, so it's in general
> impossible to find one from the other.  We can at best add heuristics.

Sure.  I _was_ talking about heuristics, based on what I see in the
sources.  We probably should have this in "Incompatible changes" part
regardless, since heuristics could fail.

Re: woman.el broken?

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Stefan, would you please make that happen?  Without that, the switch
> to easymenu is an incompatible change, ironic as it may be.

I've poked around a bit here, and I'm not quite sure what the right fix
is.

Typically, in the old-style definitions, it looks like this:

    (define-key map [menu-bar subdir]

The easy-menu incantation is:

(easy-menu-define dired-mode-subdir-menu dired-mode-map
  "Subdir menu for Dired mode."
  '("Subdir"

It then constructs (define-key map [menu-bar Subdir] ...) by just
interning that string.

Just downcasing that string would probably fix the problem (after
reverting the two quick fixes I applied) -- we commonly only use small
characters in these things for naming.

It could also be tweaked in access_keymap_1, but...  we're comparing
there with EQ, and we want to continue doing that.

Another way would be to allow a :name (or something) keyword to specify
the symbol if we don't want the default name, so ":name 'subdir" in this
case.

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

Re: woman.el broken?

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Just downcasing that string would probably fix the problem (after
> reverting the two quick fixes I applied) -- we commonly only use small
> characters in these things for naming.

There are no in-tree instances of lookup-key with an upper case
character (except the ones I added) in the Emacs tree, so that seems
like a somewhat promising (and easy) fix...

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

Re: woman.el broken?

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> There are no in-tree instances of lookup-key with an upper case
> character

(in menu-bar lookups)

> (except the ones I added) in the Emacs tree, so that seems
> like a somewhat promising (and easy) fix...

Pushed now.

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

Re: woman.el broken?

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

>> > So this means that converting any Lisp file to use easymenu will
>> > potentially break all the Lisp programs out there that reference its
>> > menus, due to letter-case issues?  That problem alone is IMO enough to
>> > stop this conversion, until we fix look up-key to not break other
>> > packages, or find some other way of preventing such problems.
>>
>> Yes, fixing the lookup to be more forgiving here would be nice.
>
> Stefan, would you please make that happen?  Without that, the switch
> to easymenu is an incompatible change, ironic as it may be.

Would something along these lines be okay or do we strictly need all of
this to be in C for some reason?

    (defun lookup-key (keymap key accept-default)
      (or (lookup-key-internal keymap key accept-default)
          (and (vectorp key)
               (let ((lc-key `[,@(mapcar
                                  (lambda (s)
                                    (intern (downcase (symbol-name s))))
                                  key)]))
                 (when (not (equal lc-key key))
                   (lookup-key-internal keymap lc-key accept-default))))))

(`lookup-key-internal' would be in C with the same definition as
`lookup-key' has today.)

Re: woman.el broken?

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Tue, 2 Mar 2021 20:23:53 -0600
> Cc: emacs-devel@gnu.org
> 
> >> Yes, fixing the lookup to be more forgiving here would be nice.
> >
> > Stefan, would you please make that happen?  Without that, the switch
> > to easymenu is an incompatible change, ironic as it may be.
> 
> Would something along these lines be okay or do we strictly need all of
> this to be in C for some reason?

If this is performant enough, I don't at the moment see any reason to
have it in C.  Of course, the few places that call Flookup_key from C
will need to be analyzed whether they need to call the internal
function or the Lisp wrapper, and modified accordingly.

>     (defun lookup-key (keymap key accept-default)
>       (or (lookup-key-internal keymap key accept-default)
>           (and (vectorp key)
>                (let ((lc-key `[,@(mapcar
>                                   (lambda (s)
>                                     (intern (downcase (symbol-name s))))
>                                   key)]))
>                  (when (not (equal lc-key key))
>                    (lookup-key-internal keymap lc-key accept-default))))))

This is not the only transformation we should apply, IMO: we should
also convert "Foo Bar Baz" into "foo-bar-baz".  See the Dired menus
for examples.

Re: woman.el broken?

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

> If this is performant enough, I don't at the moment see any reason to
> have it in C.  Of course, the few places that call Flookup_key from C
> will need to be analyzed whether they need to call the internal
> function or the Lisp wrapper, and modified accordingly.

OK.  I will write up the patch and do some benchmarks.

>>     (defun lookup-key (keymap key accept-default)
>>       (or (lookup-key-internal keymap key accept-default)
>>           (and (vectorp key)
>>                (let ((lc-key `[,@(mapcar
>>                                   (lambda (s)
>>                                     (intern (downcase (symbol-name s))))
>>                                   key)]))
>>                  (when (not (equal lc-key key))
>>                    (lookup-key-internal keymap lc-key accept-default))))))
>
> This is not the only transformation we should apply, IMO: we should
> also convert "Foo Bar Baz" into "foo-bar-baz".  See the Dired menus
> for examples.

Yes, I will add that too.

For "Foo Bar Baz", is it enough to look for "foo-bar-baz" or would we
need to look for both "Foo-Bar-Baz" as well?

Re: woman.el broken?

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Wed, 3 Mar 2021 08:19:44 -0600
> Cc: larsi@gnus.org, emacs-devel@gnu.org
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > If this is performant enough, I don't at the moment see any reason to
> > have it in C.  Of course, the few places that call Flookup_key from C
> > will need to be analyzed whether they need to call the internal
> > function or the Lisp wrapper, and modified accordingly.
> 
> OK.  I will write up the patch and do some benchmarks.

Thanks.

> > This is not the only transformation we should apply, IMO: we should
> > also convert "Foo Bar Baz" into "foo-bar-baz".  See the Dired menus
> > for examples.
> 
> Yes, I will add that too.
> 
> For "Foo Bar Baz", is it enough to look for "foo-bar-baz" or would we
> need to look for both "Foo-Bar-Baz" as well?

Based on what I see in our tree, only the former, but please
double-check to make sure I didn't miss anything.

Re: woman.el broken?

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

>> For "Foo Bar Baz", is it enough to look for "foo-bar-baz" or would we
>> need to look for both "Foo-Bar-Baz" as well?
>
> Based on what I see in our tree, only the former, but please
> double-check to make sure I didn't miss anything.

Will do, thanks.

Re: woman.el broken?

[Stefan Kangas]

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

Stefan Kangas <stefan@marxist.se> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>> If this is performant enough, I don't at the moment see any reason to
>> have it in C.  Of course, the few places that call Flookup_key from C
>> will need to be analyzed whether they need to call the internal
>> function or the Lisp wrapper, and modified accordingly.
>
> OK.  I will write up the patch and do some benchmarks.

Turns out this was actually easier and faster to just do in C, as there
were less places that needed changing.  See the attached diff.

The thing that is missing now is converting Foo\ Bar to foo-bar, but
what is the best method for doing that from C?  Should I just call out
to `string-replace' or is there a better way?

[-- Attachment #2: lookup-key.diff --]
[-- Type: text/x-diff, Size: 4298 bytes --]

diff --git a/src/keymap.c b/src/keymap.c
index 782931fadf..94d49ac733 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,46 @@ 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);
+
+  /* Since menu definitions sometimes use mixed case identifiers
+     (notably in old versions of `easy-menu-define'), also look for
+     the lowercase version.  */
+  if ((NILP (found) || NUMBERP (found))
+      && VECTORP (key) && EQ (AREF (key, 0), Qmenu_bar))
+    {
+      Lisp_Object new_key = Fmake_vector (make_fixnum (ASIZE (key)), Qnil);
+      for (int i = 0; i < ASIZE (key); i++)
+	  ASET (new_key, i, Fintern (Fdowncase (Fsymbol_name (AREF (key, i))),
+				     Qnil));
+      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 a9b0cb502d..19b4013b21 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"

Re: woman.el broken?

[Eli Zaretskii]

> From: Stefan Kangas <stefan@marxist.se>
> Date: Fri, 5 Mar 2021 19:43:24 -0800
> Cc: larsi@gnus.org, emacs-devel@gnu.org
> 
> >> If this is performant enough, I don't at the moment see any reason to
> >> have it in C.  Of course, the few places that call Flookup_key from C
> >> will need to be analyzed whether they need to call the internal
> >> function or the Lisp wrapper, and modified accordingly.
> >
> > OK.  I will write up the patch and do some benchmarks.
> 
> Turns out this was actually easier and faster to just do in C, as there
> were less places that needed changing.  See the attached diff.

Thanks.

> The thing that is missing now is converting Foo\ Bar to foo-bar, but
> what is the best method for doing that from C?  Should I just call out
> to `string-replace' or is there a better way?

Something like this should do:

  . copy the original string data to a local buffer (use
    USE_SAFE_ALLOCA and SAFE_ALLOCA to allocate a suitable buffer)
  . replace each space with a '-' in a simple loop that examines each
    character in the above buffer
  . use build_string to create a new string from the replaced contents

> +      Lisp_Object new_key = Fmake_vector (make_fixnum (ASIZE (key)), Qnil);

I think it's better to use make_vector here.

A general comment: many Emacs primitives are just thin wrappers around
C functions; those wrappers typically take care of checking the type
of the arguments, converting Lisp data types to C data types, etc.  In
this case, you will see that Fmake_vector calls make_vector, which is
its workhorse.

When calling those primitives from C, it is generally better to call
the C workhorse instead of the primitive, because that avoids wasting
cycles on checking data types that are already known in advance (by C
rules) to be correct, and consing Lisp data from the underlying C data
(like the call to make_fixnum in this case, but it's even more start
when you need to make a Lisp string from a C string).  The convenience
of such calls from C is actually one reason why we generally prefer to
implement primitives in this 2-layer fashion.