Skip to content

resizeY grow path bloats Height via clear/ensureHeight feedback#3

Open
th3godfather wants to merge 1 commit into
vito:mainfrom
th3godfather:fix/resize-grow-height-bloat
Open

resizeY grow path bloats Height via clear/ensureHeight feedback#3
th3godfather wants to merge 1 commit into
vito:mainfrom
th3godfather:fix/resize-grow-height-bloat

Conversation

@th3godfather
Copy link
Copy Markdown

The grow branch of Screen.resizeY read v.Height fresh on every inner iteration as the offset for the next row to clear, but the clear() call inside that loop goes through paintensureHeight, which mutates v.Height between iterations. Each new row therefore compounded its offset by row+1 per column, so growing a 24×40 terminal to 29×50 ended with v.Height = 624 and len(v.Content) = 1219. A trailing v.Height = h reset the field, but only after resizeX had already iterated the bloated Content and re-grown v.Height to match.

Fix

ensureHeight already does exactly what the loop intended: append a row to Content with all cells initialised to ' ', paint EmptyFormat on every cell via the canvas, tick Changes, increment v.Height — once per new row, no feedback loop. Letting it do the work directly keeps Content / Changes / Height / Canvas in lockstep with no per-cell redundancy.

Test

TestResizeGrowingHeightDoesNotBloatCanvas pins the size invariants (Height, Width, len(Content), len(Changes), per-row width, and the rendered line count) across four growth shapes, including the 24×40 → 29×50 original repro and a width-only-grow control case that was never affected. Pre-fix the height-growth cases failed with v.Height in the hundreds or hundreds of thousands; post-fix all four pass.

The grow branch of Screen.resizeY read v.Height fresh on every inner
iteration as the offset for the next row to clear, but the very
clear() call inside that loop goes through paint → ensureHeight,
which mutates v.Height between iterations. Each new "row" therefore
compounded its offset by row+1 per column, so growing a 24×40
terminal to 29×50 ended with v.Height=624 and len(v.Content)=1219.
A trailing v.Height = h reset the field, but only after resizeX had
already iterated the bloated Content and re-grown v.Height to match.

Embedders that wrap midterm as a pane (terminal multiplexers,
agent dashboards) saw the side-effect as their host renderer
clipping content off-screen — the pane itself drew fine, only the
chrome below it disappeared once the bloated Render output
overflowed the host viewport.

ensureHeight already does exactly what the loop intended: append a
row to Content with all cells initialised to ' ', paint EmptyFormat
on every cell via the canvas, tick Changes, increment v.Height —
once per new row, no feedback loop. Letting it do the work
directly keeps Content / Changes / Height / Canvas in lockstep with
no per-cell redundancy.

TestResizeGrowingHeightDoesNotBloatCanvas pins the size invariants
(Height, Width, len(Content), len(Changes), per-row width, and the
rendered line count) across four growth shapes, including the
24×40 → 29×50 original repro and a width-only-grow control case
that was never affected. Pre-fix the height-growth cases failed
with v.Height in the hundreds or hundreds of thousands; post-fix
all four pass.
Copy link
Copy Markdown
Owner

@vito vito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! No need for the big AI slop comment though, referencing how it "previously" worked is confusing, and the new code is simple + obvious enough to not need a comment at all.

Comment thread screen.go
Comment on lines +96 to +106
// Grow: ensureHeight already appends empty rows, paints
// EmptyFormat on each new cell, and ticks Changes — exactly
// the per-row initialization this branch used to do
// cell-by-cell via clear(). The previous loop read v.Height
// fresh on every iteration as the row offset, and clear()'s
// internal ensureHeight call mutated v.Height between
// iterations, so each new row compounded the offset by
// row+1 per column (Height grew from 24 to 624 on a
// 24×40 → 29×50 resize). Letting ensureHeight do the work
// directly keeps the canvas, Content, Changes, and Height
// in lockstep.
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Grow: ensureHeight already appends empty rows, paints
// EmptyFormat on each new cell, and ticks Changes — exactly
// the per-row initialization this branch used to do
// cell-by-cell via clear(). The previous loop read v.Height
// fresh on every iteration as the row offset, and clear()'s
// internal ensureHeight call mutated v.Height between
// iterations, so each new row compounded the offset by
// row+1 per column (Height grew from 24 to 624 on a
// 24×40 → 29×50 resize). Letting ensureHeight do the work
// directly keeps the canvas, Content, Changes, and Height
// in lockstep.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants