From 063e4a5064adb1a1534caa5ffacdbbd6467c87dd Mon Sep 17 00:00:00 2001 From: thomas Date: Tue, 28 Jul 2026 09:14:14 +0800 Subject: [PATCH 1/2] fix(render): keep underlines on natively-drawn cells, and stroke weight uniform at fractional DPI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two follow-ups to #229. Underlines ride on the `TextRun` that `paint_glyphs` builds, and the Solo arm returned early for every natively-drawn cell — so an `ESC[4m` span or a hovered URL showed a one-column hole wherever it crossed a box-drawing character. The mechanism predates #229 (the Powerline branch has always had it), but #229 widened it from a dozen private-use separators to all 256 characters of U+2500–U+259F. Such a cell now shapes a space in its own style instead of returning, so gpui draws the line from the same `UnderlineStyle` — curly and double included — that every other cell uses. Stroke weight varied between cells at fractional device scale. `rectb` snaps a rect's two edges independently, which is what makes neighbouring cells tile, but two edges `w` apart land `w × scale` device pixels apart: when that is not a whole number the two roundings straddle it, so a 1-logical-pixel rule came out 1 device pixel wide in one column and 2 in the next. At Windows' default 125%/150% scaling that alternated thin/thick across every column of a TUI table, and down every row for horizontal rules. Integer scales are blind to it by construction, which is why 1x and 2x looked right. `light_thickness` now quantises to whole device pixels, and `vstroke` / `hstroke` lay that width off from the snapped near edge rather than inferring it from a second snap — float ties at `.5` made the quantisation alone insufficient. Stroke ends still snap, so #229's tiling guarantee is untouched. Block elements stay on `rectb`: they are area fills, not strokes. Co-Authored-By: Claude Opus 5 --- src/terminal/boxdraw.rs | 188 +++++++++++++++++++++++++++++++++++----- src/terminal/element.rs | 101 ++++++++++++++++++--- 2 files changed, 257 insertions(+), 32 deletions(-) diff --git a/src/terminal/boxdraw.rs b/src/terminal/boxdraw.rs index 2ddc6648..61d1cd92 100644 --- a/src/terminal/boxdraw.rs +++ b/src/terminal/boxdraw.rs @@ -69,11 +69,8 @@ enum Arm { Heavy, } -/// Cell geometry in f32, plus the light stroke thickness. -/// -/// Thickness derives from the cell *width* — a pure font-size proxy — never the -/// height: the height carries the line-height stretch, and a `─` that fattens -/// when the user opens up their line spacing would look broken. +/// Cell geometry in f32, plus the light stroke thickness `t` (see +/// [`light_thickness`] for how that one is chosen). struct Cell { x0: f32, y0: f32, @@ -85,12 +82,36 @@ struct Cell { scale: f32, } +/// The light stroke thickness for a cell `cell_width` wide, in logical pixels. +/// +/// Two rules, in order: +/// +/// 1. Derive from the cell *width* — a pure font-size proxy — never the height: +/// the height carries the line-height stretch, and a `─` that fattens when +/// the user opens up their line spacing would look broken. +/// 2. Then quantise so the result covers a whole number of device pixels. +/// +/// Rule 2 keeps the nominal weight and the painted weight in agreement: +/// [`Cell::vstroke`] lays a stroke off in whole device pixels, and everything +/// positioned relative to `t` (the arm overshoot, the double-line separation, +/// `heavy = 2 × light`) should be reasoning about the same value the rasteriser +/// will actually produce. +/// +/// Rounding the logical value *first* is what keeps 1x and 2x byte-identical to +/// what this module shipped with — those are the scales it was tuned and +/// visually verified at, so the fractional-scale fix must not disturb them. +fn light_thickness(cell_width: f32, scale: f32) -> f32 { + let logical = (cell_width * 0.15).round().max(1.); + (logical * scale).round().max(1.) / scale +} + impl Cell { fn new(b: &Bounds, scale: f32) -> Self { let x0 = b.origin.x.as_f32(); let y0 = b.origin.y.as_f32(); let x1 = x0 + b.size.width.as_f32(); let y1 = y0 + b.size.height.as_f32(); + let scale = scale.max(0.1); Cell { x0, y0, @@ -98,8 +119,8 @@ impl Cell { y1, cx: (x0 + x1) / 2., cy: (y0 + y1) / 2., - t: ((x1 - x0) * 0.15).round().max(1.), - scale: scale.max(0.1), + t: light_thickness(x1 - x0, scale), + scale, } } @@ -123,6 +144,49 @@ impl Cell { Ink::Rect(self.rectb(x, y, w, h)) } + /// A logical thickness as a whole number of device pixels, back in logical + /// units. Never zero: a stroke that rounds away is worse than one that is + /// a touch too thick. + fn stroke_px(&self, w: f32) -> f32 { + (w * self.scale).round().max(1.) / self.scale + } + + /// A vertical stroke of logical width `w`, centred on `x`, spanning + /// `ya..yb`. + /// + /// The two *ends* snap like any other edge, so a stroke that runs to a cell + /// boundary still shares that boundary exactly with the cell beyond it — + /// the tiling property [`rectb`](Self::rectb) exists for. + /// + /// The *width* is deliberately not a second pair of independent snaps. Two + /// edges `w` apart land `w × scale` device pixels apart, and unless that is + /// exactly a whole number the two `round`s straddle it — rounding apart in + /// some cells and together in others, which made vertical rules alternate + /// thin/thick across the columns of a TUI table at Windows' default 125% / + /// 150% scaling. [`light_thickness`] picks `w` so the product is integral, + /// but `f32` cannot always represent it exactly (a `1.5×` scale gives + /// `2/1.5 × 1.5 = 2.0000001`), and a coordinate landing on a `.5` tie then + /// rounds whichever way the error points. Laying the width off from the + /// snapped near edge sidesteps the tie entirely: same weight everywhere, + /// by construction rather than by luck. + fn vstroke(&self, x: f32, w: f32, ya: f32, yb: f32) -> Ink { + let (x0, y0, y1) = (self.snap(x - w / 2.), self.snap(ya), self.snap(yb)); + Ink::Rect(Bounds::new( + point(px(x0), px(y0)), + size(px(self.stroke_px(w)), px(y1 - y0)), + )) + } + + /// A horizontal stroke of logical width `w`, centred on `y`, spanning + /// `xa..xb`. See [`vstroke`](Self::vstroke). + fn hstroke(&self, y: f32, w: f32, xa: f32, xb: f32) -> Ink { + let (y0, x0, x1) = (self.snap(y - w / 2.), self.snap(xa), self.snap(xb)); + Ink::Rect(Bounds::new( + point(px(x0), px(y0)), + size(px(x1 - x0), px(self.stroke_px(w))), + )) + } + /// The light/heavy arm combinations: one rectangle per arm, each running /// from its cell edge to just past the centre. /// @@ -141,16 +205,16 @@ impl Cell { let m = wu.max(wd).max(wl).max(wr) / 2.; let mut ink = Vec::new(); if wu > 0. { - ink.push(self.rect(self.cx - wu / 2., self.y0, wu, self.cy + m - self.y0)); + ink.push(self.vstroke(self.cx, wu, self.y0, self.cy + m)); } if wd > 0. { - ink.push(self.rect(self.cx - wd / 2., self.cy - m, wd, self.y1 - (self.cy - m))); + ink.push(self.vstroke(self.cx, wd, self.cy - m, self.y1)); } if wl > 0. { - ink.push(self.rect(self.x0, self.cy - wl / 2., self.cx + m - self.x0, wl)); + ink.push(self.hstroke(self.cy, wl, self.x0, self.cx + m)); } if wr > 0. { - ink.push(self.rect(self.cx - m, self.cy - wr / 2., self.x1 - (self.cx - m), wr)); + ink.push(self.hstroke(self.cy, wr, self.cx - m, self.x1)); } ink } @@ -173,8 +237,8 @@ impl Cell { let (x0, x1, y0, y1, cx, cy) = (self.x0, self.x1, self.y0, self.y1, self.cx, self.cy); let (va, vb) = (cx - d, cx + d); let (ha, hb) = (cy - d, cy + d); - let v = |x: f32, ya: f32, yb: f32| self.rect(x - h, ya, t, yb - ya); - let hz = |y: f32, xa: f32, xb: f32| self.rect(xa, y - h, xb - xa, t); + let v = |x: f32, ya: f32, yb: f32| self.vstroke(x, t, ya, yb); + let hz = |y: f32, xa: f32, xb: f32| self.hstroke(y, t, xa, xb); Some(match c { '═' => vec![hz(ha, x0, x1), hz(hb, x0, x1)], '║' => vec![v(va, y0, y1), v(vb, y0, y1)], @@ -294,16 +358,14 @@ impl Cell { // seam exactly where they hand off. let lap = 1. / self.scale; if sy > 0. { - let top = cy + r - lap; - ink.push(self.rect(cx - h, top, self.t, self.y1 - top)); + ink.push(self.vstroke(cx, self.t, cy + r - lap, self.y1)); } else { - ink.push(self.rect(cx - h, self.y0, self.t, (cy - r + lap) - self.y0)); + ink.push(self.vstroke(cx, self.t, self.y0, cy - r + lap)); } if sx > 0. { - let left = cx + r - lap; - ink.push(self.rect(left, cy - h, self.x1 - left, self.t)); + ink.push(self.hstroke(cy, self.t, cx + r - lap, self.x1)); } else { - ink.push(self.rect(self.x0, cy - h, (cx - r + lap) - self.x0, self.t)); + ink.push(self.hstroke(cy, self.t, self.x0, cx - r + lap)); } // The arc band, from the vertical stub (θ=0) to the horizontal one // (θ=π/2) around the arc centre one radius into the quadrant. @@ -399,9 +461,9 @@ impl Cell { let s = a0 + seg * (i as f32 + 0.15); let len = seg * 0.7; if vertical { - self.rect(self.cx - w / 2., s, w, len) + self.vstroke(self.cx, w, s, s + len) } else { - self.rect(s, self.cy - w / 2., len, w) + self.hstroke(self.cy, w, s, s + len) } }) .collect(); @@ -820,4 +882,88 @@ mod tests { let top = extents(&glyph('│', below, scale).unwrap()).2; assert_eq!(bottom, top, "adjacent │ cells no longer tile"); } + + /// Every column must draw `│` at the *same* weight, and every row must draw + /// `─` at the same weight, at any scale factor — not just the integer ones. + /// + /// Note what the test above does *not* catch: it asserts each edge lands on + /// the device grid, which a 1-device-pixel stroke and a 2-device-pixel + /// stroke both satisfy. Windows' default 125%/150% display scaling put a + /// 1-logical-pixel stroke a non-integer number of device pixels wide, and + /// the two independent edge snaps then rounded apart in some columns and + /// together in others: vertical rules alternated thin/thick across a TUI + /// table, horizontal rules alternated down it. Both 1x and 2x are blind to + /// it by construction, so the earlier fixtures could never have failed. + #[test] + fn stroke_weight_is_uniform_across_cells_at_any_scale() { + // Realistic cell metrics: a 13/15/16px font's advance, line_height 1.4. + for (cw, lh) in [(7.8f32, 18.0f32), (9.03, 21.0), (9.6, 22.0), (10.8, 25.0)] { + for scale in [1.0f32, 1.25, 1.5, 1.75, 2.0, 2.5, 3.0] { + let widths: Vec = (0..24) + .map(|i| { + let b = Bounds::new(point(px(cw * i as f32), px(0.)), size(px(cw), px(lh))); + let Ink::Rect(r) = &glyph('│', b, scale).unwrap()[0] else { + panic!("│ should be a rect") + }; + (r.size.width.as_f32() * scale).round() + }) + .collect(); + let (lo, hi) = ( + widths.iter().cloned().fold(f32::MAX, f32::min), + widths.iter().cloned().fold(f32::MIN, f32::max), + ); + assert_eq!( + lo, hi, + "│ weight varies {lo}..{hi} device px across columns \ + (cell_width {cw}, scale {scale}): {widths:?}" + ); + assert!(lo >= 1., "│ thinner than a device pixel at scale {scale}"); + + let heights: Vec = (0..24) + .map(|r| { + let b = Bounds::new(point(px(0.), px(lh * r as f32)), size(px(cw), px(lh))); + let Ink::Rect(rect) = &glyph('─', b, scale).unwrap()[0] else { + panic!("─ should be a rect") + }; + (rect.size.height.as_f32() * scale).round() + }) + .collect(); + let (lo, hi) = ( + heights.iter().cloned().fold(f32::MAX, f32::min), + heights.iter().cloned().fold(f32::MIN, f32::max), + ); + assert_eq!( + lo, hi, + "─ weight varies {lo}..{hi} device px across rows \ + (cell_width {cw}, scale {scale}): {heights:?}" + ); + } + } + } + + /// Quantising the thickness in device space must not change what 1x and 2x + /// already rendered — those are the two scales the module was tuned and + /// visually verified at. + #[test] + fn integer_scales_keep_their_previous_thickness() { + for (cw, lh) in [(7.8f32, 18.0f32), (9.03, 21.0), (9.6, 22.0), (10.8, 25.0)] { + for scale in [1.0f32, 2.0, 3.0] { + let previous = (cw * 0.15).round().max(1.); + assert_eq!( + light_thickness(cw, scale), + previous, + "cell_width {cw} at scale {scale} changed weight" + ); + // And heavy stays exactly twice light, as `arms` assumes. + let b = Bounds::new(point(px(0.), px(0.)), size(px(cw), px(lh))); + let Ink::Rect(l) = &glyph('│', b, scale).unwrap()[0] else { + panic!() + }; + let Ink::Rect(h) = &glyph('┃', b, scale).unwrap()[0] else { + panic!() + }; + assert!(h.size.width.as_f32() > l.size.width.as_f32()); + } + } + } } diff --git a/src/terminal/element.rs b/src/terminal/element.rs index 0e64be90..da6fc897 100644 --- a/src/terminal/element.rs +++ b/src/terminal/element.rs @@ -756,6 +756,27 @@ fn powerline_path(bounds: Bounds, shape: PowerlineShape) -> gpui::Path

Option { + style.draws_on_blanks().then_some(' ') +} + fn seg_clip_width(solo: bool, cells: usize, cell_width: Pixels) -> Pixels { if solo { cell_width * 2. @@ -838,17 +859,19 @@ fn paint_glyphs( point(geom.origin.x + geom.cell_width * (col as f32), y), size(geom.cell_width, geom.line_height), ); - if let Some(shape) = PowerlineShape::of(cell.c) { + // Two families paint as native geometry rather than as a + // font glyph: Powerline separators, and the box-drawing / + // block characters (`boxdraw`) — a font glyph only covers + // the font's own line height, which broke every vertical + // run of `│`/`╭`/`╰` into dashes at line_height > 1.0. + // Either way the cell may still owe an underline, so this + // records whether the ink is already down rather than + // returning outright. + let native = if let Some(shape) = PowerlineShape::of(cell.c) { let path = powerline_path(cell_bounds, shape); window.paint_path(path, GlyphStyle::of(cell).fg); - continue; - } - // Box-drawing / block characters paint as native geometry - // sized to the actual (line-height-stretched) cell. A font - // glyph only covers the font's own line height, which is - // what broke every vertical run of `│`/`╭`/`╰` into dashes - // at line_height > 1.0 — see `boxdraw`. - if let Some(ink) = + true + } else if let Some(ink) = super::boxdraw::glyph(cell.c, cell_bounds, window.scale_factor()) { let fg = GlyphStyle::of(cell).fg; @@ -863,9 +886,20 @@ fn paint_glyphs( super::boxdraw::Ink::Path(p) => window.paint_path(p, fg), } } - continue; + true + } else { + false + }; + if !native { + (col, 1, char_string(cell.c), None, true) + } else { + match native_cell_residue(&GlyphStyle::of(cell)) { + None => continue, + // `solo: false` clips the space to its own single + // column so the underline can't spill sideways. + Some(c) => (col, 1, char_string(c), None, false), + } } - (col, 1, char_string(cell.c), None, true) } // Same pinning as the batched runs, just for one base: two // columns get `force_width` so a fallback emoji face can't @@ -2092,6 +2126,51 @@ mod tests { ); } + /// A natively-drawn cell keeps its underline. + /// + /// Underlines ride on the `TextRun`, so the Solo arm's early return for + /// Powerline separators and box-drawing characters used to drop them: an + /// `ESC[4m` span or a hovered URL containing `─`, `│` or `` showed a + /// one-column hole where the line should have run through. The residue is + /// what closes it — a space shaped in the cell's own style, carrying the + /// underline and no glyph ink. + #[test] + fn natively_drawn_cells_still_carry_their_underline() { + let plain = GlyphStyle::of(&cell('│')); + assert_eq!( + native_cell_residue(&plain), + None, + "an unstyled box character has nothing left to shape" + ); + + for kind in [ + UnderlineKind::Single, + UnderlineKind::Double, + UnderlineKind::Curly, + ] { + let mut c = cell('│'); + c.underline = kind; + assert_eq!( + native_cell_residue(&GlyphStyle::of(&c)), + Some(' '), + "{kind:?} underline dropped on a box-drawing cell" + ); + } + + // A hovered link underlines even without an emulator underline, and + // the characters it spans may well be box drawing or a separator. + for ch in ['│', '─', '╭', '█', '\u{e0b0}'] { + let mut c = cell(ch); + c.link_hover = true; + assert_eq!( + native_cell_residue(&GlyphStyle::of(&c)), + Some(' '), + "hovered-link underline dropped on U+{:04X}", + ch as u32 + ); + } + } + #[test] fn segment_row_keeps_powerline_separators_solo() { // The native-draw intercept lives in the Solo arm of `paint_glyphs`; From 7ffd6afe65ac05d80658c77f2f89c8b6f1044288 Mon Sep 17 00:00:00 2001 From: thomas Date: Tue, 28 Jul 2026 10:36:46 +0800 Subject: [PATCH 2/2] docs(render): reattach seg_clip_width's doc comment native_cell_residue was inserted between seg_clip_width's doc block and the function itself, so rustdoc attached the clip-width prose to the residue helper and left seg_clip_width undocumented. Move the helper (with its own doc) above the block instead. No code change. Co-Authored-By: Claude Fable 5 --- src/terminal/element.rs | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/src/terminal/element.rs b/src/terminal/element.rs index da6fc897..b4044521 100644 --- a/src/terminal/element.rs +++ b/src/terminal/element.rs @@ -738,24 +738,6 @@ fn powerline_path(bounds: Bounds, shape: PowerlineShape) -> gpui::Path

Option { style.draws_on_blanks().then_some(' ') } +/// The width `paint_glyphs` clips a segment's paint to. +/// +/// A batched `Run`/`Wide` segment clips to its exact column span (`cells` +/// columns): its glyphs come from faces whose advance matches the cell, so +/// nothing should spill past that span. A lone `solo` glyph is different — it +/// can be a symbol whose face paints ink well past the single cell the grid +/// reserved for it: a non-Mono Nerd Font sets a *one-cell advance* on its icons +/// yet draws up to ~1.9 cells of ink (measured across Hasklug / Meslo / +/// JetBrainsMono NF), and the OS cascade serves a proportional `➜`/`❯` the same +/// way. Clipping that to one cell severs the glyph mid-ink — the incomplete +/// icons and the cut-off arrow in issue #17. +/// +/// Advance is no signal there (it reads one cell for exactly those overflowing +/// icons), so a solo glyph gets a two-cell window instead. A glyph that already +/// fits is untouched — it has no ink to spill — while a symbol that overflows +/// renders whole, bleeding into a trailing blank the way iTerm2 and Terminal.app +/// do with non-Mono faces. The two-cell bound keeps a pathological face from +/// smearing a lone glyph across the row. fn seg_clip_width(solo: bool, cells: usize, cell_width: Pixels) -> Pixels { if solo { cell_width * 2.