bug#46243: 26.3; If invoke menu item that reads a char, get keystrokes echo

bug#46243: 26.3; If invoke menu item that reads a char, get keystrokes echo

[Drew Adams]

There's no doubt a simpler thing to repro this with.  Sorry, I'm pressed
for time.

I see this starting with Emacs 24, and still in Emacs 27.1.  There's no
such problem before Emacs 24.

If I have a menu item that reads a character, the prompt for that read
is not seen; the menu selection key sequence gets echoed instead.  This
shouldn't happen (should it?).

Binding `echo-keystrokes' in the command's `interactive' spec doesn't
prevent this.  The workaround I've found is to add a call to (message
nil).

Example command:

(defun diredp-mark-with-char (char &optional arg)
  "Mark this line with CHAR.
With numeric prefix arg N, mark the next N lines."
  (interactive
   (progn (message nil)
	  (list (read-char "Mark this line with char: ")
		(prefix-numeric-value current-prefix-arg))))
  (let ((dired-marker-char  char))
    (dired-mark arg)))

If the `interactive' spec does not have that (message nil) then it
doesn't work - you then see the menu-invocation key sequence instead of
the prompt.  And this is so whether I use "cChar: \np" or a sexp with
`read-char'.

If I change to reading a string then there's no such problem:

(defun diredp-mark-with-char (char &optional arg)
  "Mark this line with CHAR.
With numeric prefix arg N, mark the next N lines."
  (interactive "sChar: \np")
  (let ((dired-marker-char  (string-to-char char)))
    (dired-mark arg)))

So that's another possible workaround.  (Is one of those better?)

Haven't been able to see why this problem occurs.

This is the menu item I have for the above command:

(define-key diredp-marks-mark-menu [diredp-mark-with-char]
  '(menu-item "Mark This with Char..." diredp-mark-with-char
              :visible (not (diredp-nonempty-region-p))
              :help "Mark this line with a character you type"))

I see the problem also with `easy-menu-create-menu':

(easy-menu-create-menu
 "This File"
 '(["Mark with Char..." diredp-mark-with-char]))

Please advise.  Am I missing something?  As I say, this was not a
problem before Emacs 24.

In GNU Emacs 26.3 (build 1, x86_64-w64-mingw32)
 of 2019-08-29
Repository revision: 96dd0196c28bc36779584e47fffcca433c9309cd
Windowing system distributor `Microsoft Corp.', version 10.0.18362
Configured using:
 `configure --without-dbus --host=x86_64-w64-mingw32
 --without-compress-install 'CFLAGS=-O2 -static -g3''

bug#46243: 26.3; If invoke menu item that reads a char, get keystrokes echo

[Drew Adams]

Actually, when I use even the workaround (with (message nil)) in a mouse-3 menu I still have the problem.  The workaround is OK for use in a menu-bar menu, at least.

bug#46243: 26.3; If invoke menu item that reads a char, get keystrokes echo

[Eli Zaretskii]

> From: Drew Adams <drew.adams@oracle.com>
> Date: Mon, 1 Feb 2021 18:50:56 +0000
> 
> If I have a menu item that reads a character, the prompt for that read
> is not seen; the menu selection key sequence gets echoed instead.  This
> shouldn't happen (should it?).

I think it should, but I couldn't verify that, because your recipes
didn't work for me.  Please provide a recipe that would work in
"emacs -Q".

bug#46243: [External] : Re: bug#46243: 26.3; If invoke menu item that reads a char, get keystrokes echo

[Drew Adams]

> > If I have a menu item that reads a character, the
> > prompt for that read is not seen; the menu selection
> > key sequence gets echoed instead.  This shouldn't
> > happen (should it?).
> 
> I think it should, but I couldn't verify that, because
> your recipes didn't work for me.  Please provide a
> recipe that would work in "emacs -Q".

emacs -Q            ; e.g. Emacs 27.1

(defun diredp-mark-with-char (char &optional arg)
  "Mark this line with CHAR.
With numeric prefix arg N, mark the next N lines."
  (interactive
   (progn (message nil)
	  (list (read-char "Mark this line with char: ")
		(prefix-numeric-value current-prefix-arg))))
  (let ((dired-marker-char  char))
    (dired-mark arg)))

(define-key dired-mode-map [menu-bar mark with-char]
  '(menu-item
     "Mark This with Char..." diredp-mark-with-char
     :help "Mark this line with a character you type"))

Now try the same thing but with (message nil) commented out.

With (message nil), you see the prompt:

   Mark this line with char:

Without (message nil), you see only this:

   menu-bar mark with-char-

The command works OK, but a user will have no clue
about typing a char.

Clear enough?  The change was introduced in Emacs 24,
AFAICT.  Regression or improvement?  Am I missing
something?

(You can forget about easy-menu for this bug report,
at least for now, please.)

bug#46243: [External] : Re: bug#46243: 26.3; If invoke menu item that reads a char, get keystrokes echo

[Eli Zaretskii]

> From: Drew Adams <drew.adams@oracle.com>
> CC: "46243@debbugs.gnu.org" <46243@debbugs.gnu.org>
> Date: Mon, 1 Feb 2021 22:30:54 +0000
> 
> > I think it should, but I couldn't verify that, because
> > your recipes didn't work for me.  Please provide a
> > recipe that would work in "emacs -Q".
> 
> emacs -Q            ; e.g. Emacs 27.1
> 
> (defun diredp-mark-with-char (char &optional arg)
>   "Mark this line with CHAR.
> With numeric prefix arg N, mark the next N lines."
>   (interactive
>    (progn (message nil)
> 	  (list (read-char "Mark this line with char: ")
> 		(prefix-numeric-value current-prefix-arg))))
>   (let ((dired-marker-char  char))
>     (dired-mark arg)))
> 
> (define-key dired-mode-map [menu-bar mark with-char]
>   '(menu-item
>      "Mark This with Char..." diredp-mark-with-char
>      :help "Mark this line with a character you type"))
> 
> Now try the same thing but with (message nil) commented out.
> 
> With (message nil), you see the prompt:
> 
>    Mark this line with char:
> 
> Without (message nil), you see only this:
> 
>    menu-bar mark with-char-
> 
> The command works OK, but a user will have no clue
> about typing a char.
> 
> Clear enough?

For the record, the recipe didn't work for me until I invoked Dired
before evaluating the second form.

> The change was introduced in Emacs 24, AFAICT.

In Emacs 24.4, to be exact.

> Regression or improvement?

Not clear yet, more analysis is required.  Stay tuned.

bug#46243: [External] : Re: bug#46243: 26.3; If invoke menu item that reads a char, get keystrokes echo

[Eli Zaretskii]

> Date: Tue, 02 Feb 2021 19:04:00 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: 46243@debbugs.gnu.org
> 
> > The change was introduced in Emacs 24, AFAICT.
> 
> In Emacs 24.4, to be exact.
> 
> > Regression or improvement?
> 
> Not clear yet, more analysis is required.  Stay tuned.

This is a consequence of fixing bug#15332.  CC'ing Stefan, who both
reported the bug and installed the fix.

Stefan, the change you made to fix that bug, viz.:

diff --git a/src/keyboard.c b/src/keyboard.c
index 440820c..020c885 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -2596,10 +2596,8 @@ read_char (int commandflag, Lisp_Object map,
 
   if (/* There currently is something in the echo area.  */
       !NILP (echo_area_buffer[0])
-      && (/* And it's either not from echoing.  */
-	  !EQ (echo_area_buffer[0], echo_message_buffer)
-	  /* Or it's an echo from a different kboard.  */
-	  || echo_kboard != current_kboard
+      && (/* It's an echo from a different kboard.  */
+	  echo_kboard != current_kboard
 	  /* Or we explicitly allow overwriting whatever there is.  */
 	  || ok_to_echo_at_next_pause == NULL))
     cancel_echoing ();
   else
     echo_dash ();

is what's causing this one: where we previously called cancel_echoing,
because !EQ (echo_area_buffer[0], echo_message_buffer) was true, we
now call echo_dash, because the remaining 2 conditions are false.

The change you installed is not explained, so I cannot reason about
the importance of echo_message_buffer.  Do you remember why you
decided to remove that 3rd condition?

bug#46243: [External] : Re: bug#46243: 26.3; If invoke menu item that reads a char, get keystrokes echo

[Stefan Monnier]

> This is a consequence of fixing bug#15332.  CC'ing Stefan, who both
> reported the bug and installed the fix.
> Stefan, the change you made to fix that bug, viz.:
>
> diff --git a/src/keyboard.c b/src/keyboard.c
> index 440820c..020c885 100644
> --- a/src/keyboard.c
> +++ b/src/keyboard.c
> @@ -2596,10 +2596,8 @@ read_char (int commandflag, Lisp_Object map,
>  
>    if (/* There currently is something in the echo area.  */
>        !NILP (echo_area_buffer[0])
> -      && (/* And it's either not from echoing.  */
> -	  !EQ (echo_area_buffer[0], echo_message_buffer)
> -	  /* Or it's an echo from a different kboard.  */
> -	  || echo_kboard != current_kboard
> +      && (/* It's an echo from a different kboard.  */
> +	  echo_kboard != current_kboard
>  	  /* Or we explicitly allow overwriting whatever there is.  */
>  	  || ok_to_echo_at_next_pause == NULL))
>      cancel_echoing ();
>    else
>      echo_dash ();
>
> is what's causing this one: where we previously called cancel_echoing,
> because !EQ (echo_area_buffer[0], echo_message_buffer) was true, we
> now call echo_dash, because the remaining 2 conditions are false.
>
> The change you installed is not explained, so I cannot reason about
> the importance of echo_message_buffer.  Do you remember why you
> decided to remove that 3rd condition?

I don't think I understood the echo-keystrokes code very back then and
I still don't understand it very much either.

This fixed the problem, but I'm not sure why.  I just tried to revert
this change and it does re-introduce part of the problem mentioned in
bug#15332:

    C-u [..wait..] 5

gives me immediately `C-u 5-` in the echo area whereas

    C-u [..wait..] 5 5

only shows `C-u 55-` after the `echo-keystrokes` delay.


        Stefan

bug#46243: [External] : Re: bug#46243: 26.3; If invoke menu item that reads a char, get keystrokes echo

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: drew.adams@oracle.com,  46243@debbugs.gnu.org
> Date: Wed, 03 Feb 2021 11:16:58 -0500
> 
> > The change you installed is not explained, so I cannot reason about
> > the importance of echo_message_buffer.  Do you remember why you
> > decided to remove that 3rd condition?
> 
> I don't think I understood the echo-keystrokes code very back then and
> I still don't understand it very much either.
> 
> This fixed the problem, but I'm not sure why.

Do you happen to remember how you came to think that
echo_message_buffer was involved in that old bug report's scenario?

bug#46243: [External] : Re: bug#46243: 26.3; If invoke menu item that reads a char, get keystrokes echo

[Stefan Monnier]

> Do you happen to remember how you came to think that
> echo_message_buffer was involved in that old bug report's scenario?

Sadly, no.


        Stefan

bug#46243: [External] : Re: bug#46243: 26.3; If invoke menu item that reads a char, get keystrokes echo

[Stefan Monnier]

>> Do you happen to remember how you came to think that
>> echo_message_buffer was involved in that old bug report's scenario?
> Sadly, no.

But I do have some relevant indirect info (it may seem unrelated at
first, but bear with me):

I was trying to write a test case for bug#15332.
Here's what I have so far:

    (ert-deftest keyboard-tests--echo-keystrokes-bug15332 ()
      (let ((msgs '())
            (unread-command-events nil)
            (redisplay--interactive t)
            (echo-keystrokes 2))
        (setq unread-command-events '(?\C-u))
        (let* ((timer1
                (run-with-timer 3 1
                                (lambda ()
                                  (setq unread-command-events '(?5)))))
               (timer2
                (run-with-timer 2.5 1
                                (lambda ()
                                  (push (current-message) msgs)))))
          (run-with-timer 5 nil
                          (lambda ()
                            (cancel-timer timer1)
                            (cancel-timer timer2)
                            (throw 'exit msgs)))
          (recursive-edit)
          (should (equal msgs '("C-u 55-" "C-u 5-" "C-u-"))))))

It works when Emacs is run interactively: when I revert the offending
patch, the test signals an error because the collected messages are only
`(nil "C-u 5-" "C-u-")` since the timer that collects the messages runs
just when the second `?5` event causes the echo to be erroneously
temporarily erased (until the next `echo-keystrokes` timeout).

Now I tried to make it work in batch mode, but so far I haven't been
successful.  As you can guess from the code above I added
a `redisplay--interactive` variable to the C code to force it code to
use the "interactive" paths at various places and I managed to make it
work well enough that the above code does indeed collect echo messages,
but somehow those messages end up correct regardless of the
offending patch.

So I suspect that the test of `echo_message_buffer` is affected by
other events (which apparently don't occur in batch mode).

I know it's a pretty faint signal, I'm sorry it's all I have so far.

See below the patch I'm using,


        Stefan


diff --git a/src/keyboard.c b/src/keyboard.c
index 9ee4c4f6d6..b2dad0ca2d 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -2539,8 +2539,9 @@ read_char (int commandflag, Lisp_Object map,
 
   if (/* There currently is something in the echo area.  */
       !NILP (echo_area_buffer[0])
-      && (/* It's an echo from a different kboard.  */
-	  echo_kboard != current_kboard
+      && (!EQ (echo_area_buffer[0], echo_message_buffer)
+	  /* It's an echo from a different kboard.  */
+	  || echo_kboard != current_kboard
 	  /* Or we explicitly allow overwriting whatever there is.  */
 	  || ok_to_echo_at_next_pause == NULL))
     cancel_echoing ();
@@ -2636,7 +2637,7 @@ read_char (int commandflag, Lisp_Object map,
       && !current_kboard->immediate_echo
       && (this_command_key_count > 0
 	  || !NILP (call0 (Qinternal_echo_keystrokes_prefix)))
-      && ! noninteractive
+      && (redisplay__interactive || ! noninteractive)
       && echo_keystrokes_p ()
       && (/* No message.  */
 	  NILP (echo_area_buffer[0])
diff --git a/src/xdisp.c b/src/xdisp.c
index efca6f641f..3985daa699 100644
--- a/src/xdisp.c
+++ b/src/xdisp.c
@@ -11150,12 +11150,13 @@ message3_nolog (Lisp_Object m)
 {
   struct frame *sf = SELECTED_FRAME ();
 
-  if (FRAME_INITIAL_P (sf))
+  if (FRAME_INITIAL_P (sf) && !redisplay__interactive)
     message_to_stderr (m);
   /* Error messages get reported properly by cmd_error, so this must be just an
      informative message; if the frame hasn't really been initialized yet, just
      toss it.  */
-  else if (INTERACTIVE && sf->glyphs_initialized_p)
+  else if ((INTERACTIVE || redisplay__interactive)
+           && sf->glyphs_initialized_p)
     {
       /* Get the frame containing the mini-buffer
 	 that the selected frame is using.  */
@@ -35603,6 +35604,10 @@ syms_of_xdisp (void)
 mouse stays within the extent of a single glyph (except for images).  */);
   mouse_fine_grained_tracking = false;
 
+  DEFVAR_BOOL ("redisplay--interactive", redisplay__interactive,
+    doc: /* If non-nil, redisplay will proceed even in batch mode.  */);
+  redisplay__interactive = false;
+
   DEFVAR_BOOL ("redisplay-skip-initial-frame", redisplay_skip_initial_frame,
     doc: /* Non-nil to skip redisplay in initial frame.
 The initial frame is not displayed anywhere, so skipping it is
diff --git a/test/src/keyboard-tests.el b/test/src/keyboard-tests.el
index 607d2eafd4..e9a6c497a9 100644
--- a/test/src/keyboard-tests.el
+++ b/test/src/keyboard-tests.el
@@ -23,14 +23,15 @@
 
 (ert-deftest keyboard-unread-command-events ()
   "Test `unread-command-events'."
-  (should (equal (progn (push ?\C-a unread-command-events)
-                        (read-event nil nil 1))
-                 ?\C-a))
-  (should (equal (progn (run-with-timer
-                         1 nil
-                         (lambda () (push '(t . ?\C-b) unread-command-events)))
-                        (read-event nil nil 2))
-                 ?\C-b)))
+  (let ((unread-command-events nil))
+    (should (equal (progn (push ?\C-a unread-command-events)
+                          (read-event nil nil 1))
+                   ?\C-a))
+    (should (equal (progn (run-with-timer
+                           1 nil
+                           (lambda () (push '(t . ?\C-b) unread-command-events)))
+                          (read-event nil nil 2))
+                   ?\C-b))))
 
 (ert-deftest keyboard-lossage-size ()
   "Test `lossage-size'."
@@ -46,6 +47,27 @@ keyboard-lossage-size
     (should-error (lossage-size (1- min-value)))
     (should (= lossage-orig (lossage-size lossage-orig)))))
 
+(ert-deftest keyboard-tests--echo-keystrokes-bug15332 ()
+  (let ((msgs '())
+        (unread-command-events nil)
+        (redisplay--interactive t)
+        (echo-keystrokes 2))
+    (setq unread-command-events '(?\C-u))
+    (let* ((timer1
+	    (run-with-timer 3 1
+			    (lambda ()
+			      (setq unread-command-events '(?5)))))
+	   (timer2
+	    (run-with-timer 2.5 1
+			    (lambda ()
+			      (push (current-message) msgs)))))
+      (run-with-timer 5 nil
+	              (lambda ()
+	                (cancel-timer timer1)
+	                (cancel-timer timer2)
+	                (throw 'exit msgs)))
+      (recursive-edit)
+      (should (equal msgs '("C-u 55-" "C-u 5-" "C-u-"))))))
 
 (provide 'keyboard-tests)
 ;;; keyboard-tests.el ends here

bug#46243: [External] : Re: bug#46243: 26.3; If invoke menu item that reads a char, get keystrokes echo

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: drew.adams@oracle.com,  46243@debbugs.gnu.org
> Date: Wed, 03 Feb 2021 12:42:03 -0500
> 
> > Do you happen to remember how you came to think that
> > echo_message_buffer was involved in that old bug report's scenario?
> 
> Sadly, no.

I feared you'd say that.

I took a look at the related code, including the changes in Emacs 23
that caused bug#15332, but didn't see any clear suspects.  Which may
not be surprising, as the complex system of variables and flags we use
in keyboard.c to DTRT with this stuff is above my pay grade.

So I propose the much simpler patch below.  It may look like a kludgey
band-aid at first sight, but OTOH consider this:

  . it seems to fix the problem without re-introducing the older bug
  . it looks like TRT because we definitely don't want any echoing
    when the prompt is displayed
  . it mimics what we do when message_with_string is called with its
    last argument non-zero: in that case message3 calls cancel_echoing
    unconditionally (whereas message3_nolog does not)

(The last point caused me to consider making this change inside
message_with_string, but there's one call of that function with last
argument zero, in keyboard.c, where we restore the last echo message,
so I decided not to do that.)

WDYT?

diff --git a/src/lread.c b/src/lread.c
index b33a312..010194c 100644
--- a/src/lread.c
+++ b/src/lread.c
@@ -804,7 +804,10 @@ DEFUN ("read-char", Fread_char, Sread_char, 0, 3, 0,
   barf_if_interaction_inhibited ();
 
   if (! NILP (prompt))
-    message_with_string ("%s", prompt, 0);
+    {
+      cancel_echoing ();
+      message_with_string ("%s", prompt, 0);
+    }
   val = read_filtered_event (1, 1, 1, ! NILP (inherit_input_method), seconds);
 
   return (NILP (val) ? Qnil
@@ -839,7 +842,10 @@ DEFUN ("read-event", Fread_event, Sread_event, 0, 3, 0,
   barf_if_interaction_inhibited ();
 
   if (! NILP (prompt))
-    message_with_string ("%s", prompt, 0);
+    {
+      cancel_echoing ();
+      message_with_string ("%s", prompt, 0);
+    }
   return read_filtered_event (0, 0, 0, ! NILP (inherit_input_method), seconds);
 }
 
@@ -875,7 +881,10 @@ DEFUN ("read-char-exclusive", Fread_char_exclusive, Sread_char_exclusive, 0, 3,
   barf_if_interaction_inhibited ();
 
   if (! NILP (prompt))
-    message_with_string ("%s", prompt, 0);
+    {
+      cancel_echoing ();
+      message_with_string ("%s", prompt, 0);
+    }
 
   val = read_filtered_event (1, 1, 0, ! NILP (inherit_input_method), seconds);
 

bug#46243: [External] : Re: bug#46243: 26.3; If invoke menu item that reads a char, get keystrokes echo

[Stefan Monnier]

> I took a look at the related code, including the changes in Emacs 23
> that caused bug#15332, but didn't see any clear suspects.  Which may
> not be surprising, as the complex system of variables and flags we use
> in keyboard.c to DTRT with this stuff is above my pay grade.

I think it's above everyone's pay grade, indeed :-(

> So I propose the much simpler patch below.  It may look like a kludgey
> band-aid at first sight, but OTOH consider this:

Looks OK to me.  It's fairly unusual to want echo-keystrokes working
when there's a prompt in any case.  The only case I can think of right
now is for `C-h k` and `C-h c` (but these use `read-key-sequence` so
I think they won't be affected).


        Stefan

bug#46243: [External] : Re: bug#46243: 26.3; If invoke menu item that reads a char, get keystrokes echo

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: drew.adams@oracle.com,  46243@debbugs.gnu.org
> Date: Thu, 04 Feb 2021 11:50:14 -0500
> 
> > So I propose the much simpler patch below.  It may look like a kludgey
> > band-aid at first sight, but OTOH consider this:
> 
> Looks OK to me.  It's fairly unusual to want echo-keystrokes working
> when there's a prompt in any case.  The only case I can think of right
> now is for `C-h k` and `C-h c` (but these use `read-key-sequence` so
> I think they won't be affected).

Thanks.  I verified that "C-h k" works, including when invoked via the
menus, and installed this on the master branch.  With that, I'm
closing this bug.

bug#46243: [External] : Re: bug#46243: 26.3; If invoke menu item that reads a char, get keystrokes echo

[Drew Adams]

Thanks for fixing this, guys.