Enable VT processing by default for conpty #2824
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
zadjii-msft
added
Product-Conpty
DHowett-MSFT
approved these changes
Sep 20, 2019
There was a problem hiding this comment.
@Tyriar: do you know of any VSCode users who would become upset about this?
|
@DHowett-MSFT I thought this is how it always worked? Not sure how to repro within vscode:
|
carlos-zamora
approved these changes
Sep 20, 2019
miniksa
approved these changes
Sep 20, 2019
Co-Authored-By: Carlos Zamora <carlos.zamora@microsoft.com>
|
Merged Carlos's comment. I'll squash this to master with the approvals on Mike's behalf since he's out today (presuming the CI completes). |
|
🎉 Handy links: |
ghost
mentioned this pull request
DHowett-MSFT
pushed a commit
that referenced
this pull request
Sep 26, 2019
This change enables VT processing by default for _all_ conpty clients. See #1965 for a discussion on why we believe this is a righteous change. Also mentioned in the issue was the idea of only checking the `VirtualTerminalLevel` reg key in the conpty startup. I don't think this would be a more difficult change, looks like all we'd need is a simple `reg.LoadGlobalsFromRegistry();` call instead of this change. # Validation Steps Performed Manually launched a scratch app in both the terminal and the console. The console launch's output mode was 0x3, and the terminal's was 0x7. 0x4 is the ` ENABLE_VIRTUAL_TERMINAL_PROCESSING` flag, which the client now had by default in the Terminal. Closes #1965 (cherry picked from commit 1c412d4)
DHowett-MSFT
pushed a commit
that referenced
this pull request
Sep 26, 2019
- Remember last-used string in the Find dialog in conhost (GH-2845) (cherry picked from commit bfb1484) - Bugfix: TextBuffer Line Wrapping from VTRenderer (GH-2797) Change VT renderer to do erase line instead of a ton of erase chars (cherry picked from commit 8afc5b2) - Add some retry support to Renderer::PaintFrame (GH-2830) If _PaintFrameForEngine returns E_PENDING, we'll give it another two tries to get itself straight. If it continues to fail, we'll take down the application. We observed that the DX renderer was failing to present the swap chain and failfast'ing when it did so; however, there are some errors from which DXGI guidance suggests we try to recover. We'll now return E_PENDING (and destroy our device resources) when we hit those errors. Fixes GH-2265. (cherry picked from commit 277acc3) - Enable VT processing by default for ConPTY (GH-2824) This change enables VT processing by default for _all_ conpty clients. See GH-1965 for a discussion on why we believe this is a righteous change. Also mentioned in the issue was the idea of only checking the `VirtualTerminalLevel` reg key in the conpty startup. I don't think this would be a more difficult change, looks like all we'd need is a simple `reg.LoadGlobalsFromRegistry();` call instead of this change. **Validation Steps Performed** Manually launched a scratch app in both the terminal and the console. The console launch's output mode was 0x3, and the terminal's was 0x7. 0x4 is the ` ENABLE_VIRTUAL_TERMINAL_PROCESSING` flag, which the client now had by default in the Terminal. Closes GH-1965 (cherry picked from commit 1c412d4) - Remove unwanted DECSTBM clipping (GH-2764) The `DECSTBM` margins are meant to define the range of lines within which certain vertical scrolling operations take place. However, we were applying these margin restrictions in the `ScrollRegion` function, which is also used in a number of places that shouldn't be affected by `DECSTBM`. This includes the `ICH` and `DCH` escape sequences (which are only affected by the horizontal margins, which we don't yet support), the `ScrollConsoleScreenBuffer` API (which is public Console API, not meant to be affected by the VT terminal emulation), and the `CSI 3 J` erase scrollback extension (which isn't really scrolling as such, but uses the `ScrollRegion` function to manipulate the scrollback buffer). This commit moves the margin clipping out of the `ScrollRegion` function, so it can be applied exclusively in the places that need it. While testing, I also noticed that one of the `ScrollRegion` calls in the `AdjustCursorPosition` function was not setting the horizontal range correctly - the scrolling should always affect the full buffer width rather than just the viewport width - so ... Related work items: #23507749, #23563809, #23563819, #23563837, #23563841, #23563844
|
Hey, would it be a big problem to add additional granularity to enabling VT processing? I have an idea for a patch #2946 , but making it secure by making OSC 52 processing opt-in seems like it would need an additional level of granularity. Note: This particular example does not run afoul of the double-VT-processing objection: On the other hand, maybe I'm misunderstanding VT-processing vs VT-rendering. I tried looking through the source code, and it was unclear to me which thing means which. |
Closed
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary of the Pull Request
This change enables VT processing by default for all conpty clients. See #1965 for a discussion on why we believe this is a righteous change.
PR Checklist
Detailed Description of the Pull Request / Additional comments
Also mentioned in the issue was the idea of only checking the
VirtualTerminalLevelreg key in the conpty startup. I don't think this would be a more difficult change, looks like all we'd need is a simplereg.LoadGlobalsFromRegistry();call instead of this change.Validation Steps Performed
Manually launched a scratch app in both the terminal and the console. The console launch's output mode was 0x3, and the terminal's was 0x7. 0x4 is the
ENABLE_VIRTUAL_TERMINAL_PROCESSINGflag, which the client now had by default in the Terminal.