Add cell metric adjusters and font-thicken for text tuning - #12
Merged
Conversation
Manual testing of #11 on macOS surfaced two remaining gaps versus cmux/ghostty: line spacing is tight (we take Pango's natural cell height with no padding), and glyph strokes look slightly thinner on Mac (Cairo doesn't apply Apple Core Text stem darkening). Both are exactly what Ghostty's font-tuning knobs are designed for. This follow-up exposes the subset that's actually about text appearance and stops there — cursor / underline / strikethrough / box-thickness adjusters are TUI alignment and stay deferred. Four new config keys, all default no-op: adjust_cell_height Ghostty syntax: 2, 2px, 10%, negatives ok adjust_cell_width same adjust_font_baseline same; vertical glyph fine-tune on top of auto-center font_thicken bool; double-draws each glyph at +0.5px X to fatten strokes A new internal/config Adjust type carries the parsed value so the application site can apply it once the natural metric is known. Apply clamps to a minimum of 1 px; Delta returns just the signed offset, useful for the glyph-y-shift math. Wiring: - measureCells applies AdjustCellWidth/Height to natural cellW/cellH, computes glyphYOffset = (cellH - naturalH)/2 + AdjustFontBaseline. The auto-center half ensures glyphs stay vertically centered when cell height grows; AdjustFontBaseline is the user's bias on top. - render.go's two glyph-paint sites pick up glyphYOffset; backgrounds, cursor rect, and selection rects stay anchored to the cell grid. - font_thicken routes both glyph paints through showGlyphLayout, which conditionally repaints the glyph at +0.5px X. Documentation: docs/reference/fonts.md gains a Cell tuning section with the four knobs, syntax, and a "Targeting the cmux look on macOS" preset (font_family = Menlo, font_size = 11, adjust_cell_height = 2px, font_thicken = true) drawn directly from the testing comparison. Tests cover the parser (px/percent/negative/empty/malformed), the config-side wiring, BuildFontConfig pass-through, and Adjust.Apply clamping. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds a cell-tuning system: parseable adjusters for cell width, height, and font baseline plus a glyph-thicken toggle; config/schema, parsing, font config, session measurement, rendering, tests, and docs updated to carry and apply these adjustments. ChangesCell Metric Tuning System
Sequence Diagram(s)sequenceDiagram
participant ConfigFile as Config File
participant Paths as Paths.Load()
participant ConfigObj as Config
participant BuildFont as BuildFontConfig
participant Session as measureCells
participant Renderer as render loop
ConfigFile->>Paths: read keys (adjust_*, font_thicken)
Paths-->>ConfigObj: populate Adjust* & FontThicken
ConfigObj->>BuildFont: used to build FontConfig
BuildFont-->>Session: FontConfig (including adjusters)
Session->>Session: compute cellW/cellH and glyphYOffset via Adjust.Apply/Delta
Session->>Renderer: metrics & FontConfig
Renderer->>Renderer: showGlyphLayout(..., y+glyphYOffset, FontThicken)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 4/5 reviews remaining, refill in 12 minutes. Comment |
Pango's natural cell metrics are tighter than mainstream terminals (cmux, ghostty, iTerm, Terminal.app) which all add a small amount of cell padding. Side-by-side testing on macOS confirmed +2px on both axes lands roost in the same visual zone with no font swap. Setting tasteful defaults here saves every user from discovering and tuning the same knobs. Opt out with `adjust_cell_* = 0` (or any other value) — the parser still treats an explicit blank value as "back to no-op" so the override semantics are intact. Knock-on changes: - TestLoadDefaultsWhenMissing pins the new adjuster defaults so they don't quietly drift. - TestLoadAdjustEmptyKeepsNoOp renamed to TestLoadAdjustEmptyOverrides- Default; the comment and assertion now reflect that an empty value in the config file overrides the non-zero default. - TestBuildFontConfigCarriesAdjusters now uses values that differ from the defaults (5 instead of 2) so the wiring assertion remains meaningful. Doc updates in fonts.md: - Cell tuning table gets a Default column showing the new 2px values for adjust_cell_width / adjust_cell_height. - Adds an "Opting out of the cell padding" example for users who want Pango's natural metrics back. - Reframes the cmux preset as a layered tweak on top of the new defaults (just font_family + font_size + font_thicken). - Notes that antialias = subpixel is a no-op on macOS (Apple removed system-wide subpixel AA in Mojave) but a safe cross-platform choice because Linux RGB-stripe panels do honor it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
charliek
added a commit
that referenced
this pull request
May 23, 2026
* fix(linux): Linux IPC robustness sweep (closes #80) Address the deferred CR items from the inline-core refactor (PR #78). Reconciled against the merged code: #1 (parse-error reply), #10 (pending HashSet), #13 (smoke-test ordering) were already done; the rest are fixed here. Project is early-stage, so the bar is long-term correctness. PTY supervisor (crates/roost-linux/src/daemon/pty.rs, tab_session.rs): - #11: wait task now removes the session on child exit (sessions behind Arc<Mutex>), so writes to a dead PTY return NotFound instead of silently succeeding. - #12: close() escalates SIGHUP -> SIGKILL after a grace period. The cloned portable-pty ChildKiller only sends SIGHUP, so a shell that traps/ignores it used to outlive close(); a reaped flag + pid watchdog force-kill it, mirroring the Mac teardown. - #8: send_input/send_resize feed one per-tab serial channel drained by a single task, so keystrokes can't reorder (the old per-call tokio::spawn could under the multi-thread runtime). Workspace state (crates/roost-linux/src/daemon/state.rs): - #4: new project/tab position uses max(position)+1, not len()/count(), so a delete-then-create can't collide. - #5: every mutator now publishes its event while still holding `inner`, so broadcast order matches commit order; persist runs after the drop. The events-resync Resync path is preserved. Server framing (crates/roost-ipc/src/server.rs): - #2: on an envelope decode failure, peel `id` from the raw JSON (string- or number-encoded) so the parse-error reply lands at the client's id, not 0. Lifecycle (crates/roost-linux/src/main.rs, ipc.rs, app.rs, messages.rs): - #7: IPC bind is now a synchronous startup requirement; bind failure aborts startup instead of leaving the UI socket-less. - #6: a second launch dials the new `app.activate` op; the handler forwards to the GTK thread which raises the running window. - #9: events.subscribe returns not-implemented (Linux + Mac) instead of a false {} ACK, so clients don't wait forever. Real streaming is deferred to its first consumer (roostctl watch). Tests: write-after-exit -> NotFound, SIGHUP-ignoring child force-killed, send_input ordering, position max+1 after delete, id-preserving parse error, bind-failure surfaced, events.subscribe not-implemented. Rust workspace + Mac build/clippy/tests green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(mac): IPC handler dispatch harness (#80 follow-up) The Rust handler has tests/ipc_dispatch.rs; the Mac IPCHandlerImpl had no equivalent, leaving its hand-written cross-cutting logic untested — strict unknown-field rejection (decodeParams), ipcDim u16 validation, mapWorkspace/mapPty error-code mapping, the not-implemented/unknown-op paths, and result encoding. The two handlers must stay behaviorally convergent over the shared wire contract, so this guards that. Calls IPCHandlerImpl.handle(op:params:) directly — no socket. Exercises only non-PTY-spawning ops to avoid the forkpty/swift-testing SIGTRAP that disables the PTY paths elsewhere (tab.open stays on the manual pass; the error-mapping ops reach the supervisor only on the lookup-fails path). Seeds 8 tests: events.subscribe→not-implemented (the #9 path), unknown-op, unknown-field strictness, project.rename→not-found, tab.resize out-of-range→invalid-param, tab.resize missing→not-found, identify profile echo, project.create→tab.list round-trip. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(linux): address CodeRabbit review on #80 sweep Resolves 4 of CodeRabbit's 6 findings on PR #86 (the persistence-ordering finding is pre-existing/orthogonal and tracked separately). pty.rs: - Major: spawn() acquired the master reader/writer AFTER spawn_command, so a try_clone_reader/take_writer failure returned with a live child and no wait task — an orphaned PTY. Acquire reader+writer BEFORE spawning, making spawn_command the last fallible step (no orphan). - Major: the exit wait task removed the session by tab_id alone, which could evict a newer session that reused the same tab_id (close() frees the slot synchronously; pty_smoke reuses id 42). Gate the remove on Arc::ptr_eq of the per-spawn `reaped` identity so only the owning waiter deletes. (My prior "monotonic ids" comment over-assumed caller discipline; the supervisor can't rely on it.) - Major: input and resize were unified at TabSession but re-split into the supervisor's two channels (select!), so mixed input/resize wasn't FIFO end-to-end. Merge them into one WriterCmd channel drained by a single ordered loop — genuine submission-order delivery. ipc.rs + messages.rs: - Minor: app.activate ACK'd any payload. Add an empty strict AppActivateParams and decode it so the op validates its envelope (rejects unknown fields) like every other op. IPCHandlerTests.swift: - Minor: replace `as? T != nil` (SwiftLint prefer_type_checking) with a meaningful empty-array assertion on a fresh project's tabs. Rust clippy --all-targets clean; roost-ipc + roost-linux tests green (A3 ordering + dup-spawn still pass under the new channel/guard). Mac swift build + 140 tests green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(linux): order state.json persistence by commit seq (CR #3) Resolves the last CodeRabbit finding on PR #86. persist_async did synchronous file I/O on the caller thread after dropping `inner`, so two concurrent mutators could write state.json out of commit order — a slow earlier commit could clobber a newer one, regressing restart durability. Keep persistence synchronous (state_persist.rs and restart-reload depend on it; an async writer thread would break that), but make it ordered: - Inner.persist_seq: a monotonic commit counter bumped under the lock when a snapshot is taken, so the seq reflects commit order. - snapshot_for_persist now returns (SnapshotFile, seq). - persist(seq, snapshot) serializes on a new persist_guard mutex and drops any snapshot whose seq is <= the highest already written, so the newest committed snapshot always wins regardless of which thread races first. Newest-wins verified by persist_drops_stale_out_of_order_writes. Rust clippy --all-targets clean; roost-ipc + roost-linux suites green (incl. state_persist). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * style: rustfmt ipc.rs import block The AppActivateParams import addition left the use block un-reflowed; CI's rust-lint (cargo fmt --all -- --check) caught it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Summary
Follow-up to #11. Manual testing on macOS surfaced two remaining tuning gaps versus cmux/ghostty: line spacing is tight (we take Pango's natural cell height with no padding), and glyph strokes look slightly thinner on Mac because Cairo doesn't apply Apple Core Text stem darkening. Both are exactly what Ghostty's font-tuning knobs are designed for. This PR exposes the subset that's actually about text appearance; cursor / underline / strikethrough / box-thickness adjusters are TUI alignment and stay deferred for a separate PR.
Four new config keys, all default no-op (existing configs unchanged):
adjust_cell_height2,2px,10%, negatives okadjust_cell_widthadjust_font_baselineadjust_cell_heightauto-center.font_thickentrue/falsePlus the documented preset for matching cmux's look on macOS:
Implementation notes
internal/config.Adjusttype carries the parsed value (Mode+Value) so the application site (measureCells) can apply it once the natural metric is known.Applyclamps to a minimum of 1 px so a runaway negative can't crash geometry;Deltareturns just the signed offset for glyph y-shift math.measureCellsnow applies the cell adjusters and computesglyphYOffset = (cellH - naturalH)/2 + AdjustFontBaseline.Delta(naturalH). Auto-center half ensures glyphs stay vertically centered when cell height grows;adjust_font_baselineis the user's bias on top.render.goglyph-paint sites pick upglyphYOffset; backgrounds, cursor rect, and selection rects stay anchored to the cell grid (intentional — only glyphs shift).font_thickenis implemented via ashowGlyphLayouthelper that wraps bothpangocairo.ShowLayoutcall sites. When enabled, the glyph is painted again at+0.5pxX. Document as approximation, not perfect parity with Apple's algorithm.Test plan
./build/build.sh,go test ./...,golangci-lint run— all green locally.adjust_cell_height = 4px— rows visibly taller; glyphs vertically centered (not stuck to top).adjust_cell_height = 10%— proportional to font size.adjust_cell_height = -2px— rows tighter; clamp prevents negative cells.adjust_cell_width = 2px— letter spacing widens; bg fills extend; cursor block widens.adjust_font_baseline = 2pxwithadjust_cell_height = 4px— glyphs shift further down on top of the auto-center.font_thicken = true— strokes visibly thicker; toggle off → reverts.adjust_cell_height = nonsenserefuses to start with a clear line/value error.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests