Remove dependency on IsGlyphFullWidth for IRM/DECSWL #16903
Conversation
I fear you may have mistaken COOKED_READ_DATA for tests since it's so repetitive. Currently We do have a pretty good unit test coverage of |
I sure did! |
| if (_modes.test(Mode::InsertReplace)) | ||
| { | ||
| textBuffer.Insert(cursorPosition.y, attributes, state); | ||
| } |
There was a problem hiding this comment.
This isn't working correctly when you have horizontal margins set. If you're inserting text while inside the margins, the content that is pushed to the right should only move as far as the right margin. I didn't look at the implementation that closely, but I'm guessing the Insert method is not taking the state.columnLimit into account.
There was a problem hiding this comment.
Test case in pwsh:
"`e7`e[?69h`e[1;20s`e8*`e[19b`r`e[4h `e[19b`e[4l`e[s`e8`n"
That should produce what is effectively a blank line, but instead we're seeing 20 * characters offset by 20 columns.
There was a problem hiding this comment.
I believe I fixed this issue. Thanks!
I noticed that after running your test string, TextBuffer::SetCurrentLineRendition won't get called anymore. Is that to be expected? (It does work again after emitting a RIS.)
There was a problem hiding this comment.
I didn't think about that, but that is actually correct. When the horizontal margin mode is enabled, line renditions are disabled. I should have reset mode ?69 at the end of that test case.
microsoft-github-policy-service
bot
added
the
Needs-Author-Feedback
microsoft-github-policy-service
bot
removed
the
Needs-Author-Feedback
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| { | ||
| return WEX::Common::NoThrowString(object.to_string().c_str()); | ||
| } | ||
| }; |
There was a problem hiding this comment.
...yes. 😑
Let me know if you dislike this removal. rle_vector uses strstream like all the other classes and TextAttribute has no string-serializer. I just removed it because it was easier. I could however write a full ToString method here if we want to.
There was a problem hiding this comment.
yeah honestly I don't particularly care for all of these
This comment has been minimized.
This comment has been minimized.
This gets rid off the implicit dependency on
IsGlyphFullWidthfor the IRM and DECSWL/DECDWL/DECDHL implementations.
Validation Steps Performed
In pwsh:
"`e[31mab`e[m`b`e[4h`e[32m$('*'*10)`e[m`e[4l"prints a red "a", 10 green "*" and a red "b" ✅
"`e[31mab`e[m`b`e[4h`e[32m$('*'*1000)`e[m`e[4l"prints a red "a" and a couple lines of green "*" ✅
"`e[31mf$('o'*70)`e[m`e#6`e#5"the right half of the row is erased ✅