Account for viewport movement when wrapping over multiple rows

[j4james]

Summary of the Pull Request

When we receive a stream of output at the bottom of the page that wraps
over more than two lines, that is expected to pan the viewport down by
multiple rows to accommodate all of the output. However, when the output
is received in a single write, that did not work correctly.

The problem was that we were reusing a Page instance across multiple
_DoLineFeed calls, and the viewport cached in that Page wasn't valid
after the first call. This PR fixes the issue by adjusting the cached
viewport when we determine it has been moved by _DoLineFeed.

References and Relevant Issues

The bug was introduced in PR #16615 when paging support was added.

Validation Steps Performed

I've verified that the test case in #17351 is now working correctly, and
have added a unit test covering this scenario.

PR Checklist

• Closes Wrapping over multiple lines in a single write fails in conhost #17351
• Tests added/passed

[DHowett]

Thanks for the quick fix!

[lhecker]

Edit: Please don't feel forced to respond in depth, or even at all. I'm mostly just curious and it's not urgent.

Only loosely related: Would it make things simpler here if we represent margins as 4 "insets" relative to the current (moving) viewport rectangle? For instance, left = +4, right = +3 would be a 4- and 3-column margin on the left/right sides respectively. If that's generally possible I imagine that it may make the _WriteToBuffer and _DoLineFeed methods a little easier to write and read, since they could operate with viewport-relative coordinates. For instance, there would be no need to scroll the margins anymore then.

[lhecker]

Oh, and even looselier related:

Assuming VtEngine is gone, and buffer snapshotting is implemented, which means that most things in _DoLineFeed, like NotifyBufferRotation, doesn't need to be called once per linefeed anymore...

Could we hypothetically move an accurate _WriteToBuffer implementation into TextBuffer if the only argument are the margins (apart from the text, etc.)? I'm asking because if we ever wanted to improve performance further again, that'd be IMO the best way to do it. The grapheme cluster PR for instance is basically a fancy wcwidth-based implementation, but it'd be way better if it was a fancy wcswidth one instead, because then the text measurement could run in a much tighter loop for much longer. If TextBuffer had such a function, I believe I could write a much more powerful text storage implementation long term.

[j4james]

Only loosely related: Would it make things simpler here if we represent margins as 4 "insets" relative to the current (moving) viewport rectangle?

They are actually stored as relative coordinates, although not insets - they're relative to the top left of the page. But the _GetVerticalMargins method has an option to return absolute coordinates, and that's what we're using in _WriteToBuffer and _DoLineFeed, because they need to be compared with the absolute cursor coordinates. If you could rewrite that code to use relative cursor coordinates, then we could use relative margin coordinates too, but I suspect that might actually be more complicated.

Could we hypothetically move an accurate _WriteToBuffer implementation into TextBuffer if the only argument are the margins (apart from the text, etc.)?

I'm not sure about that. If that's also got to include the _DoLineFeed implementation, then you need to be able to move the viewport, and that's currently an ITerminalApi call. There are also a few modes that'll you'll need to know about, like auto wrap mode, and erase color mode (possibly others). Maybe not a big deal, but I just want to be clear that it's more than just the margin state.