diff mbox

Fix the processing of Meta-key commands in TUI

Message ID 1408740286-29326-1-git-send-email-patrick@parcs.ath.cx
State New
Headers show

Commit Message

Patrick Palka Aug. 22, 2014, 8:44 p.m. UTC
This patch fixes the annoying bug where key sequences such as Alt_F or
Alt_B (go forward or backwards by a word) do not behave promptly in TUI.
You have to press a third key in order for the key sequence to register.

This is mostly ncurses' fault.  Calling wgetch() normally causes ncurses
to read only a single key from stdin.  However if the key read is the
start-sequence key (^[ a.k.a. ESC) then wgetch() reads TWO keys from
stdin, storing the 2nd key into an internal FIFO buffer and returning
the start-sequence key.  The extraneous read of the 2nd key makes us
miss its corresponding stdin event, so the event loop blocks until a
third key is pressed.  This explains why such key sequences do not
behave promptly in TUI.

To fix this issue, we must somehow compensate for the missed stdin event
corresponding to the 2nd byte of a key sequence.  This patch achieves
this by hacking  up the stdin event handler to conditionally execute the
readline callback multiple multiple times in a row.  This is done via a
new global variable, call_stdin_event_handler_again_p, which is set from
tui_getc() when we receive a start-sequence key and notice extra pending
input in the ncurses buffer.

Tested on x86_64-unknown-linux-gnu.
---
 gdb/event-top.c  | 13 ++++++++++++-
 gdb/event-top.h  |  1 +
 gdb/tui/tui-io.c | 22 +++++++++++++++++++++-
 3 files changed, 34 insertions(+), 2 deletions(-)

Comments

Pedro Alves Aug. 28, 2014, 11:31 a.m. UTC | #1
On 08/22/2014 09:44 PM, Patrick Palka wrote:
> +
> +  if (async_command_editing_p && key_is_start_sequence (ch))

I think the key_is_start_sequence check means that we'll
fail to compensate in case the sequence if longer than
2 bytes?  That is, we'll compensate for the second char,
but fail to compensate for the third, because by then,
ch will not be a start sequence key.

> +    {
> +      int ch_pending;
> +
> +      nodelay (w, TRUE);
> +      ch_pending = wgetch (w);
> +      nodelay (w, FALSE);
> +

Thanks,
Pedro Alves
Patrick Palka Aug. 28, 2014, 2:18 p.m. UTC | #2
On Thu, Aug 28, 2014 at 7:31 AM, Pedro Alves <palves@redhat.com> wrote:
> On 08/22/2014 09:44 PM, Patrick Palka wrote:
>> +
>> +  if (async_command_editing_p && key_is_start_sequence (ch))
>
> I think the key_is_start_sequence check means that we'll
> fail to compensate in case the sequence if longer than
> 2 bytes?  That is, we'll compensate for the second char,
> but fail to compensate for the third, because by then,
> ch will not be a start sequence key.

Yes I think so.  I only took into account common 2-byte sequences such
as Alt_F.  I suppose that the check could be removed to account for
3+-byte key sequences too, but I haven't thought out the consequences
of such change.  And I'm not sure that readline uses any of such
sequences.

>
>> +    {
>> +      int ch_pending;
>> +
>> +      nodelay (w, TRUE);
>> +      ch_pending = wgetch (w);
>> +      nodelay (w, FALSE);
>> +
>
> Thanks,
> Pedro Alves
>
Pedro Alves Aug. 28, 2014, 4:13 p.m. UTC | #3
On 08/28/2014 03:18 PM, Patrick Palka wrote:
> On Thu, Aug 28, 2014 at 7:31 AM, Pedro Alves <palves@redhat.com> wrote:
>> On 08/22/2014 09:44 PM, Patrick Palka wrote:
>>> +
>>> +  if (async_command_editing_p && key_is_start_sequence (ch))
>>
>> I think the key_is_start_sequence check means that we'll
>> fail to compensate in case the sequence if longer than
>> 2 bytes?  That is, we'll compensate for the second char,
>> but fail to compensate for the third, because by then,
>> ch will not be a start sequence key.
> 
> Yes I think so.  I only took into account common 2-byte sequences such
> as Alt_F.  I suppose that the check could be removed to account for
> 3+-byte key sequences too, but I haven't thought out the consequences
> of such change.  And I'm not sure that readline uses any of such
> sequences.

It's fine with me to leave this as is, if we add a comment
mentioning this issue.

Thanks,
Pedro Alves
Patrick Palka Aug. 28, 2014, 4:25 p.m. UTC | #4
On Thu, Aug 28, 2014 at 12:13 PM, Pedro Alves <palves@redhat.com> wrote:
> On 08/28/2014 03:18 PM, Patrick Palka wrote:
>> On Thu, Aug 28, 2014 at 7:31 AM, Pedro Alves <palves@redhat.com> wrote:
>>> On 08/22/2014 09:44 PM, Patrick Palka wrote:
>>>> +
>>>> +  if (async_command_editing_p && key_is_start_sequence (ch))
>>>
>>> I think the key_is_start_sequence check means that we'll
>>> fail to compensate in case the sequence if longer than
>>> 2 bytes?  That is, we'll compensate for the second char,
>>> but fail to compensate for the third, because by then,
>>> ch will not be a start sequence key.
>>
>> Yes I think so.  I only took into account common 2-byte sequences such
>> as Alt_F.  I suppose that the check could be removed to account for
>> 3+-byte key sequences too, but I haven't thought out the consequences
>> of such change.  And I'm not sure that readline uses any of such
>> sequences.
>
> It's fine with me to leave this as is, if we add a comment
> mentioning this issue.

I'll do that and send a revised patch later today.

>
> Thanks,
> Pedro Alves
>
Patrick Palka Nov. 14, 2014, 7:12 p.m. UTC | #5
On Thu, Aug 28, 2014 at 12:13 PM, Pedro Alves <palves@redhat.com> wrote:
> On 08/28/2014 03:18 PM, Patrick Palka wrote:
>> On Thu, Aug 28, 2014 at 7:31 AM, Pedro Alves <palves@redhat.com> wrote:
>>> On 08/22/2014 09:44 PM, Patrick Palka wrote:
>>>> +
>>>> +  if (async_command_editing_p && key_is_start_sequence (ch))
>>>
>>> I think the key_is_start_sequence check means that we'll
>>> fail to compensate in case the sequence if longer than
>>> 2 bytes?  That is, we'll compensate for the second char,
>>> but fail to compensate for the third, because by then,
>>> ch will not be a start sequence key.
>>
>> Yes I think so.  I only took into account common 2-byte sequences such
>> as Alt_F.  I suppose that the check could be removed to account for
>> 3+-byte key sequences too, but I haven't thought out the consequences
>> of such change.  And I'm not sure that readline uses any of such
>> sequences.
>
> It's fine with me to leave this as is, if we add a comment
> mentioning this issue.
>
> Thanks,
> Pedro Alves
>

Attached is the patch that Pedro approved, augmented with a comment
describing the potential problem with 3+ byte sequences.  I wonder if
someone could commit this for me?
diff mbox

Patch

diff --git a/gdb/event-top.c b/gdb/event-top.c
index 833f49d..df1351d 100644
--- a/gdb/event-top.c
+++ b/gdb/event-top.c
@@ -120,6 +120,11 @@  int exec_done_display_p = 0;
    read commands from.  */
 int input_fd;
 
+/* Used by the stdin event handler to compensate for missed stdin events.
+   Setting this value inside an stdin callback makes the callback run
+   again.  */
+int call_stdin_event_handler_again_p;
+
 /* Signal handling variables.  */
 /* Each of these is a pointer to a function that the event loop will
    invoke if the corresponding signal has received.  The real signal
@@ -370,7 +375,13 @@  stdin_event_handler (int error, gdb_client_data client_data)
       quit_command ((char *) 0, stdin == instream);
     }
   else
-    (*call_readline) (client_data);
+    {
+      do
+	{
+	  call_stdin_event_handler_again_p = 0;
+	  (*call_readline) (client_data);
+	} while (call_stdin_event_handler_again_p != 0);
+    }
 }
 
 /* Re-enable stdin after the end of an execution command in
diff --git a/gdb/event-top.h b/gdb/event-top.h
index 2d05d45..c7b1224 100644
--- a/gdb/event-top.h
+++ b/gdb/event-top.h
@@ -61,6 +61,7 @@  extern void (*call_readline) (void *);
 extern void (*input_handler) (char *);
 extern int input_fd;
 extern void (*after_char_processing_hook) (void);
+extern int call_stdin_event_handler_again_p;
 
 extern void cli_command_loop (void *);
 
diff --git a/gdb/tui/tui-io.c b/gdb/tui/tui-io.c
index a890678..32cc96e 100644
--- a/gdb/tui/tui-io.c
+++ b/gdb/tui/tui-io.c
@@ -695,7 +695,27 @@  tui_getc (FILE *fp)
     TUI_CMD_WIN->detail.command_info.curch = 0;
   if (ch == KEY_BACKSPACE)
     return '\b';
-  
+
+  if (async_command_editing_p && key_is_start_sequence (ch))
+    {
+      int ch_pending;
+
+      nodelay (w, TRUE);
+      ch_pending = wgetch (w);
+      nodelay (w, FALSE);
+
+      /* If we have pending input following a start sequence, call the event
+	 handler again.  ncurses may have already read and stored the input into
+	 its internal buffer, meaning that we won't get an stdin event for it.  If we
+	 don't compensate for this missed stdin event, key sequences as Alt_F (^[f)
+	 will not behave promptly.  */
+      if (ch_pending != ERR)
+	{
+	  ungetch (ch_pending);
+	  call_stdin_event_handler_again_p = 1;
+	}
+    }
+
   return ch;
 }