fix: apply current review findings
- commit
- 60a6a56f543bfaad4e746a19697369ca7c8d4c1c
- author
- brice · · 2026-08-02 13:30 UTC
- parent
- 530da1f
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
7 files · +90 -26
M internal/adapters/sshstdio/transport_test.go +3 -0
| @@ -183,6 +183,9 @@ func TestTransportUsesCanonicalFrameMaximum(t *testing.T) { | |||
| 183 | 183 | if boundaryFrame.Type != ports.MsgOutput || len(boundaryFrame.Payload) != len(boundaryPayload) { | |
| 184 | 184 | t.Fatalf("boundary frame = type %d, payload %d bytes; want type %d, payload %d bytes", boundaryFrame.Type, len(boundaryFrame.Payload), ports.MsgOutput, len(boundaryPayload)) | |
| 185 | 185 | } | |
| 186 | + | if !bytes.Equal(boundaryFrame.Payload, boundaryPayload) { | |
| 187 | + | t.Fatal("boundary payload was corrupted") | |
| 188 | + | } | |
| 186 | 189 | ||
| 187 | 190 | send := NewTransport(nil, io.Discard, nil) | |
| 188 | 191 | err = send.Send(ports.Frame{Type: ports.MsgOutput, Payload: make([]byte, ports.MaxFrameLen)}) | |
| @@ -183,6 +183,9 @@ func TestTransportUsesCanonicalFrameMaximum(t *testing.T) { | |||||
| 183 | if boundaryFrame.Type != ports.MsgOutput || len(boundaryFrame.Payload) != len(boundaryPayload) { | 183 | if boundaryFrame.Type != ports.MsgOutput || len(boundaryFrame.Payload) != len(boundaryPayload) { | ||
| 184 | t.Fatalf("boundary frame = type %d, payload %d bytes; want type %d, payload %d bytes", boundaryFrame.Type, len(boundaryFrame.Payload), ports.MsgOutput, len(boundaryPayload)) | 184 | t.Fatalf("boundary frame = type %d, payload %d bytes; want type %d, payload %d bytes", boundaryFrame.Type, len(boundaryFrame.Payload), ports.MsgOutput, len(boundaryPayload)) | ||
| 185 | } | 185 | } | ||
| 186 | + | if !bytes.Equal(boundaryFrame.Payload, boundaryPayload) { | |||
| 187 | + | t.Fatal("boundary payload was corrupted") | |||
| 188 | + | } | |||
| 186 | 189 | ||||
| 187 | send := NewTransport(nil, io.Discard, nil) | 190 | send := NewTransport(nil, io.Discard, nil) | ||
| 188 | err = send.Send(ports.Frame{Type: ports.MsgOutput, Payload: make([]byte, ports.MaxFrameLen)}) | 191 | err = send.Send(ports.Frame{Type: ports.MsgOutput, Payload: make([]byte, ports.MaxFrameLen)}) | ||
M internal/usecase/daemon/proxy_screen_state.go +1 -1
| @@ -199,7 +199,7 @@ func (s *proxyScreenState) recordUpdate(update ports.ScreenUpdate) { | |||
| 199 | 199 | if s.damageFullRedrawSticky { | |
| 200 | 200 | return | |
| 201 | 201 | } | |
| 202 | - | if update.Scroll != nil && len(s.damage) > 0 && !(len(s.damage) == 1 && s.damage[0].Kind == renderer.DamageFullRedraw) { | |
| 202 | + | if update.Scroll != nil && len(s.damage) > 0 && (len(s.damage) != 1 || s.damage[0].Kind != renderer.DamageFullRedraw) { | |
| 203 | 203 | s.setFullRedraw() | |
| 204 | 204 | return | |
| 205 | 205 | } | |
| @@ -199,7 +199,7 @@ func (s *proxyScreenState) recordUpdate(update ports.ScreenUpdate) { | |||||
| 199 | if s.damageFullRedrawSticky { | 199 | if s.damageFullRedrawSticky { | ||
| 200 | return | 200 | return | ||
| 201 | } | 201 | } | ||
| 202 | - | if update.Scroll != nil && len(s.damage) > 0 && !(len(s.damage) == 1 && s.damage[0].Kind == renderer.DamageFullRedraw) { | 202 | + | if update.Scroll != nil && len(s.damage) > 0 && (len(s.damage) != 1 || s.damage[0].Kind != renderer.DamageFullRedraw) { |
| 203 | s.setFullRedraw() | 203 | s.setFullRedraw() | ||
| 204 | return | 204 | return | ||
| 205 | } | 205 | } | ||
M internal/usecase/daemon/screen_output_stream_test.go +6 -0
| @@ -201,6 +201,12 @@ func TestStructuredOutputStreamNonNoopCommitForcesSnapshotRetry(t *testing.T) { | |||
| 201 | 201 | require.NoError(t, err) | |
| 202 | 202 | require.NotEmpty(t, pending.data) | |
| 203 | 203 | pending.commitNoSend() | |
| 204 | + | var senderCalled bool | |
| 205 | + | require.NoError(t, pending.send(func(ports.Frame) error { | |
| 206 | + | senderCalled = true | |
| 207 | + | return nil | |
| 208 | + | })) | |
| 209 | + | require.False(t, senderCalled) | |
| 204 | 210 | require.Equal(t, before, stream.shadow, "non-noop commit must not publish its candidate") | |
| 205 | 211 | require.Equal(t, uint64(1), state.next) | |
| 206 | 212 | require.True(t, stream.forceSnapshot) | |
| @@ -201,6 +201,12 @@ func TestStructuredOutputStreamNonNoopCommitForcesSnapshotRetry(t *testing.T) { | |||||
| 201 | require.NoError(t, err) | 201 | require.NoError(t, err) | ||
| 202 | require.NotEmpty(t, pending.data) | 202 | require.NotEmpty(t, pending.data) | ||
| 203 | pending.commitNoSend() | 203 | pending.commitNoSend() | ||
| 204 | + | var senderCalled bool | |||
| 205 | + | require.NoError(t, pending.send(func(ports.Frame) error { | |||
| 206 | + | senderCalled = true | |||
| 207 | + | return nil | |||
| 208 | + | })) | |||
| 209 | + | require.False(t, senderCalled) | |||
| 204 | require.Equal(t, before, stream.shadow, "non-noop commit must not publish its candidate") | 210 | require.Equal(t, before, stream.shadow, "non-noop commit must not publish its candidate") | ||
| 205 | require.Equal(t, uint64(1), state.next) | 211 | require.Equal(t, uint64(1), state.next) | ||
| 206 | require.True(t, stream.forceSnapshot) | 212 | require.True(t, stream.forceSnapshot) | ||
M pkg/renderer/delta_plan.go +28 -24
| @@ -23,48 +23,53 @@ type DeltaPlan struct { | |||
| 23 | 23 | Spans []Span | |
| 24 | 24 | } | |
| 25 | 25 | ||
| 26 | - | // DeltaCandidate holds a plan and owns a source frame snapshot until Commit. | |
| 26 | + | // DeltaCandidate holds a plan and owns a source frame snapshot when the plan | |
| 27 | + | // has work to commit. Empty candidates are true no-ops. | |
| 27 | 28 | type DeltaCandidate struct { | |
| 28 | 29 | Plan DeltaPlan | |
| 29 | 30 | frame Frame | |
| 30 | 31 | } | |
| 31 | 32 | ||
| 33 | + | func newDeltaCandidate(frame Frame, plan DeltaPlan) DeltaCandidate { | |
| 34 | + | candidate := DeltaCandidate{Plan: plan} | |
| 35 | + | if plan.Snapshot || plan.Scroll.Height != 0 || len(plan.Spans) != 0 { | |
| 36 | + | candidate.frame = frame.Clone() | |
| 37 | + | } | |
| 38 | + | return candidate | |
| 39 | + | } | |
| 40 | + | ||
| 32 | 41 | // PlanDelta prepares a non-mutating update from committed to frame. | |
| 33 | 42 | func PlanDelta(frame Frame, damage []Damage, committed Frame, reset bool) (DeltaCandidate, error) { | |
| 34 | 43 | if err := frame.Validate(); err != nil { | |
| 35 | 44 | return DeltaCandidate{}, err | |
| 36 | 45 | } | |
| 37 | 46 | ||
| 38 | - | candidate := DeltaCandidate{frame: frame.Clone()} | |
| 39 | 47 | if reset || frame.Width != committed.Width || frame.Height != committed.Height { | |
| 40 | - | candidate.Plan.Snapshot = true | |
| 41 | - | return candidate, nil | |
| 48 | + | return newDeltaCandidate(frame, DeltaPlan{Snapshot: true}), nil | |
| 42 | 49 | } | |
| 43 | 50 | if err := committed.Validate(); err != nil { | |
| 44 | 51 | return DeltaCandidate{}, err | |
| 45 | 52 | } | |
| 46 | 53 | ||
| 47 | 54 | if len(damage) == 1 && (damage[0].Kind == DamageText || damage[0].Kind == DamageClear) { | |
| 48 | - | candidate.Plan = planSingleDamage(frame, damage[0]) | |
| 49 | - | return candidate, nil | |
| 55 | + | return newDeltaCandidate(frame, planSingleDamage(frame, damage[0])), nil | |
| 50 | 56 | } | |
| 51 | 57 | ||
| 52 | 58 | if needsFull(damage) { | |
| 53 | 59 | dirty, full := buildDirtyLinePlan(frame, committed) | |
| 54 | 60 | if len(dirty) > 0 || full { | |
| 55 | - | candidate.Plan.Snapshot = true | |
| 56 | - | candidate.Plan.Spans = dirty | |
| 61 | + | return newDeltaCandidate(frame, DeltaPlan{Snapshot: true, Spans: dirty}), nil | |
| 57 | 62 | } | |
| 58 | - | return candidate, nil | |
| 63 | + | return DeltaCandidate{}, nil | |
| 59 | 64 | } | |
| 60 | 65 | ||
| 66 | + | var plan DeltaPlan | |
| 61 | 67 | var skip *Damage | |
| 62 | 68 | if scroll, ok := findSafeScroll(frame, damage); ok && canApplyScrollAgainst(frame, scroll, damage, committed) { | |
| 63 | - | candidate.Plan.Scroll = Scroll{Y: scroll.Y, Height: scroll.Height, Count: scroll.Count} | |
| 69 | + | plan.Scroll = Scroll{Y: scroll.Y, Height: scroll.Height, Count: scroll.Count} | |
| 64 | 70 | skip = &scroll | |
| 65 | 71 | } else if hasScrollDamage(damage) { | |
| 66 | - | candidate.Plan.Snapshot = true | |
| 67 | - | return candidate, nil | |
| 72 | + | return newDeltaCandidate(frame, DeltaPlan{Snapshot: true}), nil | |
| 68 | 73 | } | |
| 69 | 74 | ||
| 70 | 75 | var spans []Span | |
| @@ -75,20 +80,16 @@ func PlanDelta(frame Frame, damage []Damage, committed Frame, reset bool) (Delta | |||
| 75 | 80 | spans, full = buildDamagePlan(frame, damage, skip) | |
| 76 | 81 | } | |
| 77 | 82 | if full { | |
| 78 | - | candidate.Plan = DeltaPlan{Snapshot: true} | |
| 79 | - | return candidate, nil | |
| 80 | - | } | |
| 81 | - | if deltaCostsSnapshot(frame, spans, candidate.Plan.Scroll.Height != 0) { | |
| 82 | - | if candidate.Plan.Scroll.Height != 0 { | |
| 83 | - | candidate.Plan = DeltaPlan{Snapshot: true} | |
| 84 | - | } else { | |
| 85 | - | candidate.Plan.Snapshot = true | |
| 86 | - | candidate.Plan.Spans = spans | |
| 83 | + | return newDeltaCandidate(frame, DeltaPlan{Snapshot: true}), nil | |
| 84 | + | } | |
| 85 | + | if deltaCostsSnapshot(frame, spans, plan.Scroll.Height != 0) { | |
| 86 | + | if plan.Scroll.Height != 0 { | |
| 87 | + | return newDeltaCandidate(frame, DeltaPlan{Snapshot: true}), nil | |
| 87 | 88 | } | |
| 88 | - | return candidate, nil | |
| 89 | + | return newDeltaCandidate(frame, DeltaPlan{Snapshot: true, Spans: spans}), nil | |
| 89 | 90 | } | |
| 90 | - | candidate.Plan.Spans = spans | |
| 91 | - | return candidate, nil | |
| 91 | + | plan.Spans = spans | |
| 92 | + | return newDeltaCandidate(frame, plan), nil | |
| 92 | 93 | } | |
| 93 | 94 | ||
| 94 | 95 | func planSingleDamage(frame Frame, d Damage) DeltaPlan { | |
| @@ -191,6 +192,9 @@ func styleRunCount(cells []Cell) int { | |||
| 191 | 192 | ||
| 192 | 193 | // Commit advances dst to the frame snapshot owned by the candidate. | |
| 193 | 194 | func (c DeltaCandidate) Commit(dst *Frame) { | |
| 195 | + | if !c.Plan.Snapshot && c.Plan.Scroll.Height == 0 && len(c.Plan.Spans) == 0 { | |
| 196 | + | return | |
| 197 | + | } | |
| 194 | 198 | if c.Plan.Snapshot || dst.Width != c.frame.Width || dst.Height != c.frame.Height { | |
| 195 | 199 | replaceFrame(dst, c.frame) | |
| 196 | 200 | return | |
| @@ -23,48 +23,53 @@ type DeltaPlan struct { | |||||
| 23 | Spans []Span | 23 | Spans []Span | ||
| 24 | } | 24 | } | ||
| 25 | 25 | ||||
| 26 | - | // DeltaCandidate holds a plan and owns a source frame snapshot until Commit. | 26 | + | // DeltaCandidate holds a plan and owns a source frame snapshot when the plan |
| 27 | + | // has work to commit. Empty candidates are true no-ops. | |||
| 27 | type DeltaCandidate struct { | 28 | type DeltaCandidate struct { | ||
| 28 | Plan DeltaPlan | 29 | Plan DeltaPlan | ||
| 29 | frame Frame | 30 | frame Frame | ||
| 30 | } | 31 | } | ||
| 31 | 32 | ||||
| 33 | + | func newDeltaCandidate(frame Frame, plan DeltaPlan) DeltaCandidate { | |||
| 34 | + | candidate := DeltaCandidate{Plan: plan} | |||
| 35 | + | if plan.Snapshot || plan.Scroll.Height != 0 || len(plan.Spans) != 0 { | |||
| 36 | + | candidate.frame = frame.Clone() | |||
| 37 | + | } | |||
| 38 | + | return candidate | |||
| 39 | + | } | |||
| 40 | + | ||||
| 32 | // PlanDelta prepares a non-mutating update from committed to frame. | 41 | // PlanDelta prepares a non-mutating update from committed to frame. | ||
| 33 | func PlanDelta(frame Frame, damage []Damage, committed Frame, reset bool) (DeltaCandidate, error) { | 42 | func PlanDelta(frame Frame, damage []Damage, committed Frame, reset bool) (DeltaCandidate, error) { | ||
| 34 | if err := frame.Validate(); err != nil { | 43 | if err := frame.Validate(); err != nil { | ||
| 35 | return DeltaCandidate{}, err | 44 | return DeltaCandidate{}, err | ||
| 36 | } | 45 | } | ||
| 37 | 46 | ||||
| 38 | - | candidate := DeltaCandidate{frame: frame.Clone()} | |||
| 39 | if reset || frame.Width != committed.Width || frame.Height != committed.Height { | 47 | if reset || frame.Width != committed.Width || frame.Height != committed.Height { | ||
| 40 | - | candidate.Plan.Snapshot = true | 48 | + | return newDeltaCandidate(frame, DeltaPlan{Snapshot: true}), nil |
| 41 | - | return candidate, nil | |||
| 42 | } | 49 | } | ||
| 43 | if err := committed.Validate(); err != nil { | 50 | if err := committed.Validate(); err != nil { | ||
| 44 | return DeltaCandidate{}, err | 51 | return DeltaCandidate{}, err | ||
| 45 | } | 52 | } | ||
| 46 | 53 | ||||
| 47 | if len(damage) == 1 && (damage[0].Kind == DamageText || damage[0].Kind == DamageClear) { | 54 | if len(damage) == 1 && (damage[0].Kind == DamageText || damage[0].Kind == DamageClear) { | ||
| 48 | - | candidate.Plan = planSingleDamage(frame, damage[0]) | 55 | + | return newDeltaCandidate(frame, planSingleDamage(frame, damage[0])), nil |
| 49 | - | return candidate, nil | |||
| 50 | } | 56 | } | ||
| 51 | 57 | ||||
| 52 | if needsFull(damage) { | 58 | if needsFull(damage) { | ||
| 53 | dirty, full := buildDirtyLinePlan(frame, committed) | 59 | dirty, full := buildDirtyLinePlan(frame, committed) | ||
| 54 | if len(dirty) > 0 || full { | 60 | if len(dirty) > 0 || full { | ||
| 55 | - | candidate.Plan.Snapshot = true | 61 | + | return newDeltaCandidate(frame, DeltaPlan{Snapshot: true, Spans: dirty}), nil |
| 56 | - | candidate.Plan.Spans = dirty | |||
| 57 | } | 62 | } | ||
| 58 | - | return candidate, nil | 63 | + | return DeltaCandidate{}, nil |
| 59 | } | 64 | } | ||
| 60 | 65 | ||||
| 66 | + | var plan DeltaPlan | |||
| 61 | var skip *Damage | 67 | var skip *Damage | ||
| 62 | if scroll, ok := findSafeScroll(frame, damage); ok && canApplyScrollAgainst(frame, scroll, damage, committed) { | 68 | if scroll, ok := findSafeScroll(frame, damage); ok && canApplyScrollAgainst(frame, scroll, damage, committed) { | ||
| 63 | - | candidate.Plan.Scroll = Scroll{Y: scroll.Y, Height: scroll.Height, Count: scroll.Count} | 69 | + | plan.Scroll = Scroll{Y: scroll.Y, Height: scroll.Height, Count: scroll.Count} |
| 64 | skip = &scroll | 70 | skip = &scroll | ||
| 65 | } else if hasScrollDamage(damage) { | 71 | } else if hasScrollDamage(damage) { | ||
| 66 | - | candidate.Plan.Snapshot = true | 72 | + | return newDeltaCandidate(frame, DeltaPlan{Snapshot: true}), nil |
| 67 | - | return candidate, nil | |||
| 68 | } | 73 | } | ||
| 69 | 74 | ||||
| 70 | var spans []Span | 75 | var spans []Span | ||
| @@ -75,20 +80,16 @@ func PlanDelta(frame Frame, damage []Damage, committed Frame, reset bool) (Delta | |||||
| 75 | spans, full = buildDamagePlan(frame, damage, skip) | 80 | spans, full = buildDamagePlan(frame, damage, skip) | ||
| 76 | } | 81 | } | ||
| 77 | if full { | 82 | if full { | ||
| 78 | - | candidate.Plan = DeltaPlan{Snapshot: true} | 83 | + | return newDeltaCandidate(frame, DeltaPlan{Snapshot: true}), nil |
| 79 | - | return candidate, nil | 84 | + | } |
| 80 | - | } | 85 | + | if deltaCostsSnapshot(frame, spans, plan.Scroll.Height != 0) { |
| 81 | - | if deltaCostsSnapshot(frame, spans, candidate.Plan.Scroll.Height != 0) { | 86 | + | if plan.Scroll.Height != 0 { |
| 82 | - | if candidate.Plan.Scroll.Height != 0 { | 87 | + | return newDeltaCandidate(frame, DeltaPlan{Snapshot: true}), nil |
| 83 | - | candidate.Plan = DeltaPlan{Snapshot: true} | |||
| 84 | - | } else { | |||
| 85 | - | candidate.Plan.Snapshot = true | |||
| 86 | - | candidate.Plan.Spans = spans | |||
| 87 | } | 88 | } | ||
| 88 | - | return candidate, nil | 89 | + | return newDeltaCandidate(frame, DeltaPlan{Snapshot: true, Spans: spans}), nil |
| 89 | } | 90 | } | ||
| 90 | - | candidate.Plan.Spans = spans | 91 | + | plan.Spans = spans |
| 91 | - | return candidate, nil | 92 | + | return newDeltaCandidate(frame, plan), nil |
| 92 | } | 93 | } | ||
| 93 | 94 | ||||
| 94 | func planSingleDamage(frame Frame, d Damage) DeltaPlan { | 95 | func planSingleDamage(frame Frame, d Damage) DeltaPlan { | ||
| @@ -191,6 +192,9 @@ func styleRunCount(cells []Cell) int { | |||||
| 191 | 192 | ||||
| 192 | // Commit advances dst to the frame snapshot owned by the candidate. | 193 | // Commit advances dst to the frame snapshot owned by the candidate. | ||
| 193 | func (c DeltaCandidate) Commit(dst *Frame) { | 194 | func (c DeltaCandidate) Commit(dst *Frame) { | ||
| 195 | + | if !c.Plan.Snapshot && c.Plan.Scroll.Height == 0 && len(c.Plan.Spans) == 0 { | |||
| 196 | + | return | |||
| 197 | + | } | |||
| 194 | if c.Plan.Snapshot || dst.Width != c.frame.Width || dst.Height != c.frame.Height { | 198 | if c.Plan.Snapshot || dst.Width != c.frame.Width || dst.Height != c.frame.Height { | ||
| 195 | replaceFrame(dst, c.frame) | 199 | replaceFrame(dst, c.frame) | ||
| 196 | return | 200 | return | ||
M pkg/renderer/delta_plan_test.go +18 -0
| @@ -163,6 +163,24 @@ func TestPlanDelta(t *testing.T) { | |||
| 163 | 163 | } | |
| 164 | 164 | } | |
| 165 | 165 | ||
| 166 | + | func TestNoOpDeltaCandidateDoesNotCloneFrame(t *testing.T) { | |
| 167 | + | frame := testFrame("abcd", "efgh") | |
| 168 | + | candidate, err := PlanDelta(frame, nil, frame, false) | |
| 169 | + | if err != nil { | |
| 170 | + | t.Fatalf("PlanDelta() error = %v", err) | |
| 171 | + | } | |
| 172 | + | if candidate.frame.Cells != nil || candidate.frame.lineOffset != nil { | |
| 173 | + | t.Fatal("no-op candidate owns frame storage") | |
| 174 | + | } | |
| 175 | + | ||
| 176 | + | committed := frame.Clone() | |
| 177 | + | before := committed.Clone() | |
| 178 | + | candidate.Commit(&committed) | |
| 179 | + | if !reflect.DeepEqual(committed, before) { | |
| 180 | + | t.Fatal("no-op candidate changed committed frame") | |
| 181 | + | } | |
| 182 | + | } | |
| 183 | + | ||
| 166 | 184 | func TestPlanDeltaDoesNotMutateCommitted(t *testing.T) { | |
| 167 | 185 | committed := testFrame("0000", "1111", "2222", "3333", "4444") | |
| 168 | 186 | committed.ScrollUp(1, 3, 1) | |
| @@ -163,6 +163,24 @@ func TestPlanDelta(t *testing.T) { | |||||
| 163 | } | 163 | } | ||
| 164 | } | 164 | } | ||
| 165 | 165 | ||||
| 166 | + | func TestNoOpDeltaCandidateDoesNotCloneFrame(t *testing.T) { | |||
| 167 | + | frame := testFrame("abcd", "efgh") | |||
| 168 | + | candidate, err := PlanDelta(frame, nil, frame, false) | |||
| 169 | + | if err != nil { | |||
| 170 | + | t.Fatalf("PlanDelta() error = %v", err) | |||
| 171 | + | } | |||
| 172 | + | if candidate.frame.Cells != nil || candidate.frame.lineOffset != nil { | |||
| 173 | + | t.Fatal("no-op candidate owns frame storage") | |||
| 174 | + | } | |||
| 175 | + | ||||
| 176 | + | committed := frame.Clone() | |||
| 177 | + | before := committed.Clone() | |||
| 178 | + | candidate.Commit(&committed) | |||
| 179 | + | if !reflect.DeepEqual(committed, before) { | |||
| 180 | + | t.Fatal("no-op candidate changed committed frame") | |||
| 181 | + | } | |||
| 182 | + | } | |||
| 183 | + | ||||
| 166 | func TestPlanDeltaDoesNotMutateCommitted(t *testing.T) { | 184 | func TestPlanDeltaDoesNotMutateCommitted(t *testing.T) { | ||
| 167 | committed := testFrame("0000", "1111", "2222", "3333", "4444") | 185 | committed := testFrame("0000", "1111", "2222", "3333", "4444") | ||
| 168 | committed.ScrollUp(1, 3, 1) | 186 | committed.ScrollUp(1, 3, 1) | ||
M pkg/renderer/renderer.go +5 -1
| @@ -67,7 +67,11 @@ func (r *Renderer) Prepare(frame Frame, damage []Damage, reset bool) (PreparedDr | |||
| 67 | 67 | var err error | |
| 68 | 68 | if !reset && len(r.shadow) != 0 && r.width == frame.Width && r.height == frame.Height && len(damage) == 1 && (damage[0].Kind == DamageText || damage[0].Kind == DamageClear) { | |
| 69 | 69 | if err = frame.Validate(); err == nil { | |
| 70 | - | candidate = DeltaCandidate{Plan: planSingleDamage(frame, damage[0]), frame: frame.Clone()} | |
| 70 | + | plan := planSingleDamage(frame, damage[0]) | |
| 71 | + | candidate = DeltaCandidate{Plan: plan} | |
| 72 | + | if plan.Snapshot || plan.Scroll.Height != 0 || len(plan.Spans) != 0 { | |
| 73 | + | candidate.frame = frame.Clone() | |
| 74 | + | } | |
| 71 | 75 | } | |
| 72 | 76 | } else { | |
| 73 | 77 | candidate, err = PlanDelta(frame, damage, r.committedFrame(), reset || len(r.shadow) == 0) | |
| @@ -67,7 +67,11 @@ func (r *Renderer) Prepare(frame Frame, damage []Damage, reset bool) (PreparedDr | |||||
| 67 | var err error | 67 | var err error | ||
| 68 | if !reset && len(r.shadow) != 0 && r.width == frame.Width && r.height == frame.Height && len(damage) == 1 && (damage[0].Kind == DamageText || damage[0].Kind == DamageClear) { | 68 | if !reset && len(r.shadow) != 0 && r.width == frame.Width && r.height == frame.Height && len(damage) == 1 && (damage[0].Kind == DamageText || damage[0].Kind == DamageClear) { | ||
| 69 | if err = frame.Validate(); err == nil { | 69 | if err = frame.Validate(); err == nil { | ||
| 70 | - | candidate = DeltaCandidate{Plan: planSingleDamage(frame, damage[0]), frame: frame.Clone()} | 70 | + | plan := planSingleDamage(frame, damage[0]) |
| 71 | + | candidate = DeltaCandidate{Plan: plan} | |||
| 72 | + | if plan.Snapshot || plan.Scroll.Height != 0 || len(plan.Spans) != 0 { | |||
| 73 | + | candidate.frame = frame.Clone() | |||
| 74 | + | } | |||
| 71 | } | 75 | } | ||
| 72 | } else { | 76 | } else { | ||
| 73 | candidate, err = PlanDelta(frame, damage, r.committedFrame(), reset || len(r.shadow) == 0) | 77 | candidate, err = PlanDelta(frame, damage, r.committedFrame(), reset || len(r.shadow) == 0) | ||
M pkg/renderer/renderer_test.go +29 -0
| @@ -134,6 +134,24 @@ func TestNoOp(t *testing.T) { | |||
| 134 | 134 | } | |
| 135 | 135 | } | |
| 136 | 136 | ||
| 137 | + | func TestPrepareNoOpDoesNotCloneFrame(t *testing.T) { | |
| 138 | + | r := New(Capabilities{}) | |
| 139 | + | frame := NewFrame(3, 2) | |
| 140 | + | markFrame(&frame) | |
| 141 | + | if _, err := r.Draw(frame, []Damage{{Kind: DamageText, X: 0, Y: 0, Width: 3, Height: 2}}); err != nil { | |
| 142 | + | t.Fatal(err) | |
| 143 | + | } | |
| 144 | + | ||
| 145 | + | prepared, err := r.Prepare(frame, []Damage{{Kind: DamageText, X: frame.Width, Y: 0, Width: 1, Height: 1}}, false) | |
| 146 | + | if err != nil { | |
| 147 | + | t.Fatal(err) | |
| 148 | + | } | |
| 149 | + | if prepared.candidate.frame.Cells != nil || prepared.candidate.frame.lineOffset != nil { | |
| 150 | + | t.Fatal("no-op prepared draw owns frame storage") | |
| 151 | + | } | |
| 152 | + | prepared.Commit() | |
| 153 | + | } | |
| 154 | + | ||
| 137 | 155 | // --------------------------------------------------------------------------- | |
| 138 | 156 | // Style reset discipline | |
| 139 | 157 | // --------------------------------------------------------------------------- | |
| @@ -1306,6 +1324,17 @@ func TestRendererPrepare(t *testing.T) { | |||
| 1306 | 1324 | }, | |
| 1307 | 1325 | want: "\x1b[1;1HABC\x1b[2;1HDEF\x1b[0m", | |
| 1308 | 1326 | }, | |
| 1327 | + | { | |
| 1328 | + | name: "synchronized full draw", | |
| 1329 | + | prepare: func(t *testing.T) PreparedDraw { | |
| 1330 | + | prepared, err := New(Capabilities{SynchronizedOutput: true}).Prepare(fullFrame, fullDamage, false) | |
| 1331 | + | if err != nil { | |
| 1332 | + | t.Fatal(err) | |
| 1333 | + | } | |
| 1334 | + | return prepared | |
| 1335 | + | }, | |
| 1336 | + | want: SyncStartCSI + "\x1b[1;1HABC\x1b[2;1HDEF\x1b[0m" + SyncEndCSI, | |
| 1337 | + | }, | |
| 1309 | 1338 | { | |
| 1310 | 1339 | name: "fragmented damage", | |
| 1311 | 1340 | prepare: func(t *testing.T) PreparedDraw { | |
| @@ -134,6 +134,24 @@ func TestNoOp(t *testing.T) { | |||||
| 134 | } | 134 | } | ||
| 135 | } | 135 | } | ||
| 136 | 136 | ||||
| 137 | + | func TestPrepareNoOpDoesNotCloneFrame(t *testing.T) { | |||
| 138 | + | r := New(Capabilities{}) | |||
| 139 | + | frame := NewFrame(3, 2) | |||
| 140 | + | markFrame(&frame) | |||
| 141 | + | if _, err := r.Draw(frame, []Damage{{Kind: DamageText, X: 0, Y: 0, Width: 3, Height: 2}}); err != nil { | |||
| 142 | + | t.Fatal(err) | |||
| 143 | + | } | |||
| 144 | + | ||||
| 145 | + | prepared, err := r.Prepare(frame, []Damage{{Kind: DamageText, X: frame.Width, Y: 0, Width: 1, Height: 1}}, false) | |||
| 146 | + | if err != nil { | |||
| 147 | + | t.Fatal(err) | |||
| 148 | + | } | |||
| 149 | + | if prepared.candidate.frame.Cells != nil || prepared.candidate.frame.lineOffset != nil { | |||
| 150 | + | t.Fatal("no-op prepared draw owns frame storage") | |||
| 151 | + | } | |||
| 152 | + | prepared.Commit() | |||
| 153 | + | } | |||
| 154 | + | ||||
| 137 | // --------------------------------------------------------------------------- | 155 | // --------------------------------------------------------------------------- | ||
| 138 | // Style reset discipline | 156 | // Style reset discipline | ||
| 139 | // --------------------------------------------------------------------------- | 157 | // --------------------------------------------------------------------------- | ||
| @@ -1306,6 +1324,17 @@ func TestRendererPrepare(t *testing.T) { | |||||
| 1306 | }, | 1324 | }, | ||
| 1307 | want: "\x1b[1;1HABC\x1b[2;1HDEF\x1b[0m", | 1325 | want: "\x1b[1;1HABC\x1b[2;1HDEF\x1b[0m", | ||
| 1308 | }, | 1326 | }, | ||
| 1327 | + | { | |||
| 1328 | + | name: "synchronized full draw", | |||
| 1329 | + | prepare: func(t *testing.T) PreparedDraw { | |||
| 1330 | + | prepared, err := New(Capabilities{SynchronizedOutput: true}).Prepare(fullFrame, fullDamage, false) | |||
| 1331 | + | if err != nil { | |||
| 1332 | + | t.Fatal(err) | |||
| 1333 | + | } | |||
| 1334 | + | return prepared | |||
| 1335 | + | }, | |||
| 1336 | + | want: SyncStartCSI + "\x1b[1;1HABC\x1b[2;1HDEF\x1b[0m" + SyncEndCSI, | |||
| 1337 | + | }, | |||
| 1309 | { | 1338 | { | ||
| 1310 | name: "fragmented damage", | 1339 | name: "fragmented damage", | ||
| 1311 | prepare: func(t *testing.T) PreparedDraw { | 1340 | prepare: func(t *testing.T) PreparedDraw { | ||