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

Handle clearTerminal message by using vscode clear command #2316

Merged

Conversation

TylerLeonhardt
Copy link
Member

PR Summary

This fixes an "issue" that comes up a lot that when you Clear-Host and are still able to scroll up.

On non-Windows this is the typical behavior of Clear-Host but on Windows the expectation is that the console is truly cleared out. This sends a message to the client to say "hey clear the terminal" and if the terminal properly supports a "true clear" in can register a handler for this notification.

Pairs with: PowerShell/PowerShellEditorServices#1108

PR Checklist

Note: Tick the boxes below that apply to this pull request by putting an x between the square brackets.
Please mark anything not applicable to this PR NA.

  • PR has a meaningful title
  • Summarized changes
  • PR has tests
  • This PR is ready to merge and is not work in progress
    • If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready

package.json Outdated
@@ -688,6 +688,10 @@
"default": false,
"description": "Falls back to the legacy (lightweight) ReadLine experience. This will disable the use of PSReadLine in the PowerShell Integrated Console."
},
"powershell.integratedConsole.useTrueClear": {
Copy link
Contributor

@rjmholt rjmholt Nov 19, 2019

Choose a reason for hiding this comment

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

How about something like powershell.integrateConsole.clearScrollbackBuffer. Or slightly more futureproof maybe: "powershell.integratedConsole.clearAction": "scrollback" | "clear". Something with the term scrollback in it, just since TrueClear sounds a bit like a background check or a bad liquor to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

LOL

Ok. I kinda like the clearAction one. Let's go with that

Copy link
Member Author

Choose a reason for hiding this comment

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

went with forceClearScrollbackBuffer because we can't change the behavior of clear-host other than forcing a full clear.

ClearTerminalNotificationType,
() => {
// We check to see if they have TrueClear on. If not, no-op because the
// overriden Clear-Host already calls [System.Console]::Clear()
Copy link
Contributor

Choose a reason for hiding this comment

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

If Clear just triggered \x1b[3J it would work everywhere as that clears scrollback.

https://github.com/xtermjs/xterm.js/blob/70babeacb62fe05264d64324ca1f4436997efa1b/src/InputHandler.ts#L908-L918

Copy link
Member Author

Choose a reason for hiding this comment

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

So glad I cc'd you on that tweet - this is so much better

Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't seem to work for me on Windows. Is support for the 3 option new? \x1b[2J works for me (in that it clears the current display) but \x1b[3J appears to be ignored.

Copy link
Contributor

@Tyriar Tyriar Nov 19, 2019

Choose a reason for hiding this comment

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

On Windows the compatibility layer might filter it out or something. I would expect this fix to be in the form of changing how clear acts in conpty to send ED 3 and 2 (i think) to clear the viewport and scrollback when Console.Clear is triggered. Maybe this is that issue? microsoft/terminal#2260

Copy link
Member Author

@TylerLeonhardt TylerLeonhardt Nov 19, 2019

Choose a reason for hiding this comment

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

@SeeminglyScience were you trying:

Write-Host "`e[3J"

Copy link
Member Author

Choose a reason for hiding this comment

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

damn ok - thought that maybe this would be viable but we'll probably have to do what this PR does and use the vscode API.

Copy link
Member Author

Choose a reason for hiding this comment

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

@SeeminglyScience one more thing:

$([char]27)

Copy link
Collaborator

Choose a reason for hiding this comment

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

@TylerLeonhardt What about it?

Choose a reason for hiding this comment

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

I got this from one of the Clear issues in the new Windows Terminal.

$host.UI.Write( "$([char]27)[3J$([char]27)[2J" )  

It mostly works but requires a few tweaks. As is, the command leaves the cursor at the bottom so I expanded on it to reset the cursor to the top:

$host.UI.RawUI.CursorPosition = New-Object System.Management.Automation.Host.Coordinates 0,0
$host.UI.Write( "$([char]27)[3J$([char]27)[2J" )  

It seems to work better in latest preview. Last time I tested this, I was still having to do manual Clear-Host in addition to it.

Remaining issue is the scrollbar doesn't reset so it looks like there is stuff there but there isn't. I still use VS Code's Clear to remove it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah VS Code's Clear is probably the most reliable here but that's great info @dragonwolf83!

@Websure-Denzil
Copy link

Thought I'd check in to see if this is any closer to being fixed?

@andyleejordan
Copy link
Member

@Websure-Denzil This is a PR from 2019. Can you please open a new issue describing what needs to be fixed?

@Websure-Denzil
Copy link

Hi @andschwa , maybe I misunderstood - but I thought this was about the fact that Clear-Host from powershell does not work when running in visual studio code.
Since this is still the case as far as I am aware, it certainly is for me I wondered if there was any official answer.

Not to worry though, as I see this is closed after 3 years - Its clear there is no enthusiasm for fixing the bug. I will try to work around the issue.

@andyleejordan
Copy link
Member

This was a PR written by an ex-maintainer three years ago implementing that feature (I think? I don't know, I didn't write nor review it, it was 2019). If something isn't work, please follow the bug report template to let me know what behavior you're expecting and the actual behavior you're seeing. I can't track bug reports from comments made on 3 year old merged PRs.

@andyleejordan
Copy link
Member

If you take a look at the latest work being done, I would think you should say there is a lot of enthusiasm for fixing bugs 😀

@PowerShell PowerShell locked as off-topic and limited conversation to collaborators Feb 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants