From 8073ca6fe0365071295f5924501bcee8355aabe6 Mon Sep 17 00:00:00 2001
From: Emil Ernerfeldt <emil.ernerfeldt@gmail.com>
Date: Mon, 18 Sep 2023 14:09:15 +0200
Subject: [PATCH] Fix problems with tabs in text (#3355)

* Move ascent out of `GlyphInfo`

* Give a function a better name

* Make tab and thin space act more like a normal space
---
 crates/epaint/src/text/font.rs        | 22 +++++++++++-----------
 crates/epaint/src/text/text_layout.rs | 11 +++++------
 2 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/crates/epaint/src/text/font.rs b/crates/epaint/src/text/font.rs
index e6b495c23a2..163697d5f89 100644
--- a/crates/epaint/src/text/font.rs
+++ b/crates/epaint/src/text/font.rs
@@ -43,12 +43,6 @@ pub struct GlyphInfo {
     /// Unit: points.
     pub advance_width: f32,
 
-    /// `ascent` value from the font metrics.
-    /// this is the distance from the top to the baseline.
-    ///
-    /// Unit: points.
-    pub ascent: f32,
-
     /// Texture coordinates.
     pub uv_rect: UvRect,
 }
@@ -59,7 +53,6 @@ impl Default for GlyphInfo {
         Self {
             id: ab_glyph::GlyphId(0),
             advance_width: 0.0,
-            ascent: 0.0,
             uv_rect: Default::default(),
         }
     }
@@ -188,7 +181,7 @@ impl FontImpl {
             if let Some(space) = self.glyph_info(' ') {
                 let glyph_info = GlyphInfo {
                     advance_width: crate::text::TAB_SIZE as f32 * space.advance_width,
-                    ..GlyphInfo::default()
+                    ..space
                 };
                 self.glyph_info_cache.write().insert(c, glyph_info);
                 return Some(glyph_info);
@@ -205,7 +198,7 @@ impl FontImpl {
                 let advance_width = f32::min(em / 6.0, space.advance_width * 0.5);
                 let glyph_info = GlyphInfo {
                     advance_width,
-                    ..GlyphInfo::default()
+                    ..space
                 };
                 self.glyph_info_cache.write().insert(c, glyph_info);
                 return Some(glyph_info);
@@ -255,6 +248,14 @@ impl FontImpl {
         self.pixels_per_point
     }
 
+    /// This is the distance from the top to the baseline.
+    ///
+    /// Unit: points.
+    #[inline(always)]
+    pub fn ascent(&self) -> f32 {
+        self.ascent
+    }
+
     fn allocate_glyph(&self, glyph_id: ab_glyph::GlyphId) -> GlyphInfo {
         assert!(glyph_id.0 != 0);
         use ab_glyph::{Font as _, ScaleFont};
@@ -305,7 +306,6 @@ impl FontImpl {
         GlyphInfo {
             id: glyph_id,
             advance_width: advance_width_in_points,
-            ascent: self.ascent,
             uv_rect,
         }
     }
@@ -442,7 +442,7 @@ impl Font {
     }
 
     #[inline]
-    pub(crate) fn glyph_info_and_font_impl(&mut self, c: char) -> (Option<&FontImpl>, GlyphInfo) {
+    pub(crate) fn font_impl_and_glyph_info(&mut self, c: char) -> (Option<&FontImpl>, GlyphInfo) {
         if self.fonts.is_empty() {
             return (None, self.replacement_glyph.1);
         }
diff --git a/crates/epaint/src/text/text_layout.rs b/crates/epaint/src/text/text_layout.rs
index 4bd101bf06f..860c19f8f5a 100644
--- a/crates/epaint/src/text/text_layout.rs
+++ b/crates/epaint/src/text/text_layout.rs
@@ -134,7 +134,7 @@ fn layout_section(
             paragraph = out_paragraphs.last_mut().unwrap();
             paragraph.empty_paragraph_height = line_height; // TODO(emilk): replace this hack with actually including `\n` in the glyphs?
         } else {
-            let (font_impl, glyph_info) = font.glyph_info_and_font_impl(chr);
+            let (font_impl, glyph_info) = font.font_impl_and_glyph_info(chr);
             if let Some(font_impl) = font_impl {
                 if let Some(last_glyph_id) = last_glyph_id {
                     paragraph.cursor_x += font_impl.pair_kerning(last_glyph_id, glyph_info.id);
@@ -146,7 +146,7 @@ fn layout_section(
                 chr,
                 pos: pos2(paragraph.cursor_x, f32::NAN),
                 size: vec2(glyph_info.advance_width, line_height),
-                ascent: glyph_info.ascent,
+                ascent: font_impl.map_or(0.0, |font| font.ascent()), // Failure to find the font here would be weird
                 uv_rect: glyph_info.uv_rect,
                 section_index,
             });
@@ -340,12 +340,12 @@ fn replace_last_glyph_with_overflow_character(
             .unwrap_or_else(|| font.row_height());
 
         let prev_glyph_id = prev_glyph.map(|prev_glyph| {
-            let (_, prev_glyph_info) = font.glyph_info_and_font_impl(prev_glyph.chr);
+            let (_, prev_glyph_info) = font.font_impl_and_glyph_info(prev_glyph.chr);
             prev_glyph_info.id
         });
 
         // undo kerning with previous glyph
-        let (font_impl, glyph_info) = font.glyph_info_and_font_impl(last_glyph.chr);
+        let (font_impl, glyph_info) = font.font_impl_and_glyph_info(last_glyph.chr);
         last_glyph.pos.x -= extra_letter_spacing
             + font_impl
                 .zip(prev_glyph_id)
@@ -356,10 +356,9 @@ fn replace_last_glyph_with_overflow_character(
 
         // replace the glyph
         last_glyph.chr = overflow_character;
-        let (font_impl, glyph_info) = font.glyph_info_and_font_impl(last_glyph.chr);
+        let (font_impl, glyph_info) = font.font_impl_and_glyph_info(last_glyph.chr);
         last_glyph.size = vec2(glyph_info.advance_width, line_height);
         last_glyph.uv_rect = glyph_info.uv_rect;
-        last_glyph.ascent = glyph_info.ascent;
 
         // reapply kerning
         last_glyph.pos.x += extra_letter_spacing