Placeholder tiles are content: printing over the cell removes the tile - #156
Conversation
A Unicode-placeholder tile exists because its cell holds the placeholder character; the protocol offers no command to remove one tile, because overwriting the cell IS the removal. The tiles were stored as classic Kitty overlay runs, which text never modifies, so an application that drew a dialog across a picture kept the picture on top of it permanently and navigating away could not erase it. A third PlacementKind, Placeholder, carries the content semantics: the split that printing performs on Sixel runs now takes placeholder runs too (a one-column run simply goes), while classic Kitty placements keep their overlay behaviour and z-ordering untouched. Writing a placeholder cell also clears any stale tile another picture left there, since the cell write itself now splits. The test asserting the old behaviour encoded the bug rather than the protocol and now asserts the removal; classic-overlay survival under text is pinned by its own new test so the two semantics cannot be conflated again. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CxS1BnzZ6BhuGovMLNtRQH
Perf comparison — this change, against its base3 run(s) of each side, alternating on one machine. Allocation is a count and is gated exactly. Time is a measurement, so its gate is derived from the spread this job just observed in itself rather than fixed in advance.
Each corpus is gated at assemblies measured
Perf comparison — cumulative, everything since 2.0.03 run(s) of each side, alternating on one machine. Allocation is a count and is gated exactly. Time is a measurement, so its gate is derived from the spread this job just observed in itself rather than fixed in advance.
Each corpus is gated at assemblies measured
|
Clone already copies them ("the runs are the picture"); CopyFrom silently
dropped them, so a host capturing frames through pooled line copies could
not represent image rows and had to read them live -- the remaining tear
for screens drawn entirely as kitty Unicode placeholders. Replace
semantics, like every other field: a recycled line that once held a
picture must not keep showing it. The lists are copied, the images shared;
a stored picture is never mutated in place, so copies stay safe.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XmKfhMXUAy4n3uLDSUBHo8
There was a problem hiding this comment.
🟡 Changes recommended
There are a couple of newly introduced comments that contradict the actual implementation (and a missing regression test for the new CopyFrom image/placement behavior), which should be corrected before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR aligns Kitty Unicode placeholder tiles with “content” semantics so that overwriting a placeholder cell removes the corresponding tile (matching kitty/image.nvim behavior), while preserving classic Kitty placement overlay/z-index behavior. It also fixes BufferLine.CopyFrom to carry image placement state so line copies don’t silently drop images.
Changes:
- Introduce
PlacementKind.Placeholderand treat placeholder-tile placements as content (split/removed on text writes), while leaving classic Kitty placements as overlays. - Update placeholder writing to emit
PlacementKind.Placeholderso buffer write paths can clear/replace tiles correctly. - Fix
BufferLine.CopyFromto copy placements/images and add/adjust tests covering the intended semantics.
File summaries
| File | Description |
|---|---|
| src/XTerm.NET/InputHandler.KittyGraphics.cs | Marks placeholder-tile placements as Placeholder so overwrites clear tiles. |
| src/XTerm.NET/Graphics/LinePlacement.cs | Adds PlacementKind.Placeholder with documentation for content semantics. |
| src/XTerm.NET/Buffer/BufferLine.cs | Updates placement-splitting logic for placeholder tiles; fixes CopyFrom to copy placement/image lists. |
| src/XTerm.NET.Tests/Graphics/KittyZIndexTests.cs | Adds regression test ensuring classic Kitty placements remain overlays under text. |
| src/XTerm.NET.Tests/Graphics/KittyPlaceholderTests.cs | Updates placeholder behavior test to assert tile removal on overwrite; adds replacement test. |
Review details
- Files reviewed: 5/5 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.
| _placements = line._placements is null | ||
| ? null | ||
| : new List<Graphics.LinePlacement>(line._placements); | ||
| _images = line._images is null | ||
| ? null | ||
| : new List<Graphics.TerminalImage>(line._images); |
| // The runs are the picture, exactly as in Clone above -- a copy that skipped them would | ||
| // silently lose it. Replaced rather than merged, like every field here: a recycled line | ||
| // that once held a picture must not keep showing it. The lists are copied but the images | ||
| // they name are shared; a stored picture is never mutated in place (a retransmission | ||
| // builds a new one), so a reader holding this copy is safe against later writes. |
A Unicode-placeholder tile exists because its cell holds the placeholder character; the protocol has no command to remove one tile, because overwriting the cell IS the removal. Tiles were stored as classic Kitty overlay runs, which text never modifies, so anything drawn across a placeholder picture (a dialog, a new screen) kept the picture on top of it permanently.
Perf: the only hot-path-adjacent change is the kind test inside SplitPlacementsAt, which flipped from
!= Sixelto== Kitty— same single compare, and the method only runs on lines that hold placements.Found from the consumer side: Consolonia's kitty renderer overwrites occluded placeholder cells exactly as the spec prescribes, and the pictures stayed on top of its dialogs.
BufferLine.CopyFrom carries placements and images
Clonealready copies them ("the runs are the picture");CopyFromsilently dropped them. A host that double-buffers frames through pooled line copies (Iciclecreek.Avalonia.Terminal'sFrameCapture) therefore could not represent image rows and had to read them live — which, for a screen drawn entirely as kitty Unicode placeholders, was the remaining paint-mid-write tear (a black band sweeping the picture). Replace semantics, like every other field there: a recycled line that once held a picture must not keep showing it. The lists are copied and the images shared — a stored picture is never mutated in place (a retransmission builds a new one), so holders of a copy are safe against later writes.Consumed by tomlm/Iciclecreek.Avalonia.Terminal#166, which captures image rows only when this fix is present and falls back to live reads against older emulators.
Suite: 2216 passed / 0 failed.
🤖 Generated with Claude Code
https://claude.ai/code/session_01CxS1BnzZ6BhuGovMLNtRQH
https://claude.ai/code/session_01XmKfhMXUAy4n3uLDSUBHo8