Add support for horizontal margin sequences #15084

j4james · 2023-04-03T01:08:49Z

This PR introduces two new escapes sequences: DECLRMM (Left Right
Margin Mode), which enables the horizontal margin support, and DECSLRM
(Set Left and Right Margins), which does exactly what the name suggests.

A whole lot of existing operations have then been updated to take those
horizontal margins into account.

Detailed Description of the Pull Request / Additional comments

The main complication was in figuring out in what way each operation is
affected, since there's a fair amount of variation.

When writing text to the buffer, we need to wrap within the horizontal
margins, but only when the cursor is also within the vertical margins,
otherwise we just wrap within the boundaries of the screen.
Not all cursor movement operations are constrained by the margins, but
for those that are, we clamp within both the vertical and horizontal
margins. But if the cursor is already outside the margins, it is just
clamped at the edges of the screen.
The ICH and DCH operations are constrained by the horizontal
margins, but only when inside the vertical margins. And if the cursor is
outside the horizontal margins, these operations have no effect at all.
The rectangular area operations are clamped within the horizontal
margins when in the origin mode, the same way they're clamped within the
vertical margins.
The scrolling operations only scroll the area inside both horizontal
and vertical margins. This includes the IL and DL operations, but
they also won't have any effect at all unless the cursor is already
inside the margin area.
CR returns to the left margin rather than the start of the line,
unless the cursor is already left of that margin, or outside the
vertical margins.
LF, VT, FF, and IND only trigger a scroll at the bottom margin
if the cursor is already inside both vertical and horizontal margins.
The same rules apply to RI when triggering a scroll at the top margin.

Another thing worth noting is the change to the ANSISYSSC operation.
Since that shares the same escape sequence as DECSLRM, they can't both
be active at the same time. However, the latter is only meant to be
active when DECLRMM is set, so by default ANSISYSC will still work,
but it'll no longer apply once the DECLRMM mode is enabled.

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 extended some of our existing unit tests to cover at least the
basic margin handling, although not all of the edge cases.

Closes #14876

j4james · 2023-04-04T18:36:44Z

For the record, I should note that this might have some impact on performance, because there's now a fair amount of additional overhead on a number of operations. I had some vague ideas for how it could be optimised (assuming it is an issue), but it's not something I was intending to look at in the near future. So if you think it's too much of a performance hit as is, feel free to reject this PR.

DHowett

I'm at 11/13, because I put off all the hard files for the end.

One question I had before reviewing was how this works for double-width glyphs that don't fit. I'm assuming the good work you and Leonard have been doing makes them truncate on the right side and wrap to the bottom left.

I didn't, in my quick overview, spy a test for both TB and LR margins... but I would guess that's in my unreviewed 905 lines of ScreenBufferTests. 😄

src/terminal/parser/ut_parser/OutputEngineTest.cpp

j4james · 2023-04-23T12:04:12Z

I didn't, in my quick overview, spy a test for both TB and LR margins...

It's quite possible there isn't one.

FYI, having just looked at these tests again (working on a follow up PR for the horizontal scrolling operations), I realised that the ones that are based on _CommonScrollingSetup are at least testing both sets of margins. That setup initializes the vertical margins by default, and we add horizontal margins on top of that. So that covers SU, SD, IL, DL, and RI.

Also, the test case I added to the existing LineFeedEscapeSequences sets both vertical and horizontal margins, so that covers NEL, IND (and by extension, LF, VT, and FF). Same goes for CursorNextPreviousLine, CursorPositionRelative, InsertChars and DeleteChars, which covers CNL and CPL, HPR and VPR, ICH and DCH.

So the coverage isn't actually as bad as I expected, but still not that good in terms of edge cases.

DHowett · 2023-04-24T22:27:54Z

That makes perfect sense. Thanks for writing it up! We're gonna try to get this reviewed for the coming 1.18 release.

DHowett

12/13, adaptDispatch is next

DHowett · 2023-04-24T22:35:13Z

src/host/ut_host/ScreenBufferTests.cpp

+    const auto saveCursor = L"\x1b\x37";
+    const auto restoreCursor = L"\x1b\x38";


I'm not sure I understand this one

This is just swapping the ANSISYSSC/ANSISYSRC controls for the equivalent DECSC/DECRC controls. For the most part they're aliases, so it shouldn't matter which of them we use, except that ANSISYSSC is disabled when the left-right margin mode is active, because it clashes with the DECSLRM sequence. So if we want to test the effect of horizontal margins, we have to use the DEC controls.

carlos-zamora · 2023-04-06T22:37:01Z

src/terminal/parser/OutputStateMachineEngine.cpp

        success = _dispatch->SetTopBottomScrollingMargins(parameters.at(0).value_or(0), parameters.at(1).value_or(0));
        TermTelemetry::Instance().Log(TermTelemetry::Codes::DECSTBM);
        break;
+    case CsiActionCodes::DECSLRM_SetLeftRightMargins:
+        // Note that this can also be ANSISYSSC, depending on the state of DECLRMM.


Do we need to do anything to handle that scenario? Why, or why not?

Yeah, if you look at the SetLeftRightScrollingMargins implementation in AdaptDispatch, there's a test there that forwards this sequence to CursorSaveState if the left-right margin mode hasn't been activated yet. So by default it can still be used to save the cursor state. It's only once DECSLRMM has been enabled that it'll be used to control the margins instead.

carlos-zamora · 2023-04-06T23:49:55Z

src/terminal/adapter/adaptDispatch.cpp

+    const auto [leftMargin, rightMargin] = _GetHorizontalMargins(textBuffer.GetSize().Width());
+
+    auto lineWidth = textBuffer.GetLineWidth(cursorPosition.y);
+    if (cursorPosition.x <= rightMargin && cursorPosition.y >= topMargin && cursorPosition.y <= bottomMargin)


Curious, why not check for left margin?

What we're calculating here is whether the cursor should wrap at the edge of the screen or at the right margin, and the left margin is of no relevance to that. You wouldn't want the text to bypass the right margin, just because the initial cursor position was outside the left margin.

carlos-zamora · 2023-04-06T23:51:09Z

src/terminal/adapter/adaptDispatch.cpp

@@ -98,7 +108,12 @@ void AdaptDispatch::_WriteToBuffer(const std::wstring_view string)
                _DoLineFeed(textBuffer, true, true);
                cursorPosition = cursor.GetPosition();
                // We need to recalculate the width when moving to a new line.
-                state.columnLimit = textBuffer.GetLineWidth(cursorPosition.y);
+                lineWidth = textBuffer.GetLineWidth(cursorPosition.y);
+                if (cursorPosition.y >= topMargin && cursorPosition.y <= bottomMargin)


Why not check the left/right margin like we do earlier? Is it because we're just handling the delayed EOL wrap, so we're only messing with the y-value here?

As mentioned above, the left margin isn't relevant, and the reason we don't need to check the right margin is because we've just performed a carriage return/line feed (in the _DoLineFeed call), so we know we should definitely be inside the right margin.

carlos-zamora · 2023-04-07T00:07:19Z

src/terminal/adapter/adaptDispatch.cpp

+            if (cursorPosition.y >= topMargin)
+            {
+                row = std::max(row, topMargin);
+            }
+            if (cursorPosition.y <= bottomMargin)
+            {
+                row = std::min(row, bottomMargin);
+            }


You could probably do an std::clamp here and below if you wanna simplify it.

I don't think that'll work, because the conditions that determine whether clamping is necessary are based on the original cursor position, not the values being clamped. I have made a few attempts to simplify this, but it always ended up with some edge case breaking.

DHowett · 2023-04-26T20:52:26Z

src/terminal/adapter/adaptDispatch.cpp

+// - bufferWidth - The width of the buffer
+// Return Value:
+// - A std::pair containing the left and right coordinates (inclusive).
+std::pair<int, int> AdaptDispatch::_GetHorizontalMargins(const til::CoordType bufferWidth) noexcept


i'm slightly alarmed that a _get function changes state, but I'll circle back after I read the rest of adaptDispatch

okay, GetVerticalMargins does as well. i'm less worried

In the long term I was hoping there'd be a way for AdaptDispatch to register a callback where it's notified of buffer and window size changes, and that's when it would recalculate the margin boundaries. If we could do that, then these _Get*Margin methods would just be returning precalculated values.

As simple as this function is, I personally don't think caching is necessary. Just eyeballing it, I suspect it runs in ~10ns on modern CPUs, or the equivalent of doing a 2 calls. 😅

It's not just the function itself, though. The vertical margin calculation depends on the viewport size, which we need to retrieve with a virtual function call, and the horizontal margin calculation depends on the buffer size, which means another virtual call to obtain the buffer. Generally speaking we're going to need the buffer anyway, so that's not really extra, but ideally I'd like to have eliminated all of those calls if possible.

You'll know better than me how much of difference that would make, so if you're happy with it how it is then that's good enough for me. I was just concerned that I might be introducing a bunch of overhead with this stuff which would undermine all the work you've been doing to improve the performance.

That's a good point! One of the big refactors I'm planning to do next in this area will be to make IRenderData less virtual and to expose the text buffer, viewport, etc. as a plain struct members, to reduce the overhead of virtual calls in our hot paths. So if all goes well, we'll solve that issue categorically soon. 🙂

DHowett · 2023-04-26T20:54:45Z

src/terminal/adapter/adaptDispatch.cpp

+std::pair<int, int> AdaptDispatch::_GetHorizontalMargins(const til::CoordType bufferWidth) noexcept
+{
+    // If the left is out of range, reset the margins completely.
+    const auto rightmostColumn = bufferWidth - 1;


will this ever be called with a width from a different text buffer, or a width that isn't the width of the active text buffer? If so, there is a risk that we blow up global state based on a local parameter

Well there's the alt buffer, and eventually we will have additional page buffers, but they should all have the same width. And in any event, the margins are meant to be a shared global state, so if they get reset by one buffer, then it's expected that they'll be reset everywhere.

Although, now that I think about it, there may be a case where we're passing in the line width rather than the buffer width. So on a double-width line, the margin could be out of range - and thus reset - when perhaps it shouldn't be. I'm not actually sure how that is expected to work. It might still be correct as it is.

So on a double-width line, the margin could be out of range

I've just been trying to trigger this now, only to realise it's not possible. I forgot that line renditions are disabled when horizontal margins are active, so this is fine as it is. Still would be nice if we could do these calculations when resizing (as I explained above), but it's not essential for now.

DHowett · 2023-05-12T23:03:27Z

@lhecker if you get time, would you mind being the clinch reviewer on this? You are my performance expert, and James had some concerns.

We've been bug-bashing this for the past month as it was planned to be included in 1.18, so I am making a feature freeze exception for it 😄

lhecker · 2023-05-13T21:39:17Z

I've tested plaintext writes at 1KB to 128KB buffer sizes with VT mode enabled, as well as my "rainbowbench" which mostly tests SGR sequences, inside OpenConsole with UseDx=2. I found that this PR has no measurable impact on our current code. If there's any impact due to this PR, it's likely within the margin of error. (To give some ballpark numbers, both before/after this PR my PC manages roughly 100MB/s @ 128KB, 90MB/s @ 16KB, 33MB/s @ 1KB writes for plaintext and 50MB/s of SGR.)

With my latest batch of not-yet-submitted performance improvements ¹ merged in, this PR starts to have a consistent impact of around 3% performance loss for any text writes. I think this is simply because they more than double the plaintext performance to >230MB/s @ 128KB, >190MB/s @ 16KB and thus uncover smaller costs like that. But that such improvements are possible at all indicates to me that a 3% performance loss is still not really that important to be honest. 😅 Finding and performing thse optimizations only took a few hours, so I'm sure there's yet (a lot) more to come too. 🙂

1. Rewrote StateMachine::ProcessString to have an outer loop with 2 much tighter inner loops: The first loop finds the first VT character and then emits the skipped part to the text buffer all at once; The second loop processes VT until the ground state has been reached. This improves performance by ~20%. 2. Added a ASCII-fastpass to ROW::WriteHelper::ReplaceText. This improves perf another ~25%. 3. Removed TextBuffer::TriggerRedrawCursor and instead the renderer pulls the latest cursor position when it renders. It caches the previous cursor position so that it can still call InvalidateCursor on the engines. This improves performance by another ~40%. 4. Added a "whitespace-only" ROW as a member to TextBuffer. In ROW::Reset it'll now simply memcpy the whitespace from that whitespace-only-ROW. This improves perf another ~30%. There's a few more subtle changes. ↩

DHowett

Thanks again for doing this, and for being patient while we figure out the release story for 1.18 plus. I'm so excited to bring this in. :D

DHowett · 2023-05-15T21:07:36Z

Alas, there's some conflicts! I think I could understand them well enough to resolve them, but I may be a poor hand at it.

j4james · 2023-05-15T21:09:28Z

Alas, there's some conflicts! I think I could understand them well enough to resolve them, but I may be a poor hand at it.

Yeah, I just saw that. I'll try and resolve that tonight.

@al20878

This PR introduces two new escapes sequences: `DECLRMM` (Left Right Margin Mode), which enables the horizontal margin support, and `DECSLRM` (Set Left and Right Margins), which does exactly what the name suggests. A whole lot of existing operations have then been updated to take those horizontal margins into account. ## Detailed Description of the Pull Request / Additional comments The main complication was in figuring out in what way each operation is affected, since there's a fair amount of variation. * When writing text to the buffer, we need to wrap within the horizontal margins, but only when the cursor is also within the vertical margins, otherwise we just wrap within the boundaries of the screen. * Not all cursor movement operations are constrained by the margins, but for those that are, we clamp within both the vertical and horizontal margins. But if the cursor is already outside the margins, it is just clamped at the edges of the screen. * The `ICH` and `DCH` operations are constrained by the horizontal margins, but only when inside the vertical margins. And if the cursor is outside the horizontal margins, these operations have no effect at all. * The rectangular area operations are clamped within the horizontal margins when in the origin mode, the same way they're clamped within the vertical margins. * The scrolling operations only scroll the area inside both horizontal and vertical margins. This includes the `IL` and `DL` operations, but they also won't have any effect at all unless the cursor is already inside the margin area. * `CR` returns to the left margin rather than the start of the line, unless the cursor is already left of that margin, or outside the vertical margins. * `LF`, `VT`, `FF`, and `IND` only trigger a scroll at the bottom margin if the cursor is already inside both vertical and horizontal margins. The same rules apply to `RI` when triggering a scroll at the top margin. Another thing worth noting is the change to the `ANSISYSSC` operation. Since that shares the same escape sequence as `DECSLRM`, they can't both be active at the same time. However, the latter is only meant to be active when `DECLRMM` is set, so by default `ANSISYSC` will still work, but it'll no longer apply once the `DECLRMM` mode is enabled. ## 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 extended some of our existing unit tests to cover at least the basic margin handling, although not all of the edge cases. Closes #14876 (cherry picked from commit b00b77a) Service-Card-Id: 89180228 Service-Version: 1.18

@al20878

This PR introduces four new escapes sequences: `DECIC` (Insert Column), `DECDC` (Delete Column), `DECBI` (Back Index), and `DECFI` (Forward Index), 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 `_ScrollRectHorizontally` method. ## 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

j4james added 20 commits April 3, 2023 02:04

Add helper methods for horizontal margins.

c4d5f4e

Hook up the DECSLRM sequence.

c375245

Add support for DECLRMM.

a062ab3

Update scroll operations to use left/right margins.

af83796

Clamp cursor ops within left/right margins.

9deb4c4

Support querying margins with DECRQSS.

e7d2246

Clamp PrintString within left/right margins.

37647ad

Clamp rect area ops within left/right margins.

8842efd

Account for left margin in DECOM cursor reports.

6821577

Only home cursor if margins are valid.

7df5171

Clamp linefeed ops within left/right margins.

3961eb9

Update delayed wrap test.

5422cbb

Update origin mode test.

0fc96c0

Update cursor save/restore tests.

751a64c

Update CNL/CPL tests.

b5e70b0

Add CUF/CUB tests.

d549862

Update scrolling margin tests.

5f5c5c7

Update NEL/IND tests.

df458ed

Update ICH/DCH tests.

5e8011a

Add test text to spelling dictionary.

358cc07

microsoft-github-policy-service bot added Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Area-VT Virtual Terminal sequence support Product-Terminal The new Windows Terminal. labels Apr 3, 2023

j4james marked this pull request as ready for review April 4, 2023 18:21

j4james mentioned this pull request Apr 4, 2023

Add support for horizontal scrolling sequences #15109

Closed

zadjii-msft added this to the Terminal v1.18 milestone Apr 4, 2023

zadjii-msft self-assigned this Apr 6, 2023

DHowett reviewed Apr 6, 2023

View reviewed changes

src/terminal/parser/ut_parser/OutputEngineTest.cpp Outdated Show resolved Hide resolved

DHowett added a commit that referenced this pull request Apr 6, 2023

PRE-MERGE #15084 Add support for horizontal margin sequences

7e2297a

DHowett added a commit that referenced this pull request Apr 14, 2023

PRE-MERGE #15084 Add support for horizontal margin sequences

fc85c0b

Add DECLRMM to DECRQM unit test.

1e641c4

DHowett requested review from lhecker and zadjii-msft April 24, 2023 22:28

DHowett reviewed Apr 24, 2023

View reviewed changes

carlos-zamora reviewed Apr 24, 2023

View reviewed changes

DHowett reviewed Apr 26, 2023

View reviewed changes

DHowett added a commit that referenced this pull request Apr 27, 2023

PRE-MERGE #15084 Add support for horizontal margin sequences

9fd6037

zadjii-msft added the zBugBash-Consider label May 9, 2023

PKRoma pushed a commit to PKRoma/Terminal that referenced this pull request May 12, 2023

PRE-MERGE microsoft#15084 Add support for horizontal margin sequences

1302738

lhecker approved these changes May 15, 2023

View reviewed changes

DHowett approved these changes May 15, 2023

View reviewed changes

Merge branch 'main' into feature-lrmargins

d384ff6

DHowett enabled auto-merge (squash) May 15, 2023 22:27

DHowett merged commit b00b77a into microsoft:main May 15, 2023

j4james mentioned this pull request May 16, 2023

Add support for horizontal scrolling sequences #15368

Merged

j4james deleted the feature-lrmargins branch May 30, 2023 00:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for horizontal margin sequences #15084

Add support for horizontal margin sequences #15084

j4james commented Apr 3, 2023 •

edited by DHowett

Loading

j4james commented Apr 4, 2023

DHowett left a comment

j4james commented Apr 23, 2023

DHowett commented Apr 24, 2023

DHowett left a comment

DHowett Apr 24, 2023

j4james Apr 24, 2023

carlos-zamora Apr 6, 2023

j4james Apr 24, 2023

carlos-zamora Apr 6, 2023

j4james Apr 24, 2023

carlos-zamora Apr 6, 2023

j4james Apr 25, 2023

carlos-zamora Apr 7, 2023

j4james Apr 25, 2023

DHowett Apr 26, 2023

DHowett Apr 26, 2023

j4james Apr 26, 2023

lhecker Apr 27, 2023

j4james Apr 27, 2023

lhecker Apr 27, 2023

DHowett Apr 26, 2023

j4james Apr 26, 2023

j4james Apr 26, 2023

j4james Apr 27, 2023

DHowett commented May 12, 2023

lhecker commented May 13, 2023 •

edited

Loading

DHowett left a comment

DHowett commented May 15, 2023

j4james commented May 15, 2023

		const auto saveCursor = L"\x1b\x37";
		const auto restoreCursor = L"\x1b\x38";

Add support for horizontal margin sequences #15084

Add support for horizontal margin sequences #15084

Conversation

j4james commented Apr 3, 2023 • edited by DHowett Loading

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

j4james commented Apr 4, 2023

DHowett left a comment

Choose a reason for hiding this comment

j4james commented Apr 23, 2023

DHowett commented Apr 24, 2023

DHowett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DHowett commented May 12, 2023

lhecker commented May 13, 2023 • edited Loading

Footnotes

DHowett left a comment

Choose a reason for hiding this comment

DHowett commented May 15, 2023

j4james commented May 15, 2023

j4james commented Apr 3, 2023 •

edited by DHowett

Loading

lhecker commented May 13, 2023 •

edited

Loading