Ensure that delayed EOL wrap is reset when necessary#14936
Ensure that delayed EOL wrap is reset when necessary#14936DHowett merged 8 commits intomicrosoft:mainfrom
Conversation
microsoft-github-policy-service
bot
added
Issue-Task
| { | ||
| 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}") |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 😝
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
DHowett
left a comment
DHowett
left a comment
There was a problem hiding this comment.
Only one open question - lovely to read as always! Thanks!
| void CopyProperties(const Cursor& OtherCursor) noexcept; | ||
|
|
||
| void DelayEOLWrap(const til::point coordDelayedAt) noexcept; | ||
| void DelayEOLWrap() noexcept; |
There was a problem hiding this comment.
I love this. Removing parameters that should be known by internal state. 😄
| { | ||
| 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}") |
microsoft-github-policy-service
bot
added
the
Needs-Author-Feedback
microsoft-github-policy-service
bot
removed
the
Needs-Author-Feedback
DHowett
added
the
AutoMerge
4 participants
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, settingDECCOLMandDECOM,resetting
DECCOLM,DECOM, andDECAWM,CUU,CUD,CUF,CUB,CUP,HVP,BS,LF,VT,FF,CR,IND,RI,NEL,ECH,DCH,ICH,EL,DECSEL,DL,IL,ED, andDECSED.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
DECSCandDECRCoperations. 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
ELsequence while theterminal 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
CursorSaveRestoretest to make sure the flag is being savedand 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
https://github.com/mattiase/wraptest/ ↩