Simplify and document parts of linux text system code (#9443)

Niklas Wimmer created

I mainly focused on improving the `font_id` function, see the
description of e286483262e525c64c29d3747838ea9d2bac7bad for more
details. The rest are some drive-by changes I could not resist to.

When I am right about af4d6c43ce87ff5f16ad39c223e13326550010dd, someone
with a Mac could change it there as well.

This PR is probably best reviewed commit by commit :)

cc @gabydd @h3mosphere

Release Notes:

- N/A

---------

Signed-off-by: Niklas Wimmer <mail@nwimmer.me>

Change summary

crates/gpui/src/platform/linux/text_system.rs | 179 ++++++++++++--------
1 file changed, 104 insertions(+), 75 deletions(-)

Detailed changes

crates/gpui/src/platform/linux/text_system.rs 🔗

@@ -6,12 +6,11 @@ use crate::{
 use anyhow::{anyhow, Context, Ok, Result};
 use collections::HashMap;
 use cosmic_text::{
-    fontdb::Query, Attrs, AttrsList, BufferLine, CacheKey, Family, Font as CosmicTextFont,
-    FontSystem, SwashCache,
+    Attrs, AttrsList, BufferLine, CacheKey, Family, Font as CosmicTextFont, FontSystem, SwashCache,
 };
 
 use itertools::Itertools;
-use parking_lot::{RwLock, RwLockUpgradableReadGuard};
+use parking_lot::RwLock;
 use pathfinder_geometry::{
     rect::{RectF, RectI},
     vector::{Vector2F, Vector2I},
@@ -24,10 +23,17 @@ pub(crate) struct LinuxTextSystem(RwLock<LinuxTextSystemState>);
 struct LinuxTextSystemState {
     swash_cache: SwashCache,
     font_system: FontSystem,
-    fonts: Vec<Arc<CosmicTextFont>>,
-    font_selections: HashMap<Font, FontId>,
-    font_ids_by_family_name: HashMap<SharedString, SmallVec<[FontId; 4]>>,
-    postscript_names_by_font_id: HashMap<FontId, String>,
+    /// Contains all already loaded fonts. Indexed by `FontId`.
+    ///
+    /// When a font is requested via `LinuxTextSystem::font_id` all faces of the requested family
+    /// are loaded at once via `Self::load_family`. Not every item in this list is therefore actually
+    /// being used to render text on the screen.
+    // A reference to these fonts is also stored in `font_system`, but calling `FontSystem::get_font`
+    // would require a mutable reference and therefore a write lock
+    loaded_fonts_store: Vec<Arc<CosmicTextFont>>,
+    /// Caches the `FontId`s associated with a specific family to avoid iterating the font database
+    /// for every font face in a family.
+    font_ids_by_family_cache: HashMap<SharedString, SmallVec<[FontId; 4]>>,
 }
 
 impl LinuxTextSystem {
@@ -40,11 +46,8 @@ impl LinuxTextSystem {
         Self(RwLock::new(LinuxTextSystemState {
             font_system,
             swash_cache: SwashCache::new(),
-            fonts: Vec::new(),
-            font_selections: HashMap::default(),
-            // font_ids_by_postscript_name: HashMap::default(),
-            font_ids_by_family_name: HashMap::default(),
-            postscript_names_by_font_id: HashMap::default(),
+            loaded_fonts_store: Vec::new(),
+            font_ids_by_family_cache: HashMap::default(),
         }))
     }
 }
@@ -55,7 +58,6 @@ impl Default for LinuxTextSystem {
     }
 }
 
-#[allow(unused)]
 impl PlatformTextSystem for LinuxTextSystem {
     fn add_fonts(&self, fonts: Vec<Cow<'static, [u8]>>) -> Result<()> {
         self.0.write().add_fonts(fonts)
@@ -79,57 +81,46 @@ impl PlatformTextSystem for LinuxTextSystem {
             .font_system
             .db()
             .faces()
-            .filter_map(|face| Some(face.families.get(0)?.0.clone()))
+            // todo(linux) this will list the same font family multiple times
+            .filter_map(|face| face.families.first().map(|family| family.0.clone()))
             .collect_vec()
     }
 
     fn font_id(&self, font: &Font) -> Result<FontId> {
         // todo(linux): Do we need to use CosmicText's Font APIs? Can we consolidate this to use font_kit?
-        let lock = self.0.upgradable_read();
-        if let Some(font_id) = lock.font_selections.get(font) {
-            Ok(*font_id)
+        let mut state = self.0.write();
+
+        let candidates = if let Some(font_ids) = state.font_ids_by_family_cache.get(&font.family) {
+            font_ids.as_slice()
         } else {
-            let mut lock = RwLockUpgradableReadGuard::upgrade(lock);
-            let candidates = if let Some(font_ids) = lock.font_ids_by_family_name.get(&font.family)
-            {
-                font_ids.as_slice()
-            } else {
-                let font_ids = lock.load_family(&font.family, font.features)?;
-                lock.font_ids_by_family_name
-                    .insert(font.family.clone(), font_ids);
-                lock.font_ids_by_family_name[&font.family].as_ref()
-            };
+            let font_ids = state.load_family(&font.family, font.features)?;
+            state
+                .font_ids_by_family_cache
+                .insert(font.family.clone(), font_ids);
+            state.font_ids_by_family_cache[&font.family].as_ref()
+        };
+
+        // todo(linux) ideally we would make fontdb's `find_best_match` pub instead of using font-kit here
+        let candidate_properties = candidates
+            .iter()
+            .map(|font_id| {
+                let database_id = state.loaded_fonts_store[font_id.0].id();
+                let face_info = state.font_system.db().face(database_id).expect("");
+                face_info_into_properties(face_info)
+            })
+            .collect::<SmallVec<[_; 4]>>();
 
-            let id = lock
-                .font_system
-                .db()
-                .query(&Query {
-                    families: &[Family::Name(&font.family)],
-                    weight: font.weight.into(),
-                    style: font.style.into(),
-                    stretch: Default::default(),
-                })
-                .context("no font")?;
-
-            let font_id = if let Some(font_id) = lock.fonts.iter().position(|font| font.id() == id)
-            {
-                FontId(font_id)
-            } else {
-                // Font isn't in fonts so add it there, this is because we query all the fonts in the db
-                // and maybe we haven't loaded it yet
-                let font_id = FontId(lock.fonts.len());
-                let font = lock.font_system.get_font(id).unwrap();
-                lock.fonts.push(font);
-                font_id
-            };
+        let ix =
+            font_kit::matching::find_best_match(&candidate_properties, &font_into_properties(font))
+                .context("requested font family contains no font matching the other parameters")?;
 
-            lock.font_selections.insert(font.clone(), font_id);
-            Ok(font_id)
-        }
+        Ok(candidates[ix])
     }
 
     fn font_metrics(&self, font_id: FontId) -> FontMetrics {
-        let metrics = self.0.read().fonts[font_id.0].as_swash().metrics(&[]);
+        let metrics = self.0.read().loaded_fonts_store[font_id.0]
+            .as_swash()
+            .metrics(&[]);
 
         FontMetrics {
             units_per_em: metrics.units_per_em as u32,
@@ -150,8 +141,9 @@ impl PlatformTextSystem for LinuxTextSystem {
 
     fn typographic_bounds(&self, font_id: FontId, glyph_id: GlyphId) -> Result<Bounds<f32>> {
         let lock = self.0.read();
-        let metrics = lock.fonts[font_id.0].as_swash().metrics(&[]);
-        let glyph_metrics = lock.fonts[font_id.0].as_swash().glyph_metrics(&[]);
+        let glyph_metrics = lock.loaded_fonts_store[font_id.0]
+            .as_swash()
+            .glyph_metrics(&[]);
         let glyph_id = glyph_id.0 as u16;
         // todo(linux): Compute this correctly
         // see https://github.com/servo/font-kit/blob/master/src/loaders/freetype.rs#L614-L620
@@ -191,10 +183,10 @@ impl PlatformTextSystem for LinuxTextSystem {
     // todo(linux) Confirm that this has been superseded by the LineWrapper
     fn wrap_line(
         &self,
-        text: &str,
-        font_id: FontId,
-        font_size: Pixels,
-        width: Pixels,
+        _text: &str,
+        _font_id: FontId,
+        _font_size: Pixels,
+        _width: Pixels,
     ) -> Vec<usize> {
         unimplemented!()
     }
@@ -217,6 +209,7 @@ impl LinuxTextSystemState {
         Ok(())
     }
 
+    // todo(linux) handle `FontFeatures`
     #[profiling::function]
     fn load_family(
         &mut self,
@@ -234,19 +227,19 @@ impl LinuxTextSystemState {
                 continue;
             };
 
-            let font_id = FontId(self.fonts.len());
+            let font_id = FontId(self.loaded_fonts_store.len());
             font_ids.push(font_id);
-            self.fonts.push(font);
+            self.loaded_fonts_store.push(font);
         }
         Ok(font_ids)
     }
 
     fn advance(&self, font_id: FontId, glyph_id: GlyphId) -> Result<Size<f32>> {
-        let width = self.fonts[font_id.0]
+        let width = self.loaded_fonts_store[font_id.0]
             .as_swash()
             .glyph_metrics(&[])
             .advance_width(glyph_id.0 as u16);
-        let height = self.fonts[font_id.0]
+        let height = self.loaded_fonts_store[font_id.0]
             .as_swash()
             .glyph_metrics(&[])
             .advance_height(glyph_id.0 as u16);
@@ -254,7 +247,10 @@ impl LinuxTextSystemState {
     }
 
     fn glyph_for_char(&self, font_id: FontId, ch: char) -> Option<GlyphId> {
-        let glyph_id = self.fonts[font_id.0].as_swash().charmap().map(ch);
+        let glyph_id = self.loaded_fonts_store[font_id.0]
+            .as_swash()
+            .charmap()
+            .map(ch);
         if glyph_id == 0 {
             None
         } else {
@@ -262,18 +258,14 @@ impl LinuxTextSystemState {
         }
     }
 
-    fn is_emoji(&self, font_id: FontId) -> bool {
-        // todo(linux): implement this correctly
-        self.postscript_names_by_font_id
-            .get(&font_id)
-            .map_or(false, |postscript_name| {
-                postscript_name == "AppleColorEmoji"
-            })
+    // todo(linux)
+    fn is_emoji(&self, _font_id: FontId) -> bool {
+        false
     }
 
     // todo(linux) both raster functions have problems because I am not sure this is the correct mapping from cosmic text to gpui system
     fn raster_bounds(&mut self, params: &RenderGlyphParams) -> Result<Bounds<DevicePixels>> {
-        let font = &self.fonts[params.font_id.0];
+        let font = &self.loaded_fonts_store[params.font_id.0];
         let font_system = &mut self.font_system;
         let image = self
             .swash_cache
@@ -306,7 +298,7 @@ impl LinuxTextSystemState {
         } else {
             // todo(linux) handle subpixel variants
             let bitmap_size = glyph_bounds.size;
-            let font = &self.fonts[params.font_id.0];
+            let font = &self.loaded_fonts_store[params.font_id.0];
             let font_system = &mut self.font_system;
             let image = self
                 .swash_cache
@@ -334,7 +326,7 @@ impl LinuxTextSystemState {
         let mut offs = 0;
         for run in font_runs {
             // todo(linux) We need to check we are doing utf properly
-            let font = &self.fonts[run.font_id.0];
+            let font = &self.loaded_fonts_store[run.font_id.0];
             let font = self.font_system.db().face(font.id()).unwrap();
             attrs_list.add_span(
                 offs..offs + run.len,
@@ -359,7 +351,7 @@ impl LinuxTextSystemState {
         for glyph in &layout.glyphs {
             let font_id = glyph.font_id;
             let font_id = FontId(
-                self.fonts
+                self.loaded_fonts_store
                     .iter()
                     .position(|font| font.id() == font_id)
                     .unwrap(),
@@ -445,3 +437,40 @@ impl From<FontStyle> for cosmic_text::Style {
         }
     }
 }
+
+fn font_into_properties(font: &crate::Font) -> font_kit::properties::Properties {
+    font_kit::properties::Properties {
+        style: match font.style {
+            crate::FontStyle::Normal => font_kit::properties::Style::Normal,
+            crate::FontStyle::Italic => font_kit::properties::Style::Italic,
+            crate::FontStyle::Oblique => font_kit::properties::Style::Oblique,
+        },
+        weight: font_kit::properties::Weight(font.weight.0),
+        stretch: Default::default(),
+    }
+}
+
+fn face_info_into_properties(
+    face_info: &cosmic_text::fontdb::FaceInfo,
+) -> font_kit::properties::Properties {
+    font_kit::properties::Properties {
+        style: match face_info.style {
+            cosmic_text::Style::Normal => font_kit::properties::Style::Normal,
+            cosmic_text::Style::Italic => font_kit::properties::Style::Italic,
+            cosmic_text::Style::Oblique => font_kit::properties::Style::Oblique,
+        },
+        // both libs use the same values for weight
+        weight: font_kit::properties::Weight(face_info.weight.0.into()),
+        stretch: match face_info.stretch {
+            cosmic_text::Stretch::Condensed => font_kit::properties::Stretch::CONDENSED,
+            cosmic_text::Stretch::Expanded => font_kit::properties::Stretch::EXPANDED,
+            cosmic_text::Stretch::ExtraCondensed => font_kit::properties::Stretch::EXTRA_CONDENSED,
+            cosmic_text::Stretch::ExtraExpanded => font_kit::properties::Stretch::EXTRA_EXPANDED,
+            cosmic_text::Stretch::Normal => font_kit::properties::Stretch::NORMAL,
+            cosmic_text::Stretch::SemiCondensed => font_kit::properties::Stretch::SEMI_CONDENSED,
+            cosmic_text::Stretch::SemiExpanded => font_kit::properties::Stretch::SEMI_EXPANDED,
+            cosmic_text::Stretch::UltraCondensed => font_kit::properties::Stretch::ULTRA_CONDENSED,
+            cosmic_text::Stretch::UltraExpanded => font_kit::properties::Stretch::ULTRA_EXPANDED,
+        },
+    }
+}