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

Support manually clearing the conpty buffer #1193

Closed
zadjii-msft opened this issue Jun 10, 2019 · 16 comments · Fixed by #10906
Closed

Support manually clearing the conpty buffer #1193

zadjii-msft opened this issue Jun 10, 2019 · 16 comments · Fixed by #10906
Labels
Area-Input Related to input processing (key presses, mouse, etc.) Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Priority-1 A description (P1) Product-Conpty For console issues specifically related to conpty Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.

Comments

@zadjii-msft
Copy link
Member

From @Tyriar in microsoft/vscode#75141

This is happening because Terminal: Clear tells the frontend (the thing rendering everything) to clear, but the way that the conpty backend works is it will sometimes reprint what it thinks should be visible. Since it doesn't know about our clear it reprints the unwanted output, this doesn't happen on a regular linux/mac backend that conpty is trying to emulate.

Is there anything that can be done here to tell conpty to not reprint the screen? I suspect python.exe does not enter alt mode so I'm not sure why a reprint is necessary.

@zadjii-msft zadjii-msft added Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Conpty For console issues specifically related to conpty Area-Input Related to input processing (key presses, mouse, etc.) labels Jun 10, 2019
@zadjii-msft zadjii-msft added this to Spec Needed ❓ in Specification Tracker via automation Jun 10, 2019
@ghost ghost added the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Jun 10, 2019
@DHowett-MSFT
Copy link
Contributor

Thoughts: new thing to put on the signal pipe? We don't want to introduce too many non-standard VT things to the in/out pipes, so the signal pipe is our "get out of jail free" card.

@DHowett-MSFT DHowett-MSFT removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Jun 10, 2019
@Tyriar
Copy link
Member

Tyriar commented Jun 10, 2019

I don't think this is a good idea for that reason.

@DHowett-MSFT
Copy link
Contributor

Sorry, I think you'll need to be more specific about what this and that reason are.

@Tyriar
Copy link
Member

Tyriar commented Jun 10, 2019

We don't want to introduce too many non-standard VT things to the in/out pipes

@DHowett-MSFT
Copy link
Contributor

So, okay, if I understand this correctly:
You're asking for us to cut this feature request, because you do not believe that we should add support for it; you do, however, still want it to exist?

@Tyriar
Copy link
Member

Tyriar commented Jun 11, 2019

I think there's a bug as @zadjii-msft indicated in the linked issue that I want fixed instead of adding non-standard stuff to conpty. Setting a variable and printing it in Python should not redraw the entire screen, see microsoft/vscode#75141 (comment) for context.

@zadjii-msft
Copy link
Member Author

Well, I think the bug might be making this issue more apparent, but I don't think fixing the bug mentioned will actually properly resolve this issue. It might temporarily hide the issue, by allowing the cursor to start printing text again at the origin. However, the text wouldn't actually be at the origin in conpty's buffer, and it's entirely possible that another action could cause conpty to need to redraw everything - at which point, all the old text would re-appear, and the new text would suddenly shift to where conpty thinks it is.

For *nix, VSCode's terminal clear works just fine because the pty is just a dumb pipe - when you clear the frontend's buffer, there's nothing on the backend that still thinks that there's any text in the buffer, because there is no buffer.

However, with conpty, because there is an actual buffer on the backend, we can't just clear the frontend like that. The conpty buffer will still think something's there. Since this is a problem unique to conpty & windows, I think it's unfortunately something that needs a unique solution.
I'm half way on board with adding another signal to the signal pipe - that'll clearly work unique to Windows, since it'd introduce another API to our API surface. If we want to avoid adding another signal here, we could maybe add the clear by making conpty resond to something like ^[[3J on the input. However, I know that doing the resize on the input pipe also had some problems with making sure the resize sequence wouldn't be broken up by other input sequences, esp for WSL and docker. Hence the addition of the signal pipe. I think the signal would end up being the safest, most robust solution.

I really want to make sure we avoid API creep on the conpty API though. We need to make sure that this is something that's definitely not accomplish-able by some other means.

@Tyriar
Copy link
Member

Tyriar commented Jun 11, 2019

@zadjii-msft I get that there will be times when it fails just as it will on nix (eg. running clear in the alt buffer) and I'm fine with the Terminal Clear command not working properly in those cases. Fixing the conpty-specific cases doesn't seem worth it since it can happen off with a native pty too.

Isn't conpty meant to be pretending to be a dumb pipe here in the same way as a native pty? I tried doing that same action to run the current Python file on a mac and it will just print a bunch of characters, CR, LF and use the character attribute escape (CSI m). In a similar way it would be lovely if conpty did not reprint the screen when resized unless something actually changed, ie. a line in the viewport is wrapped or will be wrapped.

@DHowett-MSFT
Copy link
Contributor

Isn't conpty meant to be pretending to be a dumb pipe here

ConPTY is pretending to be as close to a dumb pipe as it can. That's the major contention here: there is a 35-year history of win32 console applications that have access to the output buffer. We cannot be a dumb pipe, because that's not something that fits into the "dumb pipe" model.

Any concessions we've made in what ConPTY can do and does do are in the pursuit of the broadest possible compatibility.

@maxhora
Copy link

maxhora commented Oct 1, 2019

Why not to support 2 modes of ConPTY: plain "dumb pipe" and ConPTY with legacy support for 35-year history of win32 console? :)

@zadjii-msft
Copy link
Member Author

@Maxris you might be interested in #1173

@yurenchen000
Copy link

yurenchen000 commented Nov 11, 2020

image

it seems that:
clear or cls will clear scroll buffer in PowerShell.
cls will clear scroll buffer in CMD.

but clear OR reset OR CSI sequence not clear scroll buffer in bash.

can we have some workaround to implement buffer clear ?
some thing like,
associate shortcut keys (or something else) to call windows clear or cls to current "tty". (Bypass the current running bash.exe , ssh.exe..),

@DHowett
Copy link
Member

DHowett commented Nov 11, 2020

@yurenchen000 Are you using Cygwin, MSYS, or MinGW bash? If so, what version of the runtime are you using? Certain versions of the Cygwin runtime delete CSIs.

Clearing the buffer works fine from bash, so long as the CSI is not deleted.

@yurenchen000
Copy link

yurenchen000 commented Nov 11, 2020

@yurenchen000 Are you using Cygwin, MSYS, or MinGW bash? If so, what version of the runtime are you using? Certain versions of the Cygwin runtime delete CSIs.

Clearing the buffer works fine from bash, so long as the CSI is not deleted.

thanks a lot.

seems printf "\e[H\e[2J\e[3J" will clear scroll buffer, in some situations (not all).

I use lots of cli environment:

  • GNU bash, version 4.4.23(1)-release //from git-bash (msys // not clear scroll buff
  • GNU bash, version 4.4.23(2)-release (x86_64-pc-msys) //from MSYS2, YES this clear worked ( found it send "\e[H\e[2J\e[3J")
  • adb shell
  • ssh to ubuntu18

can I ignore the CLI environment,
and have a terminal itself's GUI option (mouse right menu or shortcut key) to clear scroll buffer?

@zadjii-msft zadjii-msft removed this from the Windows vNext milestone May 11, 2021
@zadjii-msft zadjii-msft added this to the Terminal Backlog milestone May 11, 2021
zadjii-msft added a commit that referenced this issue Jul 29, 2021
  Unfortunately, I need to reset the cursor position, and I actually just need
  to do this differently entirely.

  iTerm actually maintains the last line of the buffer entirely. That's kind of
  important! Otherwise the prompt just disappears too.

  They're actually even smarter than that:
  * https://gitlab.com/gnachman/iterm2/-/issues/1330
  * https://gitlab.com/gnachman/iterm2/-/issues/4393

  and know where the prompt starts and ends, and keep all of multi-line prompts.
  That's a very 2023 feature, but we should keep at least one line.
@zadjii-msft
Copy link
Member Author

zadjii-msft commented Jul 29, 2021

Alright, worked on this a bit in dev/migrie/fhl-2021/clear-buffer-action. Small bits of trickiness:

  • We need to preserve the cursor line. SCREEN_INFORMATION::VtEraseAll is not the thing we'd want to use to do this - that pretty blindly just scrolls away the old buffer contents into scrollback. We could:
    1. Just scroll everything above the cursor out, and leave everything below it. This would be weird for something like vim.
    2. Copy the contents of the cursor row into a temp buffer, scroll everything, then restore that line to the top of the viewport. This would also be weird for something like vim.

I mean, this almost certainly will never do the right thing for a full screen app so we may as well do i. because it's easier now.

  • I should revert the changes I made to AdaptDispatch and DispatchCommon. These changes won't be necessary anymore, so better to remove them, and reduce the likelyhood that someone else uses them wrong.
  • I should add a new DispatchCommon::s_ClearBuffer method, that PtySignalInputThread::_DoClearBuffer calls.
  • That should get plumbed through to something just like SCREEN_INFORMATION::VtEraseAll, but with clearing everything above the cursor, and leaving the cursor on the top row of the viewport.
  • Maybe just ignore doing this always when in the alt buffer? Would reduce cases where vim et al. will definitely do the wrong thing.

@ghost ghost added the In-PR This issue has a related PR label Aug 9, 2021
@ghost ghost closed this as completed in #10906 Sep 2, 2021
@ghost ghost removed the In-PR This issue has a related PR label Sep 2, 2021
ghost pushed a commit that referenced this issue Sep 2, 2021
…buffer (#10906)

## Summary of the Pull Request

![clear-buffer-000](https://user-images.githubusercontent.com/18356694/127570078-90c6089e-0430-4dfc-bcd4-a0cde20c9167.gif)

This adds a new action, `clearBuffer`. It accepts 3 values for the `clear` type:
* `"clear": "screen"`: Clear the terminal viewport content. Leaves the scrollback untouched. Moves the cursor row to the top of the viewport (unmodified).
* `"clear": "scrollback"`: Clear the scrollback. Leaves the viewport untouched.
* `"clear": "all"`: (**default**) Clear the scrollback and the visible viewport. Moves the cursor row to the top of the viewport (unmodified).

"Clear Buffer" has also been added to `defaults.json`.

## References
* From microsoft/vscode#75141 originally

## PR Checklist
* [x] Closes #1193
* [x] Closes #1882
* [x] I work here
* [x] Tests added/passed
* [ ] Requires documentation to be updated

## Detailed Description of the Pull Request / Additional comments

This is a bit tricky, because we need to plumb it all the way through conpty to clear the buffer. If we don't, then conpty will immediately just redraw the screen. So this sends a signal to the attached conpty, and then waits for conpty to draw the updated, cleared, screen back to us.

## Validation Steps Performed
* works for each of the three clear types as expected
* tests pass.
* works even with `ping -t 8.8.8.8` as you'd hope.
@ghost ghost added the Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. label Sep 2, 2021
@ghost
Copy link

ghost commented Oct 20, 2021

🎉This issue was addressed in #10906, which has now been successfully released as Windows Terminal Preview v1.12.2922.0.:tada:

Handy links:

@zadjii-msft zadjii-msft moved this from Spec Needed ❓ to Work completed (without spec) in Specification Tracker Nov 22, 2021
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Input Related to input processing (key presses, mouse, etc.) Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Priority-1 A description (P1) Product-Conpty For console issues specifically related to conpty Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
Specification Tracker
  
Work completed (without spec)
Development

Successfully merging a pull request may close this issue.

6 participants