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

[BDisp]

Fixes

• Fixes #3767

Proposed Changes/Todos

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

Pull Request checklist:

• [x] I've named my PR in the form of "Fixes #issue. Terse description."
• [x] 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.
• [x] My code follows the Terminal.Gui library design guidelines
• [x] I ran dotnet test before commit
• [x] I have made corresponding changes to the API documentation (using /// style comments)
• [x] My changes generate no new warnings
• [x] I have checked my code and corrected any poor grammar or misspellings
• [x] 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 https://github.com/tznind/gui.cs/pull/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 https://github.com/gui-cs/Terminal.Gui/pull/3768/commits/dfedcd88e19da7551ab3d8665fd7309d54108491. 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 https://github.com/gui-cs/Terminal.Gui/pull/3768/commits/31dbae779125cefc4cfd18a09c7e09a24f6c8a0f.

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

[tznind]

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

WindowsDriver.

It works with Windows Terminal just not the preview version - the one I am using to test sixel.

For reference this is what the new terminal preview returns for DAR (with #3791 )

Also works correctly with NetDriver

[BDisp]

I really don't know if we can rely on Windows Terminal Preview for now. See below. It's a regression or it's some workaround used in the current WT version that now was fixed on the WT Preview or vice versa? This is only happening in the WindowsDriver.

[BDisp]

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

When a StringBuilder is initialized it fill m_ChunkChars with null chars, so you are seeing it with a full of \0. The cause for this not been working is because the Console.KeyAvailable always returns false and thus don't read the response.

[BDisp]

I really don't know if we can rely on Windows Terminal Preview for now. See below. It's a regression or it's some workaround used in the current WT version that now was fixed on the WT Preview or vice versa? This is only happening in the WindowsDriver.

I already fixed this in this commit https://github.com/gui-cs/Terminal.Gui/pull/3768/commits/91d31bd1c48f999cb25f1681cef6afb6915c632d.

[BDisp]

The only driver that doesn't work well with sixel is the NetDriver because it use the same std-in and std-out and when I press Esc to exit the sixel scenario it doesn't exit. Since the WindowsDriver and CursesDriver they use their own std-in and std-out, allows the Console.ReadKey can read the input separately.

[BDisp]

The only driver that doesn't work well with sixel is the NetDriver because it use the same std-in and std-out and when I press Esc to exit the sixel scenario it doesn't exit. Since the WindowsDriver and CursesDriver they use their own std-in and std-out, allows the Console.ReadKey can read the input separately.

This issue seems to be fixed in https://github.com/gui-cs/Terminal.Gui/pull/3768/commits/884011e99cdb76fe330af2d80bf3ca1343350dee.

[tznind]

@BDisp / @tig / @dodexahedron do you want me to continue looking at processing ANSI requests without suspending the input stream (see #3791)?

There are 2 approaches here:

• The approach in this PR to suspend drivers and flush input while we then run the ANSI send/read (i.e. blocking), or
• Make ansi request detection a standard part of the input processing pipeline (i.e. non blocking)

I don't think there is much point in having both approaches. I don't mind going with the blocking approach if that is the consensus - and I can close my PR.

There are advantages to each:

Blocking

• Immediate response from user code
• Error handling for missed requests etc

Non Blocking

• Simpler interaction with console i.e. no Thread.Sleep, Flush input etc

[BDisp]

@BDisp / @tig / @dodexahedron do you want me to continue looking at processing ANSI requests without suspending the input stream (see #3791)?

Yes I want. I'm loving your work but I'm afraid that for immediate response from a request that needs to be handle in the same thread that may not exist anymore, because the scenario was closed. So, the action may run out of race. Imagine you want to update something in a scenario that is expecting a request response, but you click esc before the action is called. If you update something on a view that was already been disposed an exception will be throw. But I'm curious to know how you'll handle that.

There are 2 approaches here:

• The approach in this PR to suspend drivers and flush input while we then run the ANSI send/read (i.e. blocking), or
• Make ansi request detection a standard part of the input processing pipeline (i.e. non blocking)

That is the behavior that most of the requests wants. I'm only suspend until the response is returned, then all events that was queued are processed.

I don't think there is much point in having both approaches. I don't mind going with the blocking approach if that is the consensus - and I can close my PR.

I prefer you don't close your PR because you may be right and I may be wrong. Maybe you find a way that could work as expected.

There are advantages to each:

Blocking

• Immediate response from user code
• Error handling for missed requests etc

For immediate action.

Non Blocking

• Simpler interaction with console i.e. no Thread.Sleep, Flush input etc

Nice for some requests that doesn't need immediate action, like the DAR that can be set later when the action is processed, which may not be in the same thread.

But please, continue your work. It's lovely. It may could improve the EscSeqUtils class.

[BDisp]

@tznind please also add more requests to your scenario. You can use mine in this PR and adapt it to your PR. You can use the predefined already existents and use any new by editing the TextField. Do stressed tests in the scenario by constantly click the mouse on the send button and also continuously press the enter key, by press and release or maintaining it pressed. With these stressed tests I found another issues in my PR and fixed the best I know. Thanks.