bug#33749: 26.1; input-decode-map to empty vector should preserve echo area

bug#33749: 26.1; input-decode-map to empty vector should preserve echo area

[Yuri Khan]

See wider context here:
http://lists.gnu.org/archive/html/emacs-devel/2018-12/msg00254.html

The Kitty terminal emulator in full keyboard mode sends sequences for
every keyboard event. This includes key release events and modifier
key press events. I would like to ignore these, and it seems to be the
most appropriate method to map them to an empty vector via
‘input-decode-map’. Currently, this works, but every time such a
sequence is received from the terminal, the echo area is cleared,
rendering invisible any prompt displayed by the previous key press.


Here’s a playground recipe with no external dependencies:

$ emacs -Q

(define-key input-decode-map (kbd "<f5> <f5>") [])

C-s <f5> <f5>


The intuitive expectation is that pressing <f5> <f5> is a no-op. The
I-search prompt should continue to be displayed, and no other action
should be taken.

The actual behavior depends on the timing:

* Pressing <f5> the first time clears the echo area immediately.

* If <f5> is pressed again within ‘echo-keystrokes’ seconds, the echo
area stays blank and the prompt invisible until the next key (other
than <f5>) is pressed.

* If ‘echo-keystrokes’ seconds elapse before the next key is pressed,
then the echo area displays the current key sequence prefix and a
dash: ‘f5-’.

* In that case, pressing <f5> again changes the echo area to ‘f5 f5-’,
as if it’s waiting for more. And it is; since <f5> <f5> is replaced by
an empty vector, it is now a prefix of every possible binding.


The call sequence that causes the echo to be cleared looks like this:

command_loop_1 (in keyboard.c)
  read_key_sequence
    read_char
      redisplay

and much of the read_char and read_key_sequence mechanics maintain
echoing of the current prefix sequence.


My suggestion is to add two changes:

* If ‘read_key_sequence’ detects that the whole key sequence has been
rewritten to an empty sequence, return -1. This is the same code as a
canceled menu selection, upon receiving which clients call
‘read_key_sequence’ again.

* In ‘command_loop_1’, save the echo area contents before calling
‘read_key_sequence’, and restore it if the return value is -1.

A proposed patch will follow.


In GNU Emacs 26.1 (build 2, x86_64-pc-linux-gnu, GTK+ Version 3.22.30)
 of 2018-05-29 built on lcy01-amd64-029
Windowing system distributor 'The X.Org Foundation', version 11.0.11906000
System Description:    Ubuntu 18.04.1 LTS

Recent messages:
For information about GNU Emacs and the GNU system, type C-h C-a.
[]
Quit
Configured using:
 'configure --build=x86_64-linux-gnu --prefix=/usr
 '--includedir=${prefix}/include' '--mandir=${prefix}/share/man'
 '--infodir=${prefix}/share/info' --sysconfdir=/etc --localstatedir=/var
 --disable-silent-rules '--libdir=${prefix}/lib/x86_64-linux-gnu'
 '--libexecdir=${prefix}/lib/x86_64-linux-gnu' --disable-maintainer-mode
 --disable-dependency-tracking --prefix=/usr --sharedstatedir=/var/lib
 --program-suffix=26 --with-modules --with-file-notification=inotify
 --with-mailutils --with-x=yes --with-x-toolkit=gtk3 --with-xwidgets
 --with-lcms2 'CFLAGS=-g -O2
 -fdebug-prefix-map=/build/emacs26-pCvJBp/emacs26-26.1~1.git07f8f9b=.
-fstack-protector-strong
 -Wformat -Werror=format-security -no-pie' 'CPPFLAGS=-Wdate-time
 -D_FORTIFY_SOURCE=2' 'LDFLAGS=-Wl,-Bsymbolic-functions -Wl,-z,relro
 -no-pie''

Configured features:
XPM JPEG TIFF GIF PNG RSVG IMAGEMAGICK SOUND GPM DBUS GSETTINGS NOTIFY
LIBSELINUX GNUTLS LIBXML2 FREETYPE M17N_FLT LIBOTF XFT ZLIB
TOOLKIT_SCROLL_BARS GTK3 X11 MODULES THREADS XWIDGETS LIBSYSTEMD LCMS2

Important settings:
  value of $LC_MONETARY: en_RU.UTF-8
  value of $LC_NUMERIC: en_RU.UTF-8
  value of $LC_TIME: en_RU.UTF-8
  value of $LANG: en_US.UTF-8
  locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t
  line-number-mode: t
  transient-mark-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message rmc puny seq byte-opt gv
bytecomp byte-compile cconv dired dired-loaddefs format-spec rfc822 mml
easymenu mml-sec password-cache epa derived epg epg-config gnus-util
rmail rmail-loaddefs mm-decode mm-bodies mm-encode mail-parse rfc2231
mailabbrev gmm-utils mailheader sendmail rfc2047 rfc2045 ietf-drums
mm-util mail-prsvr mail-utils misearch multi-isearch edmacro kmacro
cl-loaddefs cl-lib elec-pair time-date mule-util tooltip eldoc electric
uniquify ediff-hook vc-hooks lisp-float-type mwheel term/x-win x-win
term/common-win x-dnd tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode elisp-mode lisp-mode
prog-mode register page menu-bar rfn-eshadow isearch timer select
scroll-bar mouse jit-lock font-lock syntax facemenu font-core
term/tty-colors frame cl-generic cham georgian utf-8-lang misc-lang
vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms cp51932
hebrew greek romanian slovak czech european ethiopic indian cyrillic
chinese composite charscript charprop case-table epa-hook jka-cmpr-hook
help simple abbrev obarray minibuffer cl-preloaded nadvice loaddefs
button faces cus-face macroexp files text-properties overlay sha1 md5
base64 format env code-pages mule custom widget hashtable-print-readable
backquote dbusbind inotify lcms2 dynamic-setting system-font-setting
font-render-setting xwidget-internal move-toolbar gtk x-toolkit x
multi-tty make-network-process emacs)

Memory information:
((conses 16 96766 11934)
 (symbols 48 20584 1)
 (miscs 40 46 179)
 (strings 32 28973 1221)
 (string-bytes 1 760346)
 (vectors 16 14880)
 (vector-slots 8 498940 11302)
 (floats 8 50 266)
 (intervals 56 264 14)
 (buffers 992 11))

bug#33749: 26.1; input-decode-map to empty vector should preserve echo area

[Eli Zaretskii]

> From: Yuri Khan <yurivkhan@gmail.com>
> Date: Sat, 15 Dec 2018 03:48:00 +0700
> 
> The call sequence that causes the echo to be cleared looks like this:
> 
> command_loop_1 (in keyboard.c)
>   read_key_sequence
>     read_char
>       redisplay
> 
> and much of the read_char and read_key_sequence mechanics maintain
> echoing of the current prefix sequence.
> 
> 
> My suggestion is to add two changes:
> 
> * If ‘read_key_sequence’ detects that the whole key sequence has been
> rewritten to an empty sequence, return -1. This is the same code as a
> canceled menu selection, upon receiving which clients call
> ‘read_key_sequence’ again.
> 
> * In ‘command_loop_1’, save the echo area contents before calling
> ‘read_key_sequence’, and restore it if the return value is -1.

Thanks for digging into this issue.

Based on bitter past experience, I generally would like to avoid
changes in these low-level mechanisms based on analysis of the code's
logic and their presumed semantics.  We had quite a few of cases where
such analyses seemed sound when they were presented, but later turned
out to miss some more or less rare situations and use cases, and
changes based on such analyses thus caused subtle bugs that were hard
or even impossible to fix without reverting to the old code (and
losing the improvements provided by the new code).

Therefore, if you want to propose a new feature in this area, I very
much prefer to trigger such a feature by some (new) variable that is
bound to a special value, rather than base the feature on decisions
made by some modified logic.  Doing this by such a variable might be
slightly less elegant and general, but OTOH it runs a much lower risk
of causing unintended consequences elsewhere.

bug#33749: 26.1; input-decode-map to empty vector should preserve echo area

[Yuri Khan]

On Sat, Dec 15, 2018 at 1:58 PM Eli Zaretskii <eliz@gnu.org> wrote:

> […] if you want to propose a new feature in this area, I very
> much prefer to trigger such a feature by some (new) variable that is
> bound to a special value, rather than base the feature on decisions
> made by some modified logic.  Doing this by such a variable might be
> slightly less elegant and general, but OTOH it runs a much lower risk
> of causing unintended consequences elsewhere.

Okay. So, amended proposed course:

* Add a variable such as ‘input-decode-preserve-echo’, boolean,
default nil, that controls the following behavior changes.

[any better ideas on the naming?]

* If the above variable is true
> > and ‘read_key_sequence’ detects that the whole key sequence has been
> > rewritten to an empty sequence, return -1.
> >
> > * In ‘command_loop_1’, save the echo area contents before calling
> > ‘read_key_sequence’.
Afterwards, if ‘input-decode-preserve-echo’ is true and the return
value is -1, restore the echo area contents.

* In the future ‘terminal-init-xterm-kitty’ function, set
‘input-decode-preserve-echo’ to true when full keyboard mode is
requested.


An alternative way could be to add hooks at start of
‘read_key_sequence’ and at each each sequence rewrite, but IMO that
would be excessive.

bug#33749: 26.1; input-decode-map to empty vector should preserve echo area

[Eli Zaretskii]

> From: Yuri Khan <yurivkhan@gmail.com>
> Date: Sat, 15 Dec 2018 15:07:03 +0700
> Cc: 33749@debbugs.gnu.org
> 
> Okay. So, amended proposed course:
> 
> * Add a variable such as ‘input-decode-preserve-echo’, boolean,
> default nil, that controls the following behavior changes.
> 
> [any better ideas on the naming?]
> 
> * If the above variable is true
> > > and ‘read_key_sequence’ detects that the whole key sequence has been
> > > rewritten to an empty sequence, return -1.
> > >
> > > * In ‘command_loop_1’, save the echo area contents before calling
> > > ‘read_key_sequence’.
> Afterwards, if ‘input-decode-preserve-echo’ is true and the return
> value is -1, restore the echo area contents.
> 
> * In the future ‘terminal-init-xterm-kitty’ function, set
> ‘input-decode-preserve-echo’ to true when full keyboard mode is
> requested.

SGTM, but let's hear what others think.

> An alternative way could be to add hooks at start of
> ‘read_key_sequence’ and at each each sequence rewrite, but IMO that
> would be excessive.

I think I agree.

bug#33749: [PATCH] Preserve echo area contents when decoding an empty key sequence (Bug#33749)

[Yuri Khan]

* src/keyboard.h (struct kboard): New terminal-local variable
  `input-decode-preserve-echo'.

* src/keyboard.c (read_key_sequence): If the current sequence is
  mapped via `input-decode-map' to an empty sequence and
  `input-decode-preserve-echo' is non-nil, return -1.

* src/keyboard.c (command_loop_1): Save echo area contents before
  calling `read-key-sequence'. If it returns -1 and
  `input-decode-preserve-echo' is non-nil, restore echo area contents.
---
 src/keyboard.c | 27 ++++++++++++++++++++++++++-
 src/keyboard.h |  9 +++++++++
 2 files changed, 35 insertions(+), 1 deletion(-)

diff --git a/src/keyboard.c b/src/keyboard.c
index baf2f514409..70390ccf88c 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -1239,6 +1239,7 @@ command_loop_1 (void)
   EMACS_INT prev_modiff = 0;
   struct buffer *prev_buffer = NULL;
   bool already_adjusted = 0;
+  volatile Lisp_Object previous_echo_area_message;
 
   kset_prefix_arg (current_kboard, Qnil);
   kset_last_prefix_arg (current_kboard, Qnil);
@@ -1341,6 +1342,7 @@ command_loop_1 (void)
       Vthis_original_command = Qnil;
       Vthis_command_keys_shift_translated = Qnil;
 
+      previous_echo_area_message = Fcurrent_message ();
       /* Read next key sequence; i gets its length.  */
       raw_keybuf_count = 0;
       Lisp_Object keybuf[READ_KEY_ELTS];
@@ -1362,6 +1364,9 @@ command_loop_1 (void)
 	 Just loop around and read another command.  */
       if (i == -1)
 	{
+	  if (! NILP (KVAR (current_kboard, Vinput_decode_preserve_echo))
+	      && ! NILP (previous_echo_area_message))
+	    message_with_string ("%s", previous_echo_area_message, 0);
 	  cancel_echoing ();
 	  this_command_key_count = 0;
 	  this_single_command_key_start = 0;
@@ -8787,7 +8792,9 @@ void init_raw_keybuf_count (void)
    storing it in KEYBUF, a buffer of size READ_KEY_ELTS.
    Prompt with PROMPT.
    Return the length of the key sequence stored.
-   Return -1 if the user rejected a command menu.
+   Return -1 if the user rejected a command menu, or
+   `input-decode-preserve-echo' is non-nil and the user entered
+   a sequence that is mapped via `input-decode-map' to an empty vector.
 
    Echo starting immediately unless `prompt' is 0.
 
@@ -9540,6 +9547,9 @@ read_key_sequence (Lisp_Object *keybuf, Lisp_Object prompt,
 	  if (done)
 	    {
 	      mock_input = diff + max (t, mock_input);
+	      if (! NILP (KVAR (current_kboard, Vinput_decode_preserve_echo))
+		  && mock_input == 0)
+		return -1;
 	      goto replay_sequence;
 	    }
 	}
@@ -10817,6 +10827,7 @@ init_kboard (KBOARD *kb, Lisp_Object type)
   kset_system_key_syms (kb, Qnil);
   kset_window_system (kb, type);
   kset_input_decode_map (kb, Fmake_sparse_keymap (Qnil));
+  kset_input_decode_preserve_echo (kb, Qnil);
   kset_local_function_key_map (kb, Fmake_sparse_keymap (Qnil));
   Fset_keymap_parent (KVAR (kb, Vlocal_function_key_map), Vfunction_key_map);
   kset_default_minibuffer_frame (kb, Qnil);
@@ -11644,6 +11655,19 @@ and its return value (a key sequence) is used.
 The events that come from bindings in `input-decode-map' are not
 themselves looked up in `input-decode-map'.  */);
 
+  DEFVAR_KBOARD ("input-decode-preserve-echo", Vinput_decode_preserve_echo,
+                 doc: /* Preserve the echo area contents while decoding input.
+
+It is possible for `input-decode-map' to map a sequence to an empty vector,
+for example, to ignore irrelevant sequences sent by the terminal.
+
+If this variable is `nil', `read-key-sequence' will loop waiting for the
+sequence to continue.  The echo area may show the original sequence.  This
+is the historic Emacs behavior.
+
+If this variable is set to `t', `read-key-sequence' will return -1, and
+the command loop will restore the echo area contents.  */);
+
   DEFVAR_LISP ("function-key-map", Vfunction_key_map,
                doc: /* The parent keymap of all `local-function-key-map' instances.
 Function key definitions that apply to all terminal devices should go
@@ -11956,6 +11980,7 @@ mark_kboards (void)
       mark_object (KVAR (kb, system_key_syms));
       mark_object (KVAR (kb, Vwindow_system));
       mark_object (KVAR (kb, Vinput_decode_map));
+      mark_object (KVAR (kb, Vinput_decode_preserve_echo));
       mark_object (KVAR (kb, Vlocal_function_key_map));
       mark_object (KVAR (kb, Vdefault_minibuffer_frame));
       mark_object (KVAR (kb, echo_string));
diff --git a/src/keyboard.h b/src/keyboard.h
index ce4630b8a37..7c778292f50 100644
--- a/src/keyboard.h
+++ b/src/keyboard.h
@@ -151,6 +151,10 @@ struct kboard
        DEFVAR for more documentation.  */
     Lisp_Object Vinput_decode_map_;
 
+    /* Save and restore the echo area across sequences that are mapped
+       to the empty vector by `input-decode-map'.  */
+    Lisp_Object Vinput_decode_preserve_echo_;
+
     /* Minibufferless frames on this display use this frame's minibuffer.  */
     Lisp_Object Vdefault_minibuffer_frame_;
 
@@ -197,6 +201,11 @@ kset_input_decode_map (struct kboard *kb, Lisp_Object val)
   kb->Vinput_decode_map_ = val;
 }
 INLINE void
+kset_input_decode_preserve_echo (struct kboard *kb, Lisp_Object val)
+{
+  kb->Vinput_decode_preserve_echo_ = val;
+}
+INLINE void
 kset_last_command (struct kboard *kb, Lisp_Object val)
 {
   kb->Vlast_command_ = val;
-- 
2.20.1

bug#33749: 26.1; input-decode-map to empty vector should preserve echo area

[Stefan Monnier]

> The call sequence that causes the echo to be cleared looks like this:
>
> command_loop_1 (in keyboard.c)
>   read_key_sequence
>     read_char
>       redisplay
>
> and much of the read_char and read_key_sequence mechanics maintain
> echoing of the current prefix sequence.

Have you been able to track down the precise call to (one of the C entry
point of) `message` which causes the echo area to be cleared in
your case?

> * src/keyboard.c (read_key_sequence): If the current sequence is
>   mapped via `input-decode-map' to an empty sequence and
>   `input-decode-preserve-echo' is non-nil, return -1.

input-decode-map doesn't only apply to the beginning of a key-sequence,
but also in the middle (e.g. in your case, when you do `C-x h` the
input-decode-map likely applies to the "release control modifier" event
that occurs between `C-x` and `h`).

Returning -1 after `C-x` and before we got to read `h` wouldn't
be right.

Also, who/where do you intend to set input-decode-preserve-echo?


        Stefan

bug#33749: 26.1; input-decode-map to empty vector should preserve echo area

[Yuri Khan]

On Thu, Dec 20, 2018 at 1:05 AM Stefan Monnier <monnier@iro.umontreal.ca> wrote:
>
> > The call sequence that causes the echo to be cleared looks like this:
> >
> > command_loop_1 (in keyboard.c)
> >   read_key_sequence
> >     read_char
> >       redisplay
> >
> > and much of the read_char and read_key_sequence mechanics maintain
> > echoing of the current prefix sequence.
>
> Have you been able to track down the precise call to (one of the C entry
> point of) `message` which causes the echo area to be cleared in
> your case?

Ah, of course, ‘redisplay’ clears the echo area visually, but it is
only enacting the state set by some previous call to ‘message’ or its
subcases.

There are many of these, that I considered it impractical to attempt
to prevent them all from happening.

* In ‘read_char’ around keyboard.c:2948:

  /* Now wipe the echo area, except for help events which do their
     own stuff with the echo area.  */
  if (!CONSP (c)
      || (!(EQ (Qhelp_echo, XCAR (c)))
          && !(EQ (Qswitch_frame, XCAR (c)))
          /* Don't wipe echo area for select window events: These might
             get delayed via `mouse-autoselect-window' (Bug#11304).  */
          && !(EQ (Qselect_window, XCAR (c)))))
    {
      if (!NILP (echo_area_buffer[0]))
        {
          safe_run_hooks (Qecho_area_clear_hook);
→         clear_message (1, 0);
        }
    }

* In ‘read_key_sequence’ around keyboard.c:8981:

  /* These are no-ops the first time through, but if we restart, they
     revert the echo area and this_command_keys to their original state.  */
  this_command_key_count = keys_start;
  if (INTERACTIVE && t < mock_input)
→   echo_truncate (echo_start);

and keyboard.c:9056:

    replay_key:
      /* These are no-ops, unless we throw away a keystroke below and
         jumped back up to replay_key; in that case, these restore the
         variables to their original state, allowing us to replay the
         loop.  */
      if (INTERACTIVE && t < mock_input)
→       echo_truncate (echo_local_start);
      this_command_key_count = keys_local_start;


> input-decode-map doesn't only apply to the beginning of a key-sequence,
> but also in the middle (e.g. in your case, when you do `C-x h` the
> input-decode-map likely applies to the "release control modifier" event
> that occurs between `C-x` and `h`).
>
> Returning -1 after `C-x` and before we got to read `h` wouldn't
> be right.

Yes, and the patch is written to only return -1 if the whole sequence
from the very beginning (i.e. start of read-key-sequence) is zeroed
out. Otherwise, after the re-reading, there is still a current prefix
and it is echoed as usual:

    $ ./emacs -Q
    (setq input-decode-preserve-echo 1)
    → 1
    (define-key input-decode-map (kbd "<f5> <f5>") [])
    → []

    <f5>
    → f5-
    <f5>
    → []

    ESC
    → ESC-
    <f5>
    → ESC f5-
    <f5>
    → ESC-


> Also, who/where do you intend to set input-decode-preserve-echo?

My current plan is:

* term/xterm-kitty.el will define a (customizable?) user variable
kitty-use-full-keyboard, boolean, default nil.
* If it is nil, ‘terminal-init-xterm-kitty’ will call
(terminal-init-xterm) and call it a day.
* If ‘kitty-use-full-keyboard’ has been set to non-nil,
‘terminal-init-xterm-kitty’ will send to the terminal the escape
sequence enabling full keyboard mode (CSI ? 2 0 1 7 h), set up
‘input-decode-map’ and ‘local-function-key-map’ as appropriate for
that mode, set ‘input-decode-preserve-echo’ to ‘t’, and perform other
xterm-like initialization such as bracketed paste, clipboard/selection
control, mouse, etc.

bug#33749: 26.1; input-decode-map to empty vector should preserve echo area

[Stefan Monnier]

> There are many of these, that I considered it impractical to attempt
> to prevent them all from happening.

They're probably not all guilty.  That's why I'm asking if you've been
able to find which one is the culprit.

>           safe_run_hooks (Qecho_area_clear_hook);
> →         clear_message (1, 0);
>         }
[...]
>   if (INTERACTIVE && t < mock_input)
> →   echo_truncate (echo_start);
>
[...]
>       if (INTERACTIVE && t < mock_input)
> →       echo_truncate (echo_local_start);
>       this_command_key_count = keys_local_start;

Are you saying that if you neuter all three of those, the problem
disappears?
If you keep any one of those is the problem still present?

>> Also, who/where do you intend to set input-decode-preserve-echo?
>
> My current plan is:
>
> * term/xterm-kitty.el will define a (customizable?) user variable
> kitty-use-full-keyboard, boolean, default nil.
> * If it is nil, ‘terminal-init-xterm-kitty’ will call
> (terminal-init-xterm) and call it a day.
> * If ‘kitty-use-full-keyboard’ has been set to non-nil,
> ‘terminal-init-xterm-kitty’ will send to the terminal the escape
> sequence enabling full keyboard mode (CSI ? 2 0 1 7 h), set up
> ‘input-decode-map’ and ‘local-function-key-map’ as appropriate for
> that mode, set ‘input-decode-preserve-echo’ to ‘t’, and perform other
> xterm-like initialization such as bracketed paste, clipboard/selection
> control, mouse, etc.

Ah, so you're planning to set it once and for all globally?
That would really make it a workaround more than a fix, I think.


        Stefan

bug#33749: 26.1; input-decode-map to empty vector should preserve echo area

[Yuri Khan]

On Fri, Dec 21, 2018 at 3:41 AM Stefan Monnier <monnier@iro.umontreal.ca> wrote:
>
> > There are many of these, that I considered it impractical to attempt
> > to prevent them all from happening.
>
> They're probably not all guilty.  That's why I'm asking if you've been
> able to find which one is the culprit.
>
> >           safe_run_hooks (Qecho_area_clear_hook);
> > →         clear_message (1, 0);
> >         }

> Are you saying that if you neuter all three of those, the problem
> disappears?
> If you keep any one of those is the problem still present?

The problem disappears if I comment out this one ‘clear_message’.

I looked for its history.

* In the beginning (1992-01-29 by Jim Blandy), it was unconditional.
* 1994-05-11 Karl Heuer: “Preserve echo area on async buffer switch.”
* 1994-08-19 Richard M. Stallman: “Don’t show buffer-events to the user.”
* 1998-08-08 Richard M. Stallman: “When input method returns no chars,
call cancel_echoing. Restore the previous echo area message and
this_command_keys, too.”
* 1999-07-22 Gerd Moellmann: (slight refactoring)
* 1999-08-22 Gerd Moellmann: (more refactoring)
* 2005-05-11 Gerd Moellmann: “Don't clear current message for help events; …”
* 2012-05-07 Jérémy Compostella: “Don't clear the echo area if there's
no message to clear.”

That is, over time, various conditions were added around this clear,
for various reasons. The patch about input method returning no
characters uses the same kind of workaround, which is where I got this
idea.

Thing is, I do not understand what the original purpose was. It only
matters during the short interval after the initial character of the
key sequence is read, and until either the sequence is finished or
‘echo-keystrokes’ seconds elapse, and it seems to only provide
immediate visual feedback for pressing a prefix key. (What am I
overlooking?)

Also, I cannot easily add a new condition at this particular point. It
happens when Emacs reads each character of the sequence that will
eventually turn out to map to an empty one, but I do not know that
will be the case until the end.

Or do I?

In the specific use case of Kitty’s full keyboard mode, the condition
that I’m looking for is “The first character of the sequence currently
being read is ESC”. This is because in this mode the <escape> key
sends not just an ESC character but a whole ESC _ K p A y ESC \
sequence. That is, visual feedback is not necessary for a sole ESC
character and any sequence starting with ESC.

That condition is detectable in read_key_sequence and would need to be
passed into read_char. I could probably add a new argument to
read_char, or invent more values for its COMMANDFLAG argument. The new
logic would be governed by a terminal-local flag that essentially
means “on this terminal, the ESC character always means a special key
and never the key <escape>”, or a terminal-local set of characters
that always start special keys.

Do you think this is a better course of action?

> >> Also, who/where do you intend to set input-decode-preserve-echo?
> Ah, so you're planning to set it once and for all globally?

Terminal-locally, if it’s any better.

> That would really make it a workaround more than a fix, I think.

bug#33749: 26.1; input-decode-map to empty vector should preserve echo area

[Stefan Monnier]

> Also, I cannot easily add a new condition at this particular point.
> It happens when Emacs reads each character of the sequence that will
> eventually turn out to map to an empty one, but I do not know that
> will be the case until the end.

I think The Right Thing to do is likely to move this code to
read_key_sequence, more specifically, move it to the point where we
*know* we really do have an event.

IOW, I think the patch below might be a better option (where we test
`indec.start > 0` to make sure some *decoded* event was read).

>> >> Also, who/where do you intend to set input-decode-preserve-echo?
>> Ah, so you're planning to set it once and for all globally?
> Terminal-locally, if it’s any better.

Only marginally.


        Stefan


diff --git a/src/keyboard.c b/src/keyboard.c
index baf2f51440..92ef79b09f 100644
--- a/src/keyboard.c
+++ b/src/keyboard.c
@@ -2928,22 +2928,6 @@ read_char (int commandflag, Lisp_Object map,
       Vinput_method_previous_message = previous_echo_area_message;
     }
 
-  /* Now wipe the echo area, except for help events which do their
-     own stuff with the echo area.  */
-  if (!CONSP (c)
-      || (!(EQ (Qhelp_echo, XCAR (c)))
-	  && !(EQ (Qswitch_frame, XCAR (c)))
-	  /* Don't wipe echo area for select window events: These might
-	     get delayed via `mouse-autoselect-window' (Bug#11304).  */
-	  && !(EQ (Qselect_window, XCAR (c)))))
-    {
-      if (!NILP (echo_area_buffer[0]))
-	{
-	  safe_run_hooks (Qecho_area_clear_hook);
-	  clear_message (1, 0);
-	}
-    }
-
  reread_for_input_method:
  from_macro:
   /* Pass this to the input method, if appropriate.  */
@@ -9070,6 +9054,23 @@ read_key_sequence (Lisp_Object *keybuf, Lisp_Object prompt,
       /* If not, we should actually read a character.  */
       else
 	{
+          /* Now wipe the echo area, except for help events which do their
+             own stuff with the echo area.  */
+          /* FIXME: This used to happen at the end of read_char (i.e. after
+             we read the first event of a key sequence), but we now do it just
+             before reading the second event, and only when we know that the
+             first event is a "real" one, rather than some internal event that
+             might be dropped altogether (e.g. help-event, switch-frame, or
+             some key that we remap to the empty sequence (bug#33749)).
+             Maybe we should even make sure that `fkey.star > 0` or maybe
+             even `keytran.start > 0`!?  */
+          if (indec.start > 0
+              && !NILP (echo_area_buffer[0]))
+	    {
+	      safe_run_hooks (Qecho_area_clear_hook);
+	      clear_message (1, 0);
+	    }
+
 	  {
 	    KBOARD *interrupted_kboard = current_kboard;
 	    struct frame *interrupted_frame = SELECTED_FRAME ();

bug#33749: 26.1; input-decode-map to empty vector should preserve echo area

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Date: Tue, 25 Dec 2018 14:35:13 -0500
> Cc: 33749@debbugs.gnu.org
> 
> I think The Right Thing to do is likely to move this code to
> read_key_sequence, more specifically, move it to the point where we
> *know* we really do have an event.
> 
> IOW, I think the patch below might be a better option (where we test
> `indec.start > 0` to make sure some *decoded* event was read).

Where in that patch is the variable that I asked for, which would
trigger this special behavior?

bug#33749: 26.1; input-decode-map to empty vector should preserve echo area

[Stefan Monnier]

>> I think The Right Thing to do is likely to move this code to
>> read_key_sequence, more specifically, move it to the point where we
>> *know* we really do have an event.
>> IOW, I think the patch below might be a better option (where we test
>> `indec.start > 0` to make sure some *decoded* event was read).
> Where in that patch is the variable that I asked for, which would
> trigger this special behavior?

Nowhere ;-)

Mostly because I don't know where we could set this variable (other
than globally).

If you want me to add a global var to control whether we use the old or
the new behavior, I'm fine with that.

Note also that the current patch is likely not quite right yet anyway:
I mostly posted it for discussion.  E.g. I think where I put it
currently, it fails to be run for single-key commands.


        Stefan

bug#33749: 26.1; input-decode-map to empty vector should preserve echo area

[Eli Zaretskii]

> From: Stefan Monnier <monnier@IRO.UMontreal.CA>
> Cc: yurivkhan@gmail.com, 33749@debbugs.gnu.org
> Date: Tue, 25 Dec 2018 15:07:50 -0500
> 
> >> I think The Right Thing to do is likely to move this code to
> >> read_key_sequence, more specifically, move it to the point where we
> >> *know* we really do have an event.
> >> IOW, I think the patch below might be a better option (where we test
> >> `indec.start > 0` to make sure some *decoded* event was read).
> > Where in that patch is the variable that I asked for, which would
> > trigger this special behavior?
> 
> Nowhere ;-)
> 
> Mostly because I don't know where we could set this variable (other
> than globally).
> 
> If you want me to add a global var to control whether we use the old or
> the new behavior, I'm fine with that.

Yes, please.

> Note also that the current patch is likely not quite right yet anyway:
> I mostly posted it for discussion.  E.g. I think where I put it
> currently, it fails to be run for single-key commands.

Yes, I understand.

bug#33749: 26.1; input-decode-map to empty vector should preserve echo area

[Yuri Khan]

On Wed, Dec 26, 2018 at 3:07 AM Stefan Monnier <monnier@iro.umontreal.ca> wrote:

> Note also that the current patch is likely not quite right yet anyway:
> I mostly posted it for discussion.  E.g. I think where I put it
> currently, it fails to be run for single-key commands.

Alas, it fails in more than that one way.

    (define-key input-decode-map (kbd "<f5> <f5>") [])

    <f5> (wait 1s)
    → f5-

    <f5> (again)
    → f5 f5-

i.e., although we are in the start state, the echo area contains the
last path that led to it;

    <f5> (third time)
    → f5-

    C-g
    → <f5> C-g is undefined

After that it breaks:

    <f5> (and wait 1s again)
    → (nothing happens) <f5> C-g is undefined

    x
    → <f5> x is undefined

i.e. it “heard” the <f5> but did not echo it.

Also:

    ESC (and wait 1s)
    → ESC-

    <f5>
    → ESC f5-

    <f5>
    →

The echo area clears but the current prefix is ESC:

    C-g
    → C-M-g is undefined


Do we have anything resembling a feature specification for echoing?
Automated tests? Even just a mechanism for testing key sequence
reading and echoing?

I see a file, test/src/keyboard-tests.el. I see there is a mechanism
to feed keystrokes, either immediately or on a timer. I will look if
these can be used to exercise the echoing code path.