Implement DECSLRM, left and right margins (CSI Pl ; Pr s)

[JohnCampionJr]

An application scrolling one column of a side-by-side layout scrolls the whole screen without this. It is the second of the two VT gaps on the roadmap — and unlike REP in #44 (now merged), it is not a small one. Rebased onto that merge and conflict-free; Copilot's two findings are fixed and their threads resolved. Margins are not a parameter on the existing scroll; they are a second scroll.

CSI s is two sequences

With DECLRMM (mode 69) set it is DECSLRM; without it, Save Cursor. So this changes the meaning of a sequence already handled rather than adding one, and getting it backwards would make an application's margins silently save the cursor.

Why a half implementation would be worse than none

An application sets mode 69, asks DECRQM whether it took, and changes its drawing on the answer. Reporting the mode as supported while scrolling ignored the margins would corrupt exactly the layouts the feature exists for — quietly, and only in the applications careful enough to ask.

So every operation that moves content honours them:

• Scrolling. ScrollUp/ScrollDown move whole lines through the ring, which is what feeds the scrollback and what line recycling depends on. Narrowed margins cannot use that path at all — only part of each line moves, and nothing is promoted to scrollback — so ScrollMarginColumns is a separate implementation. The choice is made inside ScrollUp, because every wrap, LF and IND arrives through it and finding all of them once means not finding them again next time.
• Autowrap, in all three print paths.
• IL and DL, which shift only the margin columns and do nothing from a cursor outside them — a cursor in the right-hand pane shifting the left pane's lines is the corruption this prevents.
• ICH and DCH, bounded by the right margin, so nothing is pushed into the next pane and the blanks appear at the margin rather than the screen edge.
• Origin mode, where the region is a box, so column 1 is the left margin.
• Resize, which clamps the margins and widens them if the pair would go degenerate.
• RIS and DECSTR, which widen them.

Three things nearly leaked past the margin

Two are the same shape as bugs found earlier in this work — a batched writer bypassing a rule the per-character path follows:

• The run writers clamped take to Cols, so a batch wrote straight through the margin — and only when the fast path took the write.
• A double-width character on the last column of a region planted its spacer in the pane next door.

The third only a test found. The cursor is allowed to rest one past the last column it wrote — that is how a pending wrap is represented. Reading that column as outside the margins handed the wrap limit back to the screen edge at exactly the moment the wrap was due, so the text ran through the margin anyway. It looked right in every code path and was wrong in the one state that matters.

CR returns to the left margin

CR follows xterm's rule (CarriageReturn in charproc.c): the left margin when the cursor is at or right of it, column 0 only when the cursor is left of it, origin mode not consulted. One method carries the rule and everything that "returns the carriage" routes through it — CR, NEL, CNL/CPL, and line feed under ConvertEol — so a CRLF inside a pane starts the next line at the pane's edge rather than in the pane next door.

BCE agrees on both paths

The fills line up whichever path a sequence takes: full-width IL/DL blank with GetBlankLine(_curAttr) and the box path with BlankCell() (also _curAttr); full-width scrolling recycles with AttributeData.Default and the box path fills with BlankFill() (also default). Same sequence, same fill, either path — stated here because it is the kind of pairing that usually diverges silently.

Cost

Measured against main with the harness from #42, at 400M characters per corpus so the noise floor is ±1%:

corpus	main	this	Δ	noise
scroll-ascii	1.67	1.67	+0.5%	±1%
sgr-churn	4.47	4.35	-2.7%	±5%
truecolor	4.58	4.52	-1.3%	±3%
alt-redraw	5.47	5.49	+0.4%	±2%
unicode	12.57	12.55	-0.2%	±2%
flood	40.73	40.31	-1.1%	±1%

Allocation byte-identical throughout.

A first pass at a quarter of that work read +4.7% on scroll-ascii against ±5% noise — which is precisely the band the CI gate marks "worth a look" rather than failing, and precisely why it does. Four times the work and it was +0.5%.

Not here, deliberately

DECIC and DECDC — insert and delete column. They are separate sequences that exist alongside margins rather than part of them, and this is already the invasive change. Happy to follow up.

Also unchanged: a double-width character at the screen edge still drops its spacer rather than wrapping to the next line. That is pre-existing and orthogonal — I bounded the spacer by the margin without touching what happens at the edge.

Tests

1177 pass, twenty of them new — the mode, the sequence, the query, and one per operation above. The batched-path clamp was verified by removing it and watching three tests fail.

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

The margin-column scrolling path does not currently preserve/clear BufferLine.IsWrapped consistently, which can break wrap-chain semantics used by selection/reflow.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds DEC left/right margin support (DECLRMM/DECSLRM) to the terminal core so side-by-side/pane layouts can scroll and edit within a column-bounded region without corrupting adjacent panes.

Changes:

• Introduces DECLRMM (mode 69) and interprets CSI … s as DECSLRM while the mode is set.
• Implements margin-aware scrolling/wrapping and bounds key editing operations (IL/DL/ICH/DCH) to the active region semantics.
• Adds a focused test suite covering mode behavior, queries, wrapping/scrolling/editing interactions, and resize/reset behavior.

File summaries

File	Description
src/XTerm.NET/Terminal.cs	Adds LeftRightMarginMode state and ensures it resets correctly.
src/XTerm.NET/InputHandler.cs	Routes CSI s based on DECLRMM, applies origin-mode column addressing, and bounds wrap/edit/IL/DL behavior to margins.
src/XTerm.NET/Common/TerminalMode.cs	Adds TerminalMode.LeftRightMargin = 69 with documentation.
src/XTerm.NET/Buffer/TerminalBuffer.cs	Stores left/right margins and implements margin-column scrolling path.
src/XTerm.NET.Tests/LeftRightMarginTests.cs	Adds coverage for DECLRMM/DECSLRM behavior and all key operations affected by margins.

Review details

Suppressed comments (2)

src/XTerm.NET/Buffer/TerminalBuffer.cs:489

• ScrollMarginColumns moves the margin region by copying cells between existing BufferLine instances, but it doesn't copy the associated IsWrapped metadata. That means wrap-chain state can become detached from the text content after a margin scroll / IL / DL, leading to incorrect reflow/selection behavior. Copy the wrap flag alongside the cells.

        var to = _lines[_yBase + toRow];
        if (from is null || to is null)
            return;

        to.CopyCellsFrom(from, left, left, width, false);

src/XTerm.NET/Buffer/TerminalBuffer.cs:496

• FillMarginColumns blanks the margin region in-place but leaves the line's IsWrapped flag unchanged. For scrolled-in blank areas (and for IL/DL-created blanks), the destination lines should not remain marked as wrapped continuations from whatever previously occupied that row.

    private void FillMarginColumns(int row, int left, int width, BufferCell fill)
    {
        var line = _lines[_yBase + row];
        line?.Fill(fill, left, left + width);
    }

• Files reviewed: 5/5 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[tomlm]

Reviewed for misaligned doc comments and medium/high impact issues. The structure is right — margins as a second scroll rather than a parameter, the choice made inside ScrollUp, DECLRMM gating CSI s, DECRQM answering for 69, and the three leaks called out in the description are all real and correctly fixed. Six findings, three of them high.

High

• InsideScrollRegion() also gates IL and DL on the no-margins path, where the cursor legitimately rests at X == Cols in the pending-wrap state. IL/DL after a full-width line silently stop working for everyone, margins or not.
• ICH and DCH are bounded by the right margin but not the left, so running them from a cursor left of the left margin shifts content straight across it into the next pane — the corruption InsideScrollRegion() was added to prevent for IL/DL.
• WrapLimit()/WrapHome() cannot distinguish "pending wrap at the right margin" from "cursor positioned at the column just right of the margin", because both are ScrollRight + 1. A write at the first column of the next pane is wrapped back into this one.

Medium

• CUF/CUB/CNL/CPL still clamp to the screen edge, so CSI 200 C walks out of the region.
• ScrollUp's <summary> still says it matches xterm.js Buffer.scroll(), which is now false for half the method, and the newly-conditional isWrapped has no <param>.
• ScrollMarginColumns is public and takes top/bottom as parameters but reads its columns from private margin state, which the doc does not mention.

Notes (checked, no action)

• The BCE attributes line up: full-width IL/DL use GetBlankLine(_curAttr) and the margin path uses BlankCell() (also _curAttr); full-width ScrollUp recycles with AttributeData.Default and the margin path uses BlankFill() (also default). Same sequence, same fill, either path — worth stating in the description since it is the kind of thing that usually diverges.
• ScrollMarginColumns copies in the direction of travel on both arms; a region taller than the distance moved does not clobber itself. The <remarks> claim holds.
• The Resize margin clamp survives the degenerate cases: newCols == 1 collapses to left == right == 0, which MarginsAreFullWidth then reads as full width rather than leaving a region nothing can write to.
• The XML docs on the new members in TerminalBuffer.cs are all attached to the right members — no orphaned <remarks> or duplicated <summary> anywhere in the diff. The two doc problems below are content drift, not misplacement.
• "either leaving margins in force across a full reset would hand the next application..." in ResetScrollRegion is missing a clause — a stray either from an edit.