Fixes #3767. Allowing any driver to request ANSI escape sequence with immediate response.

[BDisp]

Fixes

• Fixes Allowing any driver to request ANSI escape sequence with immediate response. #3767

Proposed Changes/Todos

• Implemented the ExecuteAnsiRequest method which can be used by any driver.
• Try request on any driver, on Windows, Linux or macOS and it will work.

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]

To me his feels really brittle. Its what I was trying to explain in other thread. I can see its possible just dangerous:

• We force a 200 ms pause every time we emit an request
• We don't check that the response matches the request (based on its terminator)
• The while loop just drains the console which could over-read. We should be stopping when we hit the expected terminator for the command e.g. 'c' for DAR
• Regardless of the state of the driver, we turn on mouse events at the end. So this method has a byproduct of turning on mouse (even if it was off before method called)

I still think we should update the existing input processing loop(s) e.g. ProcessInputQueue - instead of writing a bespoke process.

I think we could make a blocking public facing method even with that approach. Just need to think about it some more. Maybe we can use async and user can just call .Result if they want to wait.

[BDisp]

To me his feels really brittle. Its what I was trying to explain in other thread. I can see its possible just dangerous:

• We force a 200 ms pause every time we emit an request

This is a minor problem because we are not requesting ANSI escape sequences all the time, like the one that are setting the colors when are writing to the console.

• We don't check that the response matches the request (based on its terminator)

My bad, sorry. I created the EscSeqUtils and the EscSeqReq where I implemented the terminator from the requests reply. I was forgetting that. Thanks for show me that I was wrong.

• The while loop just drains the console which could over-read. We should be stopping when we hit the expected terminator for the command e.g. 'c' for DAR

Done. Thanks.

• Regardless of the state of the driver, we turn on mouse events at the end. So this method has a byproduct of turning on mouse (even if it was off before method called)

You are right. Thanks for call my attention. I added a flag into the NetDriver and CursesDriver that inform if the mouse moving report was enable or not.

I still think we should update the existing input processing loop(s) e.g. ProcessInputQueue - instead of writing a bespoke process.

But the immediate reply will still not prevail. When I found the current solution I never like how it's working because in some situations I wanted the response in the request method, to do some actions that depended on the response, but without success. I found it useless.

I think we could make a blocking public facing method even with that approach. Just need to think about it some more. Maybe we can use async and user can just call .Result if they want to wait.

I think it won't block because I first stopped the mouse move reports which will decrease significantly the number of chars to read.
I tried the async approach but was give me unexpected results because we need to get the request replies as soon as possible.

You can use your approach as well because my PR doesn't break it, only changed from string to the class I created but still use the same behavior. The only disadvantage is that your approach only work with the NetDriver.

If you want to test put these line in the Init method of all drivers the below lines:

        var response1 = AnsiEscapeSequenceRequest.ExecuteAnsiRequest (EscSeqUtils.CSI_SendDeviceAttributes);
        var response2 = AnsiEscapeSequenceRequest.ExecuteAnsiRequest (EscSeqUtils.CSI_RequestCursorPositionReport);
        var request = EscSeqUtils.CSI_RequestCursorPositionReport;
        AnsiEscapeSequenceResponse response3 = null;
        request.ResponseReceived += (s, e) => response3 = e;
        var response4 = AnsiEscapeSequenceRequest.ExecuteAnsiRequest (request);
        Debug.Assert (response3 == response4);

[tznind]

What is Value? I cant understand the comment. My example has 61...

The value expected in the response e.g.EscSeqUtils.CSI_ReportTerminalSizeInChars.Value which will have a 't' as terminator but also other different request may return the same terminator with a different value.

Can we also please have a helper property called Successful so you don't have to eg. null or whitespace the error field.

[tznind]

I can confirm that this works and have added it into a new branch of the main sixel branch I am working on.

See tznind#169

[BDisp]

What is Value? I cant understand the comment. My example has 61...

The value expected in the response e.g.EscSeqUtils.CSI_ReportTerminalSizeInChars.Value which will have a 't' as terminator but also other different request may return the same terminator with a different value.

Value is the number that appear at the beginning of the reply after the CSI. See https://terminalguide.namepad.de/seq/csi_st-18/ and https://terminalguide.namepad.de/seq/csi_st-15/. Both requests use the 't' as terminator and the reply also returns 't' as terminator. What distinguish each other are the Value. The prior returns 8 and the last returns 5.

Can we also please have a helper property called Successful so you don't have to eg. null or whitespace the error field.

It's not enough to check the error with string.IsNullOrEmpty? If you really prefer the Successful property I can add it.

[tznind]

Yes please either add Successful or have the method return bool and use out like TryParse style methods do.

I started with this ugliness:

        if (string.IsNullOrWhiteSpace (darResponse.Error) && !string.IsNullOrWhiteSpace (darResponse.Response))
        {
            // it worked
        }
        

[tznind]

@tig please can you take a look at this when you get the chance?

Currently I am working on 2 sixel branches (one with this merged in). If its going in right direction I will just collapse and work on only 1 branch.

[BDisp]

@tznind done in dfedcd8. Let me know if it's need to do something more. Thanks.

[tznind]

Cool thanks, although I meant for tryparse as an example of the pattern only. It should probably still be same name e.g. TryExecuteAnsi...

[BDisp]

Cool thanks, although I meant for tryparse as an example of the pattern only. It should probably still be same name e.g. TryExecuteAnsi...

Ops I misunderstood. Changed on 31dbae7.

[tznind]

Got a strange error in sixel-encoder-is-supported in visual studio terminal. When I open Images scenario and SixelDetector runs its ansi queries it seems to leave something bad in the input buffer that then crashes the windows driver:

1. Launch debugger mode
2. Tab to scenarios
3. Type "Image" (to move selection to the images scenario)
4. Hit enter

See error

System.ArgumentException
  HResult=0x80070057
  Message=Invalid KeyCode: Space, C is invalid. (Parameter 'value')
  Source=Terminal.Gui
  StackTrace:
   at Terminal.Gui.Key.set_KeyCode(KeyCode value) in D:\Repos\temp\gui.cs\Terminal.Gui\Input\Key.cs:line 214
   at Terminal.Gui.Key..ctor(KeyCode k) in D:\Repos\temp\gui.cs\Terminal.Gui\Input\Key.cs:line 79
   at Terminal.Gui.WindowsDriver.ProcessInput(InputRecord inputEvent) in D:\Repos\temp\gui.cs\Terminal.Gui\ConsoleDrivers\WindowsDriver.cs:line 1497
   at Terminal.Gui.WindowsMainLoop.Terminal.Gui.IMainLoopDriver.Iteration() in D:\Repos\temp\gui.cs\Terminal.Gui\ConsoleDrivers\WindowsDriver.cs:line 2249
   at Terminal.Gui.MainLoop.RunIteration() in D:\Repos\temp\gui.cs\Terminal.Gui\Application\MainLoop.cs:line 273
   at Terminal.Gui.Application.RunIteration(RunState& state, Boolean& firstIteration) in D:\Repos\temp\gui.cs\Terminal.Gui\Application\Application.Run.cs:line 528
   at Terminal.Gui.Application.RunLoop(RunState state) in D:\Repos\temp\gui.cs\Terminal.Gui\Application\Application.Run.cs:line 502
   at Terminal.Gui.Application.Run(Toplevel view, Func`2 errorHandler) in D:\Repos\temp\gui.cs\Terminal.Gui\Application\Application.Run.cs:line 383
   at UICatalog.Scenarios.Images.Main() in D:\Repos\temp\gui.cs\UICatalog\Scenarios\Images.cs:line 149
   at UICatalog.UICatalogApp.UICatalogMain(Options options) in D:\Repos\temp\gui.cs\UICatalog\UICatalog.cs:line 348
   at UICatalog.UICatalogApp.Main(String[] args) in D:\Repos\temp\gui.cs\UICatalog\UICatalog.cs:line 171

[BDisp]

I can't reproduce the error as you said but I can reproduce it by running the AnsiEscapeSequenceRequest scenario, delete the 'c' in the terminator text and click on the "Send Request" button. So, sending a request with a wrong terminator will cause this error because it'll continue reading keys until find the terminator key. The exception is throw in below, I'll try to fix this based on the response error. Please check if you receive any error response in the requests. Thanks.

[BDisp]

I have a fix to this. But I need your opinion, if the request terminator is empty and the response is successfully, it's need to write to the error property "Terminator request is empty." or ignore it? If I write to the error, the response will not return true. I think it's a good practice to include always the terminator in the request, for the Console.ReadKey (true) breaks when the terminator is found. The advantage for this is to avoid reading several requests at once returning an invalid response.

[BDisp]

Terminator request is mandatory to stopping read when it's found. So, it'll return false and the user will check what he does wrong.

[tznind]

Crash is not because of lack of terminator. You can see it is set.

Issue seems to be just that there is something left in buffer after reading in ANSII request class (Space+C)

Open Images crashes after sending DAR

[tznind]

I think maybe we are terminating our ASCII processing on the 'key down' of the response terminator and the 'key up' is still coming through from driver?

Could be an oddity of the console, this is the 2022 community edition visual studio out of the box console.

[BDisp]

Crash is not because of lack of terminator. You can see it is set.

Do you are using the updated branch after the latest fixes? When I said the crash is due the lack of terminator, I referred the one that is on the request and not in the response.

Issue seems to be just that there is something left in buffer after reading in ANSII request class (Space+C)

What causes this exception is because the key read is a 'c' that the IsKeyCodeAtoZ method only validade as true if ('c' | KeyCode.Space) != 0

I think maybe we are terminating our ASCII processing on the 'key down' of the response terminator and the 'key up' is still coming through from driver?

It's a possibility. WindowsDriver is the only that handle key down and key up.

Could be an oddity of the console, this is the 2022 community edition visual studio out of the box console.

It's a nor normal behavior of the Win32 API. The problem is that I'm using the Console.ReadKey to get the response on demand and it doesn't support the key up. How can I reproduce what you are facing?

[tznind]

This code is already quite complicated and coupled to the specific drivers e.g. switch statement on Driver Type.

Maybe we can reuse the existing message pumps that the drivers are already running and move the new code to a Parser pattern? Each driver would then have its own pump but reuse something like this.

We wouldn't stop the pumps we would just add an extra 'filtering' stage to the existing input processing pipeline?

I can have a go at this unless you can see reasons why this won't work and better decouple concerns?

/// <summary>
/// Driver agnostic parser that you can stream console output to as part of normal
/// input processing. Spots ansi escape sequenes as they pass through stream and
/// matches them to the current outstanding <see cref="AnsiEscapeSequenceRequest"/>
/// </summary>
public class AnsiRequestParser ()
{
    [CanBeNull]
    private AnsiEscapeSequenceRequest currentRequest;
    StringBuilder currentResponse = new StringBuilder ();
    private bool areInEscapeSequence = false;

    Queue<AnsiEscapeSequenceRequest> outstandingRequests = new Queue<AnsiEscapeSequenceRequest> ();


    /// <summary>
    /// Sends the given <paramref name="request"/> to the terminal or queues
    /// it if there is already an outstanding request.
    /// </summary>
    /// <param name="request"></param>
    public void QueueRequest (AnsiEscapeSequenceRequest request)
    {
        if (currentRequest == null)
        {
            SendRequest (request);
        }
        else
        {
            outstandingRequests.Enqueue (request);
        }
    }

    private void SendRequest (AnsiEscapeSequenceRequest request)
    {
        currentRequest = request;
        Console.Write ("... the request");
    }

    /// <summary>
    /// Takes the given key read from console input stream.  Returns true
    /// if input should be considered 'handled' (is part of an ongoing
    /// ANSI response code)
    /// </summary>
    /// <param name="key"></param>
    /// <returns></returns>
    public bool Process (char key)
    {
        // Existing code in this PR for parsing


        // If completes request
            currentRequest = null;
            // send next from queue
            return true;


        // We have not read an Esc so we are not in a response
        return false;
    }
}

[tznind]

Do you are using the updated branch after the latest fixes? When I said the crash is due the lack of terminator, I referred the one that is on the request and not in the response.

Have tried and get no difference in behaviour.

I think you are right and that it is the mixing of Console Console.ReadKey and win32 API.

How can I reproduce what you are facing?

Not sure... this is why I was wanting to go for the parser approach as it is much easier to test. And each driver can concern itself with its own message pump oddities.

I guess an alternative would be to try and add Read and Write methods to IConsoleDriver. This could itself be handy for sixel output writing.

[tznind]

Get an issue running this code with the new Windows Terminal Preview (the one that has sixel support). Seems the response you read is just full of \0.

I have tested in both 1.22.2702.0 and 1.22.2362.0

[BDisp]

In which driver? I think I know why with my last refactor.

[BDisp]

Guys, I'm diving into reviewing this and I'm confused.

Can you please explain the relationship between this PR and the ANSI Parser PR?

There is no relationship between both. Only we have an idea for later the user choice one of them to send requests.

Is one dependent on the other?

That I know of, there is no dependencies on each other, but if one is merged first the other will have much work to do to implement and adapt each other. Mine have much more driver changes which will I think will help some iterations.
The mainly diference of both is that mine PR try to get immediately the request response or none if there is a bad request and the ANSI Parser schedule the requests and wait for the response or cancel it after a timeout. So, you have to decide on how this will go on first.

[tznind]

These are basically 2 ways of solving the same problem

• ansi-parser branch adds a 'pre filter' to all console input loops that picks up responses. Independently there is a scheduler that handles sends.
• ansi-escape-sequence-all-drivers allows client code to force process the input loop outside of its main iteration until it finds some input it likes (ansi code). So it sends then operates the console input loop till it finds response.

The first is harder to use for api of our library because it is based on events. The second is blocking so user gets answer right away but is imo harder to maintain/test and may be less stable.

@BDisp please correct me if I misunderstood your implementation.

We could try and support both or pick one. Either way it needs a ton of testing on old terminals too because if you fumble an escape code and it ends up going through input loop as a keypress then users view just closes and app exits. Or escape codes start going into text boxes etc

Also needs tested with things like ssh and screen etc

[tig]

PR with these comments and a bunch of code clean up inbound.

Please look for new // TODO: and // QUESTION: in the code.

This is really, really, really great stuff!!!

[tig]

Should this be virtual with a default implementation that does the common stuff?
Looking at the implementations of this method, there is TONs of duplicated code.
We should figure out how to find just the things that are unique to each driver and
create more fine-grained APIs to handle those.

[tig]

Review comments above and a PR:

• Code Review of #3768 BDisp/Terminal.Gui#203

[BDisp]

These are basically 2 ways of solving the same problem

• ansi-parser branch adds a 'pre filter' to all console input loops that picks up responses. Independently there is a scheduler that handles sends.
• ansi-escape-sequence-all-drivers allows client code to force process the input loop outside of its main iteration until it finds some input it likes (ansi code). So it sends then operates the console input loop till it finds response.

This isn't now true because it doesn't force process the input loop outside of its main iteration anymore. All drivers now uses their own input loop to get the requests responses. So, any key or mouse event are always handle during the process. But initially I was using the method you described but not now.

The first is harder to use for api of our library because it is based on events. The second is blocking so user gets answer right away but is imo harder to maintain/test and may be less stable.

Doesn't blocking anymore. The request is always returned, with a valid or not valid response, but always return. I admit that can have some delay when a lot of requests are constantly sent, but I think there is an reasonable response.

@BDisp please correct me if I misunderstood your implementation.

I already done above.

We could try and support both or pick one. Either way it needs a ton of testing on old terminals too because if you fumble an escape code and it ends up going through input loop as a keypress then users view just closes and app exits. Or escape codes start going into text boxes etc

I admit that I concentrated all my unit tests to the EscSeqUtils class which the requests feature use because it had many bugs that I think I already fixed. For ANSI requests unit tests I have to create a mock responses for that, but at least in the UICatalog it's was full tested.

Also needs tested with things like ssh and screen etc

I agree. But I think we can use both implementations using different classes, like now, and the user instantiate the one he want, mine with event or yours with action.

[BDisp]

Review comments above and a PR:

• Code Review of #3768 BDisp/Terminal.Gui#203

Great. Thanks.

[tig]

But I think we can use both implementations using different classes, like now, and the user instantiate the one he want, mine with event or yours with action.

This makes no sense to me. There is only one "user" of these classes: Us, the maintainers of TG.

We should not have two implementations of the same thing.

[BDisp]

This makes no sense to me. There is only one "user" of these classes: Us, the maintainers of TG.

We should not have two implementations of the same thing.

But we was think about was using a public flag or enum for the user choose the type of request. If the user choose AnsiParser then the drivers will initialize the appropriate class and the returning response. So, only TG handle that.

[tznind]

This code is in all 3 drivers. It looks like you block 100ms to let the sent request get processed and then 'force run' the input stream for a bit, then look for a response?

Is it possible for you to explain in some pseudocode how it is working?

This PR is enormous and has so much in it that isn't related to ansi response send/capture it's hard to understand what is going on.

I've tried to standardise input processing with AnsiParser to replace NetEvents and similar code in the other drivers. But now all the drivers have huge changes in input handling its going to be difficult to retro fit.

I don't think we need 2 solutions to ansi send. I'd be happy if we just did

• get rid of all the locking instead use BlockingCollections
• standardise the parsing of request across all 3 drivers into a single class (i.e. `AnsiRequestParser).

We can still do the blocking on response but the class that identifies responses in input streams needs to be centralised into a single class

I will change by branch to draft in the meantime.

[BDisp]

This code is in all 3 drivers. It looks like you block 100ms to let the sent request get processed and then 'force run' the input stream for a bit, then look for a response?

Is it possible for you to explain in some pseudocode how it is working?

It's a ManualResetEventSlim like it's used in the MainLoop that wait to be signaled. It only wait until has a response or until two retries and return immediately.

This PR is enormous and has so much in it that isn't related to ansi response send/capture it's hard to understand what is going on.

I really refactored the CursesDriver to leverage using non blocking input, like I did in the WindowsDriver.

I've tried to standardise input processing with AnsiParser to replace NetEvents and similar code in the other drivers. But now all the drivers have huge changes in input handling its going to be difficult to retro fit.

I think that isn't very difficult to implement, beside the changes.

I don't think we need 2 solutions to ansi send. I'd be happy if we just did

• get rid of all the locking instead use BlockingCollections

I'm open on that but I need some explanation of that. I thought using the ManualResetEventSlim was a good idea, but I would like to know about alternatives, of course.

• standardise the parsing of request across all 3 drivers into a single class (i.e. AnsiRequestParser).

The NetDriver and the CursesDriver relies in the EscSeqUtil.DecodeEscSeq method because they are handling key and mouse events, but the WindowsDriver relies on the WindowsConsole.ReadConsoleInput because any other key that it isn't a normal key or a mouse event, is handled in this method.

We can still do the blocking on response but the class that identifies responses in input streams needs to be centralised into a single class

Well due to my above explanation NetDriver and the CursesDriver relies in the EscSeqUtil.DecodeEscSeq and the WindowsDriver relies on the WindowsConsole.ReadConsoleInput. So, I don't know if they can be in a single class. But I understand your concern.

[tig]

This makes no sense to me. There is only one "user" of these classes: Us, the maintainers of TG.
We should not have two implementations of the same thing.

But we was think about was using a public flag or enum for the user choose the type of request. If the user choose AnsiParser then the drivers will initialize the appropriate class and the returning response. So, only TG handle that.

Why would we ask/force app developers to have to understand all this? Is there some clear use-case where developer would pick one vs. the other?

[BDisp]

Why would we ask/force app developers to have to understand all this? Is there some clear use-case where developer would pick one vs. the other?

The user may understand if he want an immediate response or a later response which can be read in another thread than the current thread. That only thoughts.

[tznind]

@BDisp I have created a pull request to show how to switch to BlockingCollection and vastly simplify the queue handling that is going on in net driver:

BDisp#204

[tig]

Why would we ask/force app developers to have to understand all this? Is there some clear use-case where developer would pick one vs. the other?

The user may understand if he want an immediate response or a later response which can be read in another thread than the current thread. That only thoughts.

Please provide an actual use-case, not an abstract concept. If you can't then there's no reason to provide choice.

TG apps should never care about any of this stuff. It should be completely hidden from app developers.

[BDisp]

Please provide an actual use-case, not an abstract concept. If you can't then there's no reason to provide choice.

TG apps should never care about any of this stuff. It should be completely hidden from app developers.

I'll discussed this with @tznind to see how this will be needed. For now I can't answers because we still are trying pruning this.

[BDisp]

@BDisp I have created a pull request to show how to switch to BlockingCollection and vastly simplify the queue handling that is going on in net driver:

BDisp#204

Thank you very much. I did much more changes. Please say what you think about, please.
I also will to switch to BlockingCollection in the WindowsDriver and CursesDriver.