bug#62207: 29.0.60; Trying to remove non-existent key binding instead adds a binding

bug#62207: 29.0.60; Trying to remove non-existent key binding instead adds a binding

[Jonas Bernoulli]

(keymap-unset map key t) is supposed to remove a binding completely,
and that works if there actually is a binding:

  (let ((map (make-sparse-keymap)))
    (keymap-set map "i" 'bound)
    (keymap-unset map "i" t)
    map)
  => (keymap)

However if you try to remove a binding that does not actually exist,
then the opposite happens, a "nil binding" is *added*:

  (let ((map (make-sparse-keymap)))
    (keymap-unset map "i" t)
    map)
  => (keymap (105))

bug#62207: 29.0.60; Trying to remove non-existent key binding instead adds a binding

[Jonas Bernoulli]

As a side-note, it would be nice if it were possible to lookup a
key in a keymap only, while ignoring bindings in its parent keymap.

I was reminded of this because I had to resurrect some old code of
mine to work around this bug.  The workaround is:

(defun my--keymap-unset (keymap key &optional remove)
  (if remove
      (when (kmu-lookup-local-key keymap key nil t)
        (keymap-unset keymap key t))
    (keymap-unset keymap key)))

And below is the code I had to resurrect in order to make that happen.
If `lookup-key', and similar, took a NOPARENT argument, all of that
could be avoided.

(defun kmu-lookup-local-key ( keymap key
                              &optional accept-default no-remap position)
  "In KEYMAP, look up key sequence KEY.  Return the definition.

Unlike `keymap-lookup' (which see) this doesn't consider bindings
made in KEYMAP's parent keymap."
  (keymap-lookup (kmu--strip-keymap keymap)
                 key accept-default no-remap position))

(defun kmu--strip-keymap (keymap)
  "Return a copy of KEYMAP with all parent keymaps removed.

This not only removes the parent keymap of KEYMAP but also recursively
the parent keymap of any keymap a key in KEYMAP is bound to."
  (cl-labels ((strip-keymap (keymap)
                (set-keymap-parent keymap nil)
                (cl-loop for _key being the key-code of keymap
                         using (key-binding binding) do
                         (and (keymapp binding)
                              (not (kmu-prefix-command-p binding))
                              (strip-keymap binding)))
                keymap))
    (strip-keymap (copy-keymap keymap))))

(defun kmu-prefix-command-p (object)
  "Return non-nil if OBJECT is a symbol whose function definition is a keymap.
The value returned is the keymap stored as OBJECT's variable
definition or else the variable which holds the keymap."
  (and (symbolp object)
       (fboundp object)
       (keymapp (symbol-function object))
       (if (and (boundp  object)
                (keymapp (symbol-value object)))
           (symbol-value object)
         (kmu-keymap-variable (symbol-function object)))))

(defun kmu-keymap-variable-p (object)
  "Return t if OBJECT is a symbol whose variable definition is a keymap."
  (and (symbolp object)
       (boundp  object)
       (keymapp (symbol-value object))))

bug#62207: 29.0.60; Trying to remove non-existent key binding instead adds a binding

[Eli Zaretskii]

> From: Jonas Bernoulli <jonas@bernoul.li>
> Date: Wed, 15 Mar 2023 17:07:42 +0100
> 
> However if you try to remove a binding that does not actually exist,
> then the opposite happens, a "nil binding" is *added*:
> 
>   (let ((map (make-sparse-keymap)))
>     (keymap-unset map "i" t)
>     map)
>   => (keymap (105))

The same happens when you call define-key with REMOVE non-nil.
keymap-unset just calls define-key, and does little else.

Stefan, it sounds like the part of store_in_keymap after the label
keymap_end should do nothing if REMOVE is non-zero, am I right?

bug#62207: 29.0.60; Trying to remove non-existent key binding instead adds a binding

[Robert Pluim]

>>>>> On Wed, 15 Mar 2023 17:51:27 +0100, Jonas Bernoulli <jonas@bernoul.li> said:

    Jonas> As a side-note, it would be nice if it were possible to lookup a
    Jonas> key in a keymap only, while ignoring bindings in its parent keymap.

A feature request and a bug report? Tsk ;-) Luckily the infrastructure
is actually there already.

The following passes my admittedly quick testing for both.

diff --git c/lisp/keymap.el i/lisp/keymap.el
index 4f02639ffe2..706da70d360 100644
--- c/lisp/keymap.el
+++ i/lisp/keymap.el
@@ -370,7 +370,7 @@ key-translate
 	    (make-char-table 'keyboard-translate-table nil)))
   (aset keyboard-translate-table (key-parse from) (key-parse to)))
 
-(defun keymap-lookup (keymap key &optional accept-default no-remap position)
+(defun keymap-lookup (keymap key &optional accept-default no-remap position noparent)
   "Return the binding for command KEY in KEYMAP.
 KEY is a string that satisfies `key-valid-p'.
 
@@ -406,8 +406,10 @@ keymap-lookup
   (keymap--check key)
   (when (and keymap position)
     (error "Can't pass in both keymap and position"))
+  (when (and (not keymap) noparent)
+    (error "Must specify keymap when noparent is t"))
   (if keymap
-      (let ((value (lookup-key keymap (key-parse key) accept-default)))
+      (let ((value (lookup-key keymap (key-parse key) accept-default noparent)))
         (if (and (not no-remap)
                    (symbolp value))
             (or (command-remapping value) value)
diff --git c/src/keymap.c i/src/keymap.c
index 23453eaa9a6..a660a687994 100644
--- c/src/keymap.c
+++ i/src/keymap.c
@@ -887,22 +887,23 @@ store_in_keymap (Lisp_Object keymap, register Lisp_Object idx,
   keymap_end:
     /* We have scanned the entire keymap, and not found a binding for
        IDX.  Let's add one.  */
-    {
-      Lisp_Object elt;
+    if (!remove)
+      {
+	Lisp_Object elt;
 
-      if (CONSP (idx) && CHARACTERP (XCAR (idx)))
-	{
-	  /* IDX specifies a range of characters, and not all of them
-	     were handled yet, which means this keymap doesn't have a
-	     char-table.  So, we insert a char-table now.  */
-	  elt = Fmake_char_table (Qkeymap, Qnil);
-	  Fset_char_table_range (elt, idx, NILP (def) ? Qt : def);
-	}
-      else
-	elt = Fcons (idx, def);
-      CHECK_IMPURE (insertion_point, XCONS (insertion_point));
-      XSETCDR (insertion_point, Fcons (elt, XCDR (insertion_point)));
-    }
+	if (CONSP (idx) && CHARACTERP (XCAR (idx)))
+	  {
+	    /* IDX specifies a range of characters, and not all of them
+	       were handled yet, which means this keymap doesn't have a
+	       char-table.  So, we insert a char-table now.  */
+	    elt = Fmake_char_table (Qkeymap, Qnil);
+	    Fset_char_table_range (elt, idx, NILP (def) ? Qt : def);
+	  }
+	else
+	  elt = Fcons (idx, def);
+	CHECK_IMPURE (insertion_point, XCONS (insertion_point));
+	XSETCDR (insertion_point, Fcons (elt, XCDR (insertion_point)));
+      }
   }
 
   return def;
@@ -1240,14 +1241,15 @@ DEFUN ("command-remapping", Fcommand_remapping, Scommand_remapping, 1, 3, 0,
   if (NILP (keymaps))
     command = Fkey_binding (command_remapping_vector, Qnil, Qt, position);
   else
-    command = Flookup_key (keymaps, command_remapping_vector, Qnil);
+    command = Flookup_key (keymaps, command_remapping_vector, Qnil, Qnil);
   return FIXNUMP (command) ? Qnil : command;
 }
 
 static Lisp_Object
-lookup_key_1 (Lisp_Object keymap, Lisp_Object key, Lisp_Object accept_default)
+lookup_key_1 (Lisp_Object keymap, Lisp_Object key, Lisp_Object accept_default, Lisp_Object noparent)
 {
   bool t_ok = !NILP (accept_default);
+  bool noinherit = !NILP (noparent);
 
   if (!CONSP (keymap) && !NILP (keymap))
     keymap = get_keymap (keymap, true, true);
@@ -1275,7 +1277,7 @@ lookup_key_1 (Lisp_Object keymap, Lisp_Object key, Lisp_Object accept_default)
       if (!FIXNUMP (c) && !SYMBOLP (c) && !CONSP (c) && !STRINGP (c))
 	message_with_string ("Key sequence contains invalid event %s", c, 1);
 
-      Lisp_Object cmd = access_keymap (keymap, c, t_ok, 0, 1);
+      Lisp_Object cmd = access_keymap (keymap, c, t_ok, noinherit, 1);
       if (idx == length)
 	return cmd;
 
@@ -1290,7 +1292,7 @@ lookup_key_1 (Lisp_Object keymap, Lisp_Object key, Lisp_Object accept_default)
 /* 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,
+DEFUN ("lookup-key", Flookup_key, Slookup_key, 2, 4, 0,
        doc: /* Look up key sequence KEY in KEYMAP.  Return the definition.
 This is a legacy function; see `keymap-lookup' for the recommended
 function to use instead.
@@ -1310,9 +1312,9 @@ DEFUN ("lookup-key", Flookup_key, Slookup_key, 2, 3, 0,
 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 keymap, Lisp_Object key, Lisp_Object accept_default, Lisp_Object noparent)
 {
-  Lisp_Object found = lookup_key_1 (keymap, key, accept_default);
+  Lisp_Object found = lookup_key_1 (keymap, key, accept_default, noparent);
   if (!NILP (found) && !NUMBERP (found))
     return found;
 
@@ -1390,7 +1392,7 @@ DEFUN ("lookup-key", Flookup_key, Slookup_key, 2, 3, 0,
 	}
 
       /* Check for match.  */
-      found = lookup_key_1 (keymap, new_key, accept_default);
+      found = lookup_key_1 (keymap, new_key, accept_default, noparent);
       if (!NILP (found) && !NUMBERP (found))
 	break;
 
@@ -1432,7 +1434,7 @@ DEFUN ("lookup-key", Flookup_key, Slookup_key, 2, 3, 0,
 	}
 
       /* Check for match.  */
-      found = lookup_key_1 (keymap, new_key, accept_default);
+      found = lookup_key_1 (keymap, new_key, accept_default, noparent);
       if (!NILP (found) && !NUMBERP (found))
 	break;
     }
@@ -1823,7 +1825,7 @@ DEFUN ("key-binding", Fkey_binding, Skey_binding, 1, 4, 0,
     }
 
   Lisp_Object value = Flookup_key (Fcurrent_active_maps (Qt, position),
-				   key, accept_default);
+				   key, accept_default, Qnil);
 
   if (NILP (value) || FIXNUMP (value))
     return Qnil;
@@ -1864,7 +1866,7 @@ DEFUN ("minor-mode-key-binding", Fminor_mode_key_binding, Sminor_mode_key_bindin
   int j;
   for (int i = j = 0; i < nmaps; i++)
     if (!NILP (maps[i])
-	&& !NILP (binding = Flookup_key (maps[i], key, accept_default))
+	&& !NILP (binding = Flookup_key (maps[i], key, accept_default, Qnil))
 	&& !FIXNUMP (binding))
       {
 	if (KEYMAPP (binding))
@@ -2013,7 +2015,7 @@ DEFUN ("accessible-keymaps", Faccessible_keymaps, Saccessible_keymaps,
     {
       /* If a prefix was specified, start with the keymap (if any) for
 	 that prefix, so we don't waste time considering other prefixes.  */
-      Lisp_Object tem = Flookup_key (keymap, prefix, Qt);
+      Lisp_Object tem = Flookup_key (keymap, prefix, Qt, Qnil);
       /* Flookup_key may give us nil, or a number,
 	 if the prefix is not defined in this particular map.
 	 It might even give us a list that isn't a keymap.  */
@@ -2453,7 +2455,7 @@ preferred_sequence_p (Lisp_Object seq)
 shadow_lookup (Lisp_Object keymap, Lisp_Object key, Lisp_Object accept_default,
 	       bool remap)
 {
-  Lisp_Object value = Flookup_key (keymap, key, accept_default);
+  Lisp_Object value = Flookup_key (keymap, key, accept_default, Qnil);
 
   if (FIXNATP (value))          /* `key' is too long!  */
     return Qnil;
@@ -3237,7 +3239,7 @@ describe_vector (Lisp_Object vector, Lisp_Object prefix, Lisp_Object args,
 	 one in the same keymap.  */
       if (!NILP (entire_map))
 	{
-	  Lisp_Object tem = Flookup_key (entire_map, kludge, Qt);
+	  Lisp_Object tem = Flookup_key (entire_map, kludge, Qt, Qnil);
 
 	  if (!EQ (tem, definition))
 	    continue;

bug#62207: 29.0.60; Trying to remove non-existent key binding instead adds a binding

[Robert Pluim]

>>>>> On Wed, 15 Mar 2023 19:13:20 +0200, Eli Zaretskii <eliz@gnu.org> said:

    >> From: Jonas Bernoulli <jonas@bernoul.li>
    >> Date: Wed, 15 Mar 2023 17:07:42 +0100
    >> 
    >> However if you try to remove a binding that does not actually exist,
    >> then the opposite happens, a "nil binding" is *added*:
    >> 
    >> (let ((map (make-sparse-keymap)))
    >> (keymap-unset map "i" t)
    >> map)
    >> => (keymap (105))

    Eli> The same happens when you call define-key with REMOVE non-nil.
    Eli> keymap-unset just calls define-key, and does little else.

    Eli> Stefan, it sounds like the part of store_in_keymap after the label
    Eli> keymap_end should do nothing if REMOVE is non-zero, am I right?

Thatʼs the conclusion I came to. See the patch in my other message.

Robert
-- 

bug#62207: 29.0.60; Trying to remove non-existent key binding instead adds a binding

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

> Stefan, it sounds like the part of store_in_keymap after the label
> keymap_end should do nothing if REMOVE is non-zero, am I right?

Sounds right, yes.  This said, the semantics of REMOVE a bit murky, so
I'd rather tell people not to use it.


        Stefan

bug#62207: 29.0.60; Trying to remove non-existent key binding instead adds a binding

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

>     Jonas> As a side-note, it would be nice if it were possible to lookup a
>     Jonas> key in a keymap only, while ignoring bindings in its parent keymap.

Could you explain why you need `keymap-unset-key` and why you need to
lookup "all but the ignore the parent"?

I'm curious because the meaning of these can be subtly more complex than
meets the eye once you consider things like `make-compose-keymap`.

> A feature request and a bug report? Tsk ;-) Luckily the infrastructure
> is actually there already.

Hmm... if you need to add yet-another-arg to `Flookup_key`, than I'm not
sure it qualifies as "the infrastructure is actually there already."

BTW, `map-keymap-internal` could be another way to attack the problem
(it has other downsides, but it stops before entering the parents).


        Stefan

bug#62207: 29.0.60; Trying to remove non-existent key binding instead adds a binding

[Jonas Bernoulli]

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

>>     Jonas> As a side-note, it would be nice if it were possible to lookup a
>>     Jonas> key in a keymap only, while ignoring bindings in its parent keymap.
>
> Could you explain why you need `keymap-unset-key`

Directional commands usually involve the keys "b", "p", "n" and "f".
These keys are mnemonic but are also scattered all across the keyboard.
So in my own Emacs I have chosen to use "physically-mnemonic" bindings
instead:     "i"
         "j" "k" "l"

[ Maybe that was a stupid decision.  But I made it very early in my
  readline/Emacs career.  There is no way back. ]

That makes it necessary to edit many keymaps.  Using keymap-unser with
non-nil REMOVE makes that much less painful.  It's still painful but any
relieve is welcome.

In magit-section-mode-map, for example, I make these changes

  default             custom  
  "p"     replaced by "i"     magit-section-backward
  "n"     replaced by "k"     magit-section-forward

magit-mode-map has the above keymap as parent.  It also binds "i"
to magit-gitignore.  I could just override that binding using

  (keymap-set magit-mode-map "i" #'magit-section-backward)

but it is much clean to

  (keymap-unset magit-mode-map "i" t)

It is also more future-proof.  Imagine the author of Magit decides
that magit-section-{forward,backward}-sibling are actually more
important than magit-section-{forward,backward}, and thus exchanges
the respective key bindings.

If I previously removed the conflicting binding in magit-mode-map,
my bindings continue to be consistent; "i"/"k" continue to do the
inverse of one another.  But if I was forced to explicitly double
down on bindings in derived keymaps, then that would be less robust.
In this case I would end up with this in magit-mode-map:

  "i" magit-section-backward
  "k" magit-section-forward-sibling

> lookup "all but the ignore the parent"?

kmu-lookup-local-key
    In KEYMAP, look up key sequence KEY.  Return the definition.
    Unlike `keymap-lookup' (which see) this doesn't consider bindings
    made in KEYMAP's parent keymap.

A long time ago I tried to automate the process of moving directional
commands to different keys.  When mapping through bindings of a keymap
in order to do that, it was important to ignore bindings made in the
parent keymap.  So I used a variant of map-keymap that did that, and it
made sense to implement a variant of lookup-key along the same line.  I
have not used these functions in a major way since I stopped trying to
automate the remapping.

But they just came in handy.  When using keymap-unset to remove a
binding but there isn't actually a binding, then that ends up adding
a binding.  To work around that bug I have to first check if there is
a binding.  But it must be a binding in the keymap itself, not a parent
keymap.

> I'm curious because the meaning of these can be subtly more complex than
> meets the eye once you consider things like `make-compose-keymap`.

My current implementation seems to treat later keymaps in composed
keymaps like parent keymaps.  Maybe that makes sense, maybe a proper
implementation should make a distinctions between parent keymaps and
"tail" keymaps.

>> A feature request and a bug report? Tsk ;-) Luckily the infrastructure
>> is actually there already.

I had a hunch the two might require changes in the same part of the
code. ;D

> Hmm... if you need to add yet-another-arg to `Flookup_key`, than I'm not
> sure it qualifies as "the infrastructure is actually there already."

What I care about the most is that the bug is fixed (instead of users
being told to not use the REMOVE argument, as you seem to suggest
elsewhere in this thread).

Key lookup that ignores parent keymaps would be nice, but not nearly as
important as the bugfix.

> BTW, `map-keymap-internal` could be another way to attack the problem
> (it has other downsides, but it stops before entering the parents).
>
>
>         Stefan

bug#62207: 29.0.60; Trying to remove non-existent key binding instead adds a binding

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Jonas Bernoulli <jonas@bernoul.li>,  Lars Ingebrigtsen <larsi@gnus.org>,
>   62207@debbugs.gnu.org
> Date: Wed, 15 Mar 2023 14:02:59 -0400
> 
> > Stefan, it sounds like the part of store_in_keymap after the label
> > keymap_end should do nothing if REMOVE is non-zero, am I right?
> 
> Sounds right, yes.

Then Robert, please install on the emacs-29 branch the part of your
suggested patch which fixes the problem with REMOVE.  The other part
should go to master, I think.

> This said, the semantics of REMOVE a bit murky, so I'd rather tell
> people not to use it.

Robert, could you please say something to this effect in the doc
string?

Thanks.

bug#62207: 29.0.60; Trying to remove non-existent key binding instead adds a binding

[Robert Pluim]

>>>>> On Fri, 17 Mar 2023 10:23:37 +0200, Eli Zaretskii <eliz@gnu.org> said:

    >> From: Stefan Monnier <monnier@iro.umontreal.ca>
    >> Cc: Jonas Bernoulli <jonas@bernoul.li>,  Lars Ingebrigtsen <larsi@gnus.org>,
    >> 62207@debbugs.gnu.org
    >> Date: Wed, 15 Mar 2023 14:02:59 -0400
    >> 
    >> > Stefan, it sounds like the part of store_in_keymap after the label
    >> > keymap_end should do nothing if REMOVE is non-zero, am I right?
    >> 
    >> Sounds right, yes.

    Eli> Then Robert, please install on the emacs-29 branch the part of your
    Eli> suggested patch which fixes the problem with REMOVE.  The other part
    Eli> should go to master, I think.

The former I can do. Stefan seemed to object to the other part. Itʼs
also not been tested a great deal 😀

    >> This said, the semantics of REMOVE a bit murky, so I'd rather tell
    >> people not to use it.

    Eli> Robert, could you please say something to this effect in the doc
    Eli> string?

Sorry to be difficult, but why? I think the semantics are clear,
although Iʼd prefer it if the first line of the docstring for
`keymap-unset' said

    Unset key sequence KEY in KEYMAP.

to reduce confusion with the REMOVE argument.

Robert
-- 

bug#62207: 29.0.60; Trying to remove non-existent key binding instead adds a binding

[Robert Pluim]

>>>>> On Fri, 17 Mar 2023 09:54:05 +0100, Robert Pluim <rpluim@gmail.com> said:
    Robert> Sorry to be difficult, but why? I think the semantics are clear,
    Robert> although Iʼd prefer it if the first line of the docstring for
    Robert> `keymap-unset' said

    Robert>     Unset key sequence KEY in KEYMAP.

    Robert> to reduce confusion with the REMOVE argument.

`keymap-unset' is also not documented in the lisp reference
manual. How about this?

@findex keymap-unset
@defun keymap-unset keymap key &optional remove
This function is the inverse of @code{keymap-set}, it unsets the
binding for @var{key} in @var{keymap}, which is the same as setting
the binding to @code{nil}.  In order to instead remove the binding
completely, specify @var{remove} as non-nil.  This only makes a
difference if @var{keymap} has a parent keymap.  When unsetting a key
in a child map, it will still shadow the same key in the parent
keymap.  Removing the binding will allow the key in the parent keymap
to be used.



Robert
-- 

bug#62207: 29.0.60; Trying to remove non-existent key binding instead adds a binding

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: Stefan Monnier <monnier@iro.umontreal.ca>,  larsi@gnus.org,
>   jonas@bernoul.li,  62207@debbugs.gnu.org
> Date: Fri, 17 Mar 2023 09:54:05 +0100
> 
> >>>>> On Fri, 17 Mar 2023 10:23:37 +0200, Eli Zaretskii <eliz@gnu.org> said:
> 
>     >> From: Stefan Monnier <monnier@iro.umontreal.ca>
>     >> Cc: Jonas Bernoulli <jonas@bernoul.li>,  Lars Ingebrigtsen <larsi@gnus.org>,
>     >> 62207@debbugs.gnu.org
>     >> Date: Wed, 15 Mar 2023 14:02:59 -0400
>     >> 
>     >> > Stefan, it sounds like the part of store_in_keymap after the label
>     >> > keymap_end should do nothing if REMOVE is non-zero, am I right?
>     >> 
>     >> Sounds right, yes.
> 
>     Eli> Then Robert, please install on the emacs-29 branch the part of your
>     Eli> suggested patch which fixes the problem with REMOVE.  The other part
>     Eli> should go to master, I think.
> 
> The former I can do. Stefan seemed to object to the other part. Itʼs
> also not been tested a great deal 😀

OK, then just the emacs-29 part for now.  The rest is not urgent
anyway.

>     >> This said, the semantics of REMOVE a bit murky, so I'd rather tell
>     >> people not to use it.
> 
>     Eli> Robert, could you please say something to this effect in the doc
>     Eli> string?
> 
> Sorry to be difficult, but why? I think the semantics are clear,
> although Iʼd prefer it if the first line of the docstring for
> `keymap-unset' said
> 
>     Unset key sequence KEY in KEYMAP.
> 
> to reduce confusion with the REMOVE argument.

If this part is still controversial, I'm okay with only fixing the bug
itself.  As for REMOVE, let's see what Stefan has to say in defense of
his request, and take it from there.

Thanks.

bug#62207: 29.0.60; Trying to remove non-existent key binding instead adds a binding

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: 62207@debbugs.gnu.org,  larsi@gnus.org,  jonas@bernoul.li,  Stefan
>  Monnier <monnier@iro.umontreal.ca>
> Date: Fri, 17 Mar 2023 10:55:23 +0100
> 
> `keymap-unset' is also not documented in the lisp reference
> manual. How about this?
> 
> @findex keymap-unset
> @defun keymap-unset keymap key &optional remove
> This function is the inverse of @code{keymap-set}, it unsets the
> binding for @var{key} in @var{keymap}, which is the same as setting
> the binding to @code{nil}.  In order to instead remove the binding
> completely, specify @var{remove} as non-nil.  This only makes a
> difference if @var{keymap} has a parent keymap.  When unsetting a key
> in a child map, it will still shadow the same key in the parent
> keymap.  Removing the binding will allow the key in the parent keymap
> to be used.

LGTM, thanks.

bug#62207: 29.0.60; Trying to remove non-existent key binding instead adds a binding

[Robert Pluim]

tags 62207 fixed
close 62207 29.1
quit

>>>>> On Fri, 17 Mar 2023 13:32:48 +0200, Eli Zaretskii <eliz@gnu.org> said:

    >> The former I can do. Stefan seemed to object to the other part. Itʼs
    >> also not been tested a great deal 😀

    Eli> OK, then just the emacs-29 part for now.  The rest is not urgent
    Eli> anyway.

done

    Eli> If this part is still controversial, I'm okay with only fixing the bug
    Eli> itself.  As for REMOVE, let's see what Stefan has to say in defense of
    Eli> his request, and take it from there.

ok

Closing.
Committed as bb3e0ded9eb

Robert
-- 

bug#62207: 29.0.60; Trying to remove non-existent key binding instead adds a binding

[Robert Pluim]

>>>>> On Fri, 17 Mar 2023 13:36:34 +0200, Eli Zaretskii <eliz@gnu.org> said:

    Eli> LGTM, thanks.

Done

Robert
-- 

bug#62207: 29.0.60; Trying to remove non-existent key binding instead adds a binding

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

> If this part is still controversial, I'm okay with only fixing the bug
> itself.  As for REMOVE, let's see what Stefan has to say in defense of
> his request, and take it from there.

I can't remember off-hand all the subtleties that can show up in corner
cases, but I'll simply note that the current removal code is already the
result of my pointing out several not-so-corner cases that the original
coder missed (even though he was not a beginner at the keymap game), and
this very bug report illustrates that the code is still buggy.

[ Which reminds me that we need a regression test for this.  ]

I think the removal code is "good enough" for uses in a user's init
code, but I'd rather add some recommendation to avoid on it in
ELisp packages.


        Stefan

bug#62207: 29.0.60; Trying to remove non-existent key binding instead adds a binding

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

>>>     Jonas> As a side-note, it would be nice if it were possible to lookup a
>>>     Jonas> key in a keymap only, while ignoring bindings in its parent keymap.
>>
>> Could you explain why you need `keymap-unset-key`
>
> Directional commands usually involve the keys "b", "p", "n" and "f".
> These keys are mnemonic but are also scattered all across the keyboard.
> So in my own Emacs I have chosen to use "physically-mnemonic" bindings
> instead:     "i"
>          "j" "k" "l"
>
> [ Maybe that was a stupid decision.  But I made it very early in my
>   readline/Emacs career.  There is no way back. ]
>
> That makes it necessary to edit many keymaps.  Using keymap-unser with
> non-nil REMOVE makes that much less painful.  It's still painful but any
> relieve is welcome.
>
> In magit-section-mode-map, for example, I make these changes
>
>   default             custom  
>   "p"     replaced by "i"     magit-section-backward
>   "n"     replaced by "k"     magit-section-forward

[ Indeed, Emacs's keymap system doesn't support this very well.
  For the same reason Evil/god-mode/boon/... need a lot of
  package-specific tweaks to "patch things up".  :-(  ]

> magit-mode-map has the above keymap as parent.  It also binds "i"
> to magit-gitignore.  I could just override that binding using
>
>   (keymap-set magit-mode-map "i" #'magit-section-backward)
>
> but it is much clean to
>
>   (keymap-unset magit-mode-map "i" t)

Agreed, this is a good use case, thanks.

> But they just came in handy.  When using keymap-unset to remove a
> binding but there isn't actually a binding, then that ends up adding
> a binding.

This is the bug we're discussing, right?  AFAIC it's a plain bug with an
easy fix (provided by Robert).

> To work around that bug I have to first check if there is a binding.
> But it must be a binding in the keymap itself, not a parent keymap.

Right, but this need is only brought about by the bug, so once the bug
is fixed, this need will disappear.  I hoped you had other needs for
that functionality.

>> I'm curious because the meaning of these can be subtly more complex than
>> meets the eye once you consider things like `make-compose-keymap`.
> My current implementation seems to treat later keymaps in composed
> keymaps like parent keymaps.  Maybe that makes sense, maybe a proper
> implementation should make a distinctions between parent keymaps and
> "tail" keymaps.

`map-keymap-internal` stops at both a parent keymap (i.e. a keymap in
the `cdr`) and a "subkeymap" (i.e. a keymap in the `car`).

> What I care about the most is that the bug is fixed

Agreed.

> (instead of users being told to not use the REMOVE argument, as you
> seem to suggest elsewhere in this thread).

I'm mostly worried about people using it in ELisp packages where it's
more likely to interact with many more situations.
For uses in an init file I think we can make it work well enough.


        Stefan

bug#62207: 29.0.60; Trying to remove non-existent key binding instead adds a binding

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Robert Pluim <rpluim@gmail.com>,  larsi@gnus.org,  jonas@bernoul.li,
>   62207@debbugs.gnu.org
> Date: Fri, 17 Mar 2023 16:51:56 -0400
> 
> I think the removal code is "good enough" for uses in a user's init
> code, but I'd rather add some recommendation to avoid on it in
> ELisp packages.

If we want to recommend that, we need to come up with a rationale and
a couple of alternative ways of achieving goals that commonly lead to
using this option.  Can you tell what to say to that effect?

bug#62207: 29.0.60; Trying to remove non-existent key binding instead adds a binding

[Robert Pluim]

>>>>> On Fri, 17 Mar 2023 16:51:56 -0400, Stefan Monnier via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org> said:

    >> If this part is still controversial, I'm okay with only fixing the bug
    >> itself.  As for REMOVE, let's see what Stefan has to say in defense of
    >> his request, and take it from there.

    Stefan> I can't remember off-hand all the subtleties that can show up in corner
    Stefan> cases, but I'll simply note that the current removal code is already the
    Stefan> result of my pointing out several not-so-corner cases that the original
    Stefan> coder missed (even though he was not a beginner at the keymap game), and
    Stefan> this very bug report illustrates that the code is still buggy.

    Stefan> [ Which reminds me that we need a regression test for this.  ]

Done. Let me know if there are corner cases I missed.

    Stefan> I think the removal code is "good enough" for uses in a user's init
    Stefan> code, but I'd rather add some recommendation to avoid on it in
    Stefan> ELisp packages.

We can put that in the elisp manual, no?

Robert
-- 

bug#62207: 29.0.60; Trying to remove non-existent key binding instead adds a binding

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

>> I think the removal code is "good enough" for uses in a user's init
>> code, but I'd rather add some recommendation to avoid on it in
>> ELisp packages.
>
> If we want to recommend that, we need to come up with a rationale and
> a couple of alternative ways of achieving goals that commonly lead to
> using this option.  Can you tell what to say to that effect?

I don't know of "goals that commonly lead to using this option", sorry.
AFAIK it's not used commonly (after all, it's brand new in Emacs-30).

Most of the requests for it I have seen were either of the "make the API
complete" kind of argument, or examples like Jonas' (which are
inherently tightly tied to a particular keymap and a particular binding
in that map, so the chance of bumping into an odd corner case is
significantly reduced).


        Stefan

bug#62207: 29.0.60; Trying to remove non-existent key binding instead adds a binding

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

> Done.

Thanks.

> Let me know if there are corner cases I missed.

I will if/when I can think of a new one.

>     Stefan> I think the removal code is "good enough" for uses in a user's init
>     Stefan> code, but I'd rather add some recommendation to avoid on it in
>     Stefan> ELisp packages.
> We can put that in the elisp manual, no?

No objection on my side :-)


        Stefan

bug#62207: 29.0.60; Trying to remove non-existent key binding instead adds a binding

[Robert Pluim]

>>>>> On Sat, 18 Mar 2023 10:07:02 -0400, Stefan Monnier via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org> said:

    >> Done.
    Bug> Thanks.

    >> Let me know if there are corner cases I missed.

    Bug> I will if/when I can think of a new one.

    Stefan> I think the removal code is "good enough" for uses in a user's init
    Stefan> code, but I'd rather add some recommendation to avoid on it in
    Stefan> ELisp packages.
    >> We can put that in the elisp manual, no?

    Bug> No objection on my side :-)

Something like this?

+Note: using @code{keymap-unset} with @var{remove} non-@code{nil} is
+intended for users to put in their init file; Emacs packages should
+avoid using it if possible, since they have complete control over
+their own keymaps anyway, and they should not be altering other
+packages' keymaps.
+

Robert
-- 

bug#62207: 29.0.60; Trying to remove non-existent key binding instead adds a binding

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: larsi@gnus.org,  Eli Zaretskii <eliz@gnu.org>,  jonas@bernoul.li,
>   62207@debbugs.gnu.org
> Date: Mon, 20 Mar 2023 10:09:03 +0100
> 
> +Note: using @code{keymap-unset} with @var{remove} non-@code{nil} is
> +intended for users to put in their init file; Emacs packages should
> +avoid using it if possible, since they have complete control over
> +their own keymaps anyway, and they should not be altering other
> +packages' keymaps.

Fine by me, but please wait for Stefan to respond.

bug#62207: 29.0.60; Trying to remove non-existent key binding instead adds a binding

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

> Something like this?
>
> +Note: using @code{keymap-unset} with @var{remove} non-@code{nil} is
> +intended for users to put in their init file; Emacs packages should
> +avoid using it if possible, since they have complete control over
> +their own keymaps anyway, and they should not be altering other
> +packages' keymaps.

Fine by me, thanks,


        Stefan

bug#62207: 29.0.60; Trying to remove non-existent key binding instead adds a binding

[Robert Pluim]

>>>>> On Mon, 20 Mar 2023 11:03:37 -0400, Stefan Monnier via "Bug reports for GNU Emacs, the Swiss army knife of text editors" <bug-gnu-emacs@gnu.org> said:

    >> Something like this?
    >> 
    >> +Note: using @code{keymap-unset} with @var{remove} non-@code{nil} is
    >> +intended for users to put in their init file; Emacs packages should
    >> +avoid using it if possible, since they have complete control over
    >> +their own keymaps anyway, and they should not be altering other
    >> +packages' keymaps.

    Stefan> Fine by me, thanks,

Done

Robert
-- 

bug#62207: 29.0.60; Trying to remove non-existent key binding instead adds a binding

[Jonas Bernoulli]

Robert Pluim <rpluim@gmail.com> writes:

> tags 62207 fixed
> close 62207 29.1
> quit
>
>>>>>> On Fri, 17 Mar 2023 13:32:48 +0200, Eli Zaretskii <eliz@gnu.org> said:
>
>     >> The former I can do. Stefan seemed to object to the other part. Itʼs
>     >> also not been tested a great deal 😀
>
>     Eli> OK, then just the emacs-29 part for now.  The rest is not urgent
>     Eli> anyway.
>
> done

Thanks a lot Robert!

     Jonas

bug#62207: 29.0.60; Trying to remove non-existent key binding instead adds a binding

[Jonas Bernoulli]

Eli Zaretskii <eliz@gnu.org> writes:

> Then Robert, please install on the emacs-29 branch the part of your
> suggested patch which fixes the problem with REMOVE.  The other part
> should go to master, I think.

The other part hasn't landed on master yet.  I agree it shouldn't go on
emacs-29, but would like to see it in master.  Stefan seems to have some
objections, which I believe boil down to "nobody has demonstrated a real
need yet, beside 'making the api complete'".

I don't have an immediate need, but I am fairly sure I would make use
of it eventually.  I haven't bothered with improving my personal key
bindings much lately, but plan to get back into the game eventually.

From the top of my head I can think of the following use-case.

The which-key package displays available bindings in some sort of popup
buffer, not unlike "C-h m", but more dynamic and less verbose.  I would
like to (eventually) create a package to display a more curated list of
bindings.

A minimal viable version of that, for my own personal, use would display
the keys I personally reserve for "directional commands", namely
[MOD]-{j,i,k,l}, along with the commands they are bound to in the
current buffer/mode.  When I start using a new mode, I would bring up
that bindings buffer, and if it showed me that, for example, "k" was
bound to sacrifice-goat, instead of some *-previous, then I would know
I had to remove that binding for my own use and possibly make other
adjustments.

At this point it would be nice if there were some indication in what
keymap the default binding, this-mode-map or parent-mode-map.  The UI
could even allow directly jumping to the definition of the appropriate
keymap.  Eventually it might even be possible to make the changes
directly from the popup buffer.

If that is to vague or obscure, I am fine with the
ignore-bindings-in-parent feature not being installed on master just
yet.  But in that case we should probably open a new issue to track it.

     Cheers,
     Jonas

bug#62207: 29.0.60; Trying to remove non-existent key binding instead adds a binding

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

> The other part hasn't landed on master yet.  I agree it shouldn't go on
> emacs-29, but would like to see it in master.  Stefan seems to have some
> objections, which I believe boil down to "nobody has demonstrated a real
> need yet, beside 'making the api complete'".

I can't remember exactly what "the other part" entails, but w.r.t
providing an optional arg that makes lookup-key ignores parents, I don't
think it would make the API complete (I can still see needs for yet more
optional arguments, most importantly one that doesn't look inside
`menu-item`s).

So I'd prefer to have a new lower-level function which does "the
minimum" but returns enough info that we can build something like
`lookup-key` on top of it.

I guess it would return 2 pieces of info: the binding that was found (if
any, potentially annotated with whether it was a default binding or
not), and the rest of the keymap.  Or maybe return the binding together
with the `cons` cell where that binding was found (i.e. along the lines
of what `member/memq` do).

> At this point it would be nice if there were some indication in what
> keymap the default binding, this-mode-map or parent-mode-map.  The UI
> could even allow directly jumping to the definition of the appropriate
> keymap.  Eventually it might even be possible to make the changes
> directly from the popup buffer.

`C-h k` tries to identify the keymap, but indeed it doesn't distinguish
between the keymap and its parent(s).  Patch welcome to improve this.


        Stefan