Repository navigation
feat(terminal): render cropped graphics placements and present decoded frames - #818
Closed
glitch-cmyk658 wants to merge 11 commits into
Closed
glitch-cmyk658 wants to merge 11 commits into
glitch-cmyk658 wants to merge 11 commits into
Conversation
The startup card was only ever printed for PowerShell. The shell was guessed by scanning the shell name for "wsl" or "bash", so cmd.exe got the PowerShell script (Clear-Host is not a cmd builtin, so it printed an error and no card) and WSL only got the fastfetch fallback line, which on a machine without fastfetch is a single line of text. Pick the intro command from the program actually spawned instead: PowerShell keeps its cached script, cmd gets a UTF-8 switch plus a plain-text copy of the card, and POSIX shells (wsl, bash, zsh, fish, nu, ...) clear the screen and print the same text, translating the temp path with wslpath or cygpath when either is available. fastfetch still wins when it is installed.
Terminals serving that protocol see two dialects: one client sends a placement with an explicit cell count, another asks for a 1x1 placement per cell and lets the cursor describe the rectangle. The grid now owns the image either way: a placement becomes an ImagePlacement whose tiles live in the cells the cursor visited, so scrolling, reflow, alt-screen swaps and erasure reuse the existing grid-owned image lifetime. - osc_cwd: parse APC commands (ESC _ G ... ESC \) into control and payload. vte drops APC, so this sniffer is the only parser for them. - kitty_graphics: reassemble chunked base64 transfers, decode raw RGB/RGBA pixels, keep one bitmap per id, union the visited cells into a single placement, publish one Event::KittyImage per transfer instead of one per placement, and reset transfer state when a reused id starts over. - Answer the CSI 16 t cell-size query from the byte stream with the pane's device-pixel cell size; the parser has no handler for it, and a client that never learns it stretches its preview across the whole pane. - Msg::CellMetrics supplies that cell size to panes whose born geometry equals their target geometry and which therefore never send a resize. - The app decodes transferred pixels on the background executor through the existing inline-image store, repacking RGB/RGBA into the BGRA the renderer wants.
A transfer that asks the terminal to display the image as it arrives (�=T) only carries the target cell box on its first chunk; the closing chunk has no action at all. The state machine treated that as a plain transfer, so the bitmap was stored and never placed, which left clients that stream a page in one go with a blank screen. The box now travels with the transfer and is placed when the last chunk lands. Also drop the stored geometry on delete so a reused image id cannot inherit the previous picture's size.
A placement may name a rectangle of the transferred bitmap (`x`/`y`/`w`/`h`). The terminal kept only the destination cells, so a pager that scrolls a page by re-placing the same image with a different `y` kept the previous band on screen forever, and after an erase the area stayed blank because the cell box had not grown. - carry the slice in `ImageLayout` and move the bitmap under its cells, so no pixels are re-uploaded to show a different part of the same picture - republish a placement when its slice changes, not only when its box grows - retarget a queued decode when a placement only moved its slice, instead of decoding the same buffer again on every scroll step - accept `o=z` payloads (inflated to the declared size), transfer headers that omit `a=t`, and placements without `c`/`r` (natural size, one image pixel per screen pixel)
The painter shifted the bitmap by the slice offset but clipped only to the cells of the run, so a slice shorter than its cell box revealed the rows that follow it. Rows are painted one run each, which made the leak worse: a 40-pixel slice in an eight-row box painted 160 pixels, four copies of the page stacked inside the box. - intersect the clip with the slice, anchored to the placement origin, so the rows below a slice stay empty - keep the slice in bitmap coordinates so a cell box that disagrees with the reported cell size cannot move the boundary
A high-DPI pane paints the bitmap smaller than its pixel size, so a placement's slice has to shrink with it. Shifting the bitmap by the raw pixel offset put the slice above the tile and left only its last row visible: an 80 pixel slice in an eight row box painted 20 pixels. image_geometry now derives the scale before placing the bitmap and uses it for both the origin and the slice rectangle.
A reader that streams a page sends one �=T header with the crop and the destination cells and then continuation chunks without any control keys. Two things broke that shape: - inish_transfer placed the image with an empty control string, so the crop in the transfer header was dropped and the whole bitmap went into the cell box. The header's cells and crop are now kept in a Display entry and handed to the placement. - Nothing recorded why a page stayed invisible, so the app-side enqueue and paint paths now log under PEBREL_DEBUG_LOG, which is how the render bug below was found. Tests cover the reader-shaped transfer, a mixed-depth batch and the crop that reaches the app.
CSI 14 t answered with the view's window size, which only commit_viewport updates; a grid-only reflow (resize without reflowing the child, the debounced path) left it at the spawn geometry. A reader that derives its page size from the text area then laid out for 151 columns on a 90 column pane and placed the page past its right edge. Also scale a placement into the cells it declared instead of its pixel size: the cells are what the grid reserved and what the clip uses, so a sender that multiplies differently no longer paints a page several cells wider than the block it asked for. Tests: the terminal graphics unit tests (16 passed), the app inline image tests (19 passed) and the grid image integration tests (12 passed).
A sender that crops to the part of a page it wants visible (a pager sending one y/h per scroll step) expects that crop to cover the block it declared. Scaling the slice by the whole bitmap instead left it a fraction of the box tall, which showed up as a page that never appeared. Keep more decoded pages cached as well: a reader uploads the pages around the cursor up front, and the previous budget of ten pages evicted the one on screen while it was still being paged to.
A page uploaded by a reader decodes off the UI thread. When the bitmap finished decoding while the window already had a frame queued, the frame drawn for it was built before the texture reached the store, so the cells painted as empty and the pane kept showing an untouched page area until the next input event — a click brought the page back. Nudge one more frame once the decode has settled, so the freshly decoded image is presented without any input. The graphics paint summary also reports how many runs survive the pane bounds, the vertical span they cover, the pane bounds, and the error from a dropped image paint; the atlas failing to insert a texture used to be silently discarded. Tests: the app inline image tests (19 passed), the terminal graphics unit tests (16 passed) and the grid image integration tests (12 passed).
`nebula_terminal/src/event_loop.rs` had crossed the 2000-line source budget after the graphics tests moved in. Keep the module focused on the stream processor and put the tests next to it as `event_loop_tests.rs`, declared through the same `#[path = "..."]` pattern the unix-only tests already use. No test bodies changed: the file only drops one level of indentation. Tests: cargo test -p nebula_terminal --offline --lib (364 passed, 2 failed - both pre-existing proxy tests that need pwsh.exe), the terminal graphics unit tests (16 passed), the grid image integration tests (12 passed), and scripts/check_architecture.py exits 0.
Contributor
Author
|
Status update: upstream Before deciding how to split this, I built current The app-side parts port cleanly onto
With those three applied to Two CI notes from this branch, for whoever runs into them: |
Contributor
Author
|
Superseded by #819. The protocol implementation in What was actually still broken on
#819 is 4 files, +50/-4. Closing this one to keep the queue single-threaded. |
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.
What
Completes the APC graphics protocol support in the terminal so full-page readers work. A PDF/EPUB reader running inside Pebrel used to leave the page area blank whenever it placed a cropped region of a page, and occasionally showed nothing at all until the window was clicked.
Seven commits, four independent defects:
3f371845— carry the placement's sub-region (x/y/w/h) through the grid. It lands inImageLayoutas aSourceRect, is republished when the region or the cell box changes, and a re-place that reuses an already decoded buffer retargets the queued or cached image instead of decoding it again. Also acceptso=zpayloads, transfers without an explicita=t, and placements withoutc/r(natural size).43fe705e— clip each row run to the slice it asked for. The slice rect used to be re-anchored at every row of cells, so a 40 px slice inside an 8-row box painted 160 px.f3ab6658— scale the slice offset with the bitmap's paint scale. The terminal reports cell sizes in device pixels while the painter works in logical pixels (1.75× at 175 % scaling), so a slice withy > 0was pushed above its box and only its last row stayed visible.e02ee7f2— keep the crop that ana=Theader carries. It was dropped before the body arrived, and the same crop is now also accepted ona=p.fc44489a— answer the text-area query from the live grid instead of the geometry captured at spawn. A stale answer made the reader believe the pane was 151×40 while it was 90×23, so it laid the page out at column 66, off screen.faa3edbd— scale the crop to fill the cells the placement owns (c/r) instead of the whole bitmap. A reader sends one band of a page at a time and the band used to occupy only a fraction of the reserved block. Raises the decoded-image cache to 32 images / 256 MiB so the page around the cursor is not evicted while it is on screen.769d58c4— present a decoded image even when its frame is already queued: aftercx.notify()the decode path schedules one extra frame 40 ms later, which is the click the user used to need. Also surfacespaint_imagefailures instead of discarding them, and adds diagnostics gated behindPEBREL_DEBUG_LOG(visible=,y=,bounds=,paint_error=).Note on the branch
The first three commits (
cdd1d648,06890480,1fc969d0) are the prerequisites this work builds on: the APC graphics protocol itself (also proposed separately), a chunked-display placement fix, and a startup-card tweak. Only the seven commits above are new here.Tests
cargo fmt --all -- --check: clean.New cases cover: a crop being published and republished when it moves, single-cell placements unioning their slices,
o=zinflation, implicita=t, natural-size placement withoutc/r, a slice not leaking below its row, device/logical scaling, a crop filling every row of its box, and a replay of the same crop reusing the queued transfer.Verification
Installed build
Pebrel 2.3.1 (769d58c4)on Windows 11, 2560×1600 at 175 % scaling, reading two books (one text, one 226 MB scanned) with a full-page reader inside a maximized Pebrel window:x/y/w/hcrop and paint every row (painted == runs,empty == 0).Known follow-up: when the window is restored rather than maximized, the pane keeps stale bounds for a while, so a placement can be drawn outside the visible window until the next relayout. Not addressed here.