coretext: clamp fallback glyphs to primary cell height#19
Conversation
📝 WalkthroughWalkthroughThe PR introduces cmux live-preview theme picker support on Darwin with real-time configuration management and macOS notifications, adds cached font metrics and fallback-face state to CoreText fonts, implements font query refactoring with internal CTFont parameters and fallback scaling, adds a CLI helper build step, and improves stdout flushing behavior. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant ListThemes as Theme Picker
participant CmuxConfig as cmux Config File
participant macOS as macOS Notification<br/>System
User->>ListThemes: Select/navigate theme
ListThemes->>ListThemes: applyCmuxSelectionForCurrentTheme()
ListThemes->>CmuxConfig: Write/update override block<br/>(# cmux themes start/end)
ListThemes->>macOS: Send com.cmuxterm.themes.reload-config<br/>notification
User->>ListThemes: Press 't' to cycle target mode
ListThemes->>CmuxConfig: Update mode (both/light/dark)
ListThemes->>macOS: Notify config reload
User->>ListThemes: Press Enter (Apply)
ListThemes->>ListThemes: Save selection, exit
Note over ListThemes,CmuxConfig: On exit without Apply:<br/>Restore original config
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR ships three distinct improvements: (1) CoreText fallback glyph clamping — the headline feature — scales oversized CJK/Hangul fallback glyphs down to the primary cell height using a uniform Confidence Score: 4/5Safe to merge after addressing the early-return state inconsistency in syncCmuxPreview; the CoreText font changes are correct and well-guarded. The headline coretext changes (metrics caching, fallback flag propagation, height clamping) are implemented correctly and robustly handle edge cases (degenerate metrics, color glyphs, explicit constraints). The one concrete logic issue is the src/cli/list_themes.zig — syncCmuxPreview early-return and dead initialTheme method Important Files Changed
Sequence DiagramsequenceDiagram
participant Col as Collection.zig
participant Face as coretext Face
participant Render as renderGlyph
Col->>Face: add(face, opts{fallback:true})
Col->>Face: setFaceFallback(&face, true)
Note over Face: face.fallback = true
Col->>Face: setSize(opts)
Face->>Face: initFontCopy → initFont
Note over Face: metrics = calcMetrics(ct_font)
Note over Face: fallback preserved from self
Render->>Face: renderGlyph(glyph, opts)
Face->>Face: fallback && !is_color && !constraint?
alt fallback scale needed
Face->>Face: fallbackHeightScale(grid_metrics)
Note over Face: scale = min(primary_top/fallback_top,
Note over Face: primary_bottom/fallback_bottom)
Face->>Face: layout_rect *= scale
Face->>Face: context.scaleCTM(width/rect.w, height/rect.h)
end
Face->>Face: drawGlyphs at -rect.origin (unscaled)
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (self.fallback and !is_color and !constraint.doesAnything()) { | ||
| const fallback_scale = self.fallbackHeightScale(metrics); | ||
| if (fallback_scale < 1.0) { | ||
| layout_rect.origin.x *= fallback_scale; | ||
| layout_rect.origin.y *= fallback_scale; | ||
| layout_rect.size.width *= fallback_scale; | ||
| layout_rect.size.height *= fallback_scale; | ||
| } | ||
| } |
There was a problem hiding this comment.
The fallback path scales layout_rect.origin.x and layout_rect.origin.y by fallback_scale, which affects px_x/px_y (atlas placement). However, the actual drawGlyphs call further down uses the unscaled rect.origin — the real size reduction is achieved by context.scaleCTM(width / rect.size.width, height / rect.size.height). The math is correct, but the asymmetry makes the code harder to follow. A comment explaining the intent would prevent future confusion:
// NOTE: drawGlyphs always uses the unscaled `rect.origin` as the glyph
// position. context.scaleCTM handles the actual size reduction; the scaled
// layout_rect.origin only affects atlas placement (px_x / px_y).Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
|
||
| const NSString = objc.getClass("NSString") orelse return error.ObjCFailed; | ||
| const center_class = objc.getClass("NSDistributedNotificationCenter") orelse | ||
| return error.ObjCFailed; | ||
| const center = center_class.msgSend(objc.Object, objc.sel("defaultCenter"), .{}); | ||
|
|
||
| const name_c = try alloc.dupeZ(u8, "com.cmuxterm.themes.reload-config"); | ||
| defer alloc.free(name_c); | ||
| const object_c = try alloc.dupeZ(u8, bundle_id); | ||
| defer alloc.free(object_c); | ||
|
|
||
| const name = NSString.msgSend( | ||
| objc.Object, | ||
| objc.sel("stringWithUTF8String:"), | ||
| .{name_c.ptr}, |
There was a problem hiding this comment.
cmux_applied_* update, leaving state inconsistent
When encodeCmuxThemeValue returns null (both cmux_preview_light and cmux_preview_dark are null while not matching the initials), the function returns early via orelse return without updating cmux_applied_light/cmux_applied_dark. This leaves the tracked applied-state out of sync with what is on disk.
In practice the path is unreachable today (preview fields only become null through restoreCmuxOriginal, which sets them equal to initial_*, so the earlier eqlOptionalTheme check would already have returned). However the correctness is implicit and fragile. Consider making the early return update the applied state:
const raw_theme_value = (try encodeCmuxThemeValue(
self.allocator,
self.cmux_preview_light,
self.cmux_preview_dark,
)) orelse {
self.cmux_applied_light = self.cmux_preview_light;
self.cmux_applied_dark = self.cmux_preview_dark;
return;
};![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/cli/list_themes.zig (1)
1020-1025:⚠️ Potential issue | 🟡 MinorFooter may overlap the last theme in the list.
The
theme_listchild window usesheight = win.height(line 1024), spanning the full terminal height. When cmux mode is active, the footer draws aty_off = win.height - 1(line 1145), which overlaps the bottom row of the theme list. This could hide the last visible theme entry when the list is scrolled to the bottom.Consider reducing the theme list height by 1 when
self.cmux != null:Suggested fix
const theme_list = win.child(.{ .x_off = 0, .y_off = 0, .width = 32, - .height = win.height, + .height = if (self.cmux != null) win.height -| 1 else win.height, });Also applies to: 1142-1171
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/list_themes.zig` around lines 1020 - 1025, The theme_list child currently uses height = win.height which lets the footer (drawn when self.cmux != null at y_off = win.height - 1) overlap the last theme; change the height calculation in the theme_list child creation (and the analogous window creations in the 1142-1171 block) to subtract 1 when self.cmux != null (e.g., height = if (self.cmux != null) win.height - 1 else win.height), ensuring you guard against negative heights if win.height can be zero.src/font/face/coretext.zig (1)
218-225:⚠️ Potential issue | 🟡 MinorRelease the
family_nameobject before returning.The function has two return paths but neither releases the object created by
copyFamilyName(), unlike every othercopy*call in the file. Adddefer family_name.release();immediately after creating the object to prevent the resource leak.Patch sketch
fn nameFont(ct_font: *macos.text.Font, buf: []u8) Allocator.Error![]const u8 { const family_name = ct_font.copyFamilyName(); + defer family_name.release(); if (family_name.cstringPtr(.utf8)) |str| return str; - // "NULL if the internal storage of theString does not allow - // this to be returned efficiently." In this case, we need - // to allocate. return family_name.cstring(buf, .utf8) orelse error.OutOfMemory; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/font/face/coretext.zig` around lines 218 - 225, The function nameFont leaks the CTString returned by ct_font.copyFamilyName() because it never releases family_name; add a defer call to family_name.release() immediately after const family_name = ct_font.copyFamilyName() so the object is always released before either returning the cstringPtr result or the allocated cstring path (ensure the defer remains in scope for both return paths).
🧹 Nitpick comments (2)
src/cli/list_themes.zig (2)
1236-1239: Skipped help entries cause misaligned row indices.When
help.help.len == 0, the entry is skipped butcaptured_istill increments. This means subsequent entries print at their original row offset, leaving gaps in the help display. For example, if thetentry (index 19) is skipped in non-cmux mode, row 20 remains blank.This is visually acceptable but could be improved by tracking a separate display row counter:
Optional improvement
+ var display_row: u16 = 0; for (key_help, 0..) |help, captured_i| { if (help.help.len == 0) continue; - const i: u16 = `@intCast`(captured_i); + display_row += 1; _ = child.printSegment( .{ .text = help.keys, .style = self.ui_standard(), }, .{ - .row_offset = i + 1, + .row_offset = display_row, .col_offset = 2, }, ); // ... similarly for other printSegment calls🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/list_themes.zig` around lines 1236 - 1239, The for-loop over key_help (for (key_help, 0..) |help, captured_i|) skips entries when help.help.len == 0 but still uses captured_i to compute the printed row (const i: u16 = `@intCast`(captured_i)), causing gaps; fix by introducing a separate display counter (e.g., display_row) incremented only when an entry is actually rendered and use that counter instead of captured_i when computing the row index for printing so skipped entries do not leave blank rows.
603-607: Cleanup restoration swallows errors silently.Both the
errdeferanddeferusecatch {}to silently ignore restoration failures. While this is common for cleanup code, a failed restoration could leave the user's config file in an inconsistent state without any indication.Consider logging the error for debuggability:
Optional improvement
- errdefer self.restoreCmuxOriginal() catch {}; - defer if (self.outcome == .cancel) { - self.restoreCmuxOriginal() catch {}; - }; + errdefer self.restoreCmuxOriginal() catch |err| { + std.log.warn("failed to restore cmux config: {}", .{err}); + }; + defer if (self.outcome == .cancel) { + self.restoreCmuxOriginal() catch |err| { + std.log.warn("failed to restore cmux config: {}", .{err}); + }; + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/list_themes.zig` around lines 603 - 607, The cleanup currently swallows errors in Preview.run by using `errdefer self.restoreCmuxOriginal() catch {}` and `defer ... self.restoreCmuxOriginal() catch {}`, which hides restoration failures; change these to capture the error result and log it instead of ignoring it (e.g., store the error from calling `restoreCmuxOriginal()` and call the repo's existing logging facility or `std.debug` to emit a clear message including the error and context like `Preview.run` and `self.outcome`), so both the errdefer and defer paths report failures while still allowing program termination to proceed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/font/face/coretext.zig`:
- Around line 230-235: The replacement face created in setSize (via
initFontCopy) currently only preserves fallback and loses synthetic bold state,
causing faces that had syntheticBold() applied to revert to regular; update
setSize (and the analogous rebuild path around the other resize/variation
function) to preserve or recompute synthetic bold by copying the synthetic_bold
flag/state from the old Face into the new face (or calling the existing
syntheticBold() helper on the new face if available) after initFontCopy but
before self.deinit()/self.* = face so the synthesized-bold behavior is retained
across rebuilds; reference initFontCopy, setSize, syntheticBold, and the
synthetic_bold field when making the change.
---
Outside diff comments:
In `@src/cli/list_themes.zig`:
- Around line 1020-1025: The theme_list child currently uses height = win.height
which lets the footer (drawn when self.cmux != null at y_off = win.height - 1)
overlap the last theme; change the height calculation in the theme_list child
creation (and the analogous window creations in the 1142-1171 block) to subtract
1 when self.cmux != null (e.g., height = if (self.cmux != null) win.height - 1
else win.height), ensuring you guard against negative heights if win.height can
be zero.
In `@src/font/face/coretext.zig`:
- Around line 218-225: The function nameFont leaks the CTString returned by
ct_font.copyFamilyName() because it never releases family_name; add a defer call
to family_name.release() immediately after const family_name =
ct_font.copyFamilyName() so the object is always released before either
returning the cstringPtr result or the allocated cstring path (ensure the defer
remains in scope for both return paths).
---
Nitpick comments:
In `@src/cli/list_themes.zig`:
- Around line 1236-1239: The for-loop over key_help (for (key_help, 0..) |help,
captured_i|) skips entries when help.help.len == 0 but still uses captured_i to
compute the printed row (const i: u16 = `@intCast`(captured_i)), causing gaps; fix
by introducing a separate display counter (e.g., display_row) incremented only
when an entry is actually rendered and use that counter instead of captured_i
when computing the row index for printing so skipped entries do not leave blank
rows.
- Around line 603-607: The cleanup currently swallows errors in Preview.run by
using `errdefer self.restoreCmuxOriginal() catch {}` and `defer ...
self.restoreCmuxOriginal() catch {}`, which hides restoration failures; change
these to capture the error result and log it instead of ignoring it (e.g., store
the error from calling `restoreCmuxOriginal()` and call the repo's existing
logging facility or `std.debug` to emit a clear message including the error and
context like `Preview.run` and `self.outcome`), so both the errdefer and defer
paths report failures while still allowing program termination to proceed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8e495330-d18a-48bc-b816-3ed600b88f47
📒 Files selected for processing (5)
build.zigsrc/cli/list_themes.zigsrc/font/Collection.zigsrc/font/face/coretext.zigsrc/main_ghostty.zig
| pub fn setSize(self: *Face, opts: font.face.Options) !void { | ||
| // We just create a copy and replace ourself | ||
| const face = try initFontCopy(self.font, opts); | ||
| var face = try initFontCopy(self.font, opts); | ||
| face.fallback = self.fallback; | ||
| self.deinit(); | ||
| self.* = face; |
There was a problem hiding this comment.
Preserve synthetic bold when rebuilding the face.
These paths only restore fallback. A face created by syntheticBold() loses synthetic_bold on the next resize or variation update, so synthesized bold text drops back to regular after Collection.setSize() walks loaded faces. Recompute or copy that state into the replacement face as well.
Patch sketch
pub fn setSize(self: *Face, opts: font.face.Options) !void {
// We just create a copy and replace ourself
var face = try initFontCopy(self.font, opts);
face.fallback = self.fallback;
+ if (self.synthetic_bold != null) {
+ const points_f64: f64 = `@floatCast`(opts.size.points);
+ face.synthetic_bold = `@max`(points_f64 / 14.0, 1);
+ }
self.deinit();
self.* = face;
}
@@
- var face = try initFont(ct_font, new_opts);
- face.fallback = self.fallback;
+ var face = try initFont(ct_font, new_opts);
+ face.fallback = self.fallback;
+ face.synthetic_bold = self.synthetic_bold;
self.deinit();
self.* = face;Also applies to: 264-269
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/font/face/coretext.zig` around lines 230 - 235, The replacement face
created in setSize (via initFontCopy) currently only preserves fallback and
loses synthetic bold state, causing faces that had syntheticBold() applied to
revert to regular; update setSize (and the analogous rebuild path around the
other resize/variation function) to preserve or recompute synthetic bold by
copying the synthetic_bold flag/state from the old Face into the new face (or
calling the existing syntheticBold() helper on the new face if available) after
initFontCopy but before self.deinit()/self.* = face so the synthesized-bold
behavior is retained across rebuilds; reference initFontCopy, setSize,
syntheticBold, and the synthetic_bold field when making the change.
…ouping Restore sidebar grouping when sidebar tabs is enabled
2 participants
Summary
Testing
Summary by cubic
Fixes oversized Hangul/CJK fallback glyphs by clamping their rendered height to the primary grid cell in the CoreText backend. Also adds a cmux theme-picker mode to
list_themesfor live preview and safe config writes. Addresses Linear issue 2162 (Korean/CJK fallback sizing).Bug Fixes
Collection.New Features
list_themes:CMUX_THEME_PICKER_*env vars (config path, bundle id, initial light/dark, target: both/light/dark, UI color scheme).com.cmuxterm.themes.reload-configvia Objective‑C.cli-helperstep depends on installing the Ghostty executable.Written for commit bb60bc9. Summary will update on new commits.
Summary by CodeRabbit
New Features
Improvements