ConPTY does not pass Device Control Strings (DCS) to 3rd-party terminals #21757

@alokedesai commented on GitHub (May 30, 2024):

@lhecker Really excited to see this happen, if Warp engineers can help out with this in anyway please let us know.

This seems like a massive rearchitecture of ConPTY and we would be able to contribute headcount if it would accelerate the process of launching this.

@alokedesai commented on GitHub (May 30, 2024): @lhecker Really excited to see this happen, if Warp engineers can help out with this in anyway please let us know. This seems like a massive rearchitecture of ConPTY and we would be able to contribute headcount if it would accelerate the process of launching this.

claunia commented

@lhecker commented on GitHub (May 31, 2024):

Is that correct?

As a tl;dr, absolutely!

This seems like a massive rearchitecture of ConPTY and we would be able to contribute headcount if it would accelerate the process of launching this.

Thanks! I'll definitely keep this in mind. 🙂 For now, however, it needs to be approved by the team and probably also run by our division architects, because all 4 steps in the spec together are a huge change that will likely take >1 year to complete.

If you're massively blocked by this issue, and if you have dev time to spare, you could consider hotfixing this issue yourself. I believe what you need is something like this:
baba406a07/src/terminal/adapter/adaptDispatch.cpp (L3762-L3767)

If that's correct, then you then need to call out to _dispatch here:
baba406a07/src/terminal/parser/OutputStateMachineEngine.cpp (L717-L756)

and do the same TriggerFlush. I suspect that this will fix the issue.

@lhecker commented on GitHub (May 31, 2024): > Is that correct? As a tl;dr, absolutely! > This seems like a massive rearchitecture of ConPTY and we would be able to contribute headcount if it would accelerate the process of launching this. Thanks! I'll definitely keep this in mind. 🙂 For now, however, it needs to be approved by the team and probably also run by our division architects, because all 4 steps in the spec together are a _huge_ change that will likely take >1 year to complete. If you're massively blocked by this issue, and if you have dev time to spare, you could consider hotfixing this issue yourself. I believe what you need is something like this: https://github.com/microsoft/terminal/blob/baba406a074e792dd4486f4239ac4b3292f95a60/src/terminal/adapter/adaptDispatch.cpp#L3762-L3767 If that's correct, then you then need to call out to `_dispatch` here: https://github.com/microsoft/terminal/blob/baba406a074e792dd4486f4239ac4b3292f95a60/src/terminal/parser/OutputStateMachineEngine.cpp#L717-L756 and do the same `TriggerFlush`. I suspect that this will fix the issue.

claunia commented

@alokedesai commented on GitHub (May 31, 2024):

Great thank you, we'll try that out! Will that suggested hotfix also solve the ordering problem? (My assumption is no but just confirming).

And to clarify, is sending DCS to the terminal + fixing the ordering problems still something you expect to take 1 month? I assume productionizing the server and shipping it in windows are what would cause this to take 1 year+?

@alokedesai commented on GitHub (May 31, 2024): Great thank you, we'll try that out! Will that suggested hotfix also solve the ordering problem? (My assumption is no but just confirming). And to clarify, is sending DCS to the terminal + fixing the ordering problems still something you expect to take 1 month? I assume productionizing the server and shipping it in windows are what would cause this to take 1 year+?

claunia commented

@j4james commented on GitHub (May 31, 2024):

Just FYI, I made a conscious decision not to pass through unrecognised DCS sequences, precisely because they typically won't work, in the same way that unrecognised OSC sequences don't work. All we'll be doing is introducing another set of bug reports for third party terminals, who'll now think they have support for DCS sequences, but which will inevitably fail in unpredictable ways.

If you follow other terminal bug trackers, you can see the kind of problems this causes. And app devs are affected by the issue as well, because instead of a particular sequence simply not being supported on Windows, it appears to work, but has subtle bugs. And they waste a whole lot of time trying to understand and work around those bugs, when it's really not something they can fix.

So if you want DCS support, and want it to actually work, I'd suggest you try and get OSC sequences to work reliably first (assuming you think that's feasible). Otherwise I think we'll just be making things worse for everyone by introduced broken DCS sequences into the mix.

Another option to consider would be adding a pass-through wrapper sequence like some multiplexers support (e.g. see the tmux FAQ). After all, the current implementation of conpty is essentially a kind of multiplexer.

But really I think the best bet is to wait for lhecker's new architecture. If it turns out that isn't going to work, we can always revisit other options later.

@j4james commented on GitHub (May 31, 2024): Just FYI, I made a conscious decision _not_ to pass through unrecognised `DCS` sequences, precisely because they typically won't work, in the same way that unrecognised `OSC` sequences don't work. All we'll be doing is introducing another set of bug reports for third party terminals, who'll now think they have support for `DCS` sequences, but which will inevitably fail in unpredictable ways. If you follow other terminal bug trackers, you can see the kind of problems this causes. And app devs are affected by the issue as well, because instead of a particular sequence simply not being supported on Windows, it _appears_ to work, but has subtle bugs. And they waste a whole lot of time trying to understand and work around those bugs, when it's really not something they can fix. So if you want `DCS` support, and want it to actually work, I'd suggest you try and get `OSC` sequences to work reliably first (assuming you think that's feasible). Otherwise I think we'll just be making things worse for everyone by introduced broken `DCS` sequences into the mix. Another option to consider would be adding a pass-through wrapper sequence like some multiplexers support (e.g. see the [tmux FAQ](https://github.com/tmux/tmux/wiki/FAQ#what-is-the-passthrough-escape-sequence-and-how-do-i-use-it)). After all, the current implementation of conpty is essentially a kind of multiplexer. But really I think the best bet is to wait for lhecker's new architecture. If it turns out that isn't going to work, we can always revisit other options later.

claunia commented

@abhishekp106 commented on GitHub (May 31, 2024):

@lhecker What's the simplest way to build your prototype from dev/lhecker/goodbye-vtengine? I'm assuming that all I need is to successfully build contpy.dll and OpenConsole.exe.

When I run the PowerShell function Invoke-OpenConsoleBuild from Windows Terminal I get a lot of errors, many of them related to tests. I assume that this is because it's trying to build every project that is present. (I imagine I don't need to refactor the test cases to get it to work).

I am able to successfully get the conpty.dll file built by using Visual Studio and only checking some of the Projects that are have conpty in the name. How can I get OpenConsole.exe as well? What is the minimum set of projects I need to include to get what I need? I tried checking off all non-test related projects but I get the following errors:

Happy to fix some of the smaller errors like type conversions or an incorrect number of arguments, but I figured I should check on what projects I actually need.

Here's the Visual Studio Build > Manage Configuration modal I'm modifying:

@abhishekp106 commented on GitHub (May 31, 2024): @lhecker What's the simplest way to build your prototype from [dev/lhecker/goodbye-vtengine](https://github.com/microsoft/terminal/tree/dev/lhecker/goodbye-vtengine)? I'm assuming that all I need is to successfully build `contpy.dll` and `OpenConsole.exe`. When I run the PowerShell function `Invoke-OpenConsoleBuild` from Windows Terminal I get a lot of errors, many of them related to tests. I assume that this is because it's trying to build every project that is present. (I imagine I don't need to refactor the test cases to get it to work). I am able to successfully get the `conpty.dll` file built by using Visual Studio and only checking some of the Projects that are have conpty in the name. How can I get `OpenConsole.exe` as well? What is the minimum set of projects I need to include to get what I need? I tried checking off all non-test related projects but I get the following errors: ![image](https://github.com/microsoft/terminal/assets/40919306/95f09eec-6cf6-4c39-b461-178d7ef5fc0e) Happy to fix some of the smaller errors like type conversions or an incorrect number of arguments, but I figured I should check on what projects I actually need. Here's the Visual Studio `Build > Manage Configuration` modal I'm modifying: ![image](https://github.com/microsoft/terminal/assets/40919306/a83cf738-e026-4c49-865c-80a8d515e7fe)

claunia commented

@alokedesai commented on GitHub (May 31, 2024):

@j4james By "getting OSCs to work reliably" do you mean ensuring they are sent to the terminal in an ordered way?

If so, very much agreed. We tried OSCs when we realized DCSs were swallowed by ConPTY and it didn't work because of the ordering issues. Regardless of which sequence we use, the biggest requirement we have is that we must be able to communicate from shell --> terminal in a synchronous way.

@alokedesai commented on GitHub (May 31, 2024): @j4james By "getting `OSCs` to work reliably" do you mean ensuring they are sent to the terminal in an ordered way? If so, very much agreed. We tried `OSC`s when we realized `DCS`s were swallowed by ConPTY and it didn't work because of the ordering issues. Regardless of which sequence we use, the biggest requirement we have is that we must be able to communicate from shell --> terminal in a synchronous way.

claunia commented

https://github.com/microsoft/terminal/assets/2256941/6d204afe-fbaf-493c-8d28-b70bc0802f88

@lhecker commented on GitHub (May 31, 2024):

What's the simplest way to build your prototype from dev/lhecker/goodbye-vtengine?

It's a different branch but I did a quick screen recording for how I do it:

I tried checking off all non-test related projects but I get the following errors:

When it comes to OpenConsole and conpty.dll: I just pushed a commit that fixes the build error. Instead of return !inPtyMode; it should've just said return true;.

@lhecker commented on GitHub (May 31, 2024): > What's the simplest way to build your prototype from dev/lhecker/goodbye-vtengine? It's a different branch but I did a quick screen recording for how I do it: https://github.com/microsoft/terminal/assets/2256941/6d204afe-fbaf-493c-8d28-b70bc0802f88 > I tried checking off all non-test related projects but I get the following errors: When it comes to OpenConsole and conpty.dll: I just pushed a commit that fixes the build error. Instead of `return !inPtyMode;` it should've just said `return true;`.

claunia commented

@j4james commented on GitHub (May 31, 2024):

By "getting OSCs to work reliably" do you mean ensuring they are sent to the terminal in an ordered way?

@alokedesai That's one issue. Another problem is that the position of the cursor when you receive the sequence won't necessarily be the position of the cursor when it was sent, so you can't depend on that. And if you change the cursor position yourself in response to a passed-through sequence, that has the potential to misposition any subsequent output. Or if you produce any output yourself in response to a passed-through sequence, that will potentially be overwritten by the conpty renderer at a later stage.

In short, there are very few scenarios that can reliably work with a passed-through sequence if conhost isn't specifically designed to handle that sequence. As a rule of thumb, if you can't make it work in tmux, it's unlikely to work through conpty.

@j4james commented on GitHub (May 31, 2024): > By "getting OSCs to work reliably" do you mean ensuring they are sent to the terminal in an ordered way? @alokedesai That's one issue. Another problem is that the position of the cursor when you receive the sequence won't necessarily be the position of the cursor when it was sent, so you can't depend on that. And if you change the cursor position yourself in response to a passed-through sequence, that has the potential to misposition any subsequent output. Or if you produce any output yourself in response to a passed-through sequence, that will potentially be overwritten by the conpty renderer at a later stage. In short, there are very few scenarios that can reliably work with a passed-through sequence if conhost isn't specifically designed to handle that sequence. As a rule of thumb, if you can't make it work in tmux, it's unlikely to work through conpty.

claunia commented

@alokedesai commented on GitHub (Jun 7, 2024):

@j4james @lhecker Thanks for the additional info.

Warp blocks work by sending a custom DCS from shell --> terminal when a command starts and finishes executing (see a blog post about this here).

Whenever a command finishes, we create a new grid so that contents from each command are fully isolated from each other.

Given that background, I don't think a fork of ConPTY that adds support for a custom OSC (or DCS) would fix the issues we're seeing. This is because ConPTY would have to actually update its own internal data model to reflect the fact that Warp is going to create a new grid, which I think is impossible to do perfectly (but I may be overestimating the complexity of this).

Assuming my understanding is correct (I'd love confirmation of that / whether there's an obvious solution I'm missing) I believe we have to wait for changes outlined in step 1 of that design doc to land. Is ~1 month still accurate?

Happy to Zoom about this if easier, I believe we chatted with some of your coworkers in the past (@DHowett and Carlos Zamora).

@alokedesai commented on GitHub (Jun 7, 2024): @j4james @lhecker Thanks for the additional info. Warp blocks work by sending a custom DCS from shell --> terminal when a command starts and finishes executing (see a blog post about this [here](https://www.warp.dev/blog/how-warp-works)). Whenever a command finishes, we create a _new_ grid so that contents from each command are fully isolated from each other. Given that background, I don't think a fork of ConPTY that adds support for a custom OSC (or DCS) would fix the issues we're seeing. This is because ConPTY would have to actually update its own internal data model to reflect the fact that Warp is going to create a new grid, which I think is impossible to do perfectly (but I may be overestimating the complexity of this). Assuming my understanding is correct (I'd love confirmation of that / whether there's an obvious solution I'm missing) I believe we _have_ to wait for changes outlined in step 1 of that design doc to land. Is ~1 month still accurate? Happy to Zoom about this if easier, I believe we chatted with some of your coworkers in the past (@DHowett and Carlos Zamora).

claunia commented

@acarl005 commented on GitHub (Jun 7, 2024):

And if you change the cursor position yourself in response to a passed-through sequence, that has the potential to misposition any subsequent output

Yeah, by starting a new "block" we're basically starting a new grid, which sets the cursor position back to (0, 0). This puts us out of sync with the console output buffer unfortunately.

@acarl005 commented on GitHub (Jun 7, 2024): > And if you change the cursor position yourself in response to a passed-through sequence, that has the potential to misposition any subsequent output Yeah, by starting a new "block" we're basically starting a new grid, which sets the cursor position back to (0, 0). This puts us out of sync with the console output buffer unfortunately.

claunia commented

@alokedesai commented on GitHub (Jun 12, 2024):

Hi @lhecker, following up here one more time. I noticed that the original design spec you posted no longer exists.

Is this something you're still working on?

@alokedesai commented on GitHub (Jun 12, 2024): Hi @lhecker, following up here one more time. I noticed that the original [design spec](https://github.com/microsoft/terminal/blob/dev/lhecker/goodbye-vtengine/doc/specs/%2313000%20-%20In-process%20ConPTY.md) you posted no longer exists. Is this something you're still working on?

claunia commented

@DHowett commented on GitHub (Jun 13, 2024):

(Replied via e-mail!)

@DHowett commented on GitHub (Jun 13, 2024): (Replied via e-mail!)

claunia commented

@zadjii-msft commented on GitHub (Jun 13, 2024):

(for everyone not on the mail thread (me too), the spec is in #17387)

@zadjii-msft commented on GitHub (Jun 13, 2024): (for everyone not on the mail thread (me too), the spec is in #17387)

claunia commented

@lhecker commented on GitHub (Jun 13, 2024):

Yep, exactly, the spec moved over to a PR now, while I continue to implement the new ConPTY. I'm currently working on a new "cooked read" implementation (= Windows' readline-like implementation for scanf and friends). The last remaining API I need to implement is SetConsoleActiveScreenBuffer, and afterwards I need to fix any remaining bugs. The good news is, we're definitely on track. But it's also not an easy change, so I'll unfortunately be largely unreachable for a while longer while I focus on completing this work.

@lhecker commented on GitHub (Jun 13, 2024): Yep, exactly, the spec moved over to a PR now, while I continue to implement the new ConPTY. I'm currently working on a new "cooked read" implementation (= Windows' readline-like implementation for `scanf` and friends). The last remaining API I need to implement is `SetConsoleActiveScreenBuffer`, and afterwards I need to fix any remaining bugs. The good news is, we're definitely on track. But it's also not an easy change, so I'll unfortunately be largely unreachable for a while longer while I focus on completing this work.

claunia commented

@alokedesai commented on GitHub (Jul 10, 2024):

Hi @lhecker, thanks for all of your hard work on this. Really exciting to see some of the PRs go by.

I'm trying to better understand the end-state after this work is complete: under what circumstances will ConHost rely on the output buffer and try to read-back what's changed to send VT sequences to the terminal?

I originally was under the impression that this change would completely remove the need for ConHost buffer, but I realize that's not possible because it it would break console functions that require reading the buffer (such as GetConsoleCursorInfo ).

This has an impact on Warp because our data model isn't a simple grid, so I'm trying to think through if the new architecture would actually fully solve our requirements.

@alokedesai commented on GitHub (Jul 10, 2024): Hi @lhecker, thanks for all of your hard work on this. Really exciting to see some of the PRs go by. I'm trying to better understand the end-state after this work is complete: under what circumstances will ConHost rely on the output buffer and try to read-back what's changed to send VT sequences to the terminal? I originally was under the impression that this change would completely remove the need for ConHost buffer, but I realize that's not possible because it it would break console functions that require reading the buffer (such as `GetConsoleCursorInfo `). This has an impact on Warp because our data model isn't a simple grid, so I'm trying to think through if the new architecture would actually fully solve our requirements.

claunia commented

@lhecker commented on GitHub (Jul 10, 2024):

ConPTY, the way it works right now (where it emits a pure stream of VT), will continue to exist forever, because applications like sshd don't really have a buffer to read from either.

The new architecture proposed in the spec is a low-level layer on which ConPTY is built on top of. If you give it the ability to read back various info, it gives you a practically zero overhead access to the Windows console API and complete freedom to determine what constitutes a grapheme cluster, etc.

In other words, you really don't need to use it, unless you want to or otherwise have a specific need for it.

Although I'm somewhat surprised you mentioned GetConsoleCursorInfo, since presumably, you should know whether the current cursor is visible or not, right? (The CONSOLE_CURSOR_INFO::dwSize can be hardcoded to 25.)

The 2nd hardest thing in the low-level API is likely implementing the read-back functionality for the buffer contents. But I think it's feasible to implement for most, because the "grid" for reading is conceptually identical to the grid for writing VT. I know that Warp divides output into "Blocks" but I'm assuming it still has support for CUP sequences and the like, so it has support for a VT viewport and its grid too, right? Since reading from the buffer uses the same grid, I think it should generally be possible to implement with your buffer architecture as well.

The hardest API is likely the fact that the console API allows the client to create arbitrarily many off-screen text buffers whose size differs from the window size (CreateConsoleScreenBuffer). The client can switch between the different buffers (SetConsoleActiveScreenBuffer) and ideally the window size would change depending on the active buffer. But even here, I think it's possible to be somewhat "loose" with the implementation of this API, and if you disallow clients to change the window size via SetConsoleActiveScreenBuffer it probably still works with most applications.
(For instance, Windows console applications sometimes use such off-screen buffers to calculate the size of Unicode text, because ambiguous width glyphs are... well... ambiguous. 😅)

All that said, and as I said above, you really don't need to use it if you have no need for it.

@lhecker commented on GitHub (Jul 10, 2024): ConPTY, the way it works right now (where it emits a pure stream of VT), will continue to exist forever, because applications like sshd don't really have a buffer to read from either. The new architecture proposed in the spec is a low-level layer on which ConPTY is built on top of. If you give it the ability to read back various info, it gives you a practically zero overhead access to the Windows console API and complete freedom to determine what constitutes a grapheme cluster, etc. In other words, you really don't need to use it, unless you want to or otherwise have a specific need for it. Although I'm somewhat surprised you mentioned `GetConsoleCursorInfo`, since presumably, you should know whether the current cursor is visible or not, right? (The `CONSOLE_CURSOR_INFO::dwSize` can be hardcoded to 25.) The 2nd hardest thing in the low-level API is likely implementing the read-back functionality for the buffer contents. But I think it's feasible to implement for most, because the "grid" for reading is conceptually identical to the grid for writing VT. I know that Warp divides output into "Blocks" but I'm assuming it still has support for CUP sequences and the like, so it has support for a VT viewport and its grid too, right? Since reading from the buffer uses the same grid, I think it should generally be possible to implement with your buffer architecture as well. The hardest API is likely the fact that the console API allows the client to create arbitrarily many off-screen text buffers whose size differs from the window size (`CreateConsoleScreenBuffer`). The client can switch between the different buffers (`SetConsoleActiveScreenBuffer`) and ideally the window size would change depending on the active buffer. But even here, I think it's possible to be somewhat "loose" with the implementation of this API, and if you disallow clients to change the window size via `SetConsoleActiveScreenBuffer` it probably still works with most applications. (For instance, Windows console applications sometimes use such off-screen buffers to calculate the size of Unicode text, because ambiguous width glyphs are... well... ambiguous. 😅) All that said, and as I said above, you really don't need to use it if you have no need for it.

claunia commented

@lhecker commented on GitHub (Jul 15, 2024):

The related PR is now more or less done, although I expect it to receive smaller fixes in the future. If you'd like to experiment with it, you can download our Canary build here: https://github.com/microsoft/terminal/discussions/16121

It currently includes my PR as a test. Theoretically it should be possible for you to replace your current OpenConsole.exe with the one from the Canary build and it should just work (you can find OpenConsole.exe inside the 3 portable ZIPs). I recommend using overlapped IO for optimal performance. If you find any bugs or issues, please let me know!

@lhecker commented on GitHub (Jul 15, 2024): The related PR is now more or less done, although I expect it to receive smaller fixes in the future. If you'd like to experiment with it, you can download our Canary build here: https://github.com/microsoft/terminal/discussions/16121 It currently includes my PR as a test. Theoretically it should be possible for you to replace your current OpenConsole.exe with the one from the Canary build and it should just work (you can find OpenConsole.exe inside the 3 portable ZIPs). I recommend using overlapped IO for optimal performance. If you find any bugs or issues, please let me know!

claunia commented

@alokedesai commented on GitHub (Aug 7, 2024):

Apologies for the delay in response here. Thanks for the detailed response; we'll test this out locally and file any issues if we see them!

@alokedesai commented on GitHub (Aug 7, 2024): Apologies for the delay in response here. Thanks for the detailed response; we'll test this out locally and file any issues if we see them!

claunia commented