-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Use E3 to completely clear console on Unix when possible #88487
Conversation
Some terminals define an extra sequence to clear the terminal scroll buffer. Using it after the clear sequence makes Clear() work like the 'clear' command. Fix dotnet#84351
Tagging subscribers to this area: @dotnet/area-system-console Issue DetailsSome terminals define an extra sequence to clear the terminal scroll buffer. Using it after the clear sequence makes Clear() work like the 'clear' command. Fix #84351 The linked issue has a detailed analysis and explanation which was the guide for this PR.
|
@dotnet-policy-bot agree |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@natehitze big thanks for your contribution! PTAL at my comments, we need to solve the macOS CI failure.
[InlineData("mach-color", "\u001B\u005B\u00335m", "\u001B\u005B\u00345m", 5)] | ||
[InlineData("mach-color", "\u001B\u005B\u003312m", "\u001B\u005B\u003412m", 12)] | ||
public void TermInfoVerification(string termToTest, string expectedForeground, string expectedBackground, int colorValue) | ||
[InlineData("xterm-256color", "\u001B\u005B\u00330m", "\u001B\u005B\u00340m", 0, true)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the test failures indicate that E3 is missing on xterm-256color, but only macOS. I wonder why: does macOS simply not provide it or it has a different name?
Please let me know if you need a copy of macOS terminfo file.
/private/tmp/helix/working/AFB20920/w/BFE50A43/e /private/tmp/helix/working/AFB20920/w/BFE50A43/e
Discovering: System.Console.Tests (method display = ClassAndMethod, method display options = None)
Discovered: System.Console.Tests (found 113 of 148 test cases)
Starting: System.Console.Tests (parallel test collections = on, max threads = 4)
Color.RedirectedOutput_EnvVarSet_EmitsAnsiCodes [SKIP]
Condition(s) not met: "TermIsSetAndRemoteExecutorIsSupported"
0
0
TermInfoTests.TermInfoVerification(termToTest: "xterm-256color", expectedForeground: "\x1b[30m", expectedBackground: "\x1b[40m", colorValue: 0, expectedScrollbackClearSupport: True) [FAIL]
Assert.Equal() Failure
Expected: True
Actual: False
Stack Trace:
/_/src/libraries/System.Console/tests/TermInfo.Unix.cs(102,0): at TermInfoTests.TermInfoVerification(String termToTest, String expectedForeground, String expectedBackground, Int32 colorValue, Boolean expectedScrollbackClearSupport)
at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
/_/src/libraries/System.Private.CoreLib/src/System/Reflection/MethodInvoker.cs(59,0): at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr)
TermInfoTests.TermInfoVerification(termToTest: "xterm-256color", expectedForeground: "\x1b[31m", expectedBackground: "\x1b[41m", colorValue: 1, expectedScrollbackClearSupport: True) [FAIL]
Assert.Equal() Failure
Expected: True
Actual: False
Stack Trace:
/_/src/libraries/System.Console/tests/TermInfo.Unix.cs(102,0): at TermInfoTests.TermInfoVerification(String termToTest, String expectedForeground, String expectedBackground, Int32 colorValue, Boolean expectedScrollbackClearSupport)
at InvokeStub_TermInfoTests.TermInfoVerification(Object, Object, IntPtr*)
at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr)
TermInfoTests.TermInfoVerification(termToTest: "xterm-256color", expectedForeground: "\x1b[90m", expectedBackground: "\x1b[100m", colorValue: 8, expectedScrollbackClearSupport: True) [FAIL]
Assert.Equal() Failure
Expected: True
Actual: False
Stack Trace:
/_/src/libraries/System.Console/tests/TermInfo.Unix.cs(102,0): at TermInfoTests.TermInfoVerification(String termToTest, String expectedForeground, String expectedBackground, Int32 colorValue, Boolean expectedScrollbackClearSupport)
at InvokeStub_TermInfoTests.TermInfoVerification(Object, Object, IntPtr*)
at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr)
Finished: System.Console.Tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to https://github.com/watchexec/clearscreen/blob/main/TERMINALS.md#platforms macOS doesn't include E3 in terminfo. That seems to be true on the mac I have access to -- and the clear
command with xterm-256color
does not clear the scrollback buffer but does clear it on my machine (PopOS 22.04) with xterm-256color
.
I'll take a look at the Console.ReadKey
tests you mentioned below and get the tests fixed up for CI. Thanks for reviewing!
[InlineData("wsvt25", "\u001B\u005B\u003311m", "\u001B\u005B\u003411m", 11, false)] | ||
[InlineData("mach-color", "\u001B\u005B\u00330m", "\u001B\u005B\u00340m", 0, false)] | ||
[InlineData("mach-color", "\u001B\u005B\u00335m", "\u001B\u005B\u00345m", 5, false)] | ||
[InlineData("mach-color", "\u001B\u005B\u003312m", "\u001B\u005B\u003412m", 12, false)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's a pity that it's not supported by more terminals. FWIW we have Console.ReadKey
tests that have copies of terminfo files for multiple terminals:
protected override string EncodedTerminalDb => "GgElACYADwCdAQIGeHRlcm0tMjU2Y29sb3J8eHRlcm0gd2l0aCAyNTYgY29sb3JzAAABAAABAAAAAQAAAAABAQAAAAAAAAABAAABAAEBAAAAAAAAAAABAFAACAAYAP//////////////////////////AAH/fwAABAAGAAgAGQAeACYAKgAuAP//OQBKAEwAUABXAP//WQBmAP//agBuAHgAfAD/////gACEAIkAjgD//6AApQCqAP//rwC0ALkAvgDHAMsA0gD//+QA6QDvAPUA////////BwH///////8ZAf//HQH///////8fAf//JAH//////////ygBLAEyATYBOgE+AUQBSgFQAVYBXAFgAf//ZQH//2kBbgFzAXcBfgH//4UBiQGRAf////////////////////////////+ZAaIB/////6sBtAG9AcYBzwHYAeEB6gHzAfwB////////BQIJAg4CEwInAjAC/////0ICRQJQAlMCVQJYArUC//+4Av///////////////7oC//////////++Av//8wL/////9wL9Av////////////////////////////8DAwcD//////////////////////////////////////////////////////////////////8LA/////8SA///////////GQMgAycD/////y4D//81A////////zwD/////////////0MDSQNPA1YDXQNkA2sDcwN7A4MDiwOTA5sDowOrA7IDuQPAA8cDzwPXA98D5wPvA/cD/wMHBA4EFQQcBCMEKwQzBDsEQwRLBFMEWwRjBGoEcQR4BH8EhwSPBJcEnwSnBK8EtwS/BMYEzQTUBP/////////////////////////////////////////////////////////////ZBOQE6QT8BAAFCQUQBf////////////////////////////9uBf///////////////////////3MF////////////////////////////////////////////////////////////////////////////////////////eQX///////99BbwF//////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////wF/wUbW1oABwANABtbJWklcDElZDslcDIlZHIAG1szZwAbW0gbWzJKABtbSwAbW0oAG1slaSVwMSVkRwAbWyVpJXAxJWQ7JXAyJWRIAAoAG1tIABtbPzI1bAAIABtbPzEybBtbPzI1aAAbW0MAG1tBABtbPzEyOzI1aAAbW1AAG1tNABsoMAAbWzVtABtbMW0AG1s/MTA0OWgbWzIyOzA7MHQAG1sybQAbWzRoABtbOG0AG1s3bQAbWzdtABtbNG0AG1slcDElZFgAGyhCABsoQhtbbQAbWz8xMDQ5bBtbMjM7MDswdAAbWzRsABtbMjdtABtbMjRtABtbPzVoJDwxMDAvPhtbPzVsABtbIXAbWz8zOzRsG1s0bBs+ABtbTAB/ABtbM34AG09CABtPUAAbWzIxfgAbT1EAG09SABtPUwAbWzE1fgAbWzE3fgAbWzE4fgAbWzE5fgAbWzIwfgAbT0gAG1syfgAbT0QAG1s2fgAbWzV+ABtPQwAbWzE7MkIAG1sxOzJBABtPQQAbWz8xbBs+ABtbPzFoGz0AG1s/MTAzNGwAG1s/MTAzNGgAG1slcDElZFAAG1slcDElZE0AG1slcDElZEIAG1slcDElZEAAG1slcDElZFMAG1slcDElZEwAG1slcDElZEQAG1slcDElZEMAG1slcDElZFQAG1slcDElZEEAG1tpABtbNGkAG1s1aQAlcDElYxtbJXAyJXsxfSUtJWRiABtjG10xMDQHABtbIXAbWz8zOzRsG1s0bBs+ABs4ABtbJWklcDElZGQAGzcACgAbTQAlPyVwOSV0GygwJWUbKEIlOxtbMCU/JXA2JXQ7MSU7JT8lcDUldDsyJTslPyVwMiV0OzQlOyU/JXAxJXAzJXwldDs3JTslPyVwNCV0OzUlOyU/JXA3JXQ7OCU7bQAbSAAJABtPRQBgYGFhZmZnZ2lpampra2xsbW1ubm9vcHBxcXJyc3N0dHV1dnZ3d3h4eXl6ent7fHx9fX5+ABtbWgAbWz83aAAbWz83bAAbT0YAG09NABtbMzsyfgAbWzE7MkYAG1sxOzJIABtbMjsyfgAbWzE7MkQAG1s2OzJ+ABtbNTsyfgAbWzE7MkMAG1syM34AG1syNH4AG1sxOzJQABtbMTsyUQAbWzE7MlIAG1sxOzJTABtbMTU7Mn4AG1sxNzsyfgAbWzE4OzJ+ABtbMTk7Mn4AG1syMDsyfgAbWzIxOzJ+ABtbMjM7Mn4AG1syNDsyfgAbWzE7NVAAG1sxOzVRABtbMTs1UgAbWzE7NVMAG1sxNTs1fgAbWzE3OzV+ABtbMTg7NX4AG1sxOTs1fgAbWzIwOzV+ABtbMjE7NX4AG1syMzs1fgAbWzI0OzV+ABtbMTs2UAAbWzE7NlEAG1sxOzZSABtbMTs2UwAbWzE1OzZ+ABtbMTc7Nn4AG1sxODs2fgAbWzE5OzZ+ABtbMjA7Nn4AG1syMTs2fgAbWzIzOzZ+ABtbMjQ7Nn4AG1sxOzNQABtbMTszUQAbWzE7M1IAG1sxOzNTABtbMTU7M34AG1sxNzszfgAbWzE4OzN+ABtbMTk7M34AG1syMDszfgAbWzIxOzN+ABtbMjM7M34AG1syNDszfgAbWzE7NFAAG1sxOzRRABtbMTs0UgAbWzFLABtbJWklZDslZFIAG1s2bgAbWz8lWzswMTIzNDU2Nzg5XWMAG1tjABtbMzk7NDltABtdMTA0BwAbXTQ7JXAxJWQ7cmdiOiVwMiV7MjU1fSUqJXsxMDAwfSUvJTIuMlgvJXAzJXsyNTV9JSolezEwMDB9JS8lMi4yWC8lcDQlezI1NX0lKiV7MTAwMH0lLyUyLjJYG1wAG1szbQAbWzIzbQAbW00AG1slPyVwMSV7OH0lPCV0MyVwMSVkJWUlcDElezE2fSU8JXQ5JXAxJXs4fSUtJWQlZTM4OzU7JXAxJWQlO20AG1slPyVwMSV7OH0lPCV0NCVwMSVkJWUlcDElezE2fSU8JXQxMCVwMSV7OH0lLSVkJWU0ODs1OyVwMSVkJTttABtsABttAAIAAABAAIIAAwMBAQAABwATABgAKgAwADoAQQBIAE8AVgBdAGQAawByAHkAgACHAI4AlQCcAKMAqgCxALgAvwDGAM0A1ADbAOIA6QDwAPcA/gAFAQwBEwEaASEBKAEvATYBPQFEAUsBUgFZAWABZwFuAXUBfAGDAYoBkQGYAZ8B//////////+mAawBAAADAAYACQAMAA8AEgAVABgAHQAiACcALAAxADUAOgA/AEQASQBOAFQAWgBgAGYAbAByAHgAfgCEAIoAjwCUAJkAngCjAKkArwC1ALsAwQDHAM0A0wDZAN8A5QDrAPEA9wD9AAMBCQEPARUBGwEfASQBKQEuATMBOAE8AUABRAFIAU0BG10xMTIHABtdMTI7JXAxJXMHABtbM0oAG101MjslcDElczslcDIlcwcAG1syIHEAG1slcDElZCBxABtbMzszfgAbWzM7NH4AG1szOzV+ABtbMzs2fgAbWzM7N34AG1sxOzJCABtbMTszQgAbWzE7NEIAG1sxOzVCABtbMTs2QgAbWzE7N0IAG1sxOzNGABtbMTs0RgAbWzE7NUYAG1sxOzZGABtbMTs3RgAbWzE7M0gAG1sxOzRIABtbMTs1SAAbWzE7NkgAG1sxOzdIABtbMjszfgAbWzI7NH4AG1syOzV+ABtbMjs2fgAbWzI7N34AG1sxOzNEABtbMTs0RAAbWzE7NUQAG1sxOzZEABtbMTs3RAAbWzY7M34AG1s2OzR+ABtbNjs1fgAbWzY7Nn4AG1s2Ozd+ABtbNTszfgAbWzU7NH4AG1s1OzV+ABtbNTs2fgAbWzU7N34AG1sxOzNDABtbMTs0QwAbWzE7NUMAG1sxOzZDABtbMTs3QwAbWzE7MkEAG1sxOzNBABtbMTs0QQAbWzE7NUEAG1sxOzZBABtbMTs3QQAbWzI5bQAbWzltAEFYAFhUAENyAENzAEUzAE1zAFNlAFNzAGtEQzMAa0RDNABrREM1AGtEQzYAa0RDNwBrRE4Aa0ROMwBrRE40AGtETjUAa0RONgBrRE43AGtFTkQzAGtFTkQ0AGtFTkQ1AGtFTkQ2AGtFTkQ3AGtIT00zAGtIT000AGtIT001AGtIT002AGtIT003AGtJQzMAa0lDNABrSUM1AGtJQzYAa0lDNwBrTEZUMwBrTEZUNABrTEZUNQBrTEZUNgBrTEZUNwBrTlhUMwBrTlhUNABrTlhUNQBrTlhUNgBrTlhUNwBrUFJWMwBrUFJWNABrUFJWNQBrUFJWNgBrUFJWNwBrUklUMwBrUklUNABrUklUNQBrUklUNgBrUklUNwBrVVAAa1VQMwBrVVA0AGtVUDUAa1VQNgBrVVA3AGthMgBrYjEAa2IzAGtjMgBybXh4AHNteHgA"; // /lib/terminfo/x/xterm-256color |
runtime/src/libraries/System.Console/tests/KeyParserTests.cs
Lines 16 to 26 in 104752a
new XTermData(), | |
new GNOMETerminalData(), | |
new LinuxConsole(), | |
new PuTTYData_xterm(), | |
new PuTTYData_linux(), | |
new PuTTYData_putty(), | |
new WindowsTerminalData(), | |
new TmuxData(), | |
new Tmux256ColorData(), | |
new RxvtUnicode(), | |
new AlacrittyData(), |
if you want to check anything related to this terminals you can just hack these tests, the TermInfo.Database
is created here:
runtime/src/libraries/System.Console/tests/KeyParserTests.cs
Lines 431 to 432 in 104752a
internal TerminalFormatStrings TerminalDb => _terminalDb ??= | |
new TerminalFormatStrings(new TermInfo.Database(Term, Convert.FromBase64String(EncodedTerminalDb))); |
You can even debug that on Windows, as these tests are platform-independent.
@@ -223,6 +223,10 @@ public static void Clear() | |||
if (!Console.IsOutputRedirected) | |||
{ | |||
WriteStdoutAnsiString(TerminalFormatStringsInstance.Clear); | |||
if (!string.IsNullOrEmpty(TerminalFormatStringsInstance.ClearScrollbackBuffer)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
according to https://unix.stackexchange.com/a/375784 we should do E3 before clear:
If the terminfo/termcap entry has an E3 capability it, it first writes out that.
It then uses the clear capability to clear the visible screen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The snippet of clear
shared in the issue had E3 being issued after clear, but I'm not sure it matters either way. I'll switch the order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume we manually validated the behavior on at least some system with the E3 and then clear?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stephentoub I had tested before, but unfortunately just assumed the switched order was okay per the linked answer. I just ran my manual test again and having E3 issued before clear does not clear the scrollback buffer.
That means this order needs reversed back to the original: Clear, then E3. Would you mind making that change on your PR? Or do you want me to submit a new PR with the optimization mentioned below + this fix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind making that change on your PR?
No problem; I switched the order back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, again big thanks for your contribution @natehitze !
if (!string.IsNullOrEmpty(TerminalFormatStringsInstance.ClearScrollbackBuffer)) | ||
{ | ||
WriteStdoutAnsiString(TerminalFormatStringsInstance.ClearScrollbackBuffer); | ||
} | ||
WriteStdoutAnsiString(TerminalFormatStringsInstance.Clear); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the only place Clear is used, right? Rather than doing two separate writes, could we just update TerminalFormatStringsInstance.Clear to be constructed as the concatenation with ClearScrollbackBuffer if it exists, and then this call site doesn't change from what it was before... it just writes out Clear, which will issue both commands if they're supported.
Some terminals define an extra sequence to clear the terminal scroll buffer. Using it after the clear sequence makes Clear() work like the 'clear' command.
Fix #84351
The linked issue has a detailed analysis and explanation which was the guide for this PR.