diff --git a/docs/manual-testing.md b/docs/manual-testing.md index 240d906..1e98521 100644 --- a/docs/manual-testing.md +++ b/docs/manual-testing.md @@ -27,6 +27,13 @@ WezTerm (and Ghostty when available) before tagging a release. stuck at its old position overlapping buffer text (issue #18). - [ ] Scrolling the image fully out of view then back in re-displays it without a visible retransmission delay (cache hit). +- [ ] `show()` the same PNG path twice at two different buffer lines + without `clear()`-ing the first in between (issue #10, multi-location + fan-out). Both copies must render correctly and simultaneously — not + just the second one, and not the first one moved/disappeared. Then + `clear()` only the first handle: the second must remain visible, + unaffected. Finally `clear()` the second handle too and confirm no + stray pixels remain from either. - [ ] Scrolling so the image is cut off at the top or bottom of the window shows a **cropped** slice of the image (the still-visible portion, correctly sized to the remaining cell span) instead of a blank gap or diff --git a/docs/spec/kitty-graphics.md b/docs/spec/kitty-graphics.md index eda1c9f..ea1f53f 100644 --- a/docs/spec/kitty-graphics.md +++ b/docs/spec/kitty-graphics.md @@ -29,7 +29,7 @@ Every command is an APC (Application Program Command) escape sequence: | `i` | image id | one of blit's reserved range, see below | | `q` | quiet | `2` (suppress all responses) always, see "Response handling" below | | `m` | more chunks | `1` (more chunks follow) / `0` (last chunk) | -| `p` | placement id | always `1` (blit's single fixed placement id — see "Placement" below) | +| `p` | placement id | a per-handle id, distinct across every concurrently-live placement — see "Placement" below | | `c`, `r` | placement columns/rows | shrink to the visible cell span when a placement is partially clipped, see "Source-rectangle cropping" below | | `x`, `y` | source rectangle pixel offset (left/top) into the transmitted image | present only when a placement is partially clipped; see "Source-rectangle cropping" below | | `w`, `h` | source rectangle pixel size | present only when a placement is partially clipped; see "Source-rectangle cropping" below | @@ -53,23 +53,34 @@ Every command is an APC (Application Program Command) escape sequence: ## Placement -- `a=p,i=,p=1` redisplays an already-transmitted image without resending - pixel data. This is how blit satisfies the performance rule that - scroll/resize redraws must reuse the existing id rather than +- `a=p,i=,p=` redisplays an already-transmitted image + without resending pixel data. This is how blit satisfies the performance + rule that scroll/resize redraws must reuse the existing id rather than re-transmitting. -- `p=1` (blit's fixed placement id, `terminal.PLACEMENT_ID`) is **always** - sent, on every placement command — both the initial `a=T` transmit+display - and every later `a=p` reposition. If `p=` is omitted, the terminal creates - a brand-new placement on every call instead of moving the existing one; - since blit repositions on every debounced `WinScrolled`/`WinResized` - redraw, this silently accumulates stacked "ghost" placements at each prior - screen position — visible as partial/duplicated image fragments while - scrolling, until a `a=d,d=i` delete (see below) clears all of them at - once. Reusing the same `i=` **and** `p=` pair on every call makes each - `a=p` update that one placement in place instead. blit never needs more - than one placement id per image id: `renderer.lua`'s cache only ever marks - a given image id "active" for a single handle at a time, so a constant - `p=1` can never collide with a second live placement of the same id. +- `p=` (a caller-supplied placement id, `blit.terminal.PlacementOpts.placement_id` + in `terminal.lua`) is **always** sent, on every placement command — both + the initial `a=T` transmit+display and every later `a=p` reposition. If + `p=` is omitted, the terminal creates a brand-new placement on every call + instead of moving the existing one; since blit repositions on every + debounced `WinScrolled`/`WinResized` redraw, this silently accumulates + stacked "ghost" placements at each prior screen position — visible as + partial/duplicated image fragments while scrolling, until a `a=d,d=i` + delete (see below) clears all of them at once. Reusing the same `i=` + **and** `p=` pair on every call makes each `a=p` update that one placement + in place instead. +- **One image id can have several concurrent placements (issue #10, + "Multi-location placement fan-out").** Earlier versions of blit sent a + single fixed `p=1` on every call, relying on `renderer.lua`'s cache never + marking a given image id "active" for more than one handle at a time. That + constraint is gone: `renderer.lua` now allocates a distinct, never-reused + placement id per handle (`alloc_placement_id()`, an ever-incrementing + session-lifetime counter — see `docs/spec/renderer-placement.md`'s + "Transmission cache" section), so a second `show()` of the same + `(path, mtime)` while the first is still live reuses the SAME image id + under a DIFFERENT placement id — a second, independent placement — instead + of transmitting a redundant copy under a new image id. Each placement is + then addressed, repositioned, and torn down independently by its own + `(id, placement_id)` pair. - Optional placement keys, in the fixed order blit emits them: `p=` (always present when placing), `x=`, `y=`, `w=`, `h=` (source rectangle, only when cropped — see "Source-rectangle cropping" below), `c=`, `r=` (target cell @@ -144,6 +155,17 @@ clip-amount math (`compute_clip`) and the pixel-space conversion - `a=d,d=i,i=` deletes the visible placement(s) for one image id blit owns; `d=I` additionally frees the terminal's stored pixel data for that id. +- `p=`, when supplied alongside `d=i`, restricts the delete to + that ONE placement of the id rather than every placement blit has made for + it — required now that one id can have several concurrent placements (see + "Placement" above). `renderer.lua` always includes it except for the + whole-id teardown case: freeing a handle's placement while the id might + still be shared by a sibling handle (`terminal.build_delete(id, { + placement_id = ... })`), vs. freeing the id's stored data entirely once no + handle references it anymore (`terminal.build_delete(id, { free_data = + true })`, no `p=`, since every placement is being torn down together at + that point anyway) — see `docs/spec/renderer-placement.md`'s Lifecycle + section for the full decision. - blit **never** emits `d=a` (delete every image on the terminal, including ones placed by other plugins like image.nvim/snacks.image). Every deletion path in `terminal.lua` is scoped to a single caller-supplied id. diff --git a/docs/spec/renderer-placement.md b/docs/spec/renderer-placement.md index 270b665..7818eaf 100644 --- a/docs/spec/renderer-placement.md +++ b/docs/spec/renderer-placement.md @@ -35,19 +35,26 @@ overrides an intentional stretch/fit. One handle = one entry in z_index, visible, native_width, native_height }`, matching `AGENTS.md`'s "one image = one handle table" rule. -**Known limitation**: a handle is bound to exactly one `win` at creation -time, but the `virt_lines` extmark carrying its reserved blank rows is -buffer-scoped, not window-scoped — Neovim renders those reserved rows in -*every* window currently showing that buffer. If the same buffer is split -into a second window (`:split`/`:vsplit`), the non-anchor window displays -the reserved blank space with no image in it, for as long as it stays -open — `compute_placement` only ever computes visibility/position against -the one `handle.win` it was given. A real fix requires per-window -placement-id fan-out (a distinct `p=` for each window showing the buffer), -which conflicts with the one-handle-per-placement data model above and is -already tracked separately (see "Transmission cache" below and issue #10, -"Multi-location placement fan-out for a single transmitted image"). -Accepted for v0.x; revisit when #10 ships. +**Known limitation, narrower since issue #10**: a handle is still bound to +exactly one `win` at creation time, but the `virt_lines` extmark carrying +its reserved blank rows is buffer-scoped, not window-scoped — Neovim +renders those reserved rows in *every* window currently showing that +buffer. If the same buffer is split into a second window +(`:split`/`:vsplit`), the non-anchor window displays the reserved blank +space with no image in it, for as long as it stays open — +`compute_placement` only ever computes visibility/position against the one +`handle.win` it was given. Issue #10 ("Multi-location placement fan-out for +a single transmitted image") added the underlying primitive this needs — a +distinct `p=` per placement, so the same transmitted image id can carry a +second, independent placement without re-transmitting (see "Transmission +cache" below) — but it does not by itself detect a `:split` and create that +second placement automatically: `M.show()` still only ever creates one +handle bound to one `win` per call. A caller can work around the split case +today by calling `M.show()` a second time for the second window explicitly +(same `path`, so the same `(path, mtime)` cache key) — that second call now +fans out onto the first's image id instead of re-transmitting, per issue +#10 — but automatic per-window fan-out for a single `show()` call remains +unimplemented. Accepted for v0.x. ## Screen coordinate conversion @@ -263,12 +270,15 @@ single write is transient and does not survive past that write. Deletion (`a=d`) is not screen-position-dependent, so no cursor bracketing is needed there. -## Transmission cache: (path, mtime) keyed, no placement-id fan-out +## Transmission cache: (path, mtime) keyed, with placement-id fan-out Cache key: `path .. ":" .. mtime.sec .. "." .. mtime.nsec` (from -`vim.uv.fs_stat`). Each cache entry is a **list** of `{ id, active, lines, -columns, native_width, native_height }` entries for that exact file content -— a list, not a single entry, because: +`vim.uv.fs_stat`). Each cache entry is a **list** of `{ id, +active_placements, lines, columns, native_width, native_height }` entries +for that exact file content — a list, not a single entry, because a given +key can transiently have both an idle entry and a stale-but-still-active +one (e.g. mid-Ghostty-resize-recovery, see below) even though the common +case settles to exactly one entry per key: `native_width`/`native_height` (the PNG's native pixel dimensions, read once via `lua/blit/png.lua`'s IHDR reader when the file is actually read for @@ -277,23 +287,41 @@ every handle that reuses the entry — a cache hit never re-reads or re-parses the file. These are used solely by `pixel_crop` (see "Visibility policy" above), never for auto-sizing. -- `show()` on a cache hit with an **idle** (`active = false`) entry reuses - that id: no re-transmission, just an `a=p` placement (or nothing yet, if - not currently visible) — this is the AGENTS.md performance rule +`active_placements` counts how many currently-live handles reference this +entry's id — 0 means idle. `show()`'s `find_reusable_entry` tries an idle +entry first (see the Ghostty exception below), and only if none exists +falls back to any entry with `active_placements > 0`: + +- **Idle reuse**: no re-transmission, just an `a=p` placement (or nothing + yet, if not currently visible) — this is the AGENTS.md performance rule ("re-placement... must reuse its ID — never re-transmit"). -- `show()` on a cache hit where every existing entry is **active** (already - placed live somewhere else) transmits a fresh copy under a new id instead - of reusing/relocating the active one. blit does emit kitty's placement-id - key (`p=`, always `terminal.PLACEMENT_ID`, a fixed constant — see - `docs/spec/kitty-graphics.md`'s Placement section), but that fixed value - only prevents ghost placements when *repositioning* an id's one - placement; it is not a per-location identity. A given image id therefore - still has only one live placement at a time — reusing an active id for a - second simultaneous location would silently move the first location's - image instead of adding a second one. Supporting true multi-location - fan-out for one transmitted image (a distinct, allocated `p=` per - location) is deferred and is not needed for the common case (showing one - image once, or showing it again after it was cleared). +- **Active reuse, i.e. fan-out (issue #10, "Multi-location placement + fan-out for a single transmitted image")**: also no re-transmission. + Earlier versions of blit always sent a single fixed placement id + (`terminal.PLACEMENT_ID = 1`) and therefore could give a given image id + only one live placement at a time — a second concurrent `show()` of the + same file had to transmit a redundant copy under a brand-new id. Every + real placement command now carries a distinct, caller-allocated + `placement_id` (`alloc_placement_id()`, defined right below "Placement id + allocation" — an ever-incrementing, never-reused, session-lifetime + counter, since placement ids only need to be unique within one image id, + not terminal-wide the way image ids do) — see `docs/spec/kitty-graphics.md`'s + Placement section — so a second `show()` of an already-active entry + instead adds a second, independent placement of the SAME id: `id` + unchanged, `entry.active_placements` incremented, a fresh + `handle.placement_id` allocated, and only an `a=p` (or nothing, if not yet + visible) written — matching the idle-reuse case exactly except that the + entry was never idle to begin with. + +`destroy_handle` (see "Lifecycle" below) is the inverse: it decrements +`active_placements` and deletes only this handle's own placement +(`a=d,d=i,i=,p=`) — a sibling placement sharing the same +id, if any, is untouched. The terminal-side pixel data itself is only ever +freed (`d=I`, no `p=`) once `active_placements` reaches `0` **and** the +caller's intent was to free it (`VimLeavePre`'s full teardown, or a fatal +failure right after `M.show()` created the handle) — an everyday +`clear()`/`clear_all()` never frees data even when it drops the count to +`0`, keeping the entry idle-but-warm for a future `show()` instead. Cache entries are otherwise never evicted except at `VimLeavePre` (or the test-only `_reset()`) — an idle entry's id and terminal-side pixel data are @@ -304,7 +332,9 @@ long session that `show()`s many distinct files; unbounded growth is accepted for v0.x (mirrors that memo's own acceptance of the range being merely "negligible collision risk", not infinite). `alloc_id()` returns `nil, err` if the range is exhausted; `show()` propagates that as a normal -`nil, err_msg` failure. +`nil, err_msg` failure. Placement ids draw from the full unsigned 32-bit +`p=` space rather than a bounded pool (see "Placement id allocation" in +`renderer.lua`), so they have no equivalent exhaustion case in practice. **Ghostty exception: idle entries recorded against a stale terminal size are evicted eagerly, at reuse time.** Ghostty discards previously- @@ -315,7 +345,7 @@ all responses, `docs/spec/kitty-graphics.md`'s "Response handling"), and an id for a placement-only `a=p` then renders nothing (issue #24). Each cache entry therefore also records `vim.o.lines`/`vim.o.columns` (the whole Neovim grid size, which tracks the real terminal's size — not a per-window -size) at the moment of transmission. `acquire_idle_entry` compares an idle +size) at the moment of transmission. `find_reusable_entry` compares an idle entry's recorded size against the current size only when `caps.terminal == "ghostty"`; a mismatch means a resize happened at some point since transmission, so the entry is treated as dead: its id is freed back to the @@ -331,7 +361,7 @@ means a persistent autocmd outside the handle-gated `augroup` — directly conflicting with AGENTS.md's "no timers or autocmds active when zero images are displayed" performance rule. The size-comparison approach needs no autocmd at all: it only ever runs inside `show()`'s own -`acquire_idle_entry` call. **Accepted false negative**: if the terminal is +`find_reusable_entry` call. **Accepted false negative**: if the terminal is resized away and back to the *exact* original `lines`/`columns` before the next `show()`, the comparison can't tell that a resize happened in between, and a dead entry could still be handed out. This is deemed rare @@ -352,7 +382,7 @@ restore it, since nothing in `redraw_all()`'s normal `a=p`/`a=d` toggling ever re-transmits. `redraw_all()` now checks, for each handle it finds visible, whether `caps.terminal == "ghostty"` and that handle's own cache entry's recorded `vim.o.lines`/`vim.o.columns` differs from the current -values (the exact same signal `acquire_idle_entry` uses above, just read +values (the exact same signal `find_reusable_entry` uses above, just read instead of also gating reuse — see `ghostty_entry_stale()` in `renderer.lua`). @@ -363,9 +393,9 @@ this path beyond a naive "re-`a=T`, same id" attempt: will not restore a placement by re-`a=T`-ing under an id it already discarded the data for, even though the write itself reports success (`q=2` suppresses all responses, so blit has no way to detect this other - than the empirical result). `retransmit_and_place()` therefore frees the - stale id and hands the handle a *fresh* one via `alloc_id()`, exactly - mirroring how `acquire_idle_entry` above already treats a stale idle + than the empirical result). `retransmit_and_place_group()` therefore frees the + stale id and hands every handle sharing it a *fresh* one via `alloc_id()`, + exactly mirroring how `find_reusable_entry` above already treats a stale idle entry (free the old id, never reuse it) — it just also updates the now-live handle's `id` field and cache entry in place rather than waiting for a future `show()` call to do so. @@ -397,7 +427,7 @@ number of retries of its own (issue #37).** `ghostty_retransmit_pass()` can find a handle still `ghostty_entry_stale()` after it runs even though no further `WinResized`/`WinScrolled` restarted the timer: `compute_placement()` can come back `nil` for that one tick (a real drag-resize's tail end can -still race `vim.fn.screenpos()`), or `retransmit_and_place()`'s write itself +still race `vim.fn.screenpos()`), or `retransmit_and_place_group()`'s write itself can fail (`write_all()` exhausting its bounded EAGAIN retries — see `terminal.lua`). Before this was fixed, either case left the placement blank until the user happened to trigger another resize purely by luck — @@ -466,11 +496,17 @@ Two distinct kinds of state transition, kept separate: - **Destroy** (`BufWinLeave` for the specific `(buf, win)` pair, `WinClosed` for a closing window, `BufWipeout` for a wiped buffer, `M.clear()`/`M.clear_all()`, and `VimLeavePre`): removes the extmark, - removes the handle from `M._handles`, marks its cache entry idle, and - deletes its terminal-side placement. Only `VimLeavePre` (and - `M.clear`/`M.clear_all`'s eventual full-teardown path) also frees the - id back to the pool and the terminal's stored pixel data - (`d=I`) — everyday `clear()` keeps the cache warm. + removes the handle from `M._handles`, decrements its cache entry's + `active_placements`, and deletes only THIS handle's own placement + (`a=d,d=i,i=,p=`) — never the whole id, since a fan-out + sibling (issue #10) may still hold a live placement on it. Only once + `active_placements` reaches `0` **and** the caller's intent was to free + data (`VimLeavePre`'s full teardown, or a fatal failure right after + `M.show()` created the handle) does `destroy_handle` also free the id back + to the pool and the terminal's stored pixel data (`d=I`, no `p=`, since + every placement sharing the id is gone by then) — everyday `clear()` never + frees data, keeping the entry idle-but-warm instead, regardless of + whether it just dropped to `0`. **`a=d` is retried a bounded number of times after destroy too, not just on the redraw path above.** Unlike a still-tracked invisible handle, a @@ -478,21 +514,34 @@ Two distinct kinds of state transition, kept separate: WezTerm drops, there is no future `redraw_all()` pass left that would ever revisit it, and no guarantee a `WinScrolled`/`WinResized` event even fires again afterward to trigger one (issue #27; the repro is `clear_all()` - followed by *no* further input at all). `destroy_handle`'s `free_data = - false` path (the everyday teardown reasons above, not `VimLeavePre`) - therefore queues its id onto a small self-scheduled retry list - (`DESTROY_DELETE_RETRIES`, currently 3 extra attempts) that rides the same - debounce timer as `redraw_all`, resending a plain `a=d` each pass until - the budget runs out. Bounded, unlike the redraw path's resend-indefinitely - behavior, so a terminal that never honors the delete can't keep the - debounce timer (and therefore the "fully quiescent idle" guarantee) alive - forever. `VimLeavePre`'s `free_data = true` path is excluded: it frees the - id immediately, and Neovim is exiting right after, so a queued retry has - nothing meaningful left to protect and only risks racing a reused id - against a process that's already gone. If `acquire_idle_entry` reclaims a - still-queued id for a fresh placement before its retries are spent, the - queued entry is cancelled — otherwise a late retry could send `a=d` for an - id a brand new placement now legitimately owns. + followed by *no* further input at all). `destroy_handle`'s everyday- + teardown path (any call where the caller didn't request a full free, or + did but a fan-out sibling still shares the id — see above) therefore + queues its `(id, placement_id)` pair onto a small self-scheduled retry + list (`DESTROY_DELETE_RETRIES`, currently 3 extra attempts) that rides the + same debounce timer as `redraw_all`, resending the same scoped `a=d` each + pass until the budget runs out. Bounded, unlike the redraw path's resend- + indefinitely behavior, so a terminal that never honors the delete can't + keep the debounce timer (and therefore the "fully quiescent idle" + guarantee) alive forever. The full-free branch (id actually freed) is + always excluded from queuing — nothing else references that id anymore. + The fan-out-sibling-still-shares-the-id case is excluded from queuing + only when the caller is truly shutting down (`opts.shutting_down`, set by + `VimLeavePre` and the test-only `_reset()`): the process (or test run) is + exiting right after, so a queued retry has nothing meaningful left to + protect and only risks racing a reused id against a process that's + already gone. A fatal `M.show()` failure tearing down a fan-out handle + whose sibling is still live does NOT set `shutting_down` — the process + keeps running and no future `redraw_all()` pass will ever revisit this + specific `(id, placement_id)` again once the handle is gone, so it gets + the same bounded retry as everyday teardown instead (issue #10). If + `find_reusable_entry` reclaims a still-queued id for a fresh + placement before its retries are spent, the queued entries for that id + are cancelled (`cancel_pending_delete`) — though even without that, a late + retry naming the OLD `placement_id` could never hit the new placement's + DIFFERENT one, since placement ids are never reused (see "Placement id + allocation" in `renderer.lua`); the cancellation is belt-and-suspenders + against wasted escape-sequence bytes, not a correctness requirement. All autocmds live in one `augroup("blit", { clear = true })`; all extmarks in one `nvim_create_namespace("blit")`, both created lazily on first `show()` diff --git a/lua/blit/renderer.lua b/lua/blit/renderer.lua index d9618fa..83f927d 100644 --- a/lua/blit/renderer.lua +++ b/lua/blit/renderer.lua @@ -27,6 +27,10 @@ local NAMESPACE = "blit" ---@class blit.Handle ---@field id integer kitty image id (blit's reserved range) +---@field placement_id integer this handle's own kitty placement id (`p=`); +---never reused across handles within a session, so several handles can +---share one image {id} — each with its own live placement — without ever +---colliding (issue #10). See docs/spec/kitty-graphics.md's Placement section. ---@field buf integer ---@field win integer ---@field extmark_id integer @@ -94,9 +98,29 @@ local function free_id(id) used_ids[id] = nil end +-- Placement id allocation ------------------------------------------------------ +-- Unlike image ids, placement ids only need to be unique within one image +-- id, not terminal-wide — so a single ever-incrementing counter (never +-- freed/reused, unlike alloc_id's bounded pool) is sufficient: at any +-- realistic show()/clear() rate this session-lifetime counter would take +-- centuries to approach the 32-bit `p=` value space. Never reusing a +-- placement id number is also what makes a stray delayed retry from +-- destroy_handle's retry queue (see "Destroy-path delete retry queue" +-- below) provably harmless without needing its own cancellation bookkeeping +-- — it can never coincide with a later, still-live placement. + +local next_placement_id = 1 + +---@return integer +local function alloc_placement_id() + local pid = next_placement_id + next_placement_id = next_placement_id + 1 + return pid +end + -- Transmission cache ---------------------------------------------------------- ----@type table +---@type table local cache = {} ---@param path string @@ -114,7 +138,7 @@ M.cache_key = cache_key -- silently renders nothing (issue #24). Each cache entry therefore records -- the Neovim grid size (`vim.o.lines`/`vim.o.columns`, which tracks the -- real terminal's size, not just a per-window size) at transmit time; --- acquire_idle_entry rejects (and frees) an idle entry recorded against a +-- find_reusable_entry rejects (and frees) an idle entry recorded against a -- stale size on Ghostty, forcing a fresh transmit instead of trusting dead -- data. This is a lazy, reuse-time check rather than a `VimResized` -- listener specifically to avoid needing a persistent autocmd outside the @@ -125,13 +149,13 @@ M.cache_key = cache_key -- -- The same size-mismatch signal also drives redraw_all's still-visible- -- handle path below (issue #34): a handle that stays active/displayed --- across a Ghostty resize is not touched by acquire_idle_entry at all (it's +-- across a Ghostty resize is not touched by find_reusable_entry at all (it's -- never idle), so without this it stayed permanently blank once its data -- was discarded — see "Redraw" below and -- docs/spec/renderer-placement.md's Transmission cache section. -- -- This one fact — which terminal actually has this quirk — is centralized --- here rather than compared inline at each call site, so acquire_idle_entry, +-- here rather than compared inline at each call site, so find_reusable_entry, -- redraw_all's stale check, and its resize-race redraw guard all agree on -- the same definition as more terminals/quirks are added over time. ---@param terminal_name "kitty"|"wezterm"|"ghostty"|nil @@ -155,23 +179,45 @@ end local DESTROY_DELETE_RETRIES = 3 ----@type { id: integer, retries: integer }[] +---@type { id: integer, placement_id: integer, retries: integer }[] local pending_deletes = {} +-- Removes every still-pending retry for {id}, regardless of which +-- placement_id each was queued for. Reused-id-only reasoning, kept as +-- belt-and-suspenders even though placement ids are never reused (see +-- "Placement id allocation" above, which already makes a stray retry +-- harmless on its own): avoids wasting escape-sequence bytes resending +-- deletes for placements this reuse has nothing to do with. ---@param id integer local function cancel_pending_delete(id) - for i, pending in ipairs(pending_deletes) do - if pending.id == id then + local i = 1 + while i <= #pending_deletes do + if pending_deletes[i].id == id then table.remove(pending_deletes, i) - return + else + i = i + 1 end end end +-- Finds an existing cache entry this key's next show() can reuse instead of +-- transmitting fresh pixel data — either an IDLE entry (no handle currently +-- references its id) or, failing that, an ACTIVE one to fan out onto (issue +-- #10: a second, independent placement of the same id, via a fresh +-- placement id — no re-transmission needed). Idle entries are preferred +-- first only because that's also the only branch that needs the Ghostty +-- staleness check below: an idle entry has no live handle whose own +-- redraw-pass check (`ghostty_entry_stale`, see "Redraw" below) would ever +-- notice/self-heal a stale one, so staleness has to be caught here, at +-- reuse time, instead. An active entry always has at least one live handle +-- already doing that self-healing check every redraw pass, so reusing it +-- for fan-out even while transiently stale is safe — the settle timer will +-- catch up every placement sharing that id together (see +-- "retransmit_and_place_group" below). ---@param key string ---@param terminal_name "kitty"|"wezterm"|"ghostty"|nil ----@return { id: integer, active: boolean, lines: integer, columns: integer, native_width: integer, native_height: integer }? -local function acquire_idle_entry(key, terminal_name) +---@return { id: integer, active_placements: integer, lines: integer, columns: integer, native_width: integer, native_height: integer }? +local function find_reusable_entry(key, terminal_name) local entries = cache[key] if not entries then return nil @@ -179,7 +225,7 @@ local function acquire_idle_entry(key, terminal_name) local i = 1 while i <= #entries do local entry = entries[i] - if entry.active then + if entry.active_placements > 0 then i = i + 1 elseif discards_pixels_on_resize(terminal_name) @@ -188,15 +234,18 @@ local function acquire_idle_entry(key, terminal_name) free_id(entry.id) table.remove(entries, i) else - entry.active = true - -- The id may still have a bounded delete retry outstanding from a + -- The id may still have bounded delete retries outstanding from a -- prior destroy (see "Destroy-path delete retry queue" above); this - -- reuse legitimately reclaims it, so a late retry must not delete the - -- placement being made here. + -- reuse legitimately reclaims it. cancel_pending_delete(entry.id) return entry end end + for _, entry in ipairs(entries) do + if entry.active_placements > 0 then + return entry + end + end return nil end @@ -208,7 +257,7 @@ local function register_cache_entry(key, id, native_width, native_height) cache[key] = cache[key] or {} table.insert(cache[key], { id = id, - active = true, + active_placements = 1, lines = vim.o.lines, columns = vim.o.columns, native_width = native_width, @@ -218,7 +267,7 @@ end ---@param key string ---@param id integer ----@return { id: integer, active: boolean, lines: integer, columns: integer, native_width: integer, native_height: integer }? +---@return { id: integer, active_placements: integer, lines: integer, columns: integer, native_width: integer, native_height: integer }? local function find_cache_entry(key, id) local entries = cache[key] if not entries then @@ -232,21 +281,6 @@ local function find_cache_entry(key, id) return nil end ----@param key string ----@param id integer -local function mark_cache_entry_idle(key, id) - local entries = cache[key] - if not entries then - return - end - for _, entry in ipairs(entries) do - if entry.id == id then - entry.active = false - return - end - end -end - ---@param key string ---@param id integer local function drop_cache_entry(key, id) @@ -548,6 +582,7 @@ end ---@return blit.terminal.PlacementOpts local function placement_opts(handle, placement) return { + placement_id = handle.placement_id, columns = placement.cols, rows = placement.rows, z_index = handle.z_index, @@ -575,14 +610,18 @@ local function place_existing(handle, placement) return ok, err end +-- Scoped to this handle's own placement_id — with multi-location fan-out +-- (issue #10) an image id can have several concurrent placements, so an +-- unscoped `a=d,d=i,i=` would wipe out every sibling placement sharing +-- this id, not just this handle's own. ---@param handle blit.Handle local function hide_existing(handle) - M._write_fn({ terminal.build_delete(handle.id) }) + M._write_fn({ terminal.build_delete(handle.id, { placement_id = handle.placement_id }) }) handle.visible = false end -- On Ghostty, a still-visible handle's transmitted data can have been --- silently discarded by the same real terminal resize acquire_idle_entry +-- silently discarded by the same real terminal resize find_reusable_entry -- guards against for idle entries (issue #24/#34) — see the "Transmission -- cache" comment above. There is no response to detect this by, so the -- only signal available is the same one: the handle's cache entry was @@ -601,60 +640,133 @@ local function ghostty_entry_stale(handle, terminal_name) return entry.lines ~= vim.o.lines or entry.columns ~= vim.o.columns end --- Re-transmits a still-visible handle's pixel data under a *fresh* id and --- updates the handle/cache to point at it, freeing the old, now-dead one — --- confirmed via manual testing on a real Ghostty resize (issue #34) that --- simply re-`a=T`-ing under the SAME id Ghostty already discarded does NOT --- bring the placement back, even though the write itself reports success --- (`q=2` suppresses all responses, so blit has no way to detect this other --- than the empirical result). This mirrors `acquire_idle_entry`'s existing --- Ghostty eviction above exactly: that path never reuses a stale id either — --- it frees it and lets a fresh `alloc_id()` hand out a new one on the next --- transmit. This is the one place outside `M.show()` that transmits rather --- than reusing `a=p` — see AGENTS.md's performance rule on reuse-over- --- retransmit for why that's normally forbidden and the narrow, Ghostty-only --- exception carved out here. ----@param handle blit.Handle ----@param placement blit.PlacementResult +---@param id integer +---@return blit.Handle[] +local function handles_with_id(id) + local out = {} + for _, h in ipairs(M._handles) do + if h.id == id then + out[#out + 1] = h + end + end + return out +end + +-- Re-transmits a stale id's pixel data under a *fresh* id and migrates +-- EVERY handle currently sharing it (issue #10 fan-out means a stale id can +-- have several live placements, not just one), freeing the old, now-dead id +-- once every one of them has moved off it — confirmed via manual testing on +-- a real Ghostty resize (issue #34) that simply re-`a=T`-ing under the SAME +-- id Ghostty already discarded does NOT bring the placement back, even +-- though the write itself reports success (`q=2` suppresses all responses, +-- so blit has no way to detect this other than the empirical result). This +-- mirrors `find_reusable_entry`'s existing Ghostty eviction above exactly: +-- that path never reuses a stale id either — it frees it and lets a fresh +-- `alloc_id()` hand out a new one on the next transmit. This is the one +-- place outside `M.show()` that transmits rather than reusing `a=p` — see +-- AGENTS.md's performance rule on reuse-over-retransmit for why that's +-- normally forbidden and the narrow, Ghostty-only exception carved out +-- here. +-- +-- Migrating the group atomically (one delete+transmit for the shared id, +-- not one per handle) matters for correctness, not just efficiency: freeing +-- the old id's cache entry after only the FIRST sharing handle's retransmit +-- would leave every other handle still pointing at an id whose cache entry +-- (and therefore `ghostty_entry_stale`'s only signal) has already vanished +-- — they would never be recognized as needing a retry again, and this +-- pass's own `still_stale` bookkeeping would silently miss them too. +-- Handles with no current placement (scrolled off, wrong tab, etc.) are +-- still migrated to the new id — so a later redraw pass places them +-- correctly once they become visible again — but only the ones WITH a +-- current placement need an actual `a=p`/`a=T` written now; one of them +-- (`display_handle`) carries the transmit itself, the rest just add a +-- placement (`a=p`) against the data it just sent. +---@param handles blit.Handle[] every handle currently sharing one stale id ---@return boolean ok -local function retransmit_and_place(handle, placement) - local bytes = read_file(handle.path) +local function retransmit_and_place_group(handles) + local first = handles[1] + local old_id = first.id + + local bytes = read_file(first.path) if not bytes then - hide_existing(handle) + for _, h in ipairs(handles) do + hide_existing(h) + end return false end local new_id = alloc_id() if not new_id then - hide_existing(handle) + for _, h in ipairs(handles) do + hide_existing(h) + end return false end - local old_id = handle.id - local sequences = { - terminal.build_delete(old_id, { free_data = true }), - terminal.build_save_cursor(), - terminal.build_move_cursor(placement.screen_row, placement.screen_col), - } - vim.list_extend( - sequences, - terminal.build_transmit( - bytes, - { id = new_id, action = "T", placement = placement_opts(handle, placement) } + ---@type table + local placements = {} + local display_handle = nil + for _, h in ipairs(handles) do + placements[h] = compute_placement(h) + if placements[h] and not display_handle then + display_handle = h + end + end + + local sequences = { terminal.build_delete(old_id, { free_data = true }) } + + if display_handle then + vim.list_extend(sequences, { + terminal.build_save_cursor(), + terminal.build_move_cursor( + placements[display_handle].screen_row, + placements[display_handle].screen_col + ), + }) + vim.list_extend( + sequences, + terminal.build_transmit(bytes, { + id = new_id, + action = "T", + placement = placement_opts(display_handle, placements[display_handle]), + }) ) - ) - table.insert(sequences, terminal.build_restore_cursor()) + table.insert(sequences, terminal.build_restore_cursor()) + else + -- No handle sharing this id is currently visible; keep the data ready + -- (transmit-only) so whichever handle becomes visible next places + -- correctly against the new id without needing its own re-transmit. + vim.list_extend(sequences, terminal.build_transmit(bytes, { id = new_id, action = "t" })) + end + + for _, h in ipairs(handles) do + if h ~= display_handle and placements[h] then + vim.list_extend(sequences, { + terminal.build_save_cursor(), + terminal.build_move_cursor(placements[h].screen_row, placements[h].screen_col), + terminal.build_placement(new_id, placement_opts(h, placements[h])), + terminal.build_restore_cursor(), + }) + end + end local ok = M._write_fn(sequences) if ok then - handle.id = new_id - drop_cache_entry(handle.cache_key, old_id) + drop_cache_entry(first.cache_key, old_id) free_id(old_id) - register_cache_entry(handle.cache_key, new_id, handle.native_width, handle.native_height) + register_cache_entry(first.cache_key, new_id, first.native_width, first.native_height) + local new_entry = find_cache_entry(first.cache_key, new_id) + new_entry.active_placements = #handles + for _, h in ipairs(handles) do + h.id = new_id + h.visible = placements[h] ~= nil + end else free_id(new_id) + for _, h in ipairs(handles) do + h.visible = false + end end - handle.visible = ok and true or false return ok end @@ -699,8 +811,9 @@ local function arm_ghostty_retransmit_timer() end -- A handle can still be `ghostty_entry_stale()` after this pass runs: --- `retransmit_and_place()` itself can fail (e.g. `write_all()` exhausted its --- bounded EAGAIN retries), or `compute_placement()` can come back `nil` for +-- `retransmit_and_place_group()` itself can fail (e.g. `write_all()` +-- exhausted its bounded EAGAIN retries), or `compute_placement()` can come +-- back `nil` for -- this one settle-timer tick even though the handle's cache entry is still -- stale — a real drag-resize's tail end can still race `vim.fn.screenpos()` -- (see its pcall guard in `compute_placement`) at the exact moment the @@ -726,10 +839,32 @@ local ghostty_retransmit_retries_left = GHOSTTY_RETRANSMIT_RETRIES ghostty_retransmit_pass = function() local caps = M._detect_fn() local still_stale = false + -- One id can now have several sharing handles (fan-out); process each + -- stale id's whole group together via retransmit_and_place_group rather + -- than per-handle, and only once per id per pass. + local processed_ids = {} for _, handle in ipairs(M._handles) do - if ghostty_entry_stale(handle, caps.terminal) then - local placement = compute_placement(handle) - if not (placement and retransmit_and_place(handle, placement)) then + if not processed_ids[handle.id] and ghostty_entry_stale(handle, caps.terminal) then + processed_ids[handle.id] = true + local group = handles_with_id(handle.id) + local any_visible = false + for _, h in ipairs(group) do + if compute_placement(h) then + any_visible = true + break + end + end + if any_visible then + if not retransmit_and_place_group(group) then + still_stale = true + end + else + -- Mirrors the single-handle behavior this replaces: a stale group + -- with nothing currently visible is left alone rather than + -- transmitted speculatively, but still counts as "still stale" so + -- the bounded retry budget keeps checking back in case a + -- momentary screenpos() race (not genuine long-term invisibility) + -- clears up within the next pass or two. still_stale = true end end @@ -807,7 +942,9 @@ local function redraw_all() if #pending_deletes > 0 then local still_pending = {} for _, pending in ipairs(pending_deletes) do - M._write_fn({ terminal.build_delete(pending.id) }) + M._write_fn({ + terminal.build_delete(pending.id, { placement_id = pending.placement_id }), + }) pending.retries = pending.retries - 1 if pending.retries > 0 then table.insert(still_pending, pending) @@ -830,10 +967,33 @@ end -- Lifecycle: destroy ------------------------------------------------------ +-- Whether it's safe to fully free {handle}'s id (terminal-side pixel data +-- + the id itself) depends on whether any OTHER handle still shares it via +-- fan-out (issue #10), not just on the caller's free_data intent: freeing +-- data out from under a sibling placement that still needs it would blank +-- it. free_data therefore only ever triggers the full free once THIS +-- handle is the last one referencing the id (`remaining <= 0`); otherwise +-- only this handle's own placement is torn down (`d=i,p=`, +-- data untouched) and the id/cache entry are left alone for whichever +-- handle(s) still use it. Since VimLeavePre destroys every handle in one +-- pass, the LAST handle sharing an id to be destroyed always ends up taking +-- the full-free branch, so the id and its data are still guaranteed to be +-- fully released by the time VimLeavePre finishes — it just may not be +-- THIS particular call that does it. +-- +-- `shutting_down` is a separate axis from `free_data`: it's only true for +-- the true end-of-session call sites (`VimLeavePre`, the test-only +-- `_reset()`), where skipping the destroy-path delete retry queue below is +-- safe because nothing is left running to revisit a dropped delete anyway. +-- A `free_data = true, shutting_down = false` call (M.show()'s own failure +-- paths tearing down a fan-out handle whose sibling is still live) keeps +-- the retry: the process keeps running and no future redraw pass will ever +-- revisit this specific (id, placement_id) again once the handle is gone. ---@param handle blit.Handle ----@param opts? { free_data?: boolean } +---@param opts? { free_data?: boolean, shutting_down?: boolean } local function destroy_handle(handle, opts) - local free_data = (opts and opts.free_data) or false + local free_data_requested = (opts and opts.free_data) or false + local shutting_down = (opts and opts.shutting_down) or false if vim.api.nvim_buf_is_valid(handle.buf) then pcall(vim.api.nvim_buf_del_extmark, handle.buf, ensure_namespace(), handle.extmark_id) @@ -846,24 +1006,54 @@ local function destroy_handle(handle, opts) end end - M._write_fn({ terminal.build_delete(handle.id, { free_data = free_data }) }) + local entry = find_cache_entry(handle.cache_key, handle.id) + local remaining = entry and math.max(0, entry.active_placements - 1) or 0 + if entry then + entry.active_placements = remaining + end - if free_data then + if free_data_requested and remaining <= 0 then + M._write_fn({ terminal.build_delete(handle.id, { free_data = true }) }) free_id(handle.id) drop_cache_entry(handle.cache_key, handle.id) - else - mark_cache_entry_idle(handle.cache_key, handle.id) - -- Only the free_data=false path (clear()/clear_all(), BufWinLeave, - -- WinClosed, BufWipeout) gets a retry: its id stays reserved and its - -- cache entry stays around for reuse, so a late retry always still - -- refers to either this same dead placement or nothing (cancelled via - -- cancel_pending_delete if acquire_idle_entry reclaims the id first). - -- VimLeavePre's free_data=true call frees the id immediately, so a - -- queued retry there could race a reused id after Neovim exits/the - -- process is gone anyway — not worth the risk for a shutdown path. - table.insert(pending_deletes, { id = handle.id, retries = DESTROY_DELETE_RETRIES }) - schedule_redraw() + return + end + + M._write_fn({ terminal.build_delete(handle.id, { placement_id = handle.placement_id }) }) + + if free_data_requested and shutting_down then + -- A sibling placement still shares this id; VimLeavePre's own remaining + -- iterations will eventually take the full-free branch above once the + -- last one goes. No retry queued here: Neovim is exiting right after, + -- so a queued retry has nothing meaningful left to protect. This is + -- distinct from the free_data_requested-but-not-shutting_down case + -- below (a fatal M.show() failure tearing down a fan-out handle mid- + -- session) — there, the process keeps running and no future redraw + -- pass will ever revisit this specific (id, placement_id) again, so a + -- dropped delete would otherwise orphan the placement for as long as + -- the sibling stays alive (issue #10). + return end + + -- Everyday teardown (clear()/clear_all(), BufWinLeave, WinClosed, + -- BufWipeout, or a fatal M.show() failure on a fan-out handle whose + -- sibling is still live) never frees data here: the id's cache entry + -- stays around — idle if this was the last handle sharing it, still + -- active otherwise — so a later show() of the same file (or a sibling + -- fan-out placement) stays cheap. A late retry always still refers to + -- either this same dead placement or nothing (cancelled via + -- cancel_pending_delete if find_reusable_entry reclaims the id first); it + -- can never hit a DIFFERENT live placement, since placement ids are never + -- reused (see "Placement id allocation" above). Only the true-shutdown + -- case above (VimLeavePre, `_reset()`) is excluded from queuing: the + -- process/test run is ending right after, so a queued retry has nothing + -- meaningful left to protect. + table.insert(pending_deletes, { + id = handle.id, + placement_id = handle.placement_id, + retries = DESTROY_DELETE_RETRIES, + }) + schedule_redraw() end local autocmds_ready = false @@ -920,7 +1110,7 @@ end local function on_vim_leave_pre() local handles = vim.list_extend({}, M._handles) for _, handle in ipairs(handles) do - destroy_handle(handle, { free_data = true }) + destroy_handle(handle, { free_data = true, shutting_down = true }) end maybe_teardown_autocmds() terminal.reset_writer() @@ -1101,7 +1291,10 @@ function M.show(path, opts) debounce_ms = opts.debounce_ms or debounce_ms local key = cache_key(path, stat.mtime) - local entry = acquire_idle_entry(key, caps.terminal) + -- Either an idle entry (ordinary re-show of a cleared handle) or an + -- active one (issue #10 fan-out: a second, concurrent placement of the + -- same already-live id) — either way, no re-transmission needed. + local entry = find_reusable_entry(key, caps.terminal) local id, bytes, needs_transmit, native_width, native_height if entry then @@ -1109,6 +1302,7 @@ function M.show(path, opts) native_width = entry.native_width native_height = entry.native_height needs_transmit = false + entry.active_placements = entry.active_placements + 1 else local read_err bytes, read_err = read_file(path) @@ -1150,6 +1344,7 @@ function M.show(path, opts) ---@type blit.Handle local handle = { id = id, + placement_id = alloc_placement_id(), buf = buf, win = win, extmark_id = extmark_id, @@ -1234,7 +1429,7 @@ end function M._reset() local handles = vim.list_extend({}, M._handles) for _, handle in ipairs(handles) do - destroy_handle(handle, { free_data = true }) + destroy_handle(handle, { free_data = true, shutting_down = true }) end M._handles = {} -- Discard any still-outstanding destroy-path retries (see "Destroy-path @@ -1246,6 +1441,7 @@ function M._reset() cache = {} used_ids = {} next_id = terminal.ID_RANGE_START + next_placement_id = 1 M._write_fn = function(sequences) return terminal.write(sequences) end diff --git a/lua/blit/terminal.lua b/lua/blit/terminal.lua index ccdc0ef..25f26bd 100644 --- a/lua/blit/terminal.lua +++ b/lua/blit/terminal.lua @@ -13,6 +13,7 @@ local APC_END = ESC .. "\\" ---@alias blit.terminal.Action "T"|"t" ---@class blit.terminal.PlacementOpts +---@field placement_id integer this placement's `p=` id — see below ---@field columns? integer ---@field rows? integer ---@field z_index? integer @@ -28,14 +29,25 @@ local APC_END = ESC .. "\\" ---@field quiet? 0|1|2 -- default 2 ---@field placement? blit.terminal.PlacementOpts --- Every blit placement uses this single fixed placement id. renderer.lua's --- cache never marks the same image id "active" for more than one handle at --- once, so a given id has at most one live placement at any time — reusing --- a constant placement id means a reposition (`a=p,i=,p=1,...`) UPDATES --- that placement in place. Omitting `p=` entirely (or varying it) makes the --- terminal create an additional, stacked placement instead of moving the --- existing one — see docs/spec/kitty-graphics.md's Placement section. -M.PLACEMENT_ID = 1 +-- Every real placement command carries a caller-supplied `p=` (placement +-- id), scoping it to one specific placement of one image id rather than +-- "the" placement — the same image id can now have several concurrent +-- placements (issue #10, "Multi-location placement fan-out"). Reusing the +-- SAME (id, placement_id) pair across calls makes a reposition +-- (`a=p,i=,p=,...`) UPDATE that one placement in place, exactly as +-- the old fixed `p=1` did; a DIFFERENT placement_id on the same id instead +-- adds a second, independent placement without re-sending pixel data. +-- Allocating distinct ids per placement — never reusing one across two +-- concurrently-live placements — is renderer.lua's responsibility (it owns +-- the handle table), per AGENTS.md's architecture; terminal.lua only knows +-- how to encode whatever id it's given. See +-- docs/spec/kitty-graphics.md's Placement section. + +---@param v any +---@return boolean +local function is_positive_integer(v) + return type(v) == "number" and v == math.floor(v) and v > 0 +end ---@param parts { [1]: string, [2]: string|integer }[] ---@return string @@ -125,6 +137,13 @@ function M.build_transmit(png_bytes, opts) end, "0, 1, or 2", }, + placement = { + opts.placement, + function(v) + return v == nil or is_positive_integer(v.placement_id) + end, + "a table with a positive integer placement_id, or nil", + }, }) local action = opts.action or "T" @@ -149,7 +168,7 @@ function M.build_transmit(png_bytes, opts) { "q", quiet }, } if opts.placement then - parts[#parts + 1] = { "p", M.PLACEMENT_ID } + parts[#parts + 1] = { "p", opts.placement.placement_id } end append_placement_parts(parts, opts.placement) parts[#parts + 1] = { "m", more } @@ -163,22 +182,44 @@ function M.build_transmit(png_bytes, opts) end ---@param id integer ----@param opts? blit.terminal.PlacementOpts +---@param opts blit.terminal.PlacementOpts ---@return string sequence function M.build_placement(id, opts) - vim.validate({ id = { id, M.is_valid_id, "a valid id in blit's reserved range" } }) - local parts = { { "a", "p" }, { "i", id }, { "p", M.PLACEMENT_ID } } + vim.validate({ + id = { id, M.is_valid_id, "a valid id in blit's reserved range" }, + opts = { opts, "table" }, + placement_id = { opts.placement_id, is_positive_integer, "a positive integer" }, + }) + local parts = { { "a", "p" }, { "i", id }, { "p", opts.placement_id } } append_placement_parts(parts, opts) return APC_START .. build_control(parts) .. APC_END end ---@param id integer ----@param opts? { free_data?: boolean } +---@param opts? { free_data?: boolean, placement_id?: integer } ---@return string sequence function M.build_delete(id, opts) - vim.validate({ id = { id, M.is_valid_id, "a valid id in blit's reserved range" } }) + vim.validate({ + id = { id, M.is_valid_id, "a valid id in blit's reserved range" }, + placement_id = { + opts and opts.placement_id, + function(v) + return v == nil or is_positive_integer(v) + end, + "a positive integer, or nil", + }, + }) local d = (opts and opts.free_data) and "I" or "i" local parts = { { "a", "d" }, { "d", d }, { "i", id } } + -- `p=` restricts the delete to one specific placement of this id instead + -- of every placement blit has made for it — required now that one id can + -- have several concurrent placements (issue #10). Omitted only for the + -- whole-id teardown case (free_data=true once no handle references the + -- id anymore — see renderer.lua's destroy_handle), where every placement + -- is being torn down together anyway. + if opts and opts.placement_id then + parts[#parts + 1] = { "p", opts.placement_id } + end return APC_START .. build_control(parts) .. APC_END end @@ -200,12 +241,6 @@ function M.build_restore_cursor() return ESC .. "8" end ----@param v any ----@return boolean -local function is_positive_integer(v) - return type(v) == "number" and v == math.floor(v) and v > 0 -end - ---@param row integer 1-indexed screen row ---@param col integer 1-indexed screen column ---@return string diff --git a/tests/test_renderer.lua b/tests/test_renderer.lua index 81d64d6..bc4adf0 100644 --- a/tests/test_renderer.lua +++ b/tests/test_renderer.lua @@ -321,13 +321,71 @@ T["show"]["cache hit on idle entry places without retransmitting"] = function() MiniTest.expect.equality(all:find("a=p", 1, true) ~= nil, true) end -T["show"]["active cache entry forces a fresh id, never relocates"] = function() +T["show"]["active cache entry fans out onto the same id, no retransmit (issue #10)"] = function() local buf, win = setup_floating(numbered_lines(10), 20, 10) local first = renderer.show(tmp_path, { width = 5, height = 3, buf = buf, win = win, lnum = 2 }) + MiniTest.expect.equality(first.visible, true) + + captured = {} local second = renderer.show(tmp_path, { width = 5, height = 3, buf = buf, win = win, lnum = 4 }) + MiniTest.expect.equality(second.visible, true) + + -- Same image id (no re-transmission), but each placement carries its own + -- distinct placement_id so neither one ever displaces the other. + MiniTest.expect.equality(second.id, first.id) + MiniTest.expect.no_equality(second.placement_id, first.placement_id) - MiniTest.expect.no_equality(first.id, second.id) + local all = table.concat(captured[1], "") + MiniTest.expect.equality(all:find("a=T", 1, true), nil) + MiniTest.expect.equality(all:find("a=p", 1, true) ~= nil, true) + MiniTest.expect.equality(all:find(",p=" .. second.placement_id, 1, true) ~= nil, true) +end + +T["show"]["clearing one fanned-out handle leaves its sibling's placement alone (issue #10)"] = function() + local buf, win = setup_floating(numbered_lines(10), 20, 10) + + local first = renderer.show(tmp_path, { width = 5, height = 3, buf = buf, win = win, lnum = 2 }) + local second = renderer.show(tmp_path, { width = 5, height = 3, buf = buf, win = win, lnum = 4 }) + + captured = {} + renderer.clear(first) + + -- Scoped to first's own placement_id only: second's placement_id never + -- appears in the delete blit sends. + local all = table.concat(captured[1], "") + MiniTest.expect.equality( + all, + ESC .. "_Ga=d,d=i,i=" .. first.id .. ",p=" .. first.placement_id .. ESC .. "\\" + ) + MiniTest.expect.equality(all:find(",p=" .. second.placement_id, 1, true), nil) + MiniTest.expect.equality(second.visible, true) +end + +T["show"]["VimLeavePre frees data only once the last fanned-out handle is gone (issue #10)"] = function() + local buf, win = setup_floating(numbered_lines(10), 20, 10) + + local first = renderer.show(tmp_path, { width = 5, height = 3, buf = buf, win = win, lnum = 2 }) + local second = renderer.show(tmp_path, { width = 5, height = 3, buf = buf, win = win, lnum = 4 }) + MiniTest.expect.equality(first.id, second.id) + + captured = {} + vim.api.nvim_exec_autocmds("VimLeavePre", {}) + + local free_count, scoped_count = 0, 0 + for _, seq in ipairs(captured) do + local all = table.concat(seq, "") + if all:find("d=I", 1, true) then + free_count = free_count + 1 + elseif all:find("d=i", 1, true) then + scoped_count = scoped_count + 1 + end + end + -- One handle's teardown only removes its own placement (the id is still + -- shared); the other, destroyed once nothing references the id anymore, + -- is the one that actually frees the terminal-side pixel data. + MiniTest.expect.equality(free_count, 1) + MiniTest.expect.equality(scoped_count, 1) end T["show"]["ghostty: reuses idle cache entry when terminal size is unchanged"] = function() @@ -483,12 +541,19 @@ T["show"]["re-placing a later handle catches up an earlier handle's stale positi { width = 5, height = 3, buf = buf, win = win, lnum = 1, debounce_ms = 5 } ) MiniTest.expect.equality(b.visible, true) - MiniTest.expect.no_equality(a.id, b.id) + -- Same file, both still active: b fans out onto a's id (issue #10) rather + -- than getting a fresh one, but each keeps its own placement_id. + MiniTest.expect.equality(a.id, b.id) + MiniTest.expect.no_equality(a.placement_id, b.placement_id) local function a_was_replaced() for _, seq in ipairs(captured) do local all = table.concat(seq, "") - if all:find("a=p", 1, true) and all:find("i=" .. a.id, 1, true) then + if + all:find("a=p", 1, true) + and all:find("i=" .. a.id, 1, true) + and all:find(",p=" .. a.placement_id, 1, true) + then return true end end @@ -509,7 +574,10 @@ T["clear"]["deletes placement and extmark"] = function() renderer.clear(handle) local all = table.concat(captured[1], "") - MiniTest.expect.equality(all, ESC .. "_Ga=d,d=i,i=" .. handle.id .. ESC .. "\\") + MiniTest.expect.equality( + all, + ESC .. "_Ga=d,d=i,i=" .. handle.id .. ",p=" .. handle.placement_id .. ESC .. "\\" + ) local ns = vim.api.nvim_create_namespace("blit") local mark = vim.api.nvim_buf_get_extmark_by_id(buf, ns, handle.extmark_id, {}) @@ -537,7 +605,10 @@ T["clear"]["self-retries a=d a bounded number of times with no further events (i MiniTest.expect.equality(#captured, 4) for _, seq in ipairs(captured) do local all = table.concat(seq, "") - MiniTest.expect.equality(all, ESC .. "_Ga=d,d=i,i=" .. handle.id .. ESC .. "\\") + MiniTest.expect.equality( + all, + ESC .. "_Ga=d,d=i,i=" .. handle.id .. ",p=" .. handle.placement_id .. ESC .. "\\" + ) end -- The retry budget is bounded: nothing further is sent once it's spent. @@ -609,7 +680,10 @@ T["redraw"]["hides when the anchor line scrolls past the top edge (issue #28)"] MiniTest.expect.equality(handle.visible, false) local all = table.concat(captured[1], "") - MiniTest.expect.equality(all, ESC .. "_Ga=d,d=i,i=" .. handle.id .. ESC .. "\\") + MiniTest.expect.equality( + all, + ESC .. "_Ga=d,d=i,i=" .. handle.id .. ",p=" .. handle.placement_id .. ESC .. "\\" + ) end T["redraw"]["reissues a=d on every pass while invisible past the top edge (issue #28)"] = function() @@ -643,7 +717,10 @@ T["redraw"]["reissues a=d on every pass while invisible past the top edge (issue MiniTest.expect.equality(handle.visible, false) MiniTest.expect.equality(#captured, 1) local all = table.concat(captured[1], "") - MiniTest.expect.equality(all, ESC .. "_Ga=d,d=i,i=" .. handle.id .. ESC .. "\\") + MiniTest.expect.equality( + all, + ESC .. "_Ga=d,d=i,i=" .. handle.id .. ",p=" .. handle.placement_id .. ESC .. "\\" + ) end T["redraw"]["hides once the window shrinks below the reserved rows"] = function() @@ -667,7 +744,10 @@ T["redraw"]["hides once the window shrinks below the reserved rows"] = function( MiniTest.expect.equality(handle.visible, false) local all = table.concat(captured[1], "") - MiniTest.expect.equality(all, ESC .. "_Ga=d,d=i,i=" .. handle.id .. ESC .. "\\") + MiniTest.expect.equality( + all, + ESC .. "_Ga=d,d=i,i=" .. handle.id .. ",p=" .. handle.placement_id .. ESC .. "\\" + ) end T["redraw"]["reissues a=d on every pass while still invisible (issue #23)"] = function() @@ -702,7 +782,10 @@ T["redraw"]["reissues a=d on every pass while still invisible (issue #23)"] = fu MiniTest.expect.equality(handle.visible, false) MiniTest.expect.equality(#captured, 1) local all = table.concat(captured[1], "") - MiniTest.expect.equality(all, ESC .. "_Ga=d,d=i,i=" .. handle.id .. ESC .. "\\") + MiniTest.expect.equality( + all, + ESC .. "_Ga=d,d=i,i=" .. handle.id .. ",p=" .. handle.placement_id .. ESC .. "\\" + ) end T["redraw"]["hides on TabLeave and restores on TabEnter (issue #16)"] = function() @@ -721,7 +804,10 @@ T["redraw"]["hides on TabLeave and restores on TabEnter (issue #16)"] = function end) MiniTest.expect.equality(handle.visible, false) local hide_all = table.concat(captured[1], "") - MiniTest.expect.equality(hide_all, ESC .. "_Ga=d,d=i,i=" .. handle.id .. ESC .. "\\") + MiniTest.expect.equality( + hide_all, + ESC .. "_Ga=d,d=i,i=" .. handle.id .. ",p=" .. handle.placement_id .. ESC .. "\\" + ) captured = {} vim.cmd("tabclose") @@ -854,7 +940,10 @@ T["redraw"]["source-rect crop"]["hides once the reserved block has fully scrolle MiniTest.expect.equality(handle.visible, false) local all = table.concat(captured[1], "") - MiniTest.expect.equality(all, ESC .. "_Ga=d,d=i,i=" .. handle.id .. ESC .. "\\") + MiniTest.expect.equality( + all, + ESC .. "_Ga=d,d=i,i=" .. handle.id .. ",p=" .. handle.placement_id .. ESC .. "\\" + ) end T["redraw"]["source-rect crop"]["two handles anchored at the same lnum hide instead of risking a wrong crop"] = function() @@ -1018,6 +1107,68 @@ T["redraw"]["ghostty: retries a failed retransmit without a further resize event MiniTest.expect.no_equality(handle.id, original_id) end +T["redraw"]["ghostty: migrates a whole fanned-out group sharing a stale id together (issue #10)"] = function() + renderer._detect_fn = function() + return { terminal = "ghostty", tmux = false, gui_embed = false, supported = true } + end + local buf, win = setup_floating(numbered_lines(10), 20, 10) + local first = renderer.show( + tmp_path, + { width = 5, height = 3, buf = buf, win = win, lnum = 2, debounce_ms = 5 } + ) + local second = renderer.show( + tmp_path, + { width = 5, height = 3, buf = buf, win = win, lnum = 4, debounce_ms = 5 } + ) + MiniTest.expect.equality(first.visible, true) + MiniTest.expect.equality(second.visible, true) + MiniTest.expect.equality(first.id, second.id) + local original_id = first.id + + local original_columns = vim.o.columns + vim.o.columns = original_columns + 1 + captured = {} + vim.api.nvim_exec_autocmds("WinResized", { pattern = tostring(win) }) + + vim.wait(1000, function() + return any_captured_has("a=T") + end) + vim.o.columns = original_columns + + -- Both siblings migrate onto the SAME fresh id, atomically: exactly one + -- a=T (the shared pixel data, sent once — not re-transmitted per + -- handle), exactly one a=p placing the other sibling onto that same new + -- id, and the old id freed exactly once, not per-handle. + MiniTest.expect.equality(first.visible, true) + MiniTest.expect.equality(second.visible, true) + MiniTest.expect.equality(first.id, second.id) + MiniTest.expect.no_equality(first.id, original_id) + + local transmit_count, free_count, sibling_placed = 0, 0, false + for _, seq in ipairs(captured) do + local all = table.concat(seq, "") + if all:find("a=T", 1, true) then + transmit_count = transmit_count + 1 + end + if all:find("d=I", 1, true) and all:find("i=" .. original_id, 1, true) then + free_count = free_count + 1 + end + if + all:find("a=p", 1, true) + and all:find("i=" .. first.id, 1, true) + and ( + all:find(",p=" .. second.placement_id, 1, true) ~= nil + or all:find(",p=" .. first.placement_id, 1, true) ~= nil + ) + then + sibling_placed = true + end + end + MiniTest.expect.equality(transmit_count, 1) + MiniTest.expect.equality(free_count, 1) + MiniTest.expect.equality(sibling_placed, true) +end + T["redraw"]["ghostty: does not retransmit a still-visible handle when size is unchanged"] = function() renderer._detect_fn = function() return { terminal = "ghostty", tmux = false, gui_embed = false, supported = true } diff --git a/tests/test_terminal_escape.lua b/tests/test_terminal_escape.lua index 0a0c793..1d802ee 100644 --- a/tests/test_terminal_escape.lua +++ b/tests/test_terminal_escape.lua @@ -89,36 +89,47 @@ T["build_transmit"]["action t (transmit-only) differs only in a="] = function() MiniTest.expect.equality(display_seq:gsub("a=T", "a=t"), transmit_seq) end -T["build_transmit"]["combined transmit+placement carries the fixed placement id"] = function() +T["build_transmit"]["combined transmit+placement carries the caller-supplied placement id"] = function() local png_bytes = "hi" - local sequences = - terminal.build_transmit(png_bytes, { id = ID, placement = { columns = 10, rows = 5 } }) + local sequences = terminal.build_transmit( + png_bytes, + { id = ID, placement = { placement_id = 7, columns = 10, rows = 5 } } + ) local payload = vim.base64.encode(png_bytes) local expected = ESC .. "_Ga=T,f=100,t=d,i=" .. ID - .. ",q=2,p=" - .. terminal.PLACEMENT_ID - .. ",c=10,r=5,m=0;" + .. ",q=2,p=7,c=10,r=5,m=0;" .. payload .. ESC .. "\\" MiniTest.expect.equality(sequences, { expected }) end +T["build_transmit"]["placement without a placement_id is rejected"] = function() + local ok = pcall(terminal.build_transmit, "hi", { id = ID, placement = { columns = 10 } }) + MiniTest.expect.equality(ok, false) +end + T["build_transmit"]["combined transmit+placement carries crop keys before c=/r="] = function() local png_bytes = "hi" local sequences = terminal.build_transmit(png_bytes, { id = ID, - placement = { crop_x = 1, crop_y = 2, crop_w = 3, crop_h = 4, columns = 10, rows = 5 }, + placement = { + placement_id = 1, + crop_x = 1, + crop_y = 2, + crop_w = 3, + crop_h = 4, + columns = 10, + rows = 5, + }, }) local payload = vim.base64.encode(png_bytes) local expected = ESC .. "_Ga=T,f=100,t=d,i=" .. ID - .. ",q=2,p=" - .. terminal.PLACEMENT_ID - .. ",x=1,y=2,w=3,h=4,c=10,r=5,m=0;" + .. ",q=2,p=1,x=1,y=2,w=3,h=4,c=10,r=5,m=0;" .. payload .. ESC .. "\\" @@ -127,7 +138,7 @@ end T["build_transmit"]["deterministic across repeated calls"] = function() local png_bytes = "some bytes" - local opts = { id = ID, placement = { columns = 10, rows = 5 } } + local opts = { id = ID, placement = { placement_id = 1, columns = 10, rows = 5 } } local first = terminal.build_transmit(png_bytes, opts) local second = terminal.build_transmit(png_bytes, opts) MiniTest.expect.equality(first, second) @@ -136,61 +147,56 @@ end T["build_placement"] = MiniTest.new_set() T["build_placement"]["minimal"] = function() - local seq = terminal.build_placement(ID) - MiniTest.expect.equality( - seq, - ESC .. "_Ga=p,i=" .. ID .. ",p=" .. terminal.PLACEMENT_ID .. ESC .. "\\" - ) + local seq = terminal.build_placement(ID, { placement_id = 3 }) + MiniTest.expect.equality(seq, ESC .. "_Ga=p,i=" .. ID .. ",p=3" .. ESC .. "\\") +end + +T["build_placement"]["requires a positive integer placement_id"] = function() + local ok = pcall(terminal.build_placement, ID, {}) + MiniTest.expect.equality(ok, false) end T["build_placement"]["with options in canonical order"] = function() - local seq = - terminal.build_placement(ID, { columns = 10, rows = 5, z_index = 3, no_move_cursor = true }) - MiniTest.expect.equality( - seq, - ESC .. "_Ga=p,i=" .. ID .. ",p=" .. terminal.PLACEMENT_ID .. ",c=10,r=5,z=3,C=1" .. ESC .. "\\" + local seq = terminal.build_placement( + ID, + { placement_id = 3, columns = 10, rows = 5, z_index = 3, no_move_cursor = true } ) + MiniTest.expect.equality(seq, ESC .. "_Ga=p,i=" .. ID .. ",p=3,c=10,r=5,z=3,C=1" .. ESC .. "\\") end T["build_placement"]["source-rect crop keys precede target cell box, in fixed order"] = function() local seq = terminal.build_placement( ID, - { crop_x = 5, crop_y = 10, crop_w = 20, crop_h = 30, columns = 4, rows = 6 } + { placement_id = 3, crop_x = 5, crop_y = 10, crop_w = 20, crop_h = 30, columns = 4, rows = 6 } ) MiniTest.expect.equality( seq, - ESC - .. "_Ga=p,i=" - .. ID - .. ",p=" - .. terminal.PLACEMENT_ID - .. ",x=5,y=10,w=20,h=30,c=4,r=6" - .. ESC - .. "\\" + ESC .. "_Ga=p,i=" .. ID .. ",p=3,x=5,y=10,w=20,h=30,c=4,r=6" .. ESC .. "\\" ) end T["build_placement"]["crop_x/crop_y of 0 are emitted, not treated as absent"] = function() - local seq = terminal.build_placement(ID, { crop_x = 0, crop_y = 0, crop_w = 5, crop_h = 5 }) + local seq = terminal.build_placement( + ID, + { placement_id = 3, crop_x = 0, crop_y = 0, crop_w = 5, crop_h = 5 } + ) MiniTest.expect.equality(seq:find(",x=0,", 1, true) ~= nil, true) MiniTest.expect.equality(seq:find(",y=0,", 1, true) ~= nil, true) end T["build_placement"]["no crop keys when omitted, unchanged from existing placement bytes"] = function() - local seq = terminal.build_placement(ID, { columns = 10, rows = 5 }) + local seq = terminal.build_placement(ID, { placement_id = 3, columns = 10, rows = 5 }) MiniTest.expect.equality(seq:find(",x=", 1, true), nil) MiniTest.expect.equality(seq:find(",y=", 1, true), nil) MiniTest.expect.equality(seq:find(",w=", 1, true), nil) MiniTest.expect.equality(seq:find(",h=", 1, true), nil) end -T["build_placement"]["always uses the same fixed placement id across calls"] = function() - local first = terminal.build_placement(ID, { columns = 10, rows = 5 }) - local second = terminal.build_placement(ID, { columns = 12, rows = 6 }) - local function placement_id(sequence) - return sequence:match(",p=(%d+),") - end - MiniTest.expect.equality(placement_id(first), placement_id(second)) +T["build_placement"]["a different placement_id on the same image id fans out (issue #10)"] = function() + local first = terminal.build_placement(ID, { placement_id = 1, columns = 10, rows = 5 }) + local second = terminal.build_placement(ID, { placement_id = 2, columns = 10, rows = 5 }) + MiniTest.expect.equality(first:find(",p=1,", 1, true) ~= nil, true) + MiniTest.expect.equality(second:find(",p=2,", 1, true) ~= nil, true) end T["build_delete"] = MiniTest.new_set() @@ -205,8 +211,13 @@ T["build_delete"]["free_data uses d=I"] = function() MiniTest.expect.equality(seq, ESC .. "_Ga=d,d=I,i=" .. ID .. ESC .. "\\") end +T["build_delete"]["placement_id scopes the delete to one placement (issue #10)"] = function() + local seq = terminal.build_delete(ID, { placement_id = 5 }) + MiniTest.expect.equality(seq, ESC .. "_Ga=d,d=i,i=" .. ID .. ",p=5" .. ESC .. "\\") +end + T["build_delete"]["never emits delete-all"] = function() - for _, opts in ipairs({ nil, { free_data = true }, { free_data = false } }) do + for _, opts in ipairs({ nil, { free_data = true }, { free_data = false }, { placement_id = 5 } }) do local seq = terminal.build_delete(ID, opts) MiniTest.expect.equality(seq:find("d=a", 1, true), nil) end