fix(addon-image): ImageStorage correctness (viewport widen, render, resize)#5996
Draft
Tyriar wants to merge 4 commits into
Draft
fix(addon-image): ImageStorage correctness (viewport widen, render, resize)#5996Tyriar wants to merge 4 commits into
Tyriar wants to merge 4 commits into
Conversation
viewportResize() should check whether cells in newly exposed columns (oldCol + 1 through cols - 1) already have content before expanding image tiles to the right. The loop used `rightCol > metrics.cols`, which is false when widening (rightCol starts at oldCol + 1), so the loop never ran, hasData stayed false, and tiles could overwrite cells that already held text. Use `rightCol < metrics.cols` so the emptiness check runs over the intended column range. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Daniel Imms <Tyriar@users.noreply.github.com>
viewportResize walks the full buffer when the terminal widens but called line.getBg without checking that lines.get(row) returned a line. During resize or trim gaps that could throw and abort tile expansion for remaining rows. Skip missing buffer lines with continue, consistent with render(). Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Daniel Imms <Tyriar@users.noreply.github.com>
Member
Author
|
@jerch do these look like good correctness improvements? |
Member
|
@Tyriar Yes the changes look correct. The |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Three independent correctness bugs in
addons/addon-image/src/ImageStorage.ts. Selection changes from the same upstream commit (0ff7ec3) are omitted here and tracked in PR 4.1. Viewport widen: inverted loop bound (
viewportResize)Bug: When the terminal widens,
viewportResize()must scan newly exposed columns (oldCol + 1…metrics.cols - 1) for existing cell content before expanding image tiles to the right. The emptiness check usedrightCol > metrics.cols, which is never true when widening, so the loop never ran,hasDatastayed false, and tiles could be written over cells that already held text.Fix: Use
rightCol < metrics.cols.Repro:
cols).2. Render: abort on missing line (
render)Bug:
render()walked viewport rows and usedif (!line) return, aborting the entire pass when any single line was missing. Other rows in range were not drawn.Fix:
return→continueso missing buffer lines are skipped and remaining rows still render.Repro:
buffer.lines.get(row + ydisp)can be undefined for some rows in the dirty range (e.g. trim/scrollback edge cases while images remain referenced).3. Viewport resize: missing scrollback lines (
viewportResize)Bug: On widen,
viewportResize()walks the full scrollback (buffer.lines.length) and calledline.getBg(oldCol)without checking thatlines.get(row)returned a line. Gaps during resize/trim could throw and abort tile expansion for remaining rows.Fix: Skip missing lines with
continue(consistent withrender()).Repro:
Related
0ff7ec39269c443e05532b6d133e17bab84590a7also containedSelectionModeltrim fixes; those are not in this PR (see PR 4).Testing
npm run build && npm run esbuildnpm run test-unit -- addons/addon-image/out-esbuild/*.test.js— 16 passingnpm run test-integration -- --suite=addon-image— passed (4 skipped)