Stop attribute-only edits from cutting pictures, and gate the rectangle family

[JohnCampionJr]

Addresses all three findings from the Copilot review on #151. 2209 passed, 0 failed.

1. Attribute-only edits were cutting pictures — the real bug

DECCARA and DECRARA wrote through SetCell, which is the text-write path: it splits the Sixel and Kitty placements at the column being written, on the reasonable assumption that a cell being written is a cell whose character is changing. For these two it never is — they set attributes and leave every character alone — so bolding a region that happened to contain a picture punched a hole in it.

The cells that change now go back through the indexer, which stores the cell and invalidates the render cache without touching the placements. FillCells still uses SetCell, deliberately: DECFRA and DECERA do replace characters, and a picture under them is genuinely being overwritten.

The no-op case is the other half of it. A request naming nothing this implements — CSI 1;1;1;10;31$r, a colour — or a DECRARA whose toggles cancel now returns before a cell is touched. Writing a cell back unchanged is still writing it as far as the placements are concerned, so a control that did nothing at all was carving up images.

That falls out of reading the parameter list once rather than per cell. It was re-walked for every cell in the area, asking a question whose answer cannot vary across it — a full-screen request re-read the whole list a parameter at a time, once per cell. It now folds into one operation per attribute before the walk starts, composing rather than appending so one pass stays faithful to the list's order: a later parameter overrides an earlier one, and a toggle applied to a pending toggle cancels it, exactly as xterm's per-cell XOR does when the same bit is named twice.

2. The rectangle family is VT400 and was ungated

The review flagged this on DECCARA/DECRARA. Gating only those two would have been worse than leaving it alone — CSI…$v would work at VT200 while CSI…$r didn't, both advertised by the same DA attribute. So the gate goes on the whole family, which is what xterm does: vtXX_level >= 4 on DECCRA, DECERA, DECFRA, DECSERA, DECCARA, DECRARA and DECRQCRA.

It is also what this terminal already says about itself — the primary DA advertises attribute 28, rectangular editing, only from level 64. Acting on the controls at a level where the DA reply denies them is the terminal contradicting itself, and a program that lowered the level with DECSCL asked to be treated as older hardware. esctest already assumes the gate: it asserts VT level 4 before reading a single cell back through DECRQCRA.

DECSACE is the one exception, and it is xterm's asymmetry rather than an oversight here: its handler has no level test where every neighbour has one. Storing which extent a program would prefer changes nothing by itself, since the two controls that read it are gated, so there is nothing to refuse. Pinned by a test.

Five of these seven controls were ungated before #151 — the review found the gap through the two controls I added.

3. DECRQTSR gating

Taken, with one correction to the finding: it is a VT320 control, not VT420 — vttest files it under VT320 reports. But the capability the primary DA offers for it, attribute 17 (terminal state interrogation), is advertised only from level 64, and declining a request the DA reply has already said the terminal does not take is the same contradiction one report over. Gated at 64 to match what we advertise.

Note on the tests

The placement test was checked against the bug — with SetCell put back it fails. That is how a regression in this very change got caught before it shipped: a global search-and-replace had also converted FillCells, which would have stopped DECFRA and DECERA from splitting placements they should split.

🤖 Generated with Claude Code