fonts: Fix shaping metrics

This was a bit of a PITA to run down; the essence of the problem
was that the shaper was returning an x_advance of 0 for U+3000,
which caused wezterm's shaping layer to elide that glyph.

I eventually tracked down the x_advance to be the result of
scaling by an x_scale of 0, which in turn is the result of
harfbuzz not knowing the font size.

The critical portion of this diff is the line that advises
harfbuzz that the font has changed after we've applied the
font size.

The rest is just stuff to make it easier to debug and verify.

This:

```
printf "x\u3000x."
```

Now correctly renders on screen as "x  x".

fixes: wez#1161

docs/changelog.md

@@ -46,6 +46,7 @@ As features stabilize some brief notes about them will accumulate here.
 * Fixed: error matching a font when that font is in a .ttc that contains multiple font families. [#1137](https://github.com/wez/wezterm/issues/1137)
 * Improved: `.deb` packages now `Provides: x-terminal-emulator`. [#1139](https://github.com/wez/wezterm/issues/1139)
 * Fixed: Wayland: panic with most recent wlroots. Thanks to [@unrelentingtech](https://github.com/unrelentingtech)! [#1144](https://github.com/wez/wezterm/issues/1144)
+* Fixed: incorrect spacing for IDEOGRAPHIC SPACE. [#1161](https://github.com/wez/wezterm/issues/1161)
 
 ### 20210814-124438-54e29167
 

term/src/test/mod.rs

@@ -466,6 +466,21 @@ fn test_semantic() {
     );
 }
 
+#[test]
+fn issue_1161() {
+    let mut term = TestTerm::new(1, 5, 0);
+    term.print("x\u{3000}x");
+    assert_visible_contents(
+        &term,
+        file!(),
+        line!(),
+        &[
+            // U+3000 is ideographic space, a double-width space
+            "x\u{3000}x ",
+        ],
+    );
+}
+
 #[test]
 fn basic_output() {
     let mut term = TestTerm::new(5, 10, 0);

termwiz/src/cell.rs

@@ -858,6 +858,7 @@ pub enum AttributeChange {
 #[cfg(test)]
 mod test {
     use super::*;
+    use unicode_segmentation::UnicodeSegmentation;
 
     #[test]
     fn teeny_string() {
@@ -948,9 +949,21 @@ mod test {
         assert_eq!(unicode_column_width(font_awesome_star), 1);
     }
 
+    #[test]
+    fn issue_1161() {
+        let x_ideographic_space_x = "x\u{3000}x";
+        assert_eq!(unicode_column_width(x_ideographic_space_x), 4);
+        assert_eq!(
+            x_ideographic_space_x.graphemes(true).collect::<Vec<_>>(),
+            vec!["x".to_string(), "\u{3000}".to_string(), "x".to_string()],
+        );
+
+        let c = Cell::new_grapheme("\u{3000}", CellAttributes::blank());
+        assert_eq!(c.width(), 2);
+    }
+
     #[test]
     fn issue_997() {
-        use unicode_segmentation::UnicodeSegmentation;
         let victory_hand = "\u{270c}";
         let victory_hand_text_presentation = "\u{270c}\u{fe0e}";
 

wezterm-font/src/hbwrap.rs

@@ -60,6 +60,12 @@ impl Font {
         }
     }
 
+    pub fn font_changed(&mut self) {
+        unsafe {
+            hb_ft_font_changed(self.font);
+        }
+    }
+
     pub fn set_load_flags(&mut self, load_flags: freetype::FT_Int32) {
         unsafe {
             hb_ft_font_set_load_flags(self.font, load_flags);
@@ -110,12 +116,14 @@ impl Buffer {
         }
     }
 
+    #[allow(dead_code)]
     pub fn set_direction(&mut self, direction: hb_direction_t) {
         unsafe {
             hb_buffer_set_direction(self.buf, direction);
         }
     }
 
+    #[allow(dead_code)]
     pub fn set_script(&mut self, script: hb_script_t) {
         unsafe {
             hb_buffer_set_script(self.buf, script);
@@ -170,4 +178,35 @@ impl Buffer {
             slice::from_raw_parts(pos, len as usize)
         }
     }
+
+    #[allow(dead_code)]
+    pub fn serialize(&self, font: Option<&Font>) -> String {
+        unsafe {
+            let len = hb_buffer_get_length(self.buf);
+            let mut text = vec![0u8; len as usize * 16];
+            let buf_len = text.len();
+            let mut text_len = 0;
+            hb_buffer_serialize(
+                self.buf,
+                0,
+                len,
+                text.as_mut_ptr() as *mut _,
+                buf_len as _,
+                &mut text_len,
+                match font {
+                    Some(f) => f.font,
+                    None => std::ptr::null_mut(),
+                },
+                harfbuzz::hb_buffer_serialize_format_t::HB_BUFFER_SERIALIZE_FORMAT_TEXT,
+                harfbuzz::hb_buffer_serialize_flags_t::HB_BUFFER_SERIALIZE_FLAG_DEFAULT,
+            );
+            String::from_utf8_lossy(&text[0..text_len as usize]).into()
+        }
+    }
+
+    pub fn guess_segment_properties(&mut self) {
+        unsafe {
+            hb_buffer_guess_segment_properties(self.buf);
+        }
+    }
 }

wezterm-font/src/shaper/harfbuzz.rs

@@ -13,22 +13,15 @@ use termwiz::cell::{unicode_column_width, Presentation};
 use thiserror::Error;
 use unicode_segmentation::UnicodeSegmentation;
 
-#[derive(Clone)]
-struct Info<'a> {
+#[derive(Clone, Debug)]
+struct Info {
     cluster: usize,
     len: usize,
     codepoint: harfbuzz::hb_codepoint_t,
-    pos: &'a harfbuzz::hb_glyph_position_t,
-}
-
-impl<'a> std::fmt::Debug for Info<'a> {
-    fn fmt(&self, fmt: &mut std::fmt::Formatter<'_>) -> Result<(), std::fmt::Error> {
-        fmt.debug_struct("Info")
-            .field("cluster", &self.cluster)
-            .field("len", &self.len)
-            .field("codepoint", &self.codepoint)
-            .finish()
-    }
+    x_advance: harfbuzz::hb_position_t,
+    y_advance: harfbuzz::hb_position_t,
+    x_offset: harfbuzz::hb_position_t,
+    y_offset: harfbuzz::hb_position_t,
 }
 
 fn make_glyphinfo(text: &str, font_idx: usize, info: &Info) -> GlyphInfo {
@@ -42,10 +35,10 @@ fn make_glyphinfo(text: &str, font_idx: usize, info: &Info) -> GlyphInfo {
         font_idx,
         glyph_pos: info.codepoint,
         cluster: info.cluster as u32,
-        x_advance: PixelLength::new(f64::from(info.pos.x_advance) / 64.0),
-        y_advance: PixelLength::new(f64::from(info.pos.y_advance) / 64.0),
-        x_offset: PixelLength::new(f64::from(info.pos.x_offset) / 64.0),
-        y_offset: PixelLength::new(f64::from(info.pos.y_offset) / 64.0),
+        x_advance: PixelLength::new(f64::from(info.x_advance) / 64.0),
+        y_advance: PixelLength::new(f64::from(info.y_advance) / 64.0),
+        x_offset: PixelLength::new(f64::from(info.x_offset) / 64.0),
+        y_offset: PixelLength::new(f64::from(info.y_offset) / 64.0),
     }
 }
 
@@ -180,6 +173,7 @@ impl HarfbuzzShaper {
         buf.set_direction(harfbuzz::hb_direction_t::HB_DIRECTION_LTR);
         buf.set_language(self.lang);
         buf.add_str(s);
+        buf.guess_segment_properties();
         buf.set_cluster_level(
             harfbuzz::hb_buffer_cluster_level_t::HB_BUFFER_CLUSTER_LEVEL_MONOTONE_GRAPHEMES,
         );
@@ -201,9 +195,18 @@ impl HarfbuzzShaper {
                         }
                     }
                     let size = pair.face.set_font_size(font_size, dpi)?;
+                    // Tell harfbuzz to recompute important font metrics!
+                    pair.font.font_changed();
                     cell_width = size.width;
                     shaped_any = pair.shaped_any;
                     pair.font.shape(&mut buf, self.features.as_slice());
+                    /*
+                    log::info!(
+                        "shaped font_idx={} as: {}",
+                        font_idx,
+                        buf.serialize(Some(&pair.font))
+                    );
+                    */
                     break;
                 }
                 None => {
@@ -273,7 +276,10 @@ impl HarfbuzzShaper {
                 cluster: info.cluster as usize,
                 len: next_pos - info.cluster as usize,
                 codepoint: info.codepoint,
-                pos,
+                x_advance: pos.x_advance,
+                y_advance: pos.y_advance,
+                x_offset: pos.x_offset,
+                y_offset: pos.y_offset,
             };
 
             if let Some(ref mut cluster) = info_clusters.last_mut() {
@@ -350,12 +356,11 @@ impl HarfbuzzShaper {
 
             let mut next_idx = 0;
             for info in infos.iter() {
-                if info.pos.x_advance == 0 {
+                if info.x_advance == 0 {
                     continue;
                 }
 
-                let nom_width =
-                    ((f64::from(info.pos.x_advance) / 64.0) / cell_width).ceil() as usize;
+                let nom_width = ((f64::from(info.x_advance) / 64.0) / cell_width).ceil() as usize;
 
                 let len;
                 if nom_width == 0 || !substr.is_char_boundary(next_idx + nom_width) {
@@ -418,7 +423,7 @@ impl FontShaper for HarfbuzzShaper {
         no_glyphs: &mut Vec<char>,
         presentation: Option<Presentation>,
     ) -> anyhow::Result<Vec<GlyphInfo>> {
-        log::trace!("shape {} `{}`", text.len(), text);
+        log::trace!("shape byte_len={} `{}`", text.len(), text.escape_debug());
         let start = std::time::Instant::now();
         let result = self.do_shape(0, text, size, dpi, no_glyphs, presentation);
         metrics::histogram!("shape.harfbuzz", start.elapsed());
@@ -720,5 +725,107 @@ mod test {
                 ]
             );
         }
+
+        {
+            let mut no_glyphs = vec![];
+            let info = shaper.shape("x x", 10., 72, &mut no_glyphs, None).unwrap();
+            assert!(no_glyphs.is_empty(), "{:?}", no_glyphs);
+            assert_eq!(
+                info,
+                vec![
+                    GlyphInfo {
+                        cluster: 0,
+                        is_space: false,
+                        font_idx: 0,
+                        glyph_pos: 350,
+                        num_cells: 1,
+                        #[cfg(debug_assertions)]
+                        text: "x".into(),
+                        x_advance: PixelLength::new(6.),
+                        x_offset: PixelLength::new(0.),
+                        y_advance: PixelLength::new(0.),
+                        y_offset: PixelLength::new(0.),
+                    },
+                    GlyphInfo {
+                        #[cfg(debug_assertions)]
+                        text: " ".into(),
+                        is_space: true,
+                        cluster: 1,
+                        num_cells: 1,
+                        font_idx: 0,
+                        glyph_pos: 686,
+                        x_advance: PixelLength::new(6.),
+                        x_offset: PixelLength::new(0.),
+                        y_advance: PixelLength::new(0.),
+                        y_offset: PixelLength::new(0.),
+                    },
+                    GlyphInfo {
+                        cluster: 2,
+                        is_space: false,
+                        font_idx: 0,
+                        glyph_pos: 350,
+                        num_cells: 1,
+                        #[cfg(debug_assertions)]
+                        text: "x".into(),
+                        x_advance: PixelLength::new(6.),
+                        x_offset: PixelLength::new(0.),
+                        y_advance: PixelLength::new(0.),
+                        y_offset: PixelLength::new(0.),
+                    }
+                ]
+            );
+        }
+
+        {
+            let mut no_glyphs = vec![];
+            let info = shaper
+                .shape("x\u{3000}x", 10., 72, &mut no_glyphs, None)
+                .unwrap();
+            assert!(no_glyphs.is_empty(), "{:?}", no_glyphs);
+            assert_eq!(
+                info,
+                vec![
+                    GlyphInfo {
+                        cluster: 0,
+                        is_space: false,
+                        font_idx: 0,
+                        glyph_pos: 350,
+                        num_cells: 1,
+                        #[cfg(debug_assertions)]
+                        text: "x".into(),
+                        x_advance: PixelLength::new(6.),
+                        x_offset: PixelLength::new(0.),
+                        y_advance: PixelLength::new(0.),
+                        y_offset: PixelLength::new(0.),
+                    },
+                    GlyphInfo {
+                        #[cfg(debug_assertions)]
+                        text: "\u{3000}".into(),
+                        is_space: false,
+                        cluster: 1,
+                        num_cells: 2,
+                        font_idx: 0,
+                        glyph_pos: 686,
+                        x_advance: PixelLength::new(10.),
+                        x_offset: PixelLength::new(0.),
+                        y_advance: PixelLength::new(0.),
+                        y_offset: PixelLength::new(0.),
+                    },
+                    GlyphInfo {
+                        cluster: 4,
+                        is_space: false,
+                        font_idx: 0,
+                        glyph_pos: 350,
+                        num_cells: 1,
+                        #[cfg(debug_assertions)]
+                        text: "x".into(),
+                        x_advance: PixelLength::new(6.),
+                        x_offset: PixelLength::new(0.),
+                        y_advance: PixelLength::new(0.),
+                        y_offset: PixelLength::new(0.),
+                    }
+                ]
+            );
+        }
     }
 }