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

Ensure that delayed EOL wrap is reset when necessary #14936

Merged
merged 8 commits into from
Mar 4, 2023

Conversation

j4james
Copy link
Collaborator

@j4james j4james commented Feb 28, 2023

When a character is written in the last column of a row, the cursor
doesn't move, but instead sets a "delayed EOL wrap" flag. If another
character is then output while that flag is still set, the cursor moves
to the start of the next line, before writing to the buffer.

That flag is supposed to be reset when certain control sequences are
executed, but prior to now we haven't always handled that correctly.
With this PR, we should be resetting the flag appropriately in all the
places that it's expected to be reset.

For the most part, I'm following the DEC STD 070 reference, which lists
a bunch of operations that are intended to reset the delayed wrap flag:

DECSTBM, DECSWL, DECDWL, DECDHL, setting DECCOLM and DECOM,
resetting DECCOLM, DECOM, and DECAWM, CUU, CUD, CUF, CUB,
CUP, HVP, BS, LF, VT, FF, CR, IND, RI, NEL, ECH,
DCH, ICH, EL, DECSEL, DL, IL, ED, and DECSED.

We were already resetting the flag for any of the operations that
performed cursor movement, since that always triggers a reset for us.
However, I've now also added manual resets in those ops that weren't
moving the cursor.

Horizontal tabs are a special case, though. Technically the standard
says they should reset the flag, but most DEC terminals never followed
that rule, and most modern terminals agree that it's best for a tab to
leave the flag as it is. Our implementation now does that too.

But as mentioned above, we automatically reset the flag on any cursor
movement, so the tab operation had to be handled as a special case,
saving and restoring the flag when the cursor is updated.

Another flaw in our implementation was that we should have been saving
and restoring the flag as part of the cursor state in the DECSC and
DECRC operations. That's now been fixed in this PR too.

I should also mention there was a change I had to make to the conpty
renderer, because it was sometimes using an EL sequence while the
terminal was in the delayed EOL wrap state. This would reset the flag,
and break subsequent output, so I've now added a check to prevent that
from happening.

Validation Steps Performed

I've added some unit tests that confirm the operations listed above are
now resetting the delayed EOL wrap as expected, and I've expanded the
existing CursorSaveRestore test to make sure the flag is being saved
and restored correctly.

I've also manually confirmed that the test case in issue #3177 now
matches XTerm's output, and I've confirmed that the results of the
wraptest script1 now match XTerm's results.

Closes #3177

Footnotes

  1. https://github.com/mattiase/wraptest/

@microsoft-github-policy-service microsoft-github-policy-service bot added Issue-Task It's a feature request, but it doesn't really need a major design. Area-Output Related to output processing (inserting text into buffer, retrieving buffer text, etc.) Product-Conhost For issues in the Console codebase labels Feb 28, 2023
@j4james j4james marked this pull request as ready for review March 1, 2023 00:12
Copy link
Member

@lhecker lhecker left a comment

Choose a reason for hiding this comment

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

The tests are great!

{
BEGIN_TEST_METHOD_PROPERTIES()
TEST_METHOD_PROPERTY(L"IsolationLevel", L"Method")
TEST_METHOD_PROPERTY(L"Data:op", L"{0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30,31,32,33,34}")
Copy link
Member

Choose a reason for hiding this comment

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

I feel like a loop over ops might be easier without compromising on the tests too much. I mean, IsolationLevel = Method means that this won't reset the console state after every round anyways, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I know this is horrible, and I did consider using a loop, but the IsolationLevel does actually give us a clean state for each op, and that's necessary in these tests to prevent them interfering with each other. Otherwise we'd have to do a bunch of manual clean up which would be a bit messy.

The other advantage of this approach is that you get a separate test case for each op, so say there's a failure in three of the ops, you'll get three separate errors, and you know exactly what's broken. With a loop, the first failure would prevent the rest of the ops from running, which isn't as nice.

Copy link
Member

Choose a reason for hiding this comment

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

I for one am a big fan of the "gross set of test properties that result in separate test runs". Pretty sure I do that all over RoundtripTests 😝

Copy link
Member

@DHowett DHowett Mar 1, 2023

Choose a reason for hiding this comment

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

I rather like this approach as well. I have it on my list to take the Reflow tests (which use a custom TAEF COM class to offer table-based testing where each test case gets its own name!) and make them "generic" so that other tests can rely on them instead of this, but at the end of the day it is still a way to make a hundred individual test cases in TAEF's eyes.

I have significant beef with the kind of test Leonard is suggesting:

TEST_METHOD(TestEverythingEverywhereAllAtOnce) {
    for(auto i = 0; i < 100; ++i) {
        VERIFY_FALSE(foo, "foo failed lol");
    }
}

which iteration failed? good luck figuring it out.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, some form of table-based testing would have been nice here. Ideally my ops array could have been declared as the property source, and each op would then trigger an individual test run. Similar to a Scenario Outline in a Gherkin test, where you have a table of examples.

Copy link
Member

Choose a reason for hiding this comment

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

Filed #14950 with a nod to this test 😄

Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

Only one open question - lovely to read as always! Thanks!

@@ -76,7 +76,7 @@ class Cursor final

void CopyProperties(const Cursor& OtherCursor) noexcept;

void DelayEOLWrap(const til::point coordDelayedAt) noexcept;
void DelayEOLWrap() noexcept;
Copy link
Member

Choose a reason for hiding this comment

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

I love this. Removing parameters that should be known by internal state. 😄

src/buffer/out/textBuffer.cpp Outdated Show resolved Hide resolved
{
BEGIN_TEST_METHOD_PROPERTIES()
TEST_METHOD_PROPERTY(L"IsolationLevel", L"Method")
TEST_METHOD_PROPERTY(L"Data:op", L"{0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30,31,32,33,34}")
Copy link
Member

Choose a reason for hiding this comment

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

Filed #14950 with a nod to this test 😄

src/terminal/adapter/adaptDispatch.cpp Outdated Show resolved Hide resolved
@microsoft-github-policy-service microsoft-github-policy-service bot added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Mar 3, 2023
@microsoft-github-policy-service microsoft-github-policy-service bot removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Mar 3, 2023
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

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

love it. thanks!

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.) AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Task It's a feature request, but it doesn't really need a major design. Product-Conhost For issues in the Console codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EL behavior at end of line
4 participants