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.

Sign up for GitHub

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

Jump to bottom

Use E3 to completely clear console on Unix when possible #88487

Merged
merged 3 commits into from
Jul 7, 2023
Merged

Use E3 to completely clear console on Unix when possible #88487

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5939770
Use E3 to clear console on Unix when possible
natehitze Jul 6, 2023
832b6c4
Use IsNullOrEmpty instead of null check
natehitze Jul 6, 2023
4bc8eb6
Send E3 before clear and fix tests
natehitze Jul 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/libraries/System.Console/src/System/ConsolePal.Unix.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,10 @@ public static void Clear()
{
if (!Console.IsOutputRedirected)
{
if (!string.IsNullOrEmpty(TerminalFormatStringsInstance.ClearScrollbackBuffer))
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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?

Copy link
Contributor Author

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.

image
image

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?

Copy link
Member

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.

{
WriteStdoutAnsiString(TerminalFormatStringsInstance.ClearScrollbackBuffer);
}
WriteStdoutAnsiString(TerminalFormatStringsInstance.Clear);
Copy link
Member

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.

}
}
Expand Down
3 changes: 3 additions & 0 deletions src/libraries/System.Console/src/System/TerminalFormatStrings.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ internal sealed class TerminalFormatStrings
public readonly string? Bell;
/// <summary>The format string to use to clear the terminal.</summary>
public readonly string? Clear;
/// <summary>The format string to use to clear the terminal scrollback buffer, if supported.</summary>
public readonly string? ClearScrollbackBuffer;
/// <summary>The format string to use to set the position of the cursor.</summary>
public readonly string? CursorAddress;
/// <summary>The format string to use to move the cursor to the left.</summary>
Expand Down Expand Up @@ -73,6 +75,7 @@ public TerminalFormatStrings(TermInfo.Database? db)
Reset = db.GetString(TermInfo.WellKnownStrings.OrigPairs) ?? db.GetString(TermInfo.WellKnownStrings.OrigColors);
Bell = db.GetString(TermInfo.WellKnownStrings.Bell);
Clear = db.GetString(TermInfo.WellKnownStrings.Clear);
ClearScrollbackBuffer = db.GetExtendedString("E3");
Columns = db.GetNumber(TermInfo.WellKnownNumbers.Columns);
Lines = db.GetNumber(TermInfo.WellKnownNumbers.Lines);
CursorVisible = db.GetString(TermInfo.WellKnownStrings.CursorVisible);
Expand Down
15 changes: 15 additions & 0 deletions src/libraries/System.Console/tests/TermInfo.Unix.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using System.IO;
using System.Linq;
using System.Reflection;
using System.Tests;
using Xunit;

[SkipOnPlatform(TestPlatforms.Android | TestPlatforms.Browser | TestPlatforms.iOS | TestPlatforms.MacCatalyst | TestPlatforms.tvOS, "Not supported on Android, Browser, iOS, MacCatalyst, or tvOS.")]
Expand Down Expand Up @@ -102,6 +103,20 @@ public void TermInfoVerification(string termToTest, string expectedForeground, s
}
}

[Fact]
[PlatformSpecific(TestPlatforms.AnyUnix)] // Tests TermInfo
public void TermInfoE3CanBeRead()
{
bool ScrollbackBufferIsSet(TerminalData data) => !string.IsNullOrEmpty(data.TerminalDb.ClearScrollbackBuffer);

// XTerm defines E3 for clearing scrollback buffer and tmux does not.
// This can't be added to TermInfoVerification because xterm-256color sometimes has E3 defined (e.g. on Ubuntu but not macOS)
XTermData xTermData = new();
Assert.True(ScrollbackBufferIsSet(xTermData));
TmuxData tmuxData = new();
Assert.False(ScrollbackBufferIsSet(tmuxData));
}

[Fact]
[PlatformSpecific(TestPlatforms.OSX)] // The file being tested is available by default only on OSX
public void EmuTermInfoDoesntBreakParser()
Expand Down