Preserve the cursor row during Clear Buffer #18976

lhecker · 2025-05-28T14:03:57Z

Introduces an ABI change to the ConptyClearPseudoConsole signal.
Otherwise, we have to make it so that the API call always retains
the row the cursor is on, but I feel like that makes it worse.

Closes #18732
Closes #18878

Validation Steps Performed

Launch ConsoleMonitor.exe
Create some text above & below the cursor in PowerShell
Clear Buffer
Buffer is cleared except for the cursor row ✅
...same in ConPTY ✅

carlos-zamora

Thanks for doing this! Love the comments too :)

I was surprised that ClearBufferType isn't a flag enum, but changing that would be more trouble than it's worth tbh.

DHowett · 2025-05-28T17:35:00Z

src/cascadia/TerminalControl/ControlCore.cpp

-            _terminal->Write(command);
+            // In absolute buffer coordinates, including the scrollback (= Y is offset by the scrollback height).
+            const auto viewport = _terminal->GetViewport();
+            // The absolute cursor coordinate.


comment says "absolute" but method says "viewport relative" o_O

Right, I changed the code and forgot to remove the comment.

DHowett · 2025-05-28T18:43:30Z

wow, we actually have tests that this caused to fail!

j4james · 2025-05-30T19:43:28Z

src/cascadia/TerminalControl/ControlCore.cpp

+            if (clearType == ClearBufferType::Screen || clearType == ClearBufferType::All)
+            {
+                // Erase any viewport contents below (but not including) the cursor row.
+                if (cursor.x < viewport.Height())


This should assumedly be cursor.y. Also I think it's off by one, so actually cursor.y + 1.

There's still an off-by-one error here. You can see that by doing a directory listing or something so that the prompt ends up on the last line of the screen, then try clearing the buffer. Note that it hasn't preserved the prompt line.

j4james · 2025-05-30T19:43:52Z

src/cascadia/TerminalControl/ControlCore.cpp

+                }
+
+                // Erase any viewport contents above (but not including) the cursor row.
+                if (cursor.x > 0)


As above, should be cursor.y.

j4james · 2025-05-30T19:51:44Z

src/cascadia/TerminalControl/ControlCore.cpp

+                // Erase any viewport contents above (but not including) the cursor row.
+                if (cursor.x > 0)
+                {
+                    fmt::format_to(std::back_inserter(sequence), FMT_COMPILE(L"\x1b[{}S\x1b[1;{}H"), cursor.y, cursor.x + 1);


This is not super important, but I'd prefer we didn't use an SU sequence here, because it's not standard (in the way modern terminals implement it). We should ideally be using a DL sequence, so something like \x1b[H\x1b[{}M in place of the \x1b[{}S.

Also, and this is important, that final cursor positioning sequence should be applied outside the if, because it's required for both branches. If you're on the top line of the page, you're going to erase below, and not above, but you'll still need to reposition the cursor.

Thank you for your review! Sorry that it took so long. It should be fixed now.

The cursor repositioning still needs to be moved out so it applies to both branches. You can see the problem by clearing the buffer once, so the prompt ends up on the first line of the display, then immediately clear the buffer again. Note that cursor now ends up underneath the prompt.

This is not super important, but I'd prefer we didn't use an SU sequence here, because it's not standard (in the way modern terminals implement it).

Reflecting on this a little bit.

The API between ControlCore and its Terminal member using VT at all is more of an implementation detail. VT emitted here never escapes out of conhost or out of terminal into another application.

In truth, I think that means SU is fine?

But also it means that we're using stringly typed magic between two components statically linked into the same executable image. ☹

I did consider moving the scroll logic to TextBuffer to make this possible, but it's currently fairly tightly ingrained with the VT implementation. IMO we first need to clean up the API between the VT and buffer layers. They're currently too low level (and too low performance). With faster, high-level functions we can probably make everything a lot easier and cleaner. Most notably, a "copy rectangle" function is missing in TextBuffer.

DHowett

James! Excellent catch!

…ar-buffer

DHowett · 2025-06-17T17:38:11Z

Still broke during tests (and now audit! may need to merge main)

DHowett · 2025-06-17T17:38:20Z

oh you merged main. alas

DHowett · 2025-06-17T17:42:52Z

PowerShell and CMD both resume writing input at the old cursor position after we clear the buffer this way:

DHowett · 2025-06-17T17:44:59Z

Clearing the buffer this way after the viewport scrolls down results in a loss of the prompt:

DHowett

noted

lhecker · 2025-06-17T17:48:20Z

Right, forgot about the tests.

PowerShell and CMD both resume writing input at the old cursor position after we clear the buffer this way:

That's already the case before this PR, though.

DHowett · 2025-06-17T17:59:26Z

That's already the case before this PR, though.

Huh. Was it the case before 1.22/the conPTY rewrite?

lhecker · 2025-06-17T22:25:38Z

Huh. Was it the case before 1.22/the conPTY rewrite?

No, I mean just before this PR, on main. It did work in 1.21 because the CMD prompt relied heavily on the conhost TextBuffer. PowerShell 5 is broken in 1.21 too, though.

lhecker · 2025-06-18T15:26:37Z

/azp run

azure-pipelines · 2025-06-18T15:26:48Z

Azure Pipelines successfully started running 1 pipeline(s).

Preserve the cursor row during Clear Buffer

f582ec0

carlos-zamora approved these changes May 28, 2025

View reviewed changes

DHowett reviewed May 28, 2025

View reviewed changes

DHowett approved these changes May 28, 2025

View reviewed changes

DHowett added this to 1.23 Servicing Pipeline and 1.22 Servicing Pipeline May 28, 2025

github-project-automation bot moved this to To Cherry Pick in 1.22 Servicing Pipeline May 28, 2025

github-project-automation bot moved this to To Cherry Pick in 1.23 Servicing Pipeline May 28, 2025

j4james suggested changes May 30, 2025

View reviewed changes

microsoft-github-policy-service bot added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label May 30, 2025

DHowett requested changes May 30, 2025

View reviewed changes

microsoft-github-policy-service bot added the No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. label Jun 6, 2025

microsoft-github-policy-service bot closed this Jun 13, 2025

carlos-zamora removed the No-Recent-Activity This issue/PR is going stale and may be auto-closed without further activity. label Jun 13, 2025

carlos-zamora reopened this Jun 13, 2025

lhecker added 2 commits June 17, 2025 17:34

Merge remote-tracking branch 'origin/main' into dev/lhecker/18732-cle…

8f6ebb9

…ar-buffer

Fix comparisons, Use DL instead of SU

267d075

lhecker requested a review from DHowett June 17, 2025 15:49

microsoft-github-policy-service bot removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jun 17, 2025

DHowett requested changes Jun 17, 2025

View reviewed changes

microsoft-github-policy-service bot added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Jun 17, 2025

microsoft-github-policy-service bot added Needs-Attention The core contributors need to come back around and look at this ASAP. and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Jun 17, 2025

lhecker added 2 commits June 18, 2025 00:33

Fix the cursor position after a clear

57866f3

Fix AuditMode, unit tests

82f2bfc

Preserve the cursor row during Clear Buffer #18976

Are you sure you want to change the base?

Preserve the cursor row during Clear Buffer #18976

Conversation

lhecker commented May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Validation Steps Performed

Uh oh!

carlos-zamora left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DHowett commented May 28, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

DHowett commented Jun 17, 2025

Uh oh!

DHowett commented Jun 17, 2025

Uh oh!

DHowett commented Jun 17, 2025

Uh oh!

DHowett commented Jun 17, 2025

Uh oh!

DHowett left a comment

Choose a reason for hiding this comment

Uh oh!

lhecker commented Jun 17, 2025

Uh oh!

DHowett commented Jun 17, 2025

Uh oh!

lhecker commented Jun 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lhecker commented Jun 18, 2025

Uh oh!

azure-pipelines bot commented Jun 18, 2025

Uh oh!

Uh oh!

lhecker commented May 28, 2025 •

edited

Loading

lhecker commented Jun 17, 2025 •

edited

Loading