Hold pictures as runs on a line, so a resize stops destroying them - #34
Merged
Conversation
A width change dropped every image in the buffer, so widening a window lost your pictures. The reasoning in the comment was right about reflow -- re-wrapping a logical line copies ranges of cells, and tiles carried through that would reassemble as a shuffled mosaic -- but it applied to lines that actually re-wrap, while the code dropped everything on any width change at all. That is a symptom, and the cause is where a picture is stored. A decoded bitmap was scattered across cells, each carrying an image reference and a tile coordinate, which makes the cell grid the picture's only storage: anything that truncates or overwrites cells destroys pixels, and there is nowhere else for them to be. A picture is now a LinePlacement -- image id, anchor column, natural width in cells, and a source rectangle -- held by the line it appears on. A picture spanning eight rows is eight runs, one per line, so ownership, scrolling and scrollback eviction work exactly as they did: the line still owns the image and its death still releases it, with no eviction pass. The decision that does the work is that Cols is the NATURAL width and is never clipped. The renderer draws min(Cols, line width). So a resize does nothing to images: narrowing shows less of a picture, widening shows more, and nothing is destroyed or needs restoring. Only a wrap chain still drops its runs, which is the case the original reasoning actually covered. It also fixes a case that could not be fixed before: a picture written into a narrow window was born as a strip, because only the clipped cells existed. The decoder always produced the whole bitmap, so widening now reveals the rest. Sixel's replace-on-write becomes explicit rather than incidental. Overwriting a cell used to destroy a tile for free; SplitPlacementsAt now divides a run around the written columns, narrowing each fragment's source rectangle. Guarded on a null field, so a line with no pictures -- nearly every line -- pays one test. SetCell, Fill and Clone are wired the same way, Clone copying the runs being what stops a cloned line silently losing its picture. BufferCell.Image and ImageTile are gone. A cell is a struct with no idea which line or column it came from, so it cannot answer for a run anchored to both; the question moves to the line as TryGetPlacementAt and TryGetImageAt. This is the API change raised in tomlm#33 -- Iciclecreek's DrawImageRun reads the old members, so the renderer needs the matching change. Three of the existing tests asserted the old behaviour and are replaced by their inverse, with the reasoning recorded in each: that a width change drops images, that a cell under a picture differs from a space, and that two cells showing different tiles are unequal. The last two mattered because renderers coalesce adjacent cells by comparing them -- and cells beneath a picture should now merge like the spaces they are, since nothing about a picture is drawn from cells. 854 tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The current budget-eviction path can drop non-doomed images and can retain strong references to images that no longer have placements, risking incorrect eviction and memory retention.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes image loss on terminal width resize by moving image storage from per-cell tiles to per-line “placements” (runs), so resizing only changes what portion of each run is visible rather than destroying image data in the cell grid.
Changes:
- Introduces
Graphics.LinePlacementand stores image runs onBufferLineinstead ofBufferCell. - Updates Sixel placement to add one run per line and updates resize logic to only drop images for wrap/reflow chains.
- Updates image-related tests to assert the new “runs on lines” behavior and adds
ImageAssertionshelpers.
File summaries
| File | Description |
|---|---|
| src/XTerm.NET/Terminal.cs | Updates image budget sweep to query images via line placements and clear images via runs. |
| src/XTerm.NET/InputHandler.cs | Changes Sixel placement from per-tile cell writes to per-line run placement. |
| src/XTerm.NET/Graphics/LinePlacement.cs | Adds the new placement/run model with truncation helpers. |
| src/XTerm.NET/Buffer/TerminalBuffer.cs | Adjusts resize behavior to only drop images for wrap-chain lines. |
| src/XTerm.NET/Buffer/BufferLine.cs | Adds placement/image storage, lookup helpers, and “replace-on-write” splitting for Sixel semantics. |
| src/XTerm.NET/Buffer/BufferCell.cs | Removes image/tile fields and updates equality/hash behavior accordingly. |
| src/XTerm.NET.Tests/Graphics/SixelPlacementTests.cs | Migrates assertions from cell image fields to placement/run assertions. |
| src/XTerm.NET.Tests/Graphics/SixelDecoderTests.cs | Updates decoder tests to query images via TryGetImageAt. |
| src/XTerm.NET.Tests/Graphics/ImageCellLifetimeTests.cs | Updates lifetime/resize tests for run-based image ownership and visibility. |
| src/XTerm.NET.Tests/Graphics/ImageAssertions.cs | Adds helpers for querying placements/images at a screen position and counting visible/total covered cells. |
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Three review catches on this PR, all the same root cause: _images was treated as something maintained alongside _placements rather than derived from them, so the two could disagree. DropImages cleared the whole line when it found one doomed image, taking that line's other pictures with it -- more destructive than the per-cell code this replaced, and it evicted images the sweep had just decided to keep. BufferLine.RemoveImages now removes only the runs showing doomed images. SplitPlacementsAt released images only once every run was gone, so printing over one picture on a line that showed two kept the overwritten one alive. Worse than a leak: the budget sweep decides what is live by walking runs, so a picture with no runs left was invisible to it and could never be reclaimed. PruneImages rebuilds ownership from the runs and is called wherever runs are removed. CollectLiveImages asked every column for its image, which costs more than it needs to and undercounts: a column covered by two overlapping runs reports only the first, so the second could be doomed while still on screen. BufferLine.Images exposes the distinct list to walk instead. Both regressions are pinned by tests driven through the public budget, and both were checked the only way worth trusting -- by reverting each fix in turn and watching its test fail. 852 tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Aug 27, 2026
Merged
Owner
|
hm...can we do this PR after Kitty PR? I a lot changed to implement kitty. |
Collaborator
Author
|
They are both going to be all over each other. If it's ok with you, Kitty was next on my list actually. This PR is the foundation that makes it run so much faster. Lets merge this then I'll get my perf work in and then rebase/redo kitty for you. 13x faster just to give you an example. |
tomlm
approved these changes
Aug 27, 2026
Owner
|
I'm rebasing my kitty on top of #34 |
tomlm
added a commit
to JohnCampionJr/XTerm.NET
that referenced
this pull request
Aug 27, 2026
Rebuilds the protocol work on top of tomlm#34, where a picture is a run held by the line rather than tiles scattered through cells. The protocol layer -- transmission, the delete matrix, placeholders, animation -- is unchanged and storage-independent. What changed is everything that touched a cell. PlaceImage emits one LinePlacement per row, each with its own slice of the source, taken from the placement rather than the image because a Kitty placement may be cropped and scaled: row 3 of a stretched box is not row 3 of the picture at its natural size. Overlap stopped needing a mechanism. Two pictures over the same columns are two runs, and covering one has no way to modify it -- so a translucent picture blends over what it covers and deleting the front one reveals the back one whole, both for free. The BufferCell.Below chain this branch used to carry is gone, along with the layer inserts, the removal rules and the character-preservation rule that went with them. What runs DO need is an identity. A placement spanning eight rows is eight structs on eight lines, and a positional delete finds it through one cell of one of them, so LinePlacement.Serial says which placement a run belongs to. That is the terminal's own identity, not Kitty's p=, which is the client's, may be zero and may repeat. ImagePlacement.Sequence went the other way: it existed to break a z-index tie between two cell layers, and age now comes from the order runs were added to the line. Two behaviours changed, both toward the protocol. A Kitty placement is an OVERLAY, so printing over one no longer destroys it -- the character lands, the z-index decides which is drawn on top, and deleting the picture gives the character back. Sixel keeps replace-on-write, which is what PlacementKind is for. And erasing now takes overlays as well as content: printing splits only a Sixel, but a cleared cell is blank and a picture showing through one would be a leak whichever protocol placed it. Two bugs found on the way over. A combining mark could attach to a cell showing a picture, because the guard against it had tested a cell field that no longer exists -- which also swallowed the placeholder diacritics that state a tile. And re-tiling a placeholder read its current tile back from the cell; recomputing it from the origin instead made each mark undo the last, so a row-then-column pair kept only the column. 1034 tests pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TzGenEw6pqZGGS2cwqb9AS
Merged
This was referenced Aug 28, 2026
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.
This isn't just a resize bug, it's also part of my perf work, which I will land after this.
Fixes the resize bug from #33, by changing where a picture is stored. Independent of #32 — different parts of
TerminalBuffer.Resize, and they rebase cleanly in either order.The bug
Any width change dropped every image in the buffer, so widening a window lost your pictures.
The reasoning in the comment was right about reflow — re-wrapping a logical line copies ranges of cells, and tiles carried through that would reassemble as a shuffled mosaic. But it applies to lines that actually re-wrap, while the code dropped everything on any width change at all.
The cause
A decoded bitmap was scattered across cells, each carrying an image reference and a tile coordinate. That makes the cell grid the picture's only storage: anything that truncates or overwrites cells destroys pixels, because there is nowhere else for them to be.
The change
A picture is now a
LinePlacement— image id, anchor column, natural width in cells, source rectangle — held by the line it appears on. A picture spanning eight rows is eight runs, one per line, so ownership, scrolling and scrollback eviction work exactly as they did: the line still owns the image, and its death still releases it with no eviction pass.The decision that does the work:
Colsis the natural width and is never clipped. The renderer drawsmin(Cols, line width). So a resize does nothing to images — narrowing shows less, widening shows more, nothing is destroyed and nothing needs restoring. Only a wrap chain still drops its runs, which is the case the original reasoning actually covered.It also fixes something that couldn't be fixed before: a picture written into a narrow window was born as a strip, because only the clipped cells existed. The decoder always produced the whole bitmap, so widening now reveals the rest.
What this costs
BufferCell.ImageandImageTileare gone. This is option 1 from #33 — a cell is a struct with no idea which line or column it came from, so it can't answer for a run anchored to both. The question moves toline.TryGetPlacementAt(col)andline.TryGetImageAt(col).Iciclecreek's
DrawImageRunreads the old members, so that repo needs the matching change. I have it working locally against both renderers and can open it whenever this lands.Sixel's replace-on-write is now explicit. Overwriting a cell used to destroy a tile for free;
SplitPlacementsAtdivides a run around the written columns and narrows each fragment's source rect. Guarded on a null field, so a line with no pictures — nearly every line — pays one test.SetCell,FillandCloneare wired the same way.Tests
Three existing tests asserted the old behaviour and are replaced by their inverse, each with the reasoning recorded inline:
A_change_of_width_drops_the_images→A_change_of_width_keeps_the_images, plus a narrow-and-widen round trip.An_image_cell_is_not_equal_to_a_plain_spaceandCells_showing_different_tiles_are_not_equal→ a cell under a picture is an ordinary space now, and should compare equal to one. Those two existed because renderers coalesce adjacent cells by comparing them and merging two different tiles would draw the wrong thing — but nothing about a picture is drawn from cells any more, so cells beneath one should merge like the spaces they are. What must stay distinguishable is the runs, and there's a test for that.854 → 850 passing on this branch (the difference is #32's tests, which aren't here).