Add support for horizontal scrolling sequences #15368
Conversation
microsoft-github-policy-service
bot
added
Issue-Task
|
SWEET! Is it weird for us to ship DECLRMM/DECSLRM support in 1.18 without this? Will applications fail in new and exciting ways? |
I don't think that will be an issue. Until we report the horizontal scrolling extension in In practice, though, I suspect a lot of apps (maybe most apps) don't know how to do proper feature detection. They're more likely to have a hard coded list of terminal names that are known to support specific features. But at least we're providing accurate information for those that do choose to look for it. |
| void ScreenBufferTests::HorizontalScrollOperations() | ||
| { | ||
| BEGIN_TEST_METHOD_PROPERTIES() | ||
| TEST_METHOD_PROPERTY(L"Data:op", L"{0, 1, 2, 3}") |
There was a problem hiding this comment.
Instead of adding cleanup annotations, you can also use method isolation. It's maybe not worth changing now, but worth considering in the future :)
I don't know what downsides exist... so that could be a problem.
There was a problem hiding this comment.
It depends how much cleanup is involved, but I usually try to avoid method isolation if I can help it, because I thought it made the tests quite a bit slower. It also makes them more painful to debug, because you can only step through a single iteration and then it complains about TAEF needing to start a second test host (I don't know if there's a workaround for that which I'm just not aware of).
| const auto [topMargin, bottomMargin] = _GetVerticalMargins(viewport, true); | ||
| const auto [leftMargin, rightMargin] = _GetHorizontalMargins(bufferWidth); | ||
| if (row >= topMargin && row <= bottomMargin && col >= leftMargin && col <= rightMargin) |
There was a problem hiding this comment.
If we had a _GetMargins function we could use a til::point / til::rect check.
BTW I recently wrote a range<T> struct for AtlasEngine (here) which has a contains() function. This might be useful for code like this too. (Although it would need to be split up into range and inclusive_range or something.)
There was a problem hiding this comment.
That range class will be very useful! I'm not going to try and integrate it now, but it's definitely worth investigating at some point. There's a lot of margin testing which could probably benefit from that.
| // - delta - Number of columns to modify (positive if inserting, negative if deleting). | ||
| // Return Value: | ||
| // - <none> | ||
| void AdaptDispatch::_InsertDeleteColumnHelper(const VTInt delta) |
There was a problem hiding this comment.
Given that _ScrollRectHorizontally takes a int32_t I think this should do the same... I think?
There was a problem hiding this comment.
Actually now that I've had a look at them, all the int32_t parameters should really be VTInt - that's what they already are in the header! I think originally the VT parameters where coming in as an unsigned int of some sort, so we had to cast to the signed int32_t when they were used as a delta. But since VTInt is now signed, that's no longer necessary.
| const auto [topMargin, bottomMargin] = _GetVerticalMargins(viewport, true); | ||
|
|
||
| // If the cursor is at the left of the margin area, we shift the buffer right. | ||
| if (cursorPosition.x == leftMargin && cursorPosition.y >= topMargin && cursorPosition.y <= bottomMargin) |
There was a problem hiding this comment.
Can the cursor be outside of the margins? I mean should we use <= here instead of ==?
There was a problem hiding this comment.
Yeah, when it's left of the margin it just keeps moving left until it hits column 0 (that's handled in the branch below this).
zadjii-msft
added
the
AutoMerge
This PR introduces four new escapes sequences:
DECIC(Insert Column),DECDC(Delete Column),DECBI(Back Index), andDECFI(ForwardIndex), which allow for horizontal scrolling within a margin area.
References and Relevant Issues
This follows on from the horizontal margins PR #15084 to complete the
requirements for the horizontal scrolling extension.
Detailed Description of the Pull Request / Additional comments
The implementation is fairly straightforward, since they're all built on
top of the existing
_ScrollRectHorizontallymethod.Validation Steps Performed
Thanks to @al20878, these operations have been extensively tested on a
number of DEC terminals and I've manually confirmed our implementation
matches their behavior.
I've also added a unit tests that covers the basic execution of each of
the operations.
Closes #15109