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

A minor ConPTY refactoring: Goodbye VtEngine Edition #17510

Merged
merged 69 commits into from
Aug 1, 2024

Conversation

lhecker
Copy link
Member

@lhecker lhecker commented Jul 3, 2024

The idea is that we can translate Console API calls directly to VT at
least as well as the current VtEngine setup can. For instance, a call
to SetConsoleCursorPosition clearly translates directly to a CUP
escape sequence. Effectively, instead of translating output
asynchronously in the renderer thread, we'll do it synchronously
right during the Console API call.

Most importantly, the this means that any VT output that an
application generates will now be given to the terminal unmodified.

Aside from reducing our project's complexity quite a bit and opening
the path towards various interesting work like sixels, Device Control
Strings, buffer snapshotting, synchronized updates, and more, it also
improves performance for mixed text output like enwik8.txt in conhost
to 1.3-2x and in Windows Terminal via ConPTY to roughly 20x.

This adds support for overlapped IO, because now that output cannot
be "skipped" anymore (VtEngine worked like a renderer after all)
it's become crucial to block conhost as little as possible.

⚠️ Intentionally unresolved changes/quirks:

  • To force a delayed EOL wrap to wrap, WriteCharsLegacy emits a
    \r\n if necessary. This breaks text reflow on window resize.
    We cannot emit \r the way readline does it, because this would
    overwrite the first column in the next row with a whitespace.
    The alternative is to read back the affected cell from the buffer
    and emit that character and its attributes followed by a \r.
    I chose to not do that, because buffer read-back is lossy (= UCS2).
    Unless the window is resized, the difference is unnoticeable
    and historically, conhost had no support for buffer reflow anyway.
  • If ENABLE_VIRTUAL_TERMINAL_PROCESSING is set while
    DISABLE_NEWLINE_AUTO_RETURN is reset, we'll blindly replace all
    LF with CRLF. This may hypothetically break DCS sequences, but it's
    the only way to do this without parsing the given VT string and
    thus the only way we can achieve passthrough mode in the future.
  • ENABLE_WRAP_AT_EOL_OUTPUT is translated to DECAWM.
    Between Windows XP and Windows 11 21H2, ENABLE_WRAP_AT_EOL_OUTPUT
    being reset would cause the cursor position to reset to wherever
    a write started, if the write, including expanded control chars,
    was less than 100 characters long. If it was longer than that,
    the cursor position would end up in an effectively random position.
    After lengthy research I believe that this is a bug introduced in
    Windows XP and that the original intention was for this mode to be
    equivalent to DECAWM. This is compounded by MSDN's description
    (emphasis mine):

    If this mode is disabled, the last character in the row is
    overwritten with any subsequent characters.

⚠️ Unresolved issues/quirks:

  • Focus/Unfocus events are injected into the output stream without
    checking whether the VT output is currently in a ground state.
    This may break whatever VT sequence is currently ongoing.
    This is an existing issue.
  • VtIo::Writer::WriteInfos should properly verify the width of
    each individual character.
  • Using SetConsoleActiveScreenBuffer destroys surrogate pairs
    and extended (VT) attributes. It could be translated to VT pages
    in the long term.
  • Similarly, ScrollConsoleScreenBuffer results in the same and
    could be translated to DECCRA and DECFRA in the near term.
    This is important because otherwise vim output may loose
    its extended attributes during scrolling.
  • Reflowing a long line until it wraps results in the cooked read
    prompt to be misaligned vertically.
  • SCREEN_INFORMATION::s_RemoveScreenBuffer should trigger a
    buffer switch similar to SetConsoleActiveScreenBuffer.
  • Translation of COMMON_LVB_GRID_HORIZONTAL to SGR 53 was dropped
    and may be reintroduced alongside UNDERSCORE = SGR 4.
  • Move the OSC 0 ; P t BEL sequence to WriteWindowTitle
    and swap the BEL with the ST (ESC \).
  • PowerShell on Windows 10 ships with PSReadLine 2.0.0-beta2
    which emits SGR 37/40 instead of 39/49. This results in black
    spaces when typing and there's no good way to fix that.
  • A test is missing that ensures that FillConsoleOutputCharacterW
    results in a CSI n J during the PowerShell shim.
  • A test is missing that ensures that PtySignal::ClearBuffer
    does not result in any VT being generated.

Closes #262
Closes #1173
Closes #3016
Closes #4129
Closes #5228
Closes #8698
Closes #12336
Closes #15014
Closes #15888
Closes #16461
Closes #16911
Closes #17151
Closes #17313

@@ -1,20 +1,15 @@
AAAAA
AAAAAAAAAAAAA
Copy link
Member Author

@lhecker lhecker Jul 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of the files in this PR will either be deletions or minor modifications. I recommend just ticking them off as "viewed" first. Afterwards it'll still be a large PR, but it'll be more "manageable", I think.

src/host/_output.cpp Outdated Show resolved Hide resolved
src/host/VtIo.cpp Outdated Show resolved Hide resolved
@DHowett

This comment was marked as resolved.

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

notes inline

@DHowett DHowett requested a review from j4james July 3, 2024 18:09
@DHowett

This comment was marked as resolved.

@DHowett

This comment was marked as resolved.

@j4james
Copy link
Collaborator

j4james commented Jul 4, 2024

Two big issues I've noticed so far (just testing in WSL on this branch by itself).

  • printf "\e[1;1;1,~" will kill the app.
  • printf "\e[6n"; read will generate two CPR reports (same thing happens with any query sequence).

The underlying problem in both cases is that we're letting the sequences be processed by conhost in addition to passing them through, and I think maybe in some cases we shouldn't be doing that, i.e. we need to retain some IsConsolePty tests in AdaptDispatch to suppress certain operations or at least handle them differently.

Edit: Actually it's probably easier to put these checks in ConhostInternalGetSet rather than AdaptDispatch, at least for these two cases.

@lhecker

This comment was marked as resolved.

This comment has been minimized.

// just don't catch other types of exceptions. They'll take out
// TAEF, which will count as a failure.

VERIFY_FAIL(L"This should have thrown");
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(FYI the old code is crashing VS 17.10 preview 2, so I changed this a bit.)

Copy link
Member Author

@lhecker lhecker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went through the PR and ticked off all files that are either wholly deleted or trivialities and... look y'all........ It's only 40 modified files:
image

No biggie. 👍 /s

@@ -26,7 +26,7 @@ namespace til // Terminal Implementation Library. Also: "Today I Learned"
// state structure for maintenance of UTF-8 partials
struct u8state
{
char partials[4];
char partials[4]{};
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(The compiler was complaining otherwise.)

@j4james
Copy link
Collaborator

j4james commented Jul 4, 2024

It seems like query responses aren't working at all now. With this test case: printf "\e[6n"; read I see nothing until I press a key.

It looks like that broke in commit 78ae6dd. Prior to that I got the double response immediately. After that I only got the one response immediately, and had to press a key to see the second response. Then once the double-response was fixed, I got nothing until I pressed a key.

DHowett pushed a commit that referenced this pull request Aug 5, 2024
This PR adds support for querying the cursor style - technically the
state of the `DECSCUSR` setting - using a `DECRQSS` escape sequence.

## References and Relevant Issues

The initial `DECRQSS` support was added in PR #11152, but it wasn't
practical to report the cursor style until conpty passthrough was added
in PR #17510.

## Detailed Description of the Pull Request / Additional comments

If the user has chosen a cursor style that isn't one of the shapes
supported by the `DECSCUSR` control, we report those as 0 (i.e. the
default style). That way, if an application later tries to restore the
cursor using the returned value, it should still be reset to its
original state.

I also took the opportunity in this PR to do some refactoring of the
other `DECRQSS` reports, since several of them were using unnecessary
appending that could be simplified to a single `fmt::format` call, or
even just static strings in some cases.

## Validation Steps Performed

I've checked the reports are working as expected in Vttest, and also
added some unit tests.

## PR Checklist
- [x] Tests added/passed
lhecker added a commit that referenced this pull request Aug 6, 2024
Roughly 4 years ago we gave Windows Terminal the ability to
differentiate between black/white and the default colors.
One of the victims was PowerShell and most importantly PSReadLine,
which emit SRG 37 & 40 when what they really want is 38 & 48.
We fixed this on our side by adding a shim.

Since the addition of VT passthrough in #17510 we now intentionally
lost the ability to translate VT sequences from one thing to another.
This meant we also lost the ability to do this shim and as such
this PR removes it. Luckily Windows 11 now ships PSReadLine 2.0.0,
which contains a proper fix for this.

Unfortunately, this is not the case for Windows 10, which ships
PSReadLine 2.0.0-beta2. Users affected by this will have to install
a newer version of PSReadLine or use the default black/white theme.

See 1bf4c08

Closes #13037
lhecker added a commit that referenced this pull request Aug 7, 2024
* Added/changed comments as mentioned.
* Improved the ugly `resize_and_overwrite` hack into the STL.
* Add `Write` functions for xterm's window API.
* The only reason we needed a move operator for `VtIo::Writer`
  is because we used it in a ternary in `CONSOLE_INFORMATION`.
  Ternaries are like if branches with hidden move assignments.
  Instead, we simply construct each `Writer` in place.
  No ternary = No move = No problems in life.
  The best benefit of this is that this makes calling `GetVtWriter`
  a hundred times cheaper.

Otherwise, I still need to extend a few tests in `VtIoTests`,
but I'm planning to do that later.
DHowett pushed a commit that referenced this pull request Aug 13, 2024
This PR introduces the framework for the `DECRQTSR` sequence which is
used to query terminal state reports. But for now I've just implemented
the `DECCTR` color table report, which provides a way for applications
to query the terminal's color scheme.

## References and Relevant Issues

This is the counterpart to the the `DECRSTS` sequence, which is used to
restore a color table report. That was implemented in PR #13139, but it
only became practical to report the color table once conpty passthrough
was added in PR #17510.

## Detailed Description of the Pull Request / Additional comments

This sequence has the option of reporting the colors as either HLS or
RGB, but in both cases the resolution is lower than 24 bits, so the
colors won't necessarily round-trip exactly when saving and restoring.
The HLS model in particular can accumulate rounding errors over time.

## Validation Steps Performed

I've added a basic unit test that confirms the colors are reported as
expected for both color models. The color values in these tests were
obtained from color reports on a real VT525 terminal.

## PR Checklist
- [x] Tests added/passed
DHowett pushed a commit that referenced this pull request Aug 22, 2024
#17510 made it so that VT requests like DA1 are passed through to the
hosting terminal and so conhost stopped responding to them on its own.
But since our input parser doesn't support proper passthrough (yet),
it swallowed the response coming from the terminal.

To solve this issue, this PR repurposes the existing boolean return
values to indicate to the parser whether the current sequence should
be flushed to the dispatcher as-is. The output parser always returns
true (success) and leaves its pass-through handler empty, while the
input parser returns false for sequences it doesn't expect.

## Validation Steps Performed
* Launch cmd
* Press `Ctrl+[`, `[`, `c`, `Enter` (= `^[[c` = DA1 request)
* DA1 response is visible ✅
DHowett added a commit that referenced this pull request Aug 23, 2024
Thanks to a string of compiler bugs, we had to use an older container
image that shipped with VS 17.9.

Unfortunately, that container image is falling further and further out
of date. The build agents don't cache it any longer, so they spend 30-45
minutes of every build pulling it from the registry.

With the changes to ConPTY in #17510 removing the need for til::bitmap,
we no longer need to work around the compiler bugs it exposed.

Furthermore, 17.10.6+ has a much more robust and presumably "working"
compiler.

(cherry picked from commit 39108a7)
Service-Card-Id: PVTI_lADOAF3p4s4AmhmszgSI7RU
Service-Version: 1.21
lhecker pushed a commit that referenced this pull request Aug 26, 2024
## Summary of the Pull Request

When a VT title sequence sets the title to a blank string, that is meant
to trigger a reset to the default starting value. This used to work in
the past because the blank value was dealt with by conhost, so Windows
Terminal never received a blank title, but that's no longer the case
with the new VT passthrough. This PR fixes the issue by getting Windows
Terminal to handle the blank title strings itself. 

## References and Relevant Issues

VT passthrough was introduced in PR #17510.

## Validation Steps Performed

I've manually verified that the `OSC 0`, `OSC 2`, and `DECSWT` sequences
now correctly reset the title when passed a blank title string. 

## PR Checklist
- [x] Closes #17800
DHowett pushed a commit that referenced this pull request Sep 4, 2024
## Summary of the Pull Request

When a VT title sequence sets the title to a blank string, that is meant
to trigger a reset to the default starting value. This used to work in
the past because the blank value was dealt with by conhost, so Windows
Terminal never received a blank title, but that's no longer the case
with the new VT passthrough. This PR fixes the issue by getting Windows
Terminal to handle the blank title strings itself.

## References and Relevant Issues

VT passthrough was introduced in PR #17510.

## Validation Steps Performed

I've manually verified that the `OSC 0`, `OSC 2`, and `DECSWT` sequences
now correctly reset the title when passed a blank title string.

## PR Checklist
- [x] Closes #17800

(cherry picked from commit 1f71568)
Service-Card-Id: PVTI_lADOAF3p4s4AmhmQzgSNvr0 PVTI_lADOAF3p4s4AmhmQzgSOFrc
Service-Version: 1.22
@vladimirshefer
Copy link

vladimirshefer commented Sep 27, 2024

@lhecker hi! do you know when this will be released?

@lhecker
Copy link
Member Author

lhecker commented Sep 27, 2024

It is available in Windows Terminal Preview 1.22 which can be found on our release page (among others). Is that what you meant or are you asking when it will be available in the stable version?

@vladimirshefer
Copy link

Yes, stable release.
Btw, Thank for this link.

@lhecker
Copy link
Member Author

lhecker commented Sep 30, 2024

The stable version always trails the preview version by 1 release. As such, we'll release 1.22 as stable in about 2-3 months.

PKRoma pushed a commit to PKRoma/Terminal that referenced this pull request Nov 4, 2024
Split off from microsoft#17510:
* `HandleWantsOverlappedIo` can be used to check if a handle requires
  overlapped IO. This is important, as `ReadFile` and `WriteFile` are
  documented to not work correctly if an overlapped handle is used
  without overlapped IO and vice versa.
  In my tests with pipes, this appears to be true.
* `CreatePipe` creates a synchronous, unidirectional pipe.
* `CreateOverlappedPipe` does what it says on the tin, while allowing
  you to specify the direction of the pipe (in, out, duplex).
* `GetOverlappedResultSameThread` is largely the same as
  `GetOverlappedResult`, but adds back a neat optimization from
  the time before Windows 7. I thought it was neat.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Interop Communication between processes Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Area-Performance Performance-related issue Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Area-Server Down in the muck of API call servicing, interprocess communication, eventing, etc. Area-VT Virtual Terminal sequence support Impact-Correctness It be wrong. Issue-Bug It either shouldn't be doing this or needs an investigation. Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Issue-Task It's a feature request, but it doesn't really need a major design. Priority-1 A description (P1) Priority-2 A description (P2) Priority-3 A description (P3) Product-Conhost For issues in the Console codebase Product-Conpty For console issues specifically related to conpty
Projects
None yet
8 participants