Skip to content

Promote characters to wide if they have space available after#286

Merged
dakra merged 4 commits into
dakra:mainfrom
emil-e:emil-e/promote-wide
May 18, 2026
Merged

Promote characters to wide if they have space available after#286
dakra merged 4 commits into
dakra:mainfrom
emil-e:emil-e/promote-wide

Conversation

@emil-e
Copy link
Copy Markdown
Collaborator

@emil-e emil-e commented May 18, 2026

This PR improves glyph rendering by allowing characters to dynamically expand into adjacent empty space, rather than always being squashed to fit their allocated cell width.

What changed

Previously, a glyph was scaled to fit either 1 or 2 cells depending on whether it was marked as wide. Now, if a glyph is wider (in aspect ratio) than its allocated slot, the renderer checks if the following characters are spaces. If so, it shrinks those spaces to zero-width via Emacs display properties, effectively claiming their cell width for the glyph while maintaining monospace alignment.

Key details

  • Lines are right-trimmed of trailing spaces, so positions at and past the newline represent empty space that can be freely claimed.
  • On a genuinely full line, the column check prevents expansion before reaching the row boundary.
  • Uses Emacs display properties (specifically (space :width 0)) to reclaim space without mutating buffer text.
  • The aspect-ratio comparison ensures we only expand when the glyph is proportionally wider than the available slot.

Files changed

  • src/Renderer.zig — added dynamic width expansion logic and tracking of column position in CellInfo
  • src/emacs.zig — added char-after, space, and :width to interned symbols

@michaelneuper
Copy link
Copy Markdown

When trying to build I get some compiler errors: https://pastebin.com/fp0VtvDJ

@dakra
Copy link
Copy Markdown
Owner

dakra commented May 18, 2026

@michaelneuper you have to use zig 15.2.

as I see you use homebrew:

brew uninstall zig
brew install zig@0.15

@emil-e emil-e force-pushed the emil-e/promote-wide branch from 1c7a679 to 78dadbc Compare May 18, 2026 13:28
@emil-e emil-e marked this pull request as ready for review May 18, 2026 13:28
@emil-e
Copy link
Copy Markdown
Collaborator Author

emil-e commented May 18, 2026

@claude Review this

@claude
Copy link
Copy Markdown

claude Bot commented May 18, 2026

Claude finished @emil-e's task in 8m 40s —— View job


Code review for #286

  • Read CLAUDE.md files for repo guidelines
  • Read changed files (Renderer.zig, emacs.zig, ghostel-test.el)
  • Verify diff against origin/main (used merge ref pull/286/merge)
  • Identify findings (BUG/QUESTION/NIT)
  • Post inline comments and summary

Summary

The patch is a nice generalization — handing each adjustable glyph off to its own function, hoisting selected-window out of the loop, and using (space :width 0) to reclaim cells is clean. The putTextProperty rework into a comptime symbol lookup is a real improvement: every callsite is now compile-time checked against the intern table.

The native test suite is red on all four test-native jobs (Linux/macOS × 29.4/snapshot). The other jobs are green.

Findings

BUG — adjustGlyph over-claims and claims-following-space fails (src/Renderer.zig:747-772). The break in the claim loop is if (glyph_aspect < cell_aspect) break; (strict <), and the past-EOL path is continue rather than break. Together those two let char_width step past the exact aspect-ratio match and keep growing as long as there's room left in the row. The failing test traces to char_width=4 (min-width=(4)) where the test expects (2). Inline comment has the per-iteration trace and a question about intended semantics — depending on the answer, either the break logic changes or the test expectation changes. Either way the four native jobs need to go green before merge.

QUESTION — char-after guards (src/Renderer.zig:761). The claim_pos >= row_end - 1 check above is the only thing that keeps the char-after call inside a valid range, and the else break is the only thing that stops (space :width 0) being splashed onto non-space buffer text. Both are correct today, but they're tightly coupled with the past-EOL continue, so changing one should re-check the other.

NIT — putTextProperty signature surfaces an implicit constraint (src/emacs.zig:307-314). Resolving prop via @field(sym, prop) gives compile-time checking (great), but the requirement that prop exists in interned_symbols isn't visible from the signature. A one-line doc comment pointing at the intern table would save the next contributor a head-scratch.

NIT — tab vs space indentation in the new tests (test/ghostel-test.el:15283-15391). The new glyph-adjust-* blocks are indented with tabs while the rest of the file uses spaces. untabify / indent-region would fix it.

Not findings

  • col tracking through SPACER_TAIL / SPACER_HEAD is correct because Zig runs the while … : (col += 1) continue-expression on continue, so a wide cell + its tail still advances col by 2.
  • The min_width / height display spec shape (((min-width (N)) (height S))) round-trips correctly with (assq 'min-width …) in the tests.
  • The bold-color tests are not new — they were moved earlier in the file to sit next to the new glyph tests, no behavior change.
  • Compile errors in @michaelneuper's pastebin are from Zig 0.14 vs 0.15 and unrelated to this PR.
    • Branch: emil-e/promote-wide

Comment thread src/Renderer.zig
Comment thread src/Renderer.zig
Comment thread src/emacs.zig
Comment thread test/ghostel-test.el Outdated
@emil-e emil-e force-pushed the emil-e/promote-wide branch 2 times, most recently from 03f8e61 to e0c74fb Compare May 18, 2026 13:47
@dakra dakra force-pushed the emil-e/promote-wide branch from e0c74fb to 80b97b0 Compare May 18, 2026 14:44
@dakra dakra merged commit 80b97b0 into dakra:main May 18, 2026
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants