Re: [PATCH] Fix the processing of Meta-key commands in TUI

[Pedro Alves <palves@redhat.com>]

On 08/28/2014 03:13 PM, Patrick Palka wrote:
> On Thu, Aug 28, 2014 at 7:22 AM, Pedro Alves <palves@redhat.com> wrote:
>> On 08/27/2014 07:25 PM, Patrick Palka wrote:
>>> On Wed, 27 Aug 2014, Pedro Alves wrote:
>>
>>>> The main reason I think there's a larger problem here, is that
>>>> if curses is reading more than one char from stdin, then that means
>>>> that is must be blocking for a bit waiting for the next character,
>>>> which is a no-no in an async world, where we want to be processing
>>>> target events at the same time.  The man page says:
>>>>
>>>>  While interpreting an input escape sequence, wgetch sets a timer while waiting for the next character.  If notimeout(win, TRUE) is called, then wgetch does not
>>>>  set a timer.  The purpose of the timeout is to differentiate between sequences received from a function key and those typed by a user.
>>>>
>>>> Looks like there's a default timeout of 1 second.  Indeed if I set a
>>>> breakpoint in wgetch and another right after wgetch is called, and
>>>> then I press escape, I see that gdb is stuck inside wgetch for around
>>>> one second.  During that time, gdb's own event loop isn't being processed.
>>>>
>>>> Not sure exactly how this is usually handled.  Seems like there
>>>> are several knobs that might be able to turn this delay off.
>>>> Sounds like we should enable that (whatever the option is),
>>>> and handle the timeout ourselves?
>>>
>>> I don't think the timeout is the issue here.  Even if the timeout is
>>> disabled via notimeout(), wgetch() will still attempt to interpret keypad
>>> sequences by reading multiple characters from stdin -- except that the
>>> read will now be a non-blocking one instead of a blocking one.
>>>
>>> One way or another, someone must read multiple keys from stdin in order
>>> to semantically distinguish between keypad keys and regular key
>>> sequences.  And when it turns out that the input is not or cannot be a
>>> keypad key then that someone must place the extraneous keys into a
>>> buffer and notify GDB's event handler that we missed their stdin events.
>>
>> Right, that's a given.  What I was talking about is fixing the
>> 1 second block in case the input stops before a sequence is complete.
>>
>>> If we handle the timeout ourselves (for instance by disabling keypad()
>>> and enabling notimeout()) then we'll be responsible for doing the
>>> lookahead, interpreting the sequences and buffering the keypresses.  I
>>> say let ncurses continue to handle the timeout so that we'll only be
>>> responsible for notifying the event handler.
>>>
>>> Though I may just be misunderstanding your proposal.
>>
>> The main idea was to not let ncurses ever block, as that prevents
>> gdb's event loop from handling target events.  If ncurses internally
>> already handled the timeout by comparing the time between
>> wgetch calls instead of doing a blocking select/poll internally, then
>> it'd be a bit easier, but it looks like it doesn't (GetEscdelay always
>> starts with the window's configured delay, on each wgetch call?), so we'd
>> need to track the timeout ourselves.  Even if it did, it wouldn't be that
>> different though.
> 
> Is the internal timeout a big deal though?  The handling of the target
> event just gets temporarily delayed, not missed entirely, right?  And
> isn't this timeout only experienced when one presses ESC (which has no
> use in TUI) and/or attempts to manually type a function key sequence?
> I'm not sure why anyone would do that.

Right.  TBC, I noticed/found this while ramping up for reviewing this patch,
reading the ncurses code and trying to make sense of the whole ncurses/tui
integration and your patch.  I'm not _that_ familiar with this
code, although I'm probably one of the most familiar nowadays...

My main motivation for pointing this out was to brainstorm.  I had
no idea if there was a canned solution for this, which your or someone
might know about.  :-)  I wasn't really sure of the complexity of the
solution until I spent a few more hours this morning working through
what it would take and write out those steps list...

> Yeah same here.  I can't seem to find the magical invocation that
> actually disables this timeout.

Thanks for poking.

>> If such delays/blocks can't be eliminated due to buggy ncurses, or
>> something missing in the APIs, then it looks like the only way
>> to fix this would be to move the wgetch call to a separate thread,
>> like, we'd create a pipe, and put one end in the event loop as
>> stdin source instead of the real stdin, and then the separate thread
>> would push the results of wgetch into this pipe...
> 
> I am not sure that it's possible to eliminate the internal timeout
> completely so it seems to me that  your thread-based solution may be
> necessary to fix this.  But is it worth the complexity to fix this
> seemingly obscure issue?

Yeah, we certainly shouldn't let the perfect be the enemy of the
good.

I'd think using vi-mode keybindings would need it, but according
to PR11383 those don't work anyway.

The thread-based solution would make your patch unnecessary,
but then again, that's easy to revert if a thread-based solution
ends up written.

I think we understand the problems and the effort required
for the potential solutions enough to be able to firmly
say: yes, let's proceed with your solution.

Thanks,
Pedro Alves