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

ConPTY adds extra newlines symbols at soft-wrapped rows #15064

Closed
Mervap opened this issue Mar 29, 2023 · 5 comments
Closed

ConPTY adds extra newlines symbols at soft-wrapped rows #15064

Mervap opened this issue Mar 29, 2023 · 5 comments
Labels
Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Conpty For console issues specifically related to conpty Resolution-Fix-Available It's available in an Insiders build or a release

Comments

@Mervap
Copy link

Mervap commented Mar 29, 2023

Windows Terminal version

Not installed

Windows build number

10.0.19045.2788

Other Software

.NET 7.0 x64

Steps to reproduce

  1. Create ConPTY from dotnet (minimized version below, full working code here, based on samples/GUIConsole.ConPTY)
  2. Run powershell.exe
  3. Send 500 'a' charachters to PTY input (as a single row)
  4. Read 500 charachters from PTY output
SafeFileHandle inputRead, inputWrite, outputRead, outputWrite;

CreatePseudoConsole(new COORD { X = 40, Y = 20 }, inputRead, outputWrite, 0, out IntPtr hPC);
ProcessFactory.Start("powershell.exe -nologo", 0x00020016, hPC);

var writer = new StreamWriter(new FileStream(inputWrite, FileAccess.Write)) { AutoFlush = true };
var reader = new StreamReader(new FileStream(outputRead, FileAccess.Read));


// Write long row
writer.Write(new string('a', 500));

// Read output
char[] buf = new char[500];
var bytesRead = reader.ReadBlock(buf, 0, 500);
var output = new string(buf.Take(bytesRead).ToArray());

// Check output
// Expected: zero '\n' characters
// Actual: a lot of '\n' characters
Console.Out.Write(output.Split('\n').Length);

Expected Behavior

Pty output contains zero newlines symbols since only one line was sent

Actual Behavior

PTY adds extra \n to output turning soft-wrapping to hard-wrapping

@Mervap Mervap added Issue-Bug It either shouldn't be doing this or needs an investigation. Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Mar 29, 2023
@zadjii-msft
Copy link
Member

Thanks for the report! I suspect this is fixed in main, but I'm gonna leave this open to validate that either there's already a test for it, or if not, to add one.

If it is fixed in main, then I'm betting it's only fixed in >22000 builds of Windows.

If it's not fixed, well, it should be ☺️

@zadjii-msft zadjii-msft added Product-Conpty For console issues specifically related to conpty Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) labels Mar 29, 2023
@zadjii-msft zadjii-msft self-assigned this Mar 29, 2023
@Mervap
Copy link
Author

Mervap commented Mar 29, 2023

Ok, thanks!
Any chance that it would be fixed in win10?

@zadjii-msft
Copy link
Member

Probably not, though, our recommendation is going to be using a nuget package to ingest ConPTY. This nuget package doesn't exist yet1, but when it does, that'll let you use the latest version of ConPTY regardless of what OS version you're on. That's effectively what the Terminal does today (so that we can use ConPTY fixes even on Windows 10). I think there's other folks that have copied what we've done, but that's firmly in the "not yet officially supported" territory.

Footnotes

  1. Originally, this was tracked in conpty: produce a NuGet package (for internal consumption, for now) #3577, but apparently we have no thread for "productize the conpty nuget" and we should. I'll take a todo item to create one.

@zadjii-msft
Copy link
Member

addenda: now tracking that in #15065

@carlos-zamora carlos-zamora removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Mar 29, 2023
@carlos-zamora carlos-zamora added this to the Terminal v1.18 milestone Mar 29, 2023
@zadjii-msft
Copy link
Member

Yep, we definitely already had a test for this one:

void ConptyRoundtripTests::TestWrappingALongString()
{
auto& g = ServiceLocator::LocateGlobals();
auto& renderer = *g.pRender;
auto& gci = g.getConsoleInformation();
auto& si = gci.GetActiveOutputBuffer();
auto& hostSm = si.GetStateMachine();
auto& hostTb = si.GetTextBuffer();
auto& termTb = *term->_mainBuffer;
_flushFirstFrame();
_checkConptyOutput = false;
const auto initialTermView = term->GetViewport();
const auto charsToWrite = gsl::narrow_cast<til::CoordType>(TestUtils::Test100CharsString.size());
VERIFY_ARE_EQUAL(100, charsToWrite);
VERIFY_ARE_EQUAL(0, initialTermView.Top());
VERIFY_ARE_EQUAL(32, initialTermView.BottomExclusive());
hostSm.ProcessString(TestUtils::Test100CharsString);
const auto secondView = term->GetViewport();
VERIFY_ARE_EQUAL(0, secondView.Top());
VERIFY_ARE_EQUAL(32, secondView.BottomExclusive());
auto verifyBuffer = [&](const TextBuffer& tb) {
auto& cursor = tb.GetCursor();
// Verify the cursor wrapped to the second line
VERIFY_ARE_EQUAL(charsToWrite % initialTermView.Width(), cursor.GetPosition().x);
VERIFY_ARE_EQUAL(1, cursor.GetPosition().y);
// Verify that we marked the 0th row as _wrapped_
const auto& row0 = tb.GetRowByOffset(0);
VERIFY_IS_TRUE(row0.WasWrapForced());
const auto& row1 = tb.GetRowByOffset(1);
VERIFY_IS_FALSE(row1.WasWrapForced());
TestUtils::VerifyExpectedString(tb, TestUtils::Test100CharsString, { 0, 0 });
};
verifyBuffer(hostTb);
VERIFY_SUCCEEDED(renderer.PaintFrame());
verifyBuffer(termTb);
}

So, I'll just say "fixed in the latest conpty, but that's probably not on Windows 10, so we'll need to start shipping a conpty nuget that folks can consume externally"

@zadjii-msft zadjii-msft closed this as not planned Won't fix, can't repro, duplicate, stale May 5, 2023
@zadjii-msft zadjii-msft removed their assignment May 5, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Tag-Fix Doesn't match tag requirements label May 5, 2023
@zadjii-msft zadjii-msft added Resolution-Fix-Available It's available in an Insiders build or a release and removed Needs-Tag-Fix Doesn't match tag requirements labels May 5, 2023
@zadjii-msft zadjii-msft removed this from the Terminal v1.18 milestone May 5, 2023
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.) Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Conpty For console issues specifically related to conpty Resolution-Fix-Available It's available in an Insiders build or a release
Projects
None yet
Development

No branches or pull requests

3 participants