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

[Patrick Palka <patrick at parcs dot ath dot cx>]

On Thu, Aug 28, 2014 at 11:59 AM, Pedro Alves <palves@redhat.com> wrote:
> 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...

Indeed.. it certainly doesn't help that the ncurses source code is
pretty difficult reading.

>
>> 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.

Interesting, I'll take a look at this aspect.

>
> 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, for your insightful and detailed review.

>
> Thanks,
> Pedro Alves
>