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

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

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

Conversation

BDisp
Copy link
Collaborator

@BDisp BDisp commented Sep 30, 2024

Fixes

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
Copy link
Collaborator

tznind commented Sep 30, 2024

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
Copy link
Collaborator Author

BDisp commented Sep 30, 2024

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);

@BDisp BDisp marked this pull request as draft October 2, 2024 14:16
@BDisp BDisp marked this pull request as ready for review October 2, 2024 16:16
tznind added a commit to tznind/gui.cs that referenced this pull request Oct 5, 2024
This work by @BDisp enables us to detect sixel support on demand
@tznind
Copy link
Collaborator

tznind commented Oct 5, 2024

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.

image

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

@tznind
Copy link
Collaborator

tznind commented Oct 5, 2024

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
Copy link
Collaborator Author

BDisp commented Oct 5, 2024

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.

image

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
Copy link
Collaborator

tznind commented Oct 5, 2024

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
Copy link
Collaborator

tznind commented Oct 5, 2024

@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
Copy link
Collaborator Author

BDisp commented Oct 5, 2024

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

@tznind
Copy link
Collaborator

tznind commented Oct 5, 2024

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
Copy link
Collaborator Author

BDisp commented Oct 5, 2024

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
Copy link
Collaborator

tznind commented Oct 7, 2024

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
Copy link
Collaborator Author

BDisp commented Oct 7, 2024

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.

image

@BDisp
Copy link
Collaborator Author

BDisp commented Oct 7, 2024

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.

image

@BDisp
Copy link
Collaborator Author

BDisp commented Oct 7, 2024

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
Copy link
Collaborator

tznind commented Oct 9, 2024

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)

asciicrash
Open Images crashes after sending DAR

@tznind
Copy link
Collaborator

tznind commented Oct 9, 2024

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
Copy link
Collaborator Author

BDisp commented Oct 9, 2024

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
Copy link
Collaborator

tznind commented Oct 9, 2024

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
Copy link
Collaborator

tznind commented Oct 9, 2024

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 tznind mentioned this pull request Oct 11, 2024
13 tasks
@BDisp BDisp force-pushed the v2_3767_ansi-escape-sequence-all-drivers branch from 9366998 to 803dbef Compare October 13, 2024 12:09
@tznind
Copy link
Collaborator

tznind commented Oct 13, 2024

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

image

@BDisp
Copy link
Collaborator Author

BDisp commented Oct 13, 2024

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

@BDisp
Copy link
Collaborator Author

BDisp commented Nov 2, 2024

If I type while sending the ansi escape requests it drops characters periodically.

Thanks for reporting this. This is due to iterations not handling the keys while it's still processing others. I'll see what I can do. Does this is happen with your PR?

@tznind
Copy link
Collaborator

tznind commented Nov 2, 2024

No, I wrote ansiparser class specifically to hold onto all keys passed to it and return anything that isn't the response back to input stream processing.

It uses the native T type of each driver e.g. ConsoleKey so there is no stop or explicit read seperate from the normal loop in the driver.

Maybe you can take just the ansiparser class into this pr?.

no-loose
fast typing with no character lost ansi-parser branch

@BDisp
Copy link
Collaborator Author

BDisp commented Nov 2, 2024

Maybe you can take just the ansiparser class into this pr?.

Can you please submit a PR to my branch with the ansiparser? Maybe you will handle better adapting the code. Thanks.

@BDisp
Copy link
Collaborator Author

BDisp commented Nov 2, 2024

I'm realize now that the Ctrl+Shift+Home/End/PgUp/PgDown only is captured on key-up on WindowsDriver. I didn't know this was happening.

@BDisp
Copy link
Collaborator Author

BDisp commented Nov 3, 2024

I'm realize now that the Ctrl+Shift+Home/End/PgUp/PgDown only is captured on key-up on WindowsDriver. I didn't know this was happening.

Windows Terminal was using these keys internally and unbounded them solved the issue.

@tznind
Copy link
Collaborator

tznind commented Nov 4, 2024

With this branch I still getting exit (Esc) when trying to get response with the default terminal in Visual Studio. Think we need to pay attention to older terminals here - if we try to send a request and an Esc goes through instead and exits users app then that is a pretty bad bug.

Cases we need to ensure work:

  • older terminals
  • tty / ssh / putty etc
  • screen

instant-exit

@BDisp
Copy link
Collaborator Author

BDisp commented Nov 4, 2024

@tznind I would like to reproduce this but this is the below what I get using the VSDebugConsole.exe.

VsDebugConsole_oHJoiNFJZC

@tznind
Copy link
Collaborator

tznind commented Nov 4, 2024

Wow that totally self destructed huh! 😱

Can you try my branch out of interest, also because you have it bomb immediately on enter scenario - while in contrast when i tested using your branch it only bombed on click the DAR

@BDisp
Copy link
Collaborator Author

BDisp commented Nov 4, 2024

@tznind, the missing letters when typing in TextView were not due to missed or unexecuted iterations, but rather due to some wrong decoding of some raw escape sequences. You can try typing again while the requests are running and you will see that there are practically no missing letters on all drivers. If there are, it is still due to some bug, which is sometimes very hard to detect.

@BDisp
Copy link
Collaborator Author

BDisp commented Nov 4, 2024

Wow that totally self destructed huh! 😱

Can you try my branch out of interest, also because you have it bomb immediately on enter scenario - while in contrast when i tested using your branch it only bombed on click the DAR

But this situation happen on only run the UICatalog without run the ansi escape sequence scenario. I also tried with your branch and the same situation is happening. But I think this is normal if we aren't using WT.

@tznind
Copy link
Collaborator

tznind commented Nov 5, 2024

But I think this is normal if we aren't using WT.

Wait as soon as you launch UICatalog the terminal does this (fills with escape codes)?

Which branches does it happen for you on?

  • v1_release
  • v1_develop
  • v2_release
  • v2_develop
  • ansi-parser

@BDisp
Copy link
Collaborator Author

BDisp commented Nov 5, 2024

Yes in all V2 branches that are run without WT.

@BDisp
Copy link
Collaborator Author

BDisp commented Nov 5, 2024

@tznind, I don't what happened but today I tried again to run against the vs console and now it work well, damn. I don't have your issue now, maybe was some bug that I already fixed.

VsDebugConsole_jhatAkyECf

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.

Allowing any driver to request ANSI escape sequence with immediate response.
3 participants