Skip to content

Preserve the cursor row during Clear Buffer #18976

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.

Sign up for GitHub

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Preserve the cursor row during Clear Buffer #18976

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f582ec0
Preserve the cursor row during Clear Buffer
lhecker May 28, 2025
8f6ebb9
Merge remote-tracking branch 'origin/main' into dev/lhecker/18732-cle…
lhecker Jun 17, 2025
267d075
Fix comparisons, Use DL instead of SU
lhecker Jun 17, 2025
57866f3
Fix the cursor position after a clear
lhecker Jun 17, 2025
82f2bfc
Fix AuditMode, unit tests
lhecker Jun 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/cascadia/TerminalConnection/ConptyConnection.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -556,13 +556,13 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
}
}

void ConptyConnection::ClearBuffer()
void ConptyConnection::ClearBuffer(bool keepCursorRow)
{
// If we haven't connected yet, then we really don't need to do
// anything. The connection should already start clear!
if (_isConnected())
{
THROW_IF_FAILED(ConptyClearPseudoConsole(_hPC.get()));
THROW_IF_FAILED(ConptyClearPseudoConsole(_hPC.get(), keepCursorRow));
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalConnection/ConptyConnection.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ namespace winrt::Microsoft::Terminal::TerminalConnection::implementation
void Resize(uint32_t rows, uint32_t columns);
void ResetSize();
void Close() noexcept;
void ClearBuffer();
void ClearBuffer(bool keepCursorRow);

void ShowHide(const bool show);

Expand Down
2 changes: 1 addition & 1 deletion src/cascadia/TerminalConnection/ConptyConnection.idl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ namespace Microsoft.Terminal.TerminalConnection
UInt16 ShowWindow { get; };

void ResetSize();
void ClearBuffer();
void ClearBuffer(Boolean keepCursorRow);

void ShowHide(Boolean show);

Expand Down
47 changes: 30 additions & 17 deletions src/cascadia/TerminalControl/ControlCore.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2259,23 +2259,37 @@ namespace winrt::Microsoft::Terminal::Control::implementation
// - <none>
void ControlCore::ClearBuffer(Control::ClearBufferType clearType)
{
std::wstring_view command;
switch (clearType)
{
case ClearBufferType::Screen:
command = L"\x1b[H\x1b[2J";
break;
case ClearBufferType::Scrollback:
command = L"\x1b[3J";
break;
case ClearBufferType::All:
command = L"\x1b[H\x1b[2J\x1b[3J";
break;
}

{
const auto lock = _terminal->LockForWriting();
_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.
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

const auto cursor = _terminal->GetViewportRelativeCursorPosition();

// GH#18732: Users want the row the cursor is on to be preserved across clears.
std::wstring sequence;

if (clearType == ClearBufferType::Scrollback || clearType == ClearBufferType::All)
{
sequence.append(L"\x1b[3J");
}

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

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

{
fmt::format_to(std::back_inserter(sequence), FMT_COMPILE(L"\x1b[{};1H\x1b[J"), cursor.y + 2);
}

// Erase any viewport contents above (but not including) the cursor row.
if (cursor.x > 0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

As above, should be cursor.y.

{
fmt::format_to(std::back_inserter(sequence), FMT_COMPILE(L"\x1b[{}S\x1b[1;{}H"), cursor.y, cursor.x + 1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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. ☹

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that means SU is fine?

Just on this point, note that our SU implementation is neither DEC compatible, nor Xterm compatible, and at some point I'd like for us to fix that. So if we use it here in a way that depends on our current implementation, it's almost certain to break in the future, and that's the kind of regression you could easily miss.

}
}

_terminal->Write(sequence);
}

if (clearType == Control::ClearBufferType::Screen || clearType == Control::ClearBufferType::All)
Expand All @@ -2284,8 +2298,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation
{
// Since the clearing of ConPTY occurs asynchronously, this call can result weird issues,
// where a console application still sees contents that we've already deleted, etc.
// The correct way would be for ConPTY to emit the appropriate CSI n J sequences.
conpty.ClearBuffer();
conpty.ClearBuffer(true);
}
}
}
Expand Down
17 changes: 13 additions & 4 deletions src/host/PtySignalInputThread.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,13 @@ try
}
case PtySignal::ClearBuffer:
{
_DoClearBuffer();
ClearBufferData msg = { 0 };
if (!_GetData(&msg, sizeof(msg)))
{
return S_OK;
}

_DoClearBuffer(msg.keepCursorRow != 0);
break;
}
case PtySignal::ResizeWindow:
Expand Down Expand Up @@ -180,7 +186,7 @@ void PtySignalInputThread::_DoResizeWindow(const ResizeWindowData& data)
_api.ResizeWindow(data.sx, data.sy);
}

void PtySignalInputThread::_DoClearBuffer() const
void PtySignalInputThread::_DoClearBuffer(const bool keepCursorRow) const
{
LockConsole();
auto Unlock = wil::scope_exit([&] { UnlockConsole(); });
Expand All @@ -196,8 +202,11 @@ void PtySignalInputThread::_DoClearBuffer() const

auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
auto& screenInfo = gci.GetActiveOutputBuffer();
auto& stateMachine = screenInfo.GetStateMachine();
stateMachine.ProcessString(L"\x1b[H\x1b[2J");
auto& tb = screenInfo.GetTextBuffer();
const auto cursor = tb.GetCursor().GetPosition();

tb.ClearScrollback(cursor.y, keepCursorRow ? 1 : 0);
tb.GetCursor().SetPosition({ keepCursorRow ? cursor.x : 0, 0 });
}

void PtySignalInputThread::_DoShowHide(const ShowHideData& data)
Expand Down
7 changes: 6 additions & 1 deletion src/host/PtySignalInputThread.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,11 @@ namespace Microsoft::Console
unsigned short show; // used as a bool, but passed as a ushort
};

struct ClearBufferData
{
unsigned short keepCursorRow;
};

struct SetParentData
{
uint64_t handle;
Expand All @@ -64,7 +69,7 @@ namespace Microsoft::Console
[[nodiscard]] bool _GetData(_Out_writes_bytes_(cbBuffer) void* const pBuffer, const DWORD cbBuffer);
void _DoResizeWindow(const ResizeWindowData& data);
void _DoSetWindowParent(const SetParentData& data);
void _DoClearBuffer() const;
void _DoClearBuffer(bool keepCursorRow) const;
void _DoShowHide(const ShowHideData& data);
void _Shutdown();

Expand Down
2 changes: 1 addition & 1 deletion src/inc/conpty-static.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ CONPTY_EXPORT HRESULT WINAPI ConptyCreatePseudoConsole(COORD size, HANDLE hInput
CONPTY_EXPORT HRESULT WINAPI ConptyCreatePseudoConsoleAsUser(HANDLE hToken, COORD size, HANDLE hInput, HANDLE hOutput, DWORD dwFlags, HPCON* phPC);

CONPTY_EXPORT HRESULT WINAPI ConptyResizePseudoConsole(HPCON hPC, COORD size);
CONPTY_EXPORT HRESULT WINAPI ConptyClearPseudoConsole(HPCON hPC);
CONPTY_EXPORT HRESULT WINAPI ConptyClearPseudoConsole(HPCON hPC, BOOL keepCursorRow);
CONPTY_EXPORT HRESULT WINAPI ConptyShowHidePseudoConsole(HPCON hPC, bool show);
CONPTY_EXPORT HRESULT WINAPI ConptyReparentPseudoConsole(HPCON hPC, HWND newParent);
CONPTY_EXPORT HRESULT WINAPI ConptyReleasePseudoConsole(HPCON hPC);
Expand Down
9 changes: 5 additions & 4 deletions src/winconpty/winconpty.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,15 +278,16 @@ HRESULT _ResizePseudoConsole(_In_ const PseudoConsole* const pPty, _In_ const CO
// Return Value:
// - S_OK if the call succeeded, else an appropriate HRESULT for failing to
// write the clear message to the pty.
HRESULT _ClearPseudoConsole(_In_ const PseudoConsole* const pPty)
HRESULT _ClearPseudoConsole(_In_ const PseudoConsole* const pPty, BOOL keepCursorRow)
{
if (pPty == nullptr)
{
return E_INVALIDARG;
}

unsigned short signalPacket[1];
unsigned short signalPacket[2];
signalPacket[0] = PTY_SIGNAL_CLEAR_WINDOW;
signalPacket[1] = keepCursorRow ? 1 : 0;

const auto fSuccess = WriteFile(pPty->hSignal, signalPacket, sizeof(signalPacket), nullptr, nullptr);
return fSuccess ? S_OK : HRESULT_FROM_WIN32(GetLastError());
Expand Down Expand Up @@ -492,13 +493,13 @@ extern "C" HRESULT WINAPI ConptyResizePseudoConsole(_In_ HPCON hPC, _In_ COORD s
// - This is used exclusively by ConPTY to support GH#1193, GH#1882. This allows
// a terminal to clear the contents of the ConPTY buffer, which is important
// if the user would like to be able to clear the terminal-side buffer.
extern "C" HRESULT WINAPI ConptyClearPseudoConsole(_In_ HPCON hPC)
extern "C" HRESULT WINAPI ConptyClearPseudoConsole(_In_ HPCON hPC, BOOL keepCursorRow)
{
const PseudoConsole* const pPty = (PseudoConsole*)hPC;
auto hr = pPty == nullptr ? E_INVALIDARG : S_OK;
if (SUCCEEDED(hr))
{
hr = _ClearPseudoConsole(pPty);
hr = _ClearPseudoConsole(pPty, keepCursorRow);
}
return hr;
}
Expand Down
Loading