Bugfix: TextBuffer Line Wrapping from VTRenderer #2797
Conversation
Change VT renderer to do erase line instead of a ton of erase chars
|
@zadjii-msft, I wrote this with Carlos on his computer this afternoon. I wasn't sure if there is ever a circumstance where the number of characters to be erased is somehow less than the right edge of the viewport. I think it could theoretically be possible if only a part of the window (not touching the right edge) was invalidated and the painting routines only handed the subsegment of the buffer text into the renderer as a part of drawing. But I'm not certain. I'm also not certain why you wouldn't have used ^[[K to begin with here to erase the line instead of the less efficient ^[[X unless you had a reason. It looks like you spent quite a bit of time here on trying to make this more efficient with some of the flags detecting whether we should even bother doing ECH. This is really waiting on your input to proceed. |
|
So you can definitely have text where we need to ECH instead of just EL. Consider: drawn to the console. There are 16 spaces after "Foo", but there's more text later. This kind of thing can happen with tmux or emacs, where they're drawing panes. Here, we want to ECH to only erase the 16 spaces, but not the rest of the line. If the application were to update the frame to the following: then EL would corrupt the right pane with "Bar" in it. That all being said, this seems like it's safe. We're still handling the case where we're not erasing all the way to the end of the line. This seems like it might have just been a missed optimization tbh. A missed optimization that just so happened to also break wrapping in the Terminal. Here's another question - should the ECH handler in the Terminal cause the line to be marked as wrapping? This change is good on it's own, but maybe we need another PR to handle that too. What happens when conhost sees |
I initially didn't want to change ECH handler because it is using the ->Write method which has implicit newlines when it runs off the end. So the solution there would have been to either change it to using ->WriteLine (with wrap potentially off, figuring it out from the outside.... YUCK.) or to wait until ElectricBoogaloo is done and have some sort of choice with ->WriteStream (which is probably what ECH wanted anyway.) |
|
I guess, unless.... if you treat all ECHs as rectangular write commands then maybe they clear the wrap status whether or not they draw all the way up to the right most cell? That's probably worse. And one more thought I had is that we could explicitly send a \r\n out the VT when it knows it is going to wrap down. That might force a break and unwrap but also require more ElectricBoogaloo work. |
For the record, this is something I meant to investigate in #405. I played with it in like RS5, but I couldn't get it right, maybe because of this nonsense. It's something that would certainly be more correct, but we'll need to play with it. Emitting a |
|
🎉 Handy links: |
ghost
mentioned this pull request
* Bugfix: line selection copy * Revert clipboard change Change VT renderer to do erase line instead of a ton of erase chars * revert TerminalApi change (cherry picked from commit 8afc5b2)
- 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
Summary of the Pull Request
Making the VT Renderer erase the entire line instead of erasing characters until it reaches the end of the line. Should be more efficient and also set wrapping properly.
PR Checklist
Detailed Description of the Pull Request / Additional comments
Honestly, not too sure about this fix. More just want it to be here so that @miniksa and @zadjii-msft can discuss.
Validation Steps Performed
If this is right, I should add a test...I'll need some help with that too though.