fix(ansi): drop committed shadow when a superseded draw is committed
A PreparedDraw superseded by a later Prepare cannot be applied (its snapshot buffer was reused), but its bytes may already have been written to the terminal. Commit now clears hasCommitted so the next Prepare emits a full snapshot instead of diffing against a shadow that may no longer match. Documented on Commit; tested by replaying outputs through a VT screen. Also moves test-only setCommittedFrame/committedFrame/replaceFrame helpers into a _test.go file and fixes the misplaced compactFrames doc comment.
- commit
- 748182f5cb3bf1f7ea4ceb2fab0ec6dcb3fb59dc
- author
- brice · · 2026-10-02 09:47 UTC
- parent
- 9d3b2c7
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
5 files · +83 -21
M ansi/delta_plan.go +0 -6
| @@ -234,9 +234,3 @@ func (c DeltaCandidate) Commit(dst *Frame) { | |||
| 234 | 234 | } | |
| 235 | 235 | } | |
| 236 | 236 | } | |
| 237 | - | ||
| 238 | - | // replaceFrame overwrites dst's own storage with a copy of src, reusing | |
| 239 | - | // capacity. It is for frames the renderer owns exclusively. | |
| 240 | - | func replaceFrame(dst *Frame, src Frame) { | |
| 241 | - | dst.CopyFrom(src) | |
| 242 | - | } | |
| @@ -234,9 +234,3 @@ func (c DeltaCandidate) Commit(dst *Frame) { | |||||
| 234 | } | 234 | } | ||
| 235 | } | 235 | } | ||
| 236 | } | 236 | } | ||
| 237 | - | ||||
| 238 | - | // replaceFrame overwrites dst's own storage with a copy of src, reusing | |||
| 239 | - | // capacity. It is for frames the renderer owns exclusively. | |||
| 240 | - | func replaceFrame(dst *Frame, src Frame) { | |||
| 241 | - | dst.CopyFrom(src) | |||
| 242 | - | } | |||
M ansi/frame.go +3 -2
| @@ -68,10 +68,11 @@ func snapshotCellSource(scratch *Frame, source CellSource) Frame { | |||
| 68 | 68 | return *scratch | |
| 69 | 69 | } | |
| 70 | 70 | ||
| 71 | - | // compactFrames reports whether both sources are core.Frame values (directly or | |
| 72 | - | // by pointer), enabling stored-cell comparison without the CellSource interface. | |
| 71 | + | // framePair holds two compact frames compared through core.RowsEqual. | |
| 73 | 72 | type framePair struct{ a, b Frame } | |
| 74 | 73 | ||
| 74 | + | // compactFrames reports whether both sources are core.Frame values (directly or | |
| 75 | + | // by pointer), enabling stored-cell comparison without the CellSource interface. | |
| 75 | 76 | func compactFrames(a, b CellSource) (framePair, bool) { | |
| 76 | 77 | fa, ok := asFrame(a) | |
| 77 | 78 | if !ok { | |
| @@ -68,10 +68,11 @@ func snapshotCellSource(scratch *Frame, source CellSource) Frame { | |||||
| 68 | return *scratch | 68 | return *scratch | ||
| 69 | } | 69 | } | ||
| 70 | 70 | ||||
| 71 | - | // compactFrames reports whether both sources are core.Frame values (directly or | 71 | + | // framePair holds two compact frames compared through core.RowsEqual. |
| 72 | - | // by pointer), enabling stored-cell comparison without the CellSource interface. | |||
| 73 | type framePair struct{ a, b Frame } | 72 | type framePair struct{ a, b Frame } | ||
| 74 | 73 | ||||
| 74 | + | // compactFrames reports whether both sources are core.Frame values (directly or | |||
| 75 | + | // by pointer), enabling stored-cell comparison without the CellSource interface. | |||
| 75 | func compactFrames(a, b CellSource) (framePair, bool) { | 76 | func compactFrames(a, b CellSource) (framePair, bool) { | ||
| 76 | fa, ok := asFrame(a) | 77 | fa, ok := asFrame(a) | ||
| 77 | if !ok { | 78 | if !ok { | ||
M ansi/renderer.go +11 -13
| @@ -64,6 +64,11 @@ func (p PreparedDraw) Bytes() []byte { return p.data } | |||
| 64 | 64 | ||
| 65 | 65 | // Commit applies the prepared delta exactly once. Discarding it leaves the | |
| 66 | 66 | // renderer's committed state unchanged. | |
| 67 | + | // | |
| 68 | + | // Committing a draw that a later Prepare has superseded does not apply it: | |
| 69 | + | // the renderer instead drops its committed state, so the next Prepare | |
| 70 | + | // re-emits a full snapshot instead of diffing against a shadow that may no | |
| 71 | + | // longer match the terminal. Draws without output are unaffected. | |
| 67 | 72 | func (p *PreparedDraw) Commit() { | |
| 68 | 73 | if p == nil || p.renderer == nil || p.commitOnce == nil { | |
| 69 | 74 | return | |
| @@ -76,7 +81,11 @@ func (p *PreparedDraw) Commit() { | |||
| 76 | 81 | } | |
| 77 | 82 | if p.generation != r.generation { | |
| 78 | 83 | // A later Prepare reused the scratch frame this draw's snapshot | |
| 79 | - | // lives in, so it was discarded. | |
| 84 | + | // lives in, so it cannot be applied. Its bytes may nevertheless | |
| 85 | + | // have reached the terminal, which would then differ from the | |
| 86 | + | // committed shadow in unknown ways: forget the shadow so the next | |
| 87 | + | // Prepare emits a full snapshot. | |
| 88 | + | r.hasCommitted = false | |
| 80 | 89 | return | |
| 81 | 90 | } | |
| 82 | 91 | if plan.Snapshot || !r.hasCommitted { | |
| @@ -113,7 +122,7 @@ func (r *Renderer) Prepare(frame CellSource, damage []Damage, reset bool) (Prepa | |||
| 113 | 122 | plan := planSingleDamage(frame, damage[0]) | |
| 114 | 123 | candidate = newDeltaCandidate(frame, plan, &r.scratch) | |
| 115 | 124 | } else { | |
| 116 | - | candidate, err = planDelta(frame, damage, r.committedFrame(), reset || !r.hasCommitted, &r.scratch) | |
| 125 | + | candidate, err = planDelta(frame, damage, r.committed, reset || !r.hasCommitted, &r.scratch) | |
| 117 | 126 | } | |
| 118 | 127 | if err != nil { | |
| 119 | 128 | return PreparedDraw{}, err | |
| @@ -171,17 +180,6 @@ func (r *Renderer) Draw(frame CellSource, damage []Damage) ([]byte, error) { | |||
| 171 | 180 | return prepared.Bytes(), nil | |
| 172 | 181 | } | |
| 173 | 182 | ||
| 174 | - | func (r *Renderer) committedFrame() Frame { | |
| 175 | - | return r.committed | |
| 176 | - | } | |
| 177 | - | ||
| 178 | - | func (r *Renderer) setCommittedFrame(frame Frame) { | |
| 179 | - | r.width = frame.Width | |
| 180 | - | r.height = frame.Height | |
| 181 | - | r.hasCommitted = true | |
| 182 | - | r.committed = frame | |
| 183 | - | } | |
| 184 | - | ||
| 185 | 183 | // copyBytes copies the buffer contents into a fresh byte slice and is used | |
| 186 | 184 | // to return output that is independent of the pooled scratch buffer. | |
| 187 | 185 | func copyBytes(buf *bytes.Buffer) []byte { | |
| @@ -64,6 +64,11 @@ func (p PreparedDraw) Bytes() []byte { return p.data } | |||||
| 64 | 64 | ||||
| 65 | // Commit applies the prepared delta exactly once. Discarding it leaves the | 65 | // Commit applies the prepared delta exactly once. Discarding it leaves the | ||
| 66 | // renderer's committed state unchanged. | 66 | // renderer's committed state unchanged. | ||
| 67 | + | // | |||
| 68 | + | // Committing a draw that a later Prepare has superseded does not apply it: | |||
| 69 | + | // the renderer instead drops its committed state, so the next Prepare | |||
| 70 | + | // re-emits a full snapshot instead of diffing against a shadow that may no | |||
| 71 | + | // longer match the terminal. Draws without output are unaffected. | |||
| 67 | func (p *PreparedDraw) Commit() { | 72 | func (p *PreparedDraw) Commit() { | ||
| 68 | if p == nil || p.renderer == nil || p.commitOnce == nil { | 73 | if p == nil || p.renderer == nil || p.commitOnce == nil { | ||
| 69 | return | 74 | return | ||
| @@ -76,7 +81,11 @@ func (p *PreparedDraw) Commit() { | |||||
| 76 | } | 81 | } | ||
| 77 | if p.generation != r.generation { | 82 | if p.generation != r.generation { | ||
| 78 | // A later Prepare reused the scratch frame this draw's snapshot | 83 | // A later Prepare reused the scratch frame this draw's snapshot | ||
| 79 | - | // lives in, so it was discarded. | 84 | + | // lives in, so it cannot be applied. Its bytes may nevertheless |
| 85 | + | // have reached the terminal, which would then differ from the | |||
| 86 | + | // committed shadow in unknown ways: forget the shadow so the next | |||
| 87 | + | // Prepare emits a full snapshot. | |||
| 88 | + | r.hasCommitted = false | |||
| 80 | return | 89 | return | ||
| 81 | } | 90 | } | ||
| 82 | if plan.Snapshot || !r.hasCommitted { | 91 | if plan.Snapshot || !r.hasCommitted { | ||
| @@ -113,7 +122,7 @@ func (r *Renderer) Prepare(frame CellSource, damage []Damage, reset bool) (Prepa | |||||
| 113 | plan := planSingleDamage(frame, damage[0]) | 122 | plan := planSingleDamage(frame, damage[0]) | ||
| 114 | candidate = newDeltaCandidate(frame, plan, &r.scratch) | 123 | candidate = newDeltaCandidate(frame, plan, &r.scratch) | ||
| 115 | } else { | 124 | } else { | ||
| 116 | - | candidate, err = planDelta(frame, damage, r.committedFrame(), reset || !r.hasCommitted, &r.scratch) | 125 | + | candidate, err = planDelta(frame, damage, r.committed, reset || !r.hasCommitted, &r.scratch) |
| 117 | } | 126 | } | ||
| 118 | if err != nil { | 127 | if err != nil { | ||
| 119 | return PreparedDraw{}, err | 128 | return PreparedDraw{}, err | ||
| @@ -171,17 +180,6 @@ func (r *Renderer) Draw(frame CellSource, damage []Damage) ([]byte, error) { | |||||
| 171 | return prepared.Bytes(), nil | 180 | return prepared.Bytes(), nil | ||
| 172 | } | 181 | } | ||
| 173 | 182 | ||||
| 174 | - | func (r *Renderer) committedFrame() Frame { | |||
| 175 | - | return r.committed | |||
| 176 | - | } | |||
| 177 | - | ||||
| 178 | - | func (r *Renderer) setCommittedFrame(frame Frame) { | |||
| 179 | - | r.width = frame.Width | |||
| 180 | - | r.height = frame.Height | |||
| 181 | - | r.hasCommitted = true | |||
| 182 | - | r.committed = frame | |||
| 183 | - | } | |||
| 184 | - | ||||
| 185 | // copyBytes copies the buffer contents into a fresh byte slice and is used | 183 | // copyBytes copies the buffer contents into a fresh byte slice and is used | ||
| 186 | // to return output that is independent of the pooled scratch buffer. | 184 | // to return output that is independent of the pooled scratch buffer. | ||
| 187 | func copyBytes(buf *bytes.Buffer) []byte { | 185 | func copyBytes(buf *bytes.Buffer) []byte { | ||
M ansi/scroll_down_test.go +53 -0
| @@ -77,3 +77,56 @@ func TestDownwardScrollMismatchFallsBack(t *testing.T) { | |||
| 77 | 77 | } | |
| 78 | 78 | } | |
| 79 | 79 | } | |
| 80 | + | ||
| 81 | + | func TestStaleCommitForcesSnapshotAndReplaysCorrectly(t *testing.T) { | |
| 82 | + | for _, deliverB := range []bool{false, true} { | |
| 83 | + | frame := vt.NewFrame(10, 4) | |
| 84 | + | for y := range frame.Height { | |
| 85 | + | frame.FillRow(y, 0, frame.Width, vt.Cell{Rune: rune('a' + y), Style: vt.DefaultStyle()}) | |
| 86 | + | } | |
| 87 | + | r := ansi.New(ansi.Capabilities{}) | |
| 88 | + | terminal := vt.NewScreen(frame.Width, frame.Height) | |
| 89 | + | initial, err := r.Draw(frame, nil) | |
| 90 | + | if err != nil { | |
| 91 | + | t.Fatal(err) | |
| 92 | + | } | |
| 93 | + | terminal.Write(initial) | |
| 94 | + | ||
| 95 | + | frame.Set(1, 1, vt.Cell{Rune: 'A', Style: vt.DefaultStyle()}) | |
| 96 | + | a, err := r.Prepare(frame, []vt.Damage{{Kind: vt.DamageText, X: 1, Y: 1, Width: 1, Height: 1}}, false) | |
| 97 | + | if err != nil { | |
| 98 | + | t.Fatal(err) | |
| 99 | + | } | |
| 100 | + | terminal.Write(a.Bytes()) | |
| 101 | + | frame.Set(2, 2, vt.Cell{Rune: 'B', Style: vt.Style{Bold: true, Foreground: 2, Background: -1}}) | |
| 102 | + | b, err := r.Prepare(frame, []vt.Damage{{Kind: vt.DamageText, X: 2, Y: 2, Width: 1, Height: 1}}, false) | |
| 103 | + | if err != nil { | |
| 104 | + | t.Fatal(err) | |
| 105 | + | } | |
| 106 | + | if deliverB { | |
| 107 | + | terminal.Write(b.Bytes()) | |
| 108 | + | } | |
| 109 | + | a.Commit() // superseded by b: must drop the committed shadow | |
| 110 | + | ||
| 111 | + | next, err := r.Prepare(frame, nil, false) | |
| 112 | + | if err != nil { | |
| 113 | + | t.Fatal(err) | |
| 114 | + | } | |
| 115 | + | if len(next.Bytes()) == 0 || !bytes.Contains(next.Bytes(), []byte("a")) || !bytes.Contains(next.Bytes(), []byte("d")) { | |
| 116 | + | t.Fatalf("deliverB=%v: next prepare is not a full snapshot: %q", deliverB, next.Bytes()) | |
| 117 | + | } | |
| 118 | + | terminal.Write(next.Bytes()) | |
| 119 | + | next.Commit() | |
| 120 | + | for y := range frame.Height { | |
| 121 | + | for x := range frame.Width { | |
| 122 | + | if !frame.Cell(x, y).Equal(terminal.Cell(x, y)) { | |
| 123 | + | t.Fatalf("deliverB=%v: cell %d,%d = %+v, want %+v", deliverB, x, y, terminal.Cell(x, y), frame.Cell(x, y)) | |
| 124 | + | } | |
| 125 | + | } | |
| 126 | + | } | |
| 127 | + | unchanged, err := r.Draw(frame, nil) | |
| 128 | + | if err != nil || len(unchanged) != 0 { | |
| 129 | + | t.Fatalf("committed shadow differs after snapshot: %q, %v", unchanged, err) | |
| 130 | + | } | |
| 131 | + | } | |
| 132 | + | } | |
| @@ -77,3 +77,56 @@ func TestDownwardScrollMismatchFallsBack(t *testing.T) { | |||||
| 77 | } | 77 | } | ||
| 78 | } | 78 | } | ||
| 79 | } | 79 | } | ||
| 80 | + | ||||
| 81 | + | func TestStaleCommitForcesSnapshotAndReplaysCorrectly(t *testing.T) { | |||
| 82 | + | for _, deliverB := range []bool{false, true} { | |||
| 83 | + | frame := vt.NewFrame(10, 4) | |||
| 84 | + | for y := range frame.Height { | |||
| 85 | + | frame.FillRow(y, 0, frame.Width, vt.Cell{Rune: rune('a' + y), Style: vt.DefaultStyle()}) | |||
| 86 | + | } | |||
| 87 | + | r := ansi.New(ansi.Capabilities{}) | |||
| 88 | + | terminal := vt.NewScreen(frame.Width, frame.Height) | |||
| 89 | + | initial, err := r.Draw(frame, nil) | |||
| 90 | + | if err != nil { | |||
| 91 | + | t.Fatal(err) | |||
| 92 | + | } | |||
| 93 | + | terminal.Write(initial) | |||
| 94 | + | ||||
| 95 | + | frame.Set(1, 1, vt.Cell{Rune: 'A', Style: vt.DefaultStyle()}) | |||
| 96 | + | a, err := r.Prepare(frame, []vt.Damage{{Kind: vt.DamageText, X: 1, Y: 1, Width: 1, Height: 1}}, false) | |||
| 97 | + | if err != nil { | |||
| 98 | + | t.Fatal(err) | |||
| 99 | + | } | |||
| 100 | + | terminal.Write(a.Bytes()) | |||
| 101 | + | frame.Set(2, 2, vt.Cell{Rune: 'B', Style: vt.Style{Bold: true, Foreground: 2, Background: -1}}) | |||
| 102 | + | b, err := r.Prepare(frame, []vt.Damage{{Kind: vt.DamageText, X: 2, Y: 2, Width: 1, Height: 1}}, false) | |||
| 103 | + | if err != nil { | |||
| 104 | + | t.Fatal(err) | |||
| 105 | + | } | |||
| 106 | + | if deliverB { | |||
| 107 | + | terminal.Write(b.Bytes()) | |||
| 108 | + | } | |||
| 109 | + | a.Commit() // superseded by b: must drop the committed shadow | |||
| 110 | + | ||||
| 111 | + | next, err := r.Prepare(frame, nil, false) | |||
| 112 | + | if err != nil { | |||
| 113 | + | t.Fatal(err) | |||
| 114 | + | } | |||
| 115 | + | if len(next.Bytes()) == 0 || !bytes.Contains(next.Bytes(), []byte("a")) || !bytes.Contains(next.Bytes(), []byte("d")) { | |||
| 116 | + | t.Fatalf("deliverB=%v: next prepare is not a full snapshot: %q", deliverB, next.Bytes()) | |||
| 117 | + | } | |||
| 118 | + | terminal.Write(next.Bytes()) | |||
| 119 | + | next.Commit() | |||
| 120 | + | for y := range frame.Height { | |||
| 121 | + | for x := range frame.Width { | |||
| 122 | + | if !frame.Cell(x, y).Equal(terminal.Cell(x, y)) { | |||
| 123 | + | t.Fatalf("deliverB=%v: cell %d,%d = %+v, want %+v", deliverB, x, y, terminal.Cell(x, y), frame.Cell(x, y)) | |||
| 124 | + | } | |||
| 125 | + | } | |||
| 126 | + | } | |||
| 127 | + | unchanged, err := r.Draw(frame, nil) | |||
| 128 | + | if err != nil || len(unchanged) != 0 { | |||
| 129 | + | t.Fatalf("committed shadow differs after snapshot: %q, %v", unchanged, err) | |||
| 130 | + | } | |||
| 131 | + | } | |||
| 132 | + | } | |||
A ansi/shadow_helpers_test.go +16 -0
| @@ -0,0 +1,16 @@ | |||
| 1 | + | package ansi | |
| 2 | + | ||
| 3 | + | // committedFrame returns the renderer's committed frame (sharing its storage). | |
| 4 | + | func (r *Renderer) committedFrame() Frame { return r.committed } | |
| 5 | + | ||
| 6 | + | // setCommittedFrame installs frame as the committed shadow. | |
| 7 | + | func (r *Renderer) setCommittedFrame(frame Frame) { | |
| 8 | + | r.width = frame.Width | |
| 9 | + | r.height = frame.Height | |
| 10 | + | r.hasCommitted = true | |
| 11 | + | r.committed = frame | |
| 12 | + | } | |
| 13 | + | ||
| 14 | + | // replaceFrame overwrites dst's own storage with a copy of src, reusing | |
| 15 | + | // capacity. | |
| 16 | + | func replaceFrame(dst *Frame, src Frame) { dst.CopyFrom(src) } | |
| @@ -0,0 +1,16 @@ | |||||
| 1 | + | package ansi | |||
| 2 | + | ||||
| 3 | + | // committedFrame returns the renderer's committed frame (sharing its storage). | |||
| 4 | + | func (r *Renderer) committedFrame() Frame { return r.committed } | |||
| 5 | + | ||||
| 6 | + | // setCommittedFrame installs frame as the committed shadow. | |||
| 7 | + | func (r *Renderer) setCommittedFrame(frame Frame) { | |||
| 8 | + | r.width = frame.Width | |||
| 9 | + | r.height = frame.Height | |||
| 10 | + | r.hasCommitted = true | |||
| 11 | + | r.committed = frame | |||
| 12 | + | } | |||
| 13 | + | ||||
| 14 | + | // replaceFrame overwrites dst's own storage with a copy of src, reusing | |||
| 15 | + | // capacity. | |||
| 16 | + | func replaceFrame(dst *Frame, src Frame) { dst.CopyFrom(src) } | |||