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

Clear Buffer is broken in Windows Terminal Preview (1.22.2362.0) #17867

Closed
valiko-ua opened this issue Sep 5, 2024 · 3 comments · Fixed by #17884
Closed

Clear Buffer is broken in Windows Terminal Preview (1.22.2362.0) #17867

valiko-ua opened this issue Sep 5, 2024 · 3 comments · Fixed by #17884
Labels
A11ySev1 Accessibility tracking A11yWCAG Accessibility tracking Area-Accessibility Issues related to accessibility HCL-E+D Accessibility tracking HCL-WindowsTerminal Accessibility tracking In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements

Comments

@valiko-ua
Copy link

valiko-ua commented Sep 5, 2024

Windows Terminal version

1.22.2362.0

Windows build number

10.0.22631.4037

Steps to reproduce

  • Settings -> Actions -> Add New -> Add "Ctrl+l" (lowercase L) for "Clear Buffer". Press Save.
  • Open tab with "Command Prompt".
  • Execute "cd C:\Windows". Execute "dir" several times to fill the screen.
  • Press Ctrl+l (lowercase L).

Expected Behavior

Clear Buffer works fine in Windows Terminal v.1.20.11781.0.

Actual Behavior

Clear Buffer does not work in Windows Terminal Preview v.1.22.2362.0.

The same problem can be reproduced with "Ubuntu 24.04 LTS" tab (WSL2).

I've just noticed that pressing Ctrl+Shift+L works differently from Ctrl+L (but still broken). And I didn't assign Ctrl+Shift+L to anything.

@valiko-ua valiko-ua added Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Sep 5, 2024
@zadjii-msft
Copy link
Member

@lhecker Didn't we fix this during the original #17510? I could have swore that I filed that, but I can't find it now

@elsaco
Copy link

elsaco commented Sep 5, 2024

@valiko-ua it's not the key binding. In 1.22.2362 the Clear buffer doesn't work. Fill the buffer with data and invoke the command via ctrl+shift+p. It won't clear the buffer!

@j4james
Copy link
Collaborator

j4james commented Sep 7, 2024

I think the problem is explained by the comment here:

// Send a signal to conpty to clear the buffer.
if (auto conpty{ _connection.try_as<TerminalConnection::ConptyConnection>() })
{
// ConPTY will emit sequences to sync up our buffer with its new
// contents.
conpty.ClearBuffer();
}

In the past when you asked conpty to clear its buffer, the vtengine would have pushed back an update that also cleared the buffer in the connected terminal, but that's not happening anymore. On the conhost side, the ClearBuffer signal is handled here:

void PtySignalInputThread::_DoClearBuffer() const
{
LockConsole();
auto Unlock = wil::scope_exit([&] { UnlockConsole(); });
// If the client app hasn't yet connected, stash the new size in the launchArgs.
// We'll later use the value in launchArgs to set up the console buffer
// We must be under lock here to ensure that someone else doesn't come in
// and set with `ConnectConsole` while we're looking and modifying this.
if (!_consoleConnected)
{
return;
}
auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
THROW_IF_FAILED(gci.GetActiveOutputBuffer().ClearBuffer());
}

Which just calls the SCREEN_INFORMATION class here:

[[nodiscard]] HRESULT SCREEN_INFORMATION::ClearBuffer()
{
// Rotate the buffer to bring the cursor row to the top of the viewport.
const auto cursorPos = _textBuffer->GetCursor().GetPosition();
for (auto i = 0; i < cursorPos.y; i++)
{
_textBuffer->IncrementCircularBuffer();
}
// Erase everything below that point.
RETURN_IF_FAILED(SetCursorPosition({ 0, 1 }, false));
auto& engine = reinterpret_cast<OutputStateMachineEngine&>(_stateMachine->Engine());
engine.Dispatch().EraseInDisplay(DispatchTypes::EraseType::ToEnd);
// Restore the original cursor x offset, but now on the first row.
RETURN_IF_FAILED(SetCursorPosition({ cursorPos.x, 0 }, false));
_textBuffer->TriggerRedrawAll();
return S_OK;
}

That's just going to clear the conhost buffer. As far as I can see, there's nothing in that code that will emit sequences to sync it up with the terminal buffer.

@github-project-automation github-project-automation bot moved this to To Cherry Pick in 1.22 Servicing Pipeline Sep 9, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot added the In-PR This issue has a related PR label Sep 9, 2024
@Priyanka-Chauhan123 Priyanka-Chauhan123 added Area-Accessibility Issues related to accessibility HCL-E+D Accessibility tracking A11yWCAG Accessibility tracking HCL-WindowsTerminal Accessibility tracking A11ySev1 Accessibility tracking labels Sep 10, 2024
@carlos-zamora carlos-zamora added this to the Terminal v1.23 milestone Sep 11, 2024
@carlos-zamora carlos-zamora removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Sep 11, 2024
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Tag-Fix Doesn't match tag requirements label Sep 11, 2024
@DHowett DHowett moved this from To Cherry Pick to Cherry Picked in 1.22 Servicing Pipeline Sep 24, 2024
DHowett pushed a commit that referenced this issue Sep 24, 2024
Without a VT "renderer" there's no implicit output anymore when
calling `ClearPseudoConsole`. The fix is trivial, but it works
slightly different from before: Previously, we would preserve
the line the cursor is on, while this PR doesn't do that.
I felt like there's not much merit in preserving the line,
because it may be a multi-line prompt which won't work with that.

Closes #17867

## Validation Steps Performed
Bind 3 different actions to the 3 variants of "Clear buffer"
and test them. They work. ✅

(cherry picked from commit 4259ce5)
Service-Card-Id: PVTI_lADOAF3p4s4AmhmQzgS3NbU
Service-Version: 1.22
@DHowett DHowett moved this from Cherry Picked to Shipped in 1.22 Servicing Pipeline Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A11ySev1 Accessibility tracking A11yWCAG Accessibility tracking Area-Accessibility Issues related to accessibility HCL-E+D Accessibility tracking HCL-WindowsTerminal Accessibility tracking In-PR This issue has a related PR Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Tag-Fix Doesn't match tag requirements
Projects
Development

Successfully merging a pull request may close this issue.

6 participants