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.

[tznind]

With WindowsDriver clicking 'SendRequest' exits (i.e. is interpretted as Esc not ansi response) in VisualStudio console only (i.e. not reproduce in Windows Terminal).

[tznind]

I think we should merge #3791 instead of this to start with. The parsing and request sending code in this PR adds even more threading and locking when we already have issues with the drivers e.g. see #3812

See also the following issues/prs which create a compelling case for simplification. I think having standalone classes doing send/schedule management rather than each driver having its own new thread with events and slims etc is the way to go

• Adds ANSIDriver - Pure "Console Virtual Terminal Sequences" driver #2940
• Refactoring, cleanup, and abstraction for ConsoleDrivers #3692

[BDisp]

With WindowsDriver clicking 'SendRequest' exits (i.e. is interpretted as Esc not ansi response) in VisualStudio console only (i.e. not reproduce in Windows Terminal).

I can't reproduce this.

[tznind]

With WindowsDriver clicking 'SendRequest' exits (i.e. is interpretted as Esc not ansi response) in VisualStudio console only (i.e. not reproduce in Windows Terminal).

I can't reproduce this.

This is the state of the read input at the point your code decides to send Escape key down

Here is the repro video:

Here is a git diff patch that adds static queue of the input events

 git diff
diff --git a/Terminal.Gui/ConsoleDrivers/WindowsDriver/WindowsConsole.cs b/Terminal.Gui/ConsoleDrivers/WindowsDriver/WindowsConsole.cs
index b27b8fe96..5c0f620e2 100644
--- a/Terminal.Gui/ConsoleDrivers/WindowsDriver/WindowsConsole.cs
+++ b/Terminal.Gui/ConsoleDrivers/WindowsDriver/WindowsConsole.cs
@@ -57,6 +57,7 @@ internal class WindowsConsole
         return null;
     }

+    public static List<InputRecord> seen = new ();
     public InputRecord? ReadConsoleInput ()
     {
         const int BUFFER_SIZE = 1;
@@ -80,6 +81,8 @@ internal class WindowsConsole
                                       BUFFER_SIZE,
                                       out numberEventsRead);

+                    seen.Add (inputRecord);
+
                     if (inputRecord.EventType == EventType.Key)
                     {
                         KeyEventRecord keyEvent = inputRecord.KeyEvent;

I guess what happens is the console flushes <esc>[? then takes a breather for a millisecond before flushing 1;0c

Probably your code is bailing too early. I handled this in AnsiParser by being agnostic of blocking/flushing etc and instead using the detection of terminator for release or fixed 'human' time for releasing Esc (and subsequent keys).

See for example this approach I took. It removes state (parser) and management (this code is in ConsoleDriver). Basically at any time you can order the parser to release whatever it has held. ConsoleDriver does this based on time/state but unit tests can just order it explicitly.

The division of concerns makes things more agile, future proof (i.e. easily modifiable) and testable:

    public IEnumerable<ConsoleKeyInfo> ShouldRelease ()
    {
        if (Parser.State == AnsiResponseParserState.ExpectingBracket &&
            DateTime.Now - Parser.StateChangedAt > _consoleDriver.EscTimeout)
        {
            return Parser.Release ().Select (o => o.Item2);
        }

        return [];
    }

[BDisp]

I made all the steps you did here and I don't get any OnKeyDown event when I mouse click on the send button. What is the _isWindowsTerminal value when you hit the event?

I have as true because it's running devenv get by the "VSAPPIDNAME" env.

[tznind]

I am not surprised that you cannot repro. Terminals can all vary a lot between versions.

This is my term exe debugger version.

It was at

C:\Program Files\Microsoft Visual Studio\2022\Community\Common7\IDE\CommonExtensions\Platform\Debugger\VsDebugConsole.exe

The important bit is that solution must deal with 'fractured' input. Where console says no key is available even mid response read for a few milliseconds even

[BDisp]

The same here but in Portuguese.

C:\Program Files\Microsoft Visual Studio\2022\Community\Common7\IDE\CommonExtensions\Platform\Debugger\VsDebugConsole.exe

[tznind]

Is this same exe path you see when opening DeviceManager while debugging and going to 'open exe location'?

You may have multiple versions installed

[BDisp]

What the net80 version do you're using? The latest VS2022 also installed the net90. Are you running the latest updated version of my branch?

[BDisp]

Is this same exe path you see when opening DeviceManager while debugging and going to 'open exe location'?

You may have multiple versions installed

Yes, this is the same when I' debugging and going to 'open exe location'.

I think you meant TaskManager.

[tznind]

Yes running latest commit i checked with git log

…

On Sun, 17 Nov 2024, 11:07 BDisp, ***@***.***> wrote: Is this same exe path you see when opening DeviceManager while debugging and going to 'open exe location'? You may have multiple versions installed Yes, this is the same when I' debugging and going to 'open exe location'. — Reply to this email directly, view it on GitHub <#3768 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHO3C5HZXLAZQAMFCHDSVLT2BB2ITAVCNFSM6AAAAABPCGIIDSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIOBRGE4DOMZXGU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[BDisp]

Yes running latest commit i checked with git log

I don't understand what it's causing this behavior to you. All drivers don't send any escape sequence response to the ProcessInput. I already prevented that but may could have some bug which I don't know yet.

[tznind]

I've written more about the direction I think we should be headed here:

#3821

[BDisp]

Definitely I agree with @tznind #3821 approach and we must give priority to #3791. I think I should close this to not confuse his work. Can I close this @tig?

[tig]

Yep!

[tig]

@BDisp & @tznind I put a bunch of work into a PR that went into this PR that cleaned up a bunch of driver stuff. mostly structrual.

https://github.com/tig/Terminal.Gui/tree/BDisp-v2_3767_ansi-escape-sequence-all-drivers

By closing this PR that work was lost.

Can you please help me get that work back and into v2_develop?

[BDisp]

@BDisp & @tznind I put a bunch of work into a PR that went into this PR that cleaned up a bunch of driver stuff. mostly structrual.

https://github.com/tig/Terminal.Gui/tree/BDisp-v2_3767_ansi-escape-sequence-all-drivers

By closing this PR that work was lost.

Can you please help me get that work back and into v2_develop?

That isn't very easy to track all the changes done because they are in a new folders and have a bunch of changes.

[tznind]

I have also begun splitting ConsoleDriver into its 3 constituent parts, each is now a seperate class with base classes etc - see #3821 and #3837

• Content (state + draw)
  • I may split this down the road into state and render classes but this is fine for now.
• Out (writing to output, screen buffers, p/invoke etc)
• In (reading from input, p/invoke etc)

Changes to the existing drivers now would need to be ported again to the new locations where appropriate - assuming my endeavours are successful.

What would help me most would be if you could review and consider merging #3791 as it does what this branch did (allow user to send ANSI escape sequences and pick up responses). That would cut down the diff on v2 drivers considerably and make it easier to see the dircetion it is going while still delivering useful functionality:

• Ability to send ANSI codes
• Ability to detect sixel support

[BDisp]

Please give me the opportunity to try to restore in a new branch all the fixes I made without the ANSI escape sequence, because I fixed other things and bugs that will be useful for merging into v2_develop. I think it won't be too difficult to merge it later. If I can't restore them in two days, I will say so and we will definitely forget about it. Accordingly?