Calibrate sixel cell size to agg's actual rendering (#84)#86
Conversation
tuirec advertised a sixel cell resolution from a static formula (~0.6*fontSize, rounded to int). That was wrong two ways: the 0.6 ratio does not match whatever monospace font agg resolves on the host, and agg's per-column advance is fractional (e.g. 8.425px at font-size 14) while terminal cell-size reports (CSI 16t) must be integers. An app that sizes a sixel raster as cells*reportedCell therefore rendered a few percent undersized — at font-size 14, ~5% (visibly short of borders). Fix: when a GIF will be rendered, measure agg's real per-column advance by rendering two probe casts that differ only in column count (the slope cancels agg's fixed padding and is independent of the resolved font), then align the grid so the integer report matches what agg renders: - Width: pick the font-size nearest the requested one whose cell rounds closest to an integer (kept as-is when already within ~0.15px; font-size is integer-only in agg, so this is the only width lever). - Height: report round(fontSize*lineHeight) and pass agg a line-height of cellH/fontSize so agg's row height equals the report exactly. The chosen font-size/line-height are used for the render too, so the cast reports and the GIF agree. Falls back to the previous formula when no GIF is produced (agg unavailable) or calibration fails. Verified end-to-end: UICatalog's About-box fire now fills the dialog interior edge-to-edge (font-size 14 auto-aligns to 15; cell 9x20; raster 540px vs ~542px interior = 99.6% vs the previous ~96%). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 18db4e016a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // cell agg actually renders and align the font so the integer cell report | ||
| // matches it exactly (gui-cs/tuirec#84). Falls back to the formula geometry | ||
| // above if calibration fails or no GIF is produced (no agg available). | ||
| if config.Output != "" && config.GIF.AggPath != "" { |
There was a problem hiding this comment.
When record.Run is used directly with the default GIF config, gif.Render will normalize an empty AggPath to "agg", but this guard skips calibration before that normalization happens. In that common zero-value API path (Config{Output: ...} with the default renderer), recordings still use the old 0.6 * fontSize cell report even though a GIF is rendered, so the fix does not apply unless callers explicitly set GIF.AggPath; normalize or treat an empty path as renderable before deciding to skip calibration.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 18ff0ae. Correct — a GIF renders whenever Output is set (the render gate is just config.Output == ""), and AggPath is normalized to "agg" downstream, so requiring a non-empty AggPath here skipped calibration for the zero-value API path while still rendering.
Replaced the guard with willRenderGIF(config), which mirrors the render condition (config.Output != "") and intentionally does not check AggPath (calibrateGeometry normalizes it). Added TestWillRenderGIF covering Output-set-with-empty-AggPath → renders/calibrates.
| return 0, fmt.Errorf("write probe cast: %w", err) | ||
| } | ||
|
|
||
| if err := Render(ctx, castPath, gifPath, config); err != nil { |
There was a problem hiding this comment.
The probe render reuses the caller's full GIF config, including Select, while the synthetic probe cast only has events at 0.1s and 0.2s. For documented --select values that trim past that point (for example 1.. to drop a lead-in), agg receives an empty selected probe and calibration fails back to the old formula, so sixel cell alignment is silently disabled for those recordings; the measurement should clear Select because geometry is independent of the timeline selection.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 18ff0ae. Good catch — the probe cast only has events at 0.1s/0.2s, so a Select like 1.. trims it to an empty selection and calibration silently falls back to the formula.
MeasureColumnWidthPx now passes the probe through probeConfig(), which clears Select (cell geometry is independent of the timeline) while preserving the options that do affect column width (FontSize, LineHeight, Font, LetterSpacing). Added TestProbeConfigClearsSelectKeepsGeometry.
Two P2s from Codex review on #86: - record.Run gated calibration on `config.Output != "" && GIF.AggPath != ""`, but a GIF renders whenever Output is set (AggPath is normalized to "agg" downstream). Zero-value-config API callers therefore rendered a GIF but skipped calibration, falling back to the 0.6*fontSize formula. Gate on a new willRenderGIF(config) that mirrors the render condition only. - MeasureColumnWidthPx reused the caller's full config, including Select. The probe cast only has events at 0.1s/0.2s, so a Select like "1.." trims it to empty and calibration silently fails back to the formula. Strip Select via probeConfig() (geometry is timeline-independent); font/spacing options that do affect column width are preserved. Added unit tests for both seams (TestWillRenderGIF, TestProbeConfig...). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Fixes Windows integration failure (TestRunRecordsKeystrokesAndRenders: "recording hit max duration") introduced by enabling calibration in the zero-value path. The recording's MaxDuration context is created at the top of Run, and calibration ran after it — so the agg probe-render wall-clock time was charged against the recording budget. With MaxDuration=1s and two agg renders on Windows, the recording timed out immediately. Move geometry computation + calibration to before the MaxDuration context is created, and run the probes under their own timeout off `parent`. The recording now gets its full budget; probe time is pre-recording setup. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Summary
Fixes #84. tuirec advertised the sixel cell resolution (
CSI 16t, plus theCSI 14t/18treports) from a static formula —~0.6 × fontSize, rounded to an integer. That is wrong in two compounding ways:0.6ratio does not match whatever monospace font agg actually resolves on the host (measured0.6018for DejaVu Sans Mono here; JetBrains Mono is0.6, but it is not always the one agg picks).cells × reportedCell(the correct, standard approach — it fills exactly on a real terminal) therefore renders undersized. At the default font-size 14 that is ~5%, visibly short of a bordered region.Fix
When a GIF will be rendered, measure agg''s real per-column advance instead of guessing it, then align the grid so the integer report matches what agg renders:
pkg/gif/measure.go): render two tiny probe casts that differ only in column count;(wideWidth − narrowWidth) / Δcolsisolates the per-column advance, cancelling agg''s fixed horizontal padding and working for whatever font agg resolves.pkg/record/calibrate.go): agg''s--font-sizeis integer-only, so the cell can only be made integer by font-size choice. Keep the requested size when its cell already rounds within ~0.15 px; otherwise pick the nearest size that rounds cleanly (e.g. 14 → 15: cell9.04, error0.04).round(fontSize × lineHeight)and pass agglineHeight = cellH / fontSize, so agg''s row height equals the report exactly.The chosen font-size/line-height are threaded into the render, so the cast''s sixel reports and the GIF agree. Falls back to the previous formula when no GIF is produced (agg unavailable) or calibration fails, so cast-only recordings are unaffected. When the font-size is nudged it is logged:
adjusted agg font-size to 15 to align the sixel cell grid (#84).Trade-off
Aligning the width grid can nudge the rendered font-size by ±1 (only when the requested size is poorly aligned). Faithful sixel geometry seemed more valuable than honoring an exact pixel font-size, and the change is ≤1px and logged. Happy to gate this behind a flag or apply it only to the default size if you prefer.
Verification
go test ./pkg/record/ ./pkg/gif/— pass (added unit tests for the alignment math);go vet,gofmtclean.8×17, raster480px, ~96% of the 60-col interior (visible right-edge gap). After:adjusted agg font-size to 15, cell9×20, raster540px, ~99.6% — the fire now fills the dialog interior edge-to-edge.Pairs with the doc fix in #85 (which tells agents to measure sixel size rather than eyeball it).
🤖 Generated with Claude Code