bug#27470: 26.0.50; this-command-keys and interactive input

bug#27470: 26.0.50; this-command-keys and interactive input

[Michael Heerdegen]

Hello,

Evaluate this:

(defun test (arg)
  (interactive (list (read-from-minibuffer "Input: ")))
  (message "%S" (cons this-command (this-command-keys)))
  arg)

(global-set-key [f12] #'test)

where f12 stands for some random key sequence.

Hit f12 and give any input.  I get the following message:

(test . "^M")

I would expect (test . f12).

Obviously, `this-command-keys' returns the key confirming the input
instead of the key that has invoked the command.  This contradicts the
descriptions in the documentation (docstring and manual).  The docs only
mention an exception when a command calls `read-key-sequence', but this
is not the case here.  FWIW, I would prefer fixing this over changing
the docs.


Thanks,

Michael.


In GNU Emacs 26.0.50 (build 3, x86_64-pc-linux-gnu, GTK+ Version 3.22.11)
 of 2017-06-19 built on drachen
Repository revision: 1b75af59b305867c89271905be72a05d06a4eff4
Windowing system distributor 'The X.Org Foundation', version 11.0.11903000
System Description:	Debian GNU/Linux 9.0 (stretch)

bug#27470: 26.0.50; this-command-keys and interactive input

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Date: Sat, 24 Jun 2017 07:58:47 +0200
> 
> (defun test (arg)
>   (interactive (list (read-from-minibuffer "Input: ")))
>   (message "%S" (cons this-command (this-command-keys)))
>   arg)
> 
> (global-set-key [f12] #'test)
> 
> where f12 stands for some random key sequence.
> 
> Hit f12 and give any input.  I get the following message:
> 
> (test . "^M")
> 
> I would expect (test . f12).

This happens because read-from-minibuffer enters recursive-edit, and
we don't preserve this-command-keys across recursive-edit invocations.

This has been Emacs behavior since forever.

The patch below seems to fix this, but I have no idea what it could
break, given how old this behavior is.  So if we are to accept this,
I'd like to have a few people step forward and run with the change for
a month or so, to see there are no adverse effects.  We've been burnt
several times lately by changes in keyboard-input stuff that broke
subtle but very useful features.

Thanks.

diff --git a/src/minibuf.c b/src/minibuf.c
index 1bbe276..5617941 100644
--- a/src/minibuf.c
+++ b/src/minibuf.c
@@ -497,6 +497,8 @@ read_minibuf (Lisp_Object map, Lisp_Object initial, Lisp_Object prompt,
 				  Fcons (Vminibuffer_history_position,
 					 Fcons (Vminibuffer_history_variable,
 						minibuf_save_list))))));
+  minibuf_save_list
+    = Fcons (Fthis_command_keys_vector (), minibuf_save_list);
 
   record_unwind_protect_void (read_minibuf_unwind);
   minibuf_level++;
@@ -836,6 +838,11 @@ read_minibuf_unwind (void)
   Fset_buffer (XWINDOW (window)->contents);
 
   /* Restore prompt, etc, from outer minibuffer level.  */
+  Lisp_Object key_vec = Fcar (minibuf_save_list);
+  eassert (VECTORP (key_vec));
+  this_command_key_count = XFASTINT (Flength (key_vec));
+  this_command_keys = key_vec;
+  minibuf_save_list = Fcdr (minibuf_save_list);
   minibuf_prompt = Fcar (minibuf_save_list);
   minibuf_save_list = Fcdr (minibuf_save_list);
   minibuf_prompt_width = XFASTINT (Fcar (minibuf_save_list));

bug#27470: 26.0.50; this-command-keys and interactive input

[Stefan Monnier]

> (defun test (arg)
>   (interactive (list (read-from-minibuffer "Input: ")))
>   (message "%S" (cons this-command (this-command-keys)))
>   arg)

Whether it's a bug or not is hard to say (the quirk is described in the
doctring).  But you can get the behavior you want with:

    (defun sm-test (arg &optional keys)
      (interactive
        (let ((keys (this-command-keys-vector)))
          (list (read-from-minibuffer "Input: ") keys)))
      (message "%S" (cons this-command keys))
      arg)


-- Stefan

bug#27470: 26.0.50; this-command-keys and interactive input

[Michael Heerdegen]

Eli Zaretskii <eliz@gnu.org> writes:

> The patch below seems to fix this, but I have no idea what it could
> break, given how old this behavior is.

Thanks.  Solves the issue for me.  I'll keep using your patch so I'm the
first tester.


Michael.

bug#27470: 26.0.50; this-command-keys and interactive input

[Eli Zaretskii]

> Date: Sat, 24 Jun 2017 10:01:17 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 27470@debbugs.gnu.org
> 
> > From: Michael Heerdegen <michael_heerdegen@web.de>
> > Date: Sat, 24 Jun 2017 07:58:47 +0200
> > 
> > (defun test (arg)
> >   (interactive (list (read-from-minibuffer "Input: ")))
> >   (message "%S" (cons this-command (this-command-keys)))
> >   arg)
> > 
> > (global-set-key [f12] #'test)
> > 
> > where f12 stands for some random key sequence.
> > 
> > Hit f12 and give any input.  I get the following message:
> > 
> > (test . "^M")
> > 
> > I would expect (test . f12).
> 
> This happens because read-from-minibuffer enters recursive-edit, and
> we don't preserve this-command-keys across recursive-edit invocations.
> 
> This has been Emacs behavior since forever.
> 
> The patch below seems to fix this, but I have no idea what it could
> break, given how old this behavior is.  So if we are to accept this,
> I'd like to have a few people step forward and run with the change for
> a month or so, to see there are no adverse effects.  We've been burnt
> several times lately by changes in keyboard-input stuff that broke
> subtle but very useful features.

Michael, any findings so far?  Do you recommend that we push that
change?

Thanks.

bug#27470: 26.0.50; this-command-keys and interactive input

[Michael Heerdegen]

Eli Zaretskii <eliz@gnu.org> writes:

> Michael, any findings so far?

No, everything seems to be fine.

> Do you recommend that we push that change?

It solves my problem, so - yes!  But I can't judge whether it could
potentially cause any trouble.  Anyway, it's small and easily
revertible, so I see no reason to hold it back.


Thanks,

Michael.

bug#27470: 26.0.50; this-command-keys and interactive input

[Eli Zaretskii]

> From: Michael Heerdegen <michael_heerdegen@web.de>
> Cc: 27470@debbugs.gnu.org
> Date: Sun, 23 Jul 2017 17:44:21 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Michael, any findings so far?
> 
> No, everything seems to be fine.
> 
> > Do you recommend that we push that change?
> 
> It solves my problem, so - yes!  But I can't judge whether it could
> potentially cause any trouble.  Anyway, it's small and easily
> revertible, so I see no reason to hold it back.

Pushed to master.

Thanks for testing this.