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-Host does not clear the previous content (Powershell) #3126

Closed
cdmihai opened this issue Oct 9, 2019 · 12 comments · Fixed by #5627
Closed

Clear-Host does not clear the previous content (Powershell) #3126

cdmihai opened this issue Oct 9, 2019 · 12 comments · Fixed by #5627
Assignees
Labels
Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Area-VT Virtual Terminal sequence support Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) 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.
Milestone

Comments

@cdmihai
Copy link

cdmihai commented Oct 9, 2019

Environment

Windows build number: Microsoft Windows NT 10.0.18362.0
Windows Terminal version (if applicable): 0.5.2762.0

Steps to reproduce

# open Terminal with powershell
# open git repo with more than 100 commits
git log -100 # to fill in the entire screen with text
Clear-Host
# scroll up

Expected behavior

Cannot scroll up, Clear-Host cleared the terminal, all previous output is gone.

Actual behavior

Scrolling up shows previous output. This repros with both Powershel 6.2.3 and 5.2.1.
This does not happen with the powershell terminal, which correctly removes all previous output.

@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Oct 9, 2019
@j4james
Copy link
Collaborator

j4james commented Oct 9, 2019

I think this is somewhat related to #2715. Not necessarily a duplicate, but I suspect that fixing this issue might also fix #2715.

@ezhikov
Copy link

ezhikov commented Oct 15, 2019

Same thing with WSL, where clear does not cleaning scrollback.

@DHowett-MSFT
Copy link
Contributor

Just checked: this will almost certainly be fixed by /dup #2715. If PowerShell is clearing the screen in another way (which it might be doing), we'll have to evaluate then. PowerShell Core, however, can always be updated to take advantage of CSI 3 J

@ghost
Copy link

ghost commented Oct 23, 2019

Hi! We've identified this issue as a duplicate of another one that already exists on this Issue Tracker. This specific instance is being closed in favor of tracking the concern over on the referenced thread. Thanks for your report!

@ghost ghost closed this as completed Oct 23, 2019
@ghost ghost added Resolution-Duplicate There's another issue on the tracker that's pretty much the same thing. and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Oct 23, 2019
@miniksa miniksa reopened this Feb 3, 2020
@miniksa miniksa removed the Resolution-Duplicate There's another issue on the tracker that's pretty much the same thing. label Feb 3, 2020
@ghost ghost added the Needs-Tag-Fix Doesn't match tag requirements label Feb 3, 2020
@miniksa miniksa added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting and removed Needs-Tag-Fix Doesn't match tag requirements labels Feb 3, 2020
@miniksa
Copy link
Member

miniksa commented Feb 3, 2020

We're going to need to look at this again. We're going to pass through CSI 3 J but that's not going to solve folks who are rotating the entire backing buffer with the API (or something else) to clear it out.

We probably need to consider and engage on fixing those.

@DHowett-MSFT DHowett-MSFT added Area-VT Virtual Terminal sequence support Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Conpty For console issues specifically related to conpty Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) labels Feb 7, 2020
@DHowett-MSFT DHowett-MSFT added this to the Terminal v1.0 milestone Feb 7, 2020
@DHowett-MSFT
Copy link
Contributor

I'm pulling the triage tag and putting this in v1.0: this is a complaint we receive so often that we'd be remiss to ship a final product without giving it an investigation.

@DHowett-MSFT DHowett-MSFT added Priority-1 A description (P1) and removed Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Feb 10, 2020
@DHowett-MSFT
Copy link
Contributor

It looks like both CMD and PowerShell...

  1. set the cursor to 0,0
  2. fill the entire console buffer with spaces

PowerShell further..

  1. stomps the attributes on the entire buffer with the (narrowed) buffer attributes

This might(?) be detectable.

@DHowett-MSFT
Copy link
Contributor

Discussion from triage room: we can probably quirk it for cmd and powershell (powershell and pwsh.exe).

@PrzemyslawKlys
Copy link

This issue was very common in VsCode/PowerShell. But they managed to fix it. Here's a PR: PowerShell/vscode-powershell#2316

Maybe it can be easily ported.

@dragonwolf83
Copy link

No, they did a workaround that is specific to that project and VS Code. It can't even be ported to other PowerShell Editor Service Clients. They run a custom console so they changed the clear command to run a VS Code command. The regular PowerShell and CMD consoles do not clear in VS Code.

@zadjii-msft
Copy link
Member

Self notes from what Dustin commented months ago during triage:

PowerShell

Powershell clears the screen with:
https://github.com/iSazonov/PowerShell/blob/8dae4915d3072cbac54a9312cf524ff7a73c977a/src/System.Management.Automation/engine/InitialSessionState.cs#L4063-L4075

            if (Platform.IsWindows)
            {
                // use $RawUI so this works over remoting where there isn't a physical console
                return @"
$RawUI = $Host.UI.RawUI
$RawUI.CursorPosition = @{X=0;Y=0}
$RawUI.SetBufferContents(
    @{Top = -1; Bottom = -1; Right = -1; Left = -1},
    @{Character = ' '; ForegroundColor = $rawui.ForegroundColor; BackgroundColor = $rawui.BackgroundColor})
# .Link
# https://go.microsoft.com/fwlink/?LinkID=225747
# .ExternalHelp System.Management.Automation.dll-help.xml
";
            }

Which then calls to:
https://github.com/PowerShell/PowerShell/blob/b1e998046e12ebe5da9dee479f20d479aa2256d7/src/Microsoft.PowerShell.ConsoleHost/host/msh/ConsoleHostRawUserInterface.cs#L957-L971

            if (region.Left == -1 && region.Right == -1 && region.Top == -1 && region.Bottom == -1)
            {
                if (bufferWidth % 2 == 1 &&
                    ConsoleControl.IsCJKOutputCodePage(out codePage) &&
                    LengthInBufferCells(fill.Character) == 2)
                {
                    throw PSTraceSource.NewArgumentException("fill");
                }

                int cells = bufferWidth * bufferHeight;

                ConsoleControl.FillConsoleOutputCharacter(handle, fill.Character, cells, origin);
                ConsoleControl.FillConsoleOutputAttribute(handle, attribute, cells, origin);
                return;
            }

CMD

CMD on the other hand calls

	ScrollConsoleScreenBuffer( handle, { 0, 0, bufferWidth, bufferHeight }, nullptr,  { 0, -bufferHeight }, {space with the current attributes});
    SetConsoleCursorPosition( handle, {0, 0} );

(more or less)

@ghost ghost added the In-PR This issue has a related PR label Apr 28, 2020
@ghost ghost closed this as completed in #5627 Apr 30, 2020
@ghost ghost removed the In-PR This issue has a related PR label Apr 30, 2020
ghost pushed a commit that referenced this issue Apr 30, 2020
## Summary of the Pull Request

This PR implements a pair of shims for `cmd` and `powershell`, so that their `cls` and `Clear-Host` functions will clear the entire terminal buffer (like they do in conhost), instead of just the viewport. With the conpty viewport and buffer being the same size, there's effectively no way to know if an application is calling these API's in this way with the intention of clearing the buffer or the viewport. We absolutely have to guess. 

Each of these shims checks to see if the way that the API is being called exactly matches the way `cmd` or `powershell` would call these APIs. If it does, we manually write a `^[[3J` to the connected terminal, to get he Terminal to clear it's own scrollback.

~~_⚠️ If another application were trying to clear the **viewport** with an exactly similar API call, this would also cause the terminal scrollback to get cleared ⚠️_~~

* [x] Should these shims be restricted to when the process that's calling them is actually `cmd.exe` or `powershell.exe`? Can I even do this? I think we've done such a good job of isolating the client process information from the rest of the host code that I can't figure out how to do this.
  - YES, this can be done, and I did it.
* [ ] **TODO**: _While I'm here_, should I have `DoSrvPrivateEraseAll` (the implementation for `^[[2J`, in `getset.cpp`) also manually trigger a EraseAll in the terminal in conpty mode?

## PR Checklist
* [x] Closes #3126
* [x] Actually closes #1305 too, which is really the same thing, but probably deserves a callout
* [x] I work here
* [x] Tests added/passed
* [n/a] Requires documentation to be updated

## Validation Steps Performed
* ran tests
* checked `cls` in the Terminal
* checked `Clear-Host` in the Terminal
* Checked running `powershell clear-host` from `cmd.exe`
@ghost ghost added the Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. label Apr 30, 2020
@ghost
Copy link

ghost commented May 5, 2020

🎉This issue was addressed in #5627, which has now been successfully released as Windows Terminal Release Candidate v0.11.1251.0 (1.0rc1).:tada:

Handy links:

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Area-VT Virtual Terminal sequence support Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) 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
None yet
Development

Successfully merging a pull request may close this issue.

9 participants