fix(display): model GNU box-run edge ownership - #287
Conversation
…ndow edge) Fixes eval-exec#284. an `:extend` face that also carries a `:box` (the shape a dock/mode face resolves to -- e.g. agentty-mode's multi-dock layout, which turns on window-divider-mode) painted a tall box hugging the window's right edge, over the content. it only showed up in the window that had a right divider, and turning window-divider-mode off made it vanish -- which is what made it look like a divider bug. it isn't. the divider draws fine (a 1px filled rect). the box is the `:extend` end-of-line FILLER inheriting the face's `:box`. GNU's `extend_face_to_end_of_line` fills the rest of the line with the extend face's colours but never boxes the filler -- the `:box` is a per-character glyph decoration that stops at the last real char. neomacs stamped the filler stretch with the full extend face id, box and all, so the renderer (faithfully) drew a box around the whole fill. the box spans the fill from end-of-text to the row's right edge, which is why it's tall; the divider only made it VISIBLE by narrowing the window so the fill's right edge landed at the divider instead of off-frame, and the box_corner_radius on the dock face is what made it a rounded outline. fix: when building the line-end extend fill, if the extend face is boxed, swap in a box-free sibling face (a distinct id, box stripped, same colours) for the filler stretch. the real boxed characters keep their box. this is `extend_face_to_end_of_line`'s GNU behaviour. content/text faces already can't carry a rounded box (from_resolved hardcodes box_corner_radius: 0), so this was the only path a rounded box could reach a full-width glyph. - new box_free_extend_fill_face() on DisplayRowActiveFaceState builds the stripped sibling and interns it; the newline line-end path installs it and uses its id for the fill. - repro test boxed_extend_fill_does_not_box_the_end_of_line_filler drives a full frame layout with an :extend + :box face over a line and asserts the fill stretch keeps the background but resolves to box_type None. it FAILS before this change (fill face box_type == Line) and passes after. known remaining edge: the visual-wrap / end-of-buffer fill path (extend_face_to_end_of_line, used by overflow/nested display sources) does not yet strip the box -- those callers don't have the frame face arena in scope. the common newline case (this bug) is fixed; threading the arena through the overflow path is a follow-up. layout-engine suite green (1989 passed; the only 2 failures are pre-existing env-dependent font weight-realization checks that also fail on clean main on this box).
There was a problem hiding this comment.
🔵 Needs a closer look
The fix hinges on subtle interactions in the content-addressed frame-face-id and face-realization pipeline plus GPU-rendered visual behavior that warrant final human verification, and it intentionally leaves the visual-wrap/end-of-buffer fill path unfixed.
Pull request overview
This PR fixes issue #284, where an :extend face that also carries a :box (as dock/mode faces resolve to under window-divider-mode) painted a tall box hugging the window's right edge over the content. The root cause was that neomacs stamped the :extend end-of-line filler stretch with the full boxed face id, so the wgpu renderer faithfully drew a box across the whole fill. GNU's extend_face_to_end_of_line fills the line with the extend face's colors but never boxes the filler. The fix swaps in a box-free sibling face for the filler stretch in the common newline path, keeping the real characters' box intact.
Changes:
- Adds
box_free_extend_fill_face()onDisplayRowActiveFaceState, which returns a box-stripped, content-addressed sibling face (same colors) when the active extend face is boxed. - Wires the newline line-end path to install and use that box-free face id for the filler stretch, while boxed characters retain their box.
- Adds a regression test asserting the fill stretch keeps the background but resolves to
box_type == None.
File summaries
| File | Description |
|---|---|
neomacs-layout-engine/src/display_row/face_state.rs |
New box_free_extend_fill_face() helper that builds and interns a box-free sibling of the extend fill face. |
neomacs-layout-engine/src/buffer_source/row_lifecycle.rs |
Newline line-end path now installs the box-free sibling and uses its id for the :extend filler stretch. |
neomacs-layout-engine/src/engine_test.rs |
Adds repro/regression test boxed_extend_fill_does_not_box_the_end_of_line_filler. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Thanks for tracing the visible artifact upstream. I think this needs a different fix before merge:
-
The box-free substitution in this PR inverts GNU semantics. GNU xdisp.c explicitly says that face extension extends both the background and the box, then keeps extend_face_id on the EOL stretch glyph. The native renderers retain that box and use left_box_line_p/right_box_line_p in xterm and the equivalent PGTK logic to control its vertical edges. This patch instead removes all box decoration, and the new regression locks in that non-GNU result. Please preserve the boxed face and fix the box-run edge geometry/representation so the filler does not become an erroneous standalone closed rounded rectangle.
-
The substitution exists only in the ExplicitNewline path. The shared visual-wrap/end-of-buffer path still passes the original boxed face_id to RowExtendFill, so the same edge-hugging artifact remains for a wrapped row or point-max without a newline. The box-edge policy should live in the canonical RowExtendFill/layout-to-render seam so every row-ending route behaves consistently.
The deeper missing abstraction appears to be GNU-style box-run topology: the protocol carries a boxed face but not authoritative left/right edge flags. A typed box-edge policy carried from layout to rendering would preserve GNU :extend behavior and prevent the renderer from inferring a complete rounded rectangle for every filler span.
PR eval-exec#287 removed :box from the :extend end-of-line filler to hide a tall vertical border at the window edge. That fixes the symptom by changing semantics: GNU xdisp extends both the background and the box. The artifact came from Neomacs treating glyphs and fillers as if they independently owned both vertical box terminals. Introduce typed box-run participation and left/right terminal ownership. Derive it from the semantic source faces before and after each run, then preserve it through clipping, wrapping, display replacements, nested and overlay strings, EOB handling, bidi reordering, retained-row hashing, media materialization, and transient pointer painting. Continuing runs retain their horizontal rails while suppressing only false internal vertical edges. Make explicit :box nil survive face inheritance and dump serialization. Carry face and edge ownership for media, distinguish image content from its margin-inclusive GNU glyph slot, and invalidate predecessor rows when successor-face lookahead changes their terminal topology. Centralize sharp, 3D, and rounded box tessellation so primary and child-frame rendering use one implementation. Keep asymmetric image margins in a compact row-local table so the hot Glyph representation does not grow. The expanded GNU-oracle regressions also exposed a line-height t placement bug: content-only rows could shrink visually but still advanced by the default font grid. Advance by the signed finished-row height delta so following rows begin at the accepted geometry. Add protocol, layout, renderer, pointer/media, incremental-reuse, bidi, overlay/display-string, and offscreen primary/child-frame coverage. Refs eval-exec#284
Resolve the nine overlapping display/layout files without discarding either line of development. Preserve PR eval-exec#287's GNU-compatible typed box-run ownership, source-boundary transport, media topology, and shared primary/child-frame tessellation. Preserve main's typed overlay attachment/point positions, fixed-pitch replacement regression, cursor-presentation refactor, and device-scale-aware box geometry. Adapt the integration boundary so ImageScaleEnvironment exposes a validated DeviceScale. Layout image expansion and both renderer box paths now use the same logical-to-device conversion instead of reviving the removed raw-width API. Verification: - cargo fmt --all -- --check - cargo check --release for protocol, layout, renderer, runtime, and VM - 8 focused layout topology tests passed - current main's fixed-pitch test reproduces the same environment-specific failure on an isolated origin/main worktree - 10 focused renderer/offscreen topology tests passed
eval-exec
changed the title
Fixes #284.
Root cause
The original patch correctly traced the visible artifact to the
:extendend-of-line filler, but its proposed box-free sibling face changed GNU semantics. GNU Emacs extends both the background and the box; it independently records whether a glyph owns the left and right vertical box terminals.Neomacs preserved the boxed face but did not preserve that topology. Ordinary glyphs and filler stretches effectively behaved as if each independently owned both terminals, so an extending filler could become a large standalone closed box at the window edge.
Fix
Model GNU box-run topology explicitly instead of removing the decoration:
DeviceScaleto layout image expansion and renderer border thickness.The expanded coverage also exposed and fixes a
line-height trow-advance bug: following rows now advance from the accepted content-only height instead of the default font grid.This is source-derived rather than reconstructed from visible adjacency, so scrolling, clipping, RTL, and incremental row reuse do not fabricate terminals at viewport boundaries.
Verification
cargo fmt --all -- --checkneomacs-display-protocol,neomacs-layout-engine,neomacs-renderer-wgpu,neomacs-display-runtime, andneovm-coreorigin/mainand this branch reproduce the same environment-specific fixed-pitch test failure (expected both candidate rows, found one), so it is not introduced by this PRThe feature commit and the main-integration merge commit contain the detailed design and conflict-resolution rationale.