-
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
Implement Console.SetWindowSize() for linux #75824
Conversation
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
Tagging subscribers to this area: @dotnet/area-system-console Issue DetailsImplement
|
e135d1b
to
ce71f65
Compare
src/libraries/apicompat/ApiCompatBaseline.NetCoreAppLatestStable.txt
Outdated
Show resolved
Hide resolved
@stephentoub Would you review this please? |
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.
@HJLeee thank you for your contribution! PTAL at my comments.
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.
HI @HJLeee
Please excuse me for the delay in the review.
I've cloned your fork and tried to add some new console manual tests:
[ConditionalFact(nameof(ManualTestsEnabled))]
[SkipOnPlatform(TestPlatforms.Browser | TestPlatforms.iOS | TestPlatforms.MacCatalyst | TestPlatforms.tvOS, "Not supported on Browser, iOS, MacCatalyst, or tvOS.")]
public static void ResizeTest()
{
bool wasResized = false;
using (ManualResetEvent manualResetEvent = new(false))
using (PosixSignalRegistration.Create(PosixSignal.SIGWINCH,
ctx =>
{
wasResized = true;
Assert.Equal(PosixSignal.SIGWINCH, ctx.Signal);
manualResetEvent.Set();
}))
{
int widthBefore = Console.WindowWidth;
int heightBefore = Console.WindowHeight;
Assert.False(wasResized);
Console.SetWindowSize(widthBefore / 2, heightBefore / 2);
Assert.True(manualResetEvent.WaitOne(TimeSpan.FromMilliseconds(50)));
Assert.True(wasResized);
Assert.Equal(widthBefore / 2, Console.WindowWidth );
Assert.Equal(heightBefore / 2, Console.WindowHeight );
try
{
AssertUserExpectedResults("terminal window getting two times smaller?");
}
finally
{
Console.SetWindowSize(widthBefore, heightBefore);
}
}
}
The problem is that the value is set correctly (and we get signal notification), but Terminal size does not change in visible way (I've tested GNOME and xterm terminals on Ubuntu). According to this StackOverflow answer it does not work by design on most of the terminals and we should use escape sequences to get it working. We are already using some:
runtime/src/libraries/System.Console/src/System/ConsolePal.Unix.cs
Lines 834 to 840 in 7fbf1c1
private static void WriteResetColorString() | |
{ | |
if (ConsoleUtils.EmitAnsiColorCodes) | |
{ | |
WriteStdoutAnsiString(TerminalFormatStringsInstance.Reset); | |
} | |
} |
Which Terminal have you used for testing?
To run any dotnet app using your local build of dotnet/runtime you need to find corerun:
find -name corerun
and run it by providing the path to the dll:
./artifacts/bin/testhost/net7.0-Linux-Release-x64/shared/Microsoft.NETCore.App/8.0.0/corerun $pathTo.dll
Hi, Thank you for the testcase and review.
I've used my ubuntu 22.04 with · ~/dotnet/tests/winsize $ cat Program.cs
// See https://aka.ms/new-console-template for more information
Console.SetWindowSize(80,30);
Console.WriteLine("Width: {0}, Height: {1}", Console.WindowWidth, Console.WindowHeight); |
Co-authored-by: Stephen Toub <[email protected]> Co-authored-by: Michał Petryka <[email protected]>
Co-authored-by: Adam Sitnik <[email protected]>
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. Thanks!
Co-authored-by: Stephen Toub <[email protected]>
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, big thanks for your contribution @HJLeee !
@ViktorHofer we want to introduce this change on purpose, what is needed to make the CI green in such case? |
Replied offline:
|
@ViktorHofer I've done that and pushed the changes. I can perform a full clean build now ( |
Looking now |
The build is passing for me locally as well and I diffed the binlogs from the failing run and the local one. There aren't any differences that would explain this behavior. After spending some time on it, I just noticed that CI wasn't triggered for your commit :( |
/azp run dotnet-linker-tests |
/azp run runtime-dev-innerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime |
Azure Pipelines successfully started running 1 pipeline(s). |
/azp run runtime-staging |
Azure Pipelines successfully started running 1 pipeline(s). |
1 similar comment
Azure Pipelines successfully started running 1 pipeline(s). |
Wow. Thank you for |
Implement
Console.SetWindowSize()
using the same system call withConsoleGetWindowSize()
(ioctl
w/ set param TIOCSWINSZ).