From 1e32ccbd2f945fcd4820bff4c10ca97318011c30 Mon Sep 17 00:00:00 2001 From: Wez Furlong Date: Sat, 5 Feb 2022 06:16:07 -0700 Subject: [PATCH] shaping: fix an issue where we'd lose combining marks like U+20d7 For a sequence like `e U+20d7` the intent is to render the `e` with a vector arrow over the top. This is typically implemented by fonts as an `e` followed by the vector glyph (or vice versa), where either one of those may have a zero advance so that the two elements are combined. There were two problems here: * During shaping we'd see the zero advance and assume that the entry was useless and skip it * During rendering, if we didn't think it had any cell width, we'd not render it Cursoring through that particular sequence can hide the vector mark if the cursor is set to the default block cursor due to annoyances in how the block cursor is rendered (it changes the fg color to match the bg, but for elements outside where we think the cursor is, this makes those elements invisible). refs: https://github.com/wez/wezterm/issues/1617 --- docs/changelog.md | 1 + wezterm-font/src/shaper/harfbuzz.rs | 6 +-- wezterm-gui/src/termwindow/render.rs | 70 +++++++++++++++++----------- 3 files changed, 45 insertions(+), 32 deletions(-) diff --git a/docs/changelog.md b/docs/changelog.md index c729d97a1..b3075b9c0 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -59,6 +59,7 @@ As features stabilize some brief notes about them will accumulate here. * Hangul text in NFD was not always correctly composed when shaping fonts. [#1573](https://github.com/wez/wezterm/issues/1573) * Avoid OOM when processing sixels with huge repeat counts [#1610](https://github.com/wez/wezterm/issues/1610) * Set the sticky bit on socket and pid files created in `XDG_RUNTIME_DIR` to avoid removal by tmpwatch [#1601](https://github.com/wez/wezterm/issues/1601) +* Shaping combining sequences like `e U+20d7` could "lose" the vector symbol if the font produced an entry with no `x_advance`. [#1617](https://github.com/wez/wezterm/issues/1617) ### 20220101-133340-7edc5b5a diff --git a/wezterm-font/src/shaper/harfbuzz.rs b/wezterm-font/src/shaper/harfbuzz.rs index dccaee001..4d2756843 100644 --- a/wezterm-font/src/shaper/harfbuzz.rs +++ b/wezterm-font/src/shaper/harfbuzz.rs @@ -463,15 +463,13 @@ impl HarfbuzzShaper { let mut remaining_cells = cluster_info.cell_width; for info in infos.iter() { - if info.x_advance == 0 { - continue; - } - // Proportional width based on relative pixel dimensions vs. other glyphs in // this same cluster let weighted_cell_width = (cluster_info.cell_width as f64 * info.x_advance as f64 / total_width) .ceil() as u8; + // Note that weighted_cell_width can legitimately compute as zero here + // for the case where a combining mark composes over another glyph let weighted_cell_width = weighted_cell_width.min(remaining_cells); remaining_cells = remaining_cells.saturating_sub(weighted_cell_width); diff --git a/wezterm-gui/src/termwindow/render.rs b/wezterm-gui/src/termwindow/render.rs index 31ebbcf29..8c22f9798 100644 --- a/wezterm-gui/src/termwindow/render.rs +++ b/wezterm-gui/src/termwindow/render.rs @@ -1921,7 +1921,11 @@ impl super::TermWindow { // Iterate each cell that comprises this glyph. There is usually // a single cell per glyph but combining characters, ligatures // and emoji can be 2 or more cells wide. - for glyph_idx in 0..info.pos.num_cells as usize { + // Conversely, a combining glyph can produce an entry that is zero + // cells wide and which combines with a neighboring glyph that we + // (will|did) emit in another iteration of the glyph_info loop + // so we must ensure that we iterate and observe that! + for glyph_idx in 0..info.pos.num_cells.max(1) as usize { let cell_idx = visual_cell_idx + glyph_idx; if cell_idx >= num_cols { @@ -2013,36 +2017,46 @@ impl super::TermWindow { // width. We only use that if we're the first cell that // comprises this glyph; if for some reason the cursor position // is in the middle of a glyph we just use a single cell. - let cursor_width = params.line.cells()[cell_idx].width() as u8; + let cursor_width = params + .line + .cells() + .get(cell_idx) + .map(|c| c.width() as u8) + // skip in weird situations, like a combining character + // "e⃗" where we have two glyphs and one of them has + // num_cells=0. + .unwrap_or(0); - let mut quad = layers[0].allocate()?; - quad.set_position( - pos_x, - pos_y, - pos_x - + if params.use_pixel_positioning { - pixel_width - } else { - cursor_width as f32 * cell_width - }, - pos_y + cell_height, - ); - quad.set_hsv(hsv); - quad.set_has_color(false); + if cursor_width > 0 { + let mut quad = layers[0].allocate()?; + quad.set_position( + pos_x, + pos_y, + pos_x + + if params.use_pixel_positioning { + pixel_width + } else { + cursor_width as f32 * cell_width + }, + pos_y + cell_height, + ); + quad.set_hsv(hsv); + quad.set_has_color(false); - quad.set_texture( - gl_state - .glyph_cache - .borrow_mut() - .cursor_sprite( - cursor_shape, - ¶ms.render_metrics, - cursor_width, - )? - .texture_coords(), - ); + quad.set_texture( + gl_state + .glyph_cache + .borrow_mut() + .cursor_sprite( + cursor_shape, + ¶ms.render_metrics, + cursor_width, + )? + .texture_coords(), + ); - quad.set_fg_color(cursor_border_color); + quad.set_fg_color(cursor_border_color); + } } }