Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ansi parser #3791

Open
wants to merge 90 commits into
base: v2_develop
Choose a base branch
from
Open

Ansi parser #3791

wants to merge 90 commits into from

Conversation

tznind
Copy link
Collaborator

@tznind tznind commented Oct 11, 2024

@BDisp I wanted to explore how we could detect the ansi sequences within input stream without having to put breaks on input like in #3768

This is where I have gone so far, what do you think?

The purpose is to stream text from console driver as read and 'transparently' pluck out the expected responses live.

Explanation of Ansi Parser

AnsiParser processes events from an input stream. Each time it will take 1 char and either add it to Held or Release it. Critically it sometimes releases more than 1 event.

Each event is a char which can have optional metadata attached (See T header below).

Consider the following use case:

User presses Esc key then types "Hi" then hits Esc again. While the user is typing a DAR response comes in <esc>[0c

image

Here is a breakdown of how AnsiParser would deal with this:

Detailed breakdown of state at each step

Stage 1 - Consume first Esc

The first call to ProcessInput is with the Esc char. This causes the parser to shift into expecting an escape code (i.e. a bracket). Because we are assuming the Esc is a response we hold it and return empty.

image

Stage 2 - Consume H

The next call to ProcessInput is with H. Because H is a _knownTerminators we realize that we have either come to the end of an escape sequence or the user was just typing away normally.

First we look to see if we are indeed waiting for a response to come in with terminator H by looking at expectedResponses. We do not find one so instead release both the Esc and the H for routine processing.

image

Stage 3 - Consume next escape sequence

This process repeats, this time for the actual response we are expecting (i.e. ending with c)

image

image

image

When we reach the first character that is a _knownTerminators (i.e. c) or matches one of our expectedResponses terminators we will leave the InResponse state.

image

If the response built up in Held matches an expectedResponses we swallow it (Release None) and raise event
If the response built up does not match (instead it matches _knownTerminators) we release the whole thing back to be processed downstream.

Stage 4 - Consume last Esc

Finally we consume the last Esc which leaves us in state Expecting Bracket. Now this may be the start of a new escape sequence or it could be that the user has just pressed Esc - we will not know which till later.

BUT we don't want to sit around waiting for the rest of the escape sequence forever. Pressing Esc could be the last thing the user does and there could be no events while user sits around watiting for app to respond.

For this reason we put timestamp on state changes StateChangedAt - this lets the caller (e.g. driver main loop)

image

BUT we don't want to sit around waiting for the rest of the escape sequence forever. Pressing Esc could be the last thing the user does and there could be no events while user sits around watiting for app to respond.

For this reason we put timestamp on state changes StateChangedAt - this lets the caller (e.g. driver main loop) force the Parser to release the Esc after a short period of time if no new input is comming:

if (Parser.State == ParserState.ExpectingBracket &&
    DateTime.Now - Parser.StateChangedAt > _escTimeout)
{
    return Parser.Release ().Select (o => o.Item2);
}

Why <T>?

I realized working exclusively in char and string made it difficult for WindowsDriver to integrate - so I have changed to AnsiParser<T> . The class now deals with sequences of char each of which has any metadata you want (type T). For WindowsDriver this means AnsiResponseParser<WindowsConsole.InputRecord>.

The parser can pull things out of the stream and later return them and we don't loose the metadata.

I will look at the other drivers, if they are dealing just in char with no other metadata I can see about putting a non generic one too that just wraps the generic like TreeView vs TreeView<T>

Outstanding Issues

  • Key down and key up means double key presses
  • Esc on its own as isolated keypress needs to have a release timeout (i.e. if we dont see '[' in 50ms then we need to release that back to input stream as nothing else is coming).
  • deal/prevent parallel outstanding requests executing at once
  • Hitting Esc key twice in quick succession or users Esc+start escape sequence

Fixes

  • Fixes #_____

Proposed Changes/Todos

  • Todo 1

Pull Request checklist:

  • I've named my PR in the form of "Fixes #issue. Terse description."
  • My code follows the style guidelines of Terminal.Gui - if you use Visual Studio, hit CTRL-K-D to automatically reformat your files before committing.
  • My code follows the Terminal.Gui library design guidelines
  • I ran dotnet test before commit
  • I have made corresponding changes to the API documentation (using /// style comments)
  • My changes generate no new warnings
  • I have checked my code and corrected any poor grammar or misspellings
  • I conducted basic QA to assure all features are working

@tznind
Copy link
Collaborator Author

tznind commented Oct 12, 2024

Key down and Key up events are a pain. Here is what response looks like when sending a Device Attributes Request on startup in WindowsDriver.

We get keydown and then keyup for each letter.
esc[?1;0c

image

So we would need another 'pre step' to the parser that paired up down/up events and only passed them around in pairs (i.e. swallow both or release both). Currently I am using T as InputRecord but now maybe it needs to be a Pair (down and up).

Any idea if this is universal behaviour for WindowsDriver? or are there cases where it only sends key downs for example?

@BDisp
Copy link
Collaborator

BDisp commented Oct 12, 2024

Any idea if this is universal behaviour for WindowsDriver? or are there cases where it only sends key downs for example?

I think that is a behavior for Wind32 API but would be interesting to know how that work with the others drivers, because you are sending any keystroke with the prefix 0x1b, right? If the other drivers respond with the same output, then there is a possibility that all drivers can deal with key-down and keu-up.

@tznind
Copy link
Collaborator Author

tznind commented Oct 12, 2024

Ok I have added WindowsDriverKeyPairer to match up input key down/up pairs - good news is no changes to the parser we just make the 'metadata' for each char the pair and return that to the main loop.

d642fb6

@tznind
Copy link
Collaborator Author

tznind commented Oct 12, 2024

Key down/up is very unreliable. Just by typing fast you get random order of down/ups (this is on v2_develop):

image

@tznind
Copy link
Collaborator Author

tznind commented Oct 12, 2024

Maybe we can ditch key up event for v2? it seems almost everything goes of key down anyway? And 2 of the 3 drivers are just making up down/up as pairs anyway - for example NetDriver:

                KeyCode map = MapKey (consoleKeyInfo);

                if (map == KeyCode.Null)
                {
                    break;
                }

                OnKeyDown (new Key (map));
                OnKeyUp (new Key (map));

                break;

In other news though, here is the current WIP, we can send DAR request on demand. It has issues though - crashes a lot. Issues are:

  • key down/up pair processing
  • multi threading/instance (enumeration modified...)
  • not implemented for curses/netdriver/fakedriver

send-dar

@tznind
Copy link
Collaborator Author

tznind commented Oct 28, 2024

Ok this now includes the NetDriver changes to have AnsiParser take over responsibility for response parsing instead of the NetEvents class

Only issues I now have revolve around some terminals sending interleaved responses

As described here: #3791 (comment)

We no longer have the DAR responses tripping each other because we schedule those to have 1 in 1 out.

But sometimes we see mouse getting interrupted by DAR response (in some terminals).

Easiest way to get this to trigger - if you have a console where it is an issue. Is to run the scenario and set to 20 DAR per second then just wave mouse around frantically.

🎥 Video Reproducing Issue

rapid-moves

It manifest in one of two ways

  • Scenario closes because 2 Esc get seen in a row (DAR interleaved with Mouse both outputting Esc then both outputting [ (or some variation thereof)
  • If in a text field - part of the mouse response code or DAR appearing written into text view as text

It may be possible to detect this situation in some cases although I need a large sample of cases - so a way of capturing the occurrances.

Here is what it looks like for mouse/DAR interruption:

image
DAR response (green) comes in mid-stream before mouse event (pink) is complete

@tznind
Copy link
Collaborator Author

tznind commented Oct 28, 2024

@j4james thanks for the help on the Sixel stuff - maybe I can pick your brains on this also?

Have you ever seen this kind of behaviour from terminals? I am pretty sure this is coming in raw like this and not a race condition in the Terminal.Gui code.

Any advice would be appreciated.

@j4james
Copy link

j4james commented Oct 28, 2024

@tznind I've not experienced that issue myself, but I could easily believe that was a bug in older versions of the conhost/conpty framework. And if you're getting \e[1;0c in the DA1 report, that suggest you're testing an old version of Windows Terminal, or an old version of conhost, or a third-party terminal which is relying on an old version of conhost.

Something similar was reported quite recently as a bug in Windows Terminal (microsoft/terminal#17775), but that has since been fixed, and was identified as a regression, so I think that would only have been evident in versions 1.19 to 1.21 of Windows Terminal (which would not have returned a \e[1;0c report). However, that does suggest that a similar bug might have been present in older versions as well.

@tznind
Copy link
Collaborator Author

tznind commented Oct 28, 2024

Interesting, thanks for linking - that does sound similar, and indeed, I have a very old version running as default terminal for Visual Studio.

The test here is pretty extreme (spamming DAR is a bit of an artificial use case) and it doesn't reproduce on more recent versions - maybe we can live with this.

I will look into it further and try with some of the Linux consoles like xterm and mlterm I have on laptop later this week.

@tznind tznind marked this pull request as ready for review November 1, 2024 18:23
@tznind
Copy link
Collaborator Author

tznind commented Nov 1, 2024

@BDisp this is also ready - which do you want to merge first if we are going to have both these systems (blocking/realtime) as options?

@BDisp
Copy link
Collaborator

BDisp commented Nov 1, 2024

@BDisp this is also ready - which do you want to merge first if we are going to have both these systems (blocking/realtime) as options?

I prefer mine is merge first because it has all driver as non-blocking input and very raw changes. I think it would more easy to merge yours later, if you don't mind.

@tznind
Copy link
Collaborator Author

tznind commented Nov 1, 2024

ok, I also have changes to driver but I think they are more limitted

@tznind
Copy link
Collaborator Author

tznind commented Nov 2, 2024

@BDisp I have also added your combo box and controls to this PR so we can test other commands.

I also added the StoppedExpecting event so we can see when the parser gives up on getting a response.

run-single

@BDisp
Copy link
Collaborator

BDisp commented Nov 2, 2024

On the request text field put a invalid request that wont response at all and see what happen. Also with a valid request maintain the enter key pressed during a long time and see if any error come up.

Comment on lines 4 to 7
"commandName": "Project",
"commandLineArgs": "-d NetDriver"
},
"UICatalog --driver NetDriver": {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is already a NetDriver for the UICatalog. This is really needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted, this was change made in UI

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants