From 1d2d412d67c0105d9b9015a48da706ce63f8b1f0 Mon Sep 17 00:00:00 2001 From: John Campion Jr Date: Mon, 31 Aug 2026 22:54:09 -0400 Subject: [PATCH] Stop attribute-only edits from cutting pictures, and gate the rectangle family Addresses the three findings on #151. A rendition change is not a text write, and DECCARA and DECRARA were making one. Both went 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 being overwritten. The parameter list is read once instead of 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 that 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. That is also what makes the no-op case free, which is the other half of the placement fix. 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. The rectangle family is VT400 and was ungated. xterm gates every one of them at vtXX_level >= 4 -- DECCRA, DECERA, DECFRA, DECSERA, DECCARA, DECRARA and DECRQCRA -- and this terminal's own primary DA already says the same thing by advertising 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 this gate: it asserts VT level 4 before it reads a single cell back through DECRQCRA. DECSACE is the one exception, and it is xterm's asymmetry rather than an oversight on this side: 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. DECRQTSR is gated with them. The control is VT320 vintage, 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. It was answering at every level. The placement test was checked against the bug: with SetCell put back it fails, which is how the FillCells regression above was caught before it shipped rather than after. 2209 passed, 0 failed. Co-Authored-By: Claude Opus 5 --- src/XTerm.NET.Tests/DeviceReportTests.cs | 27 +++ .../Graphics/ImageCellLifetimeTests.cs | 61 ++++++ src/XTerm.NET.Tests/RectangleOpsTests.cs | 56 +++++ src/XTerm.NET/InputHandler.Csi.cs | 15 ++ src/XTerm.NET/InputHandler.Rectangles.cs | 202 +++++++++++++----- 5 files changed, 308 insertions(+), 53 deletions(-) diff --git a/src/XTerm.NET.Tests/DeviceReportTests.cs b/src/XTerm.NET.Tests/DeviceReportTests.cs index b7d14f5..9805492 100644 --- a/src/XTerm.NET.Tests/DeviceReportTests.cs +++ b/src/XTerm.NET.Tests/DeviceReportTests.cs @@ -139,4 +139,31 @@ public void Decrqtsr_WithNoParameterAsksForNothing() terminal.Write(Esc + "[$u"); Assert.Empty(replies); } + + [Fact] + public void Decrqcra_IsSilentBelowLevel64() + { + var (terminal, replies) = Create(); + terminal.Write(Esc + "[64\"p"); + terminal.Write(Esc + "[1;0;1;1;1;1*y"); + Assert.Single(replies); // answered at VT400 + + replies.Clear(); + terminal.Write(Esc + "[62\"p"); // DECSCL: VT200 + terminal.Write(Esc + "[1;0;1;1;1;1*y"); + Assert.Empty(replies); + } + + [Fact] + public void Decrqtsr_IsSilentBelowLevel64() + { + // The control is VT320 vintage, but the capability the primary DA offers for it -- + // attribute 17, terminal state interrogation -- is advertised only from level 64. + // Declining a request the DA reply has already said the terminal does not take is the + // terminal contradicting itself. + var (terminal, replies) = Create(); + terminal.Write(Esc + "[62\"p"); + terminal.Write(Esc + "[1$u"); + Assert.Empty(replies); + } } diff --git a/src/XTerm.NET.Tests/Graphics/ImageCellLifetimeTests.cs b/src/XTerm.NET.Tests/Graphics/ImageCellLifetimeTests.cs index 752a1b7..9d6ccea 100644 --- a/src/XTerm.NET.Tests/Graphics/ImageCellLifetimeTests.cs +++ b/src/XTerm.NET.Tests/Graphics/ImageCellLifetimeTests.cs @@ -386,4 +386,65 @@ public void The_alternate_buffer_keeps_its_own_images() terminal.Write($"{Esc}[?1049l"); // and back Assert.Equal(8, ImageCellCount(terminal)); } + + /// + /// The counterpart to every test above: a control that changes RENDITION and not text must + /// leave the picture alone. + /// + /// + /// DECCARA sets attributes over an area and never touches a character, so a picture inside that + /// area is not being overwritten and must not be split. Going through the ordinary text-write + /// path made it look like one -- that path splits the placement at the column being written, + /// which is right for printing over a picture and wrong for recolouring the cell under it. + /// + [Fact] + public void An_attribute_change_over_a_picture_leaves_it_whole() + { + var terminal = Fresh(); + WriteSixel(terminal); + var image = ImageAssertions.ImageAt(terminal, 0, 0); + Assert.NotNull(image); + Assert.Equal(8, ImageCellCount(terminal)); + + // Bold over the whole picture and then some. + terminal.Write($"{Esc}[2*x{Esc}[1;1;4;4;1$r"); + + Assert.Equal(8, ImageCellCount(terminal)); + Assert.True(ReferenceEquals(ImageAssertions.ImageAt(terminal, 0, 0), image)); + Assert.True(ReferenceEquals(ImageAssertions.ImageAt(terminal, 1, 3), image)); + Assert.True(terminal.Buffer.Lines[terminal.Buffer.YBase]![0].Attributes.IsBold()); + } + + /// + /// And a request that changes nothing must not even write the cells back, because writing a + /// cell back unchanged is still a write as far as the placements are concerned. + /// + [Fact] + public void An_attribute_change_naming_nothing_we_implement_touches_no_cell() + { + var terminal = Fresh(); + WriteSixel(terminal); + var image = ImageAssertions.ImageAt(terminal, 0, 0); + Assert.Equal(8, ImageCellCount(terminal)); + + // 31 is a colour, which DECCARA does not carry; the request names nothing this implements. + terminal.Write($"{Esc}[2*x{Esc}[1;1;4;4;31$r"); + + Assert.Equal(8, ImageCellCount(terminal)); + Assert.True(ReferenceEquals(ImageAssertions.ImageAt(terminal, 0, 0), image)); + } + + /// A DECRARA whose toggles cancel each other is the same nothing. + [Fact] + public void Toggles_that_cancel_leave_the_picture_and_the_rendition_alone() + { + var terminal = Fresh(); + WriteSixel(terminal); + Assert.Equal(8, ImageCellCount(terminal)); + + terminal.Write($"{Esc}[2*x{Esc}[1;1;4;4;1;1$t"); + + Assert.Equal(8, ImageCellCount(terminal)); + Assert.False(terminal.Buffer.Lines[terminal.Buffer.YBase]![0].Attributes.IsBold()); + } } diff --git a/src/XTerm.NET.Tests/RectangleOpsTests.cs b/src/XTerm.NET.Tests/RectangleOpsTests.cs index e4e1618..b6f2d3e 100644 --- a/src/XTerm.NET.Tests/RectangleOpsTests.cs +++ b/src/XTerm.NET.Tests/RectangleOpsTests.cs @@ -301,4 +301,60 @@ public void Decsace_survives_a_soft_reset_and_not_a_hard_one() terminal.Write($"{Esc}[2;3;3;6;1$r"); Assert.True(AttrAt(terminal, 1, 9).IsBold()); // back to a stream } + + /// + /// The whole family is VT400, which is the gate xterm puts on each of them and what this + /// terminal's own primary DA already says by advertising attribute 28 only from level 64. + /// A program that lowered the level with DECSCL asked to be treated as older hardware. + /// + [Theory] + [InlineData("[42;1;1;2;4$x")] // DECFRA + [InlineData("[1;1;2;4$z")] // DECERA + [InlineData("[1;1;2;4${")] // DECSERA + [InlineData("[1;1;1;4;2;1$v")] // DECCRA + [InlineData("[1;1;2;4;1$r")] // DECCARA + [InlineData("[1;1;2;4;1$t")] // DECRARA + public void The_rectangle_family_is_refused_below_level_64(string sequence) + { + var terminal = NewTerminal(); + terminal.Write("abcdefghij"); + var before = Row(terminal, 0, 10); + var boldBefore = AttrAt(terminal, 0, 0).IsBold(); + + terminal.Write($"{Esc}[62\"p"); // DECSCL: VT200 + terminal.Write($"{Esc}{sequence}"); + + Assert.Equal(before, Row(terminal, 0, 10)); + Assert.Equal(boldBefore, AttrAt(terminal, 0, 0).IsBold()); + } + + [Fact] + public void The_rectangle_family_works_again_at_level_64() + { + var terminal = NewTerminal(); + terminal.Write($"{Esc}[62\"p{Esc}[64\"p"); // down to VT200 and back up to VT400 + terminal.Write($"{Esc}[42;1;1;1;3$x"); + + Assert.Equal("*** ", Row(terminal, 0, 10)); + } + + /// + /// DECSACE is the family's one exception, and it is xterm's asymmetry rather than an oversight: + /// its handler has no level test where every neighbour has one. Storing which extent a program + /// would prefer changes nothing by itself -- the two controls that read it are gated -- so + /// there is nothing to refuse. + /// + [Fact] + public void Decsace_is_stored_at_every_level() + { + var terminal = NewTerminal(); + var replies = new List(); + terminal.DataReceived += (_, e) => replies.Add(e.Data); + + terminal.Write($"{Esc}[62\"p"); // VT200 + terminal.Write($"{Esc}[2*x"); // DECSACE 2 + terminal.Write($"{Esc}P$q*x{Esc}\\"); // DECRQSS + + Assert.Equal($"{Esc}P1$r2*x{Esc}\\", Assert.Single(replies)); + } } diff --git a/src/XTerm.NET/InputHandler.Csi.cs b/src/XTerm.NET/InputHandler.Csi.cs index 3689f8e..13994fe 100644 --- a/src/XTerm.NET/InputHandler.Csi.cs +++ b/src/XTerm.NET/InputHandler.Csi.cs @@ -828,6 +828,15 @@ private void RequestUserPreferredSupplementalSet() /// private void RequestTerminalStateReport(Params parameters) { + // The refusal is still a report, and this terminal's primary DA offers attribute 17, + // terminal state interrogation, only from level 64. Answering below that would have the + // terminal declining a request it had already said it does not take -- so a program that + // lowered the level with DECSCL gets the same silence every other unavailable control + // gives it. The control itself is VT320 vintage; the capability it belongs to is what the + // DA reply gates, and matching the reply is what keeps the two from disagreeing. + if (_terminal.ConformanceLevel < 64) + return; + if (parameters.GetParam(0, 0) == 0) return; @@ -1744,6 +1753,12 @@ private void RestoreCursor() /// private void RequestChecksumRectangularArea(Params parameters) { + // VT400 and up, with the rest of the rectangle family. esctest asserts the level before it + // reads a single cell back through this -- AssertVTLevel(4, "checksum") -- so the gate is + // one the conformance suite already assumes is here. + if (!RectangularEditingAvailable) + return; + var id = parameters.GetParam(0, 0); // parameters[1] is the page, ignored. Coordinates are read in the ORIGIN MODE system, // like a cursor address and like every rectangle operation: a program that addresses its diff --git a/src/XTerm.NET/InputHandler.Rectangles.cs b/src/XTerm.NET/InputHandler.Rectangles.cs index 22e4a48..c7aede8 100644 --- a/src/XTerm.NET/InputHandler.Rectangles.cs +++ b/src/XTerm.NET/InputHandler.Rectangles.cs @@ -44,6 +44,24 @@ private bool TryReadRectangle(Params parameters, int first, return top >= 0 && left >= 0 && top <= bottom && left <= right; } + /// + /// Whether the DEC rectangular-editing controls are available at the current operating level. + /// + /// + /// VT400 and up, which is the gate xterm puts on every one of them -- + /// screen->vtXX_level >= 4 on DECCRA, DECERA, DECFRA, DECSERA, DECCARA, DECRARA + /// and DECRQCRA alike. It is also what this terminal's own primary DA already says: attribute + /// 28, rectangular editing, is advertised 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 specifically to be treated as older hardware has asked not to + /// be given them. + /// DECSACE is deliberately NOT gated, which is xterm's asymmetry rather than an oversight + /// on this side: its handler has no level test where every neighbour does. Storing which extent + /// a program would prefer costs nothing and changes nothing on its own -- the two controls that + /// read it are gated here, so a stored preference below level 64 simply never gets used. + /// + private bool RectangularEditingAvailable => _terminal.ConformanceLevel >= 64; + /// DECFRA -- fills the rectangle with one character, in the CURRENT rendition. /// /// The character must be printable -- xterm accepts 32..126 and 160 up -- and an @@ -52,6 +70,9 @@ private bool TryReadRectangle(Params parameters, int first, /// private void FillRectangularArea(Params parameters) { + if (!RectangularEditingAvailable) + return; + var ch = parameters.GetParam(0, 0); if (ch < 32 || (ch > 126 && ch < 160)) return; @@ -65,6 +86,8 @@ private void FillRectangularArea(Params parameters) /// DECERA -- erases the rectangle to blanks, with the erase attributes. private void EraseRectangularArea(Params parameters) { + if (!RectangularEditingAvailable) + return; if (!TryReadRectangle(parameters, 0, out var top, out var left, out var bottom, out var right)) return; @@ -79,6 +102,8 @@ private void EraseRectangularArea(Params parameters) /// private void SelectiveEraseRectangularArea(Params parameters) { + if (!RectangularEditingAvailable) + return; if (!TryReadRectangle(parameters, 0, out var top, out var left, out var bottom, out var right)) return; @@ -124,6 +149,8 @@ private void FillCells(int top, int left, int bottom, int right, ref BufferCell /// private void CopyRectangularArea(Params parameters) { + if (!RectangularEditingAvailable) + return; if (!TryReadRectangle(parameters, 0, out var top, out var left, out var bottom, out var right)) return; @@ -171,15 +198,21 @@ private void CopyRectangularArea(Params parameters) } } + /// + /// What the parameter list asks of one attribute. Toggling twice is the same as not asking, + /// which is why this composes rather than accumulating a list. + /// + private enum AreaAttributeOp : byte { None, Set, Clear, Toggle } + + /// The five attributes DECCARA and DECRARA can name, in the order the ops are held. + private const int AreaBold = 0, AreaUnderline = 1, AreaBlink = 2, AreaInverse = 3, AreaInvisible = 4; + /// /// DECCARA (CSI Pt;Pl;Pb;Pr;Pm $ r) and DECRARA (CSI Pt;Pl;Pb;Pr;Pm $ t) -- set /// or toggle the named SGR attributes over an area, leaving the characters alone. /// /// - /// The attribute half of the rectangle family, and the only consumer DECSACE has. That - /// setting was parsed, stored and read back by DECRQSS while nothing acted on it, because the - /// two controls it governs did not exist: a terminal reporting a rectangle-or-stream choice it - /// then ignored. + /// The attribute half of the rectangle family, and the only consumer DECSACE has. /// DECSACE 2 means the RECTANGLE the four coordinates describe. Anything else -- the /// default included -- means the STREAM running from the top-left position to the bottom-right /// one, so the first row runs from its column to the end of the line, the last row from the @@ -190,6 +223,18 @@ private void CopyRectangularArea(Params parameters) /// and reverses rather than clears them under DECRARA. Everything else in the list is ignored; /// colours are not in the standard, and honouring an SGR parameter here that a real VT420 would /// not is how a program's careful rectangle ends up recoloured on one terminal only. + /// The list is read ONCE, into one op per attribute, rather than re-walked for every + /// cell: the answer cannot vary across the area, and a full-screen request asked the same + /// question a parameter at a time for every one of its cells. Reading it first is also what + /// makes the next paragraph possible. + /// A request that changes nothing -- CSI 1;1;1;10;31 $ r, naming only a colour + /// this does not implement, or a DECRARA whose toggles cancel -- returns before a cell is + /// touched. That is NOT an optimisation. Writing a cell back unchanged still counts as writing + /// it, and the write path splits any Sixel or Kitty placement covering that column, on the + /// reasonable assumption that a cell being written is a cell whose character is changing. Here + /// it never is: these two controls change rendition and nothing else, so the cells that DO + /// change go back through the INDEXER, which stores the cell and invalidates the render cache + /// without disturbing the picture over it. /// Every cell in the area is marked, the trailing half of a wide character included. xterm /// skips cells it has never drawn -- it tracks that per cell, and a blank it has never touched /// is not a blank it will colour -- but a line here is born full of spaces, so there is no such @@ -198,6 +243,14 @@ private void CopyRectangularArea(Params parameters) /// private void MarkRectangularArea(Params parameters, bool reverse) { + // VT400 and up, with the rest of the family; see RectangularEditingAvailable. + if (!RectangularEditingAvailable) + return; + + Span ops = stackalloc AreaAttributeOp[5]; + if (!ReadAreaAttributeOps(parameters, 4, ops, reverse)) + return; + if (!TryReadRectangle(parameters, 0, out var top, out var left, out var bottom, out var right)) return; @@ -215,71 +268,114 @@ private void MarkRectangularArea(Params parameters, bool reverse) for (var col = from; col <= to && col < line.Length; col++) { var cell = line[col]; - ApplyAreaAttributes(parameters, 4, ref cell.Attributes, reverse); - line.SetCell(col, ref cell); + ApplyAreaAttributeOps(ops, ref cell.Attributes); + + // The indexer, NOT SetCell: see the remarks. SetCell is the text-write path and + // splits this line's placements, so a rendition change over a picture would punch + // a hole in it. + line[col] = cell; } } } /// - /// Applies the DECCARA/DECRARA attribute list starting at to one - /// cell's rendition. + /// Reads the DECCARA/DECRARA parameter list into one operation per attribute. /// - private static void ApplyAreaAttributes(Params parameters, int first, ref AttributeData attributes, bool reverse) + /// + /// Composing rather than appending is what keeps one pass faithful to the list's order: a later + /// parameter overrides an earlier one for the same attribute, 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. + /// + /// False when the list would change nothing, so the caller can touch no cells at all. + private static bool ReadAreaAttributeOps(Params parameters, int first, Span ops, bool reverse) { + var on = reverse ? AreaAttributeOp.Toggle : AreaAttributeOp.Set; + var off = reverse ? AreaAttributeOp.Toggle : AreaAttributeOp.Clear; + for (var i = first; i < parameters.Length; i++) { switch (parameters.GetParam(i, 0)) { case 0: - if (reverse) - { - attributes.SetBold(!attributes.IsBold()); - attributes.SetUnderline(!attributes.IsUnderline()); - attributes.SetBlink(!attributes.IsBlink()); - attributes.SetInverse(!attributes.IsInverse()); - } - else - { - attributes.SetBold(false); - attributes.SetUnderline(false); - attributes.SetBlink(false); - attributes.SetInverse(false); - } - break; - case 1: - attributes.SetBold(reverse ? !attributes.IsBold() : true); - break; - case 4: - attributes.SetUnderline(reverse ? !attributes.IsUnderline() : true); - break; - case 5: - attributes.SetBlink(reverse ? !attributes.IsBlink() : true); - break; - case 7: - attributes.SetInverse(reverse ? !attributes.IsInverse() : true); - break; - case 8: - attributes.SetInvisible(reverse ? !attributes.IsInvisible() : true); + // xterm's SGR_MASK: bold, underline, blink and inverse -- not invisible, which + // has its own 8 and 28. + Note(ops, AreaBold, off); + Note(ops, AreaUnderline, off); + Note(ops, AreaBlink, off); + Note(ops, AreaInverse, off); break; + case 1: Note(ops, AreaBold, on); break; + case 4: Note(ops, AreaUnderline, on); break; + case 5: Note(ops, AreaBlink, on); break; + case 7: Note(ops, AreaInverse, on); break; + case 8: Note(ops, AreaInvisible, on); break; // The resets have no meaning under DECRARA -- reversing an attribute already says // both directions -- so xterm reads them only when setting, and so does this. - case 22 when !reverse: - attributes.SetBold(false); - break; - case 24 when !reverse: - attributes.SetUnderline(false); - break; - case 25 when !reverse: - attributes.SetBlink(false); - break; - case 27 when !reverse: - attributes.SetInverse(false); - break; - case 28 when !reverse: - attributes.SetInvisible(false); - break; + case 22 when !reverse: Note(ops, AreaBold, AreaAttributeOp.Clear); break; + case 24 when !reverse: Note(ops, AreaUnderline, AreaAttributeOp.Clear); break; + case 25 when !reverse: Note(ops, AreaBlink, AreaAttributeOp.Clear); break; + case 27 when !reverse: Note(ops, AreaInverse, AreaAttributeOp.Clear); break; + case 28 when !reverse: Note(ops, AreaInvisible, AreaAttributeOp.Clear); break; } } + + foreach (var op in ops) + { + if (op != AreaAttributeOp.None) + return true; + } + + return false; + } + + /// Folds one parameter's request into what is already asked of that attribute. + private static void Note(Span ops, int attribute, AreaAttributeOp op) => + ops[attribute] = op is not AreaAttributeOp.Toggle + ? op + : ops[attribute] switch + { + AreaAttributeOp.None => AreaAttributeOp.Toggle, + AreaAttributeOp.Toggle => AreaAttributeOp.None, + AreaAttributeOp.Set => AreaAttributeOp.Clear, + _ => AreaAttributeOp.Set, + }; + + /// Applies the ops read by to one cell's rendition. + private static void ApplyAreaAttributeOps(ReadOnlySpan ops, ref AttributeData attributes) + { + switch (ops[AreaBold]) + { + case AreaAttributeOp.Set: attributes.SetBold(true); break; + case AreaAttributeOp.Clear: attributes.SetBold(false); break; + case AreaAttributeOp.Toggle: attributes.SetBold(!attributes.IsBold()); break; + } + + switch (ops[AreaUnderline]) + { + case AreaAttributeOp.Set: attributes.SetUnderline(true); break; + case AreaAttributeOp.Clear: attributes.SetUnderline(false); break; + case AreaAttributeOp.Toggle: attributes.SetUnderline(!attributes.IsUnderline()); break; + } + + switch (ops[AreaBlink]) + { + case AreaAttributeOp.Set: attributes.SetBlink(true); break; + case AreaAttributeOp.Clear: attributes.SetBlink(false); break; + case AreaAttributeOp.Toggle: attributes.SetBlink(!attributes.IsBlink()); break; + } + + switch (ops[AreaInverse]) + { + case AreaAttributeOp.Set: attributes.SetInverse(true); break; + case AreaAttributeOp.Clear: attributes.SetInverse(false); break; + case AreaAttributeOp.Toggle: attributes.SetInverse(!attributes.IsInverse()); break; + } + + switch (ops[AreaInvisible]) + { + case AreaAttributeOp.Set: attributes.SetInvisible(true); break; + case AreaAttributeOp.Clear: attributes.SetInvisible(false); break; + case AreaAttributeOp.Toggle: attributes.SetInvisible(!attributes.IsInvisible()); break; + } } }