gpui: Make TextSystem::line_wrapper non-fallible. (#4022)

Piotr Osiewicz , Conrad , and Conrad Irwin created 2 years ago

Editors WrapMap could become desynchronised if user had an invalid font
specified in their config. Compared to Zed1, WrapMap ignored the
resolution failure instead of panicking. Now, if there's an invalid font
in the user config, we just fall back to an arbitrary default.


Release Notes:
- Fixed the editor panic in presence of invalid font name in the config
(fixes https://github.com/zed-industries/community/issues/2397)

---------

Co-authored-by: Conrad <[e-mail]>
Co-authored-by: Conrad Irwin <[e-mail]>

Change summary

crates/editor/src/display_map/wrap_map.rs | 52 +++++++++++-------------
crates/gpui/src/text_system.rs            | 22 +++-------
2 files changed, 30 insertions(+), 44 deletions(-)

Detailed changes

crates/editor/src/display_map/wrap_map.rs

@@ -11,7 +11,6 @@ use smol::future::yield_now;
 use std::{cmp, collections::VecDeque, mem, ops::Range, time::Duration};
 use sum_tree::{Bias, Cursor, SumTree};
 use text::Patch;
-use util::ResultExt;
 
 pub use super::tab_map::TextSummary;
 pub type WrapEdit = text::Edit<u32>;
@@ -154,26 +153,24 @@ impl WrapMap {
 
         if let Some(wrap_width) = self.wrap_width {
             let mut new_snapshot = self.snapshot.clone();
-            let mut edits = Patch::default();
+
             let text_system = cx.text_system().clone();
             let (font, font_size) = self.font_with_size.clone();
             let task = cx.background_executor().spawn(async move {
-                if let Some(mut line_wrapper) = text_system.line_wrapper(font, font_size).log_err()
-                {
-                    let tab_snapshot = new_snapshot.tab_snapshot.clone();
-                    let range = TabPoint::zero()..tab_snapshot.max_point();
-                    edits = new_snapshot
-                        .update(
-                            tab_snapshot,
-                            &[TabEdit {
-                                old: range.clone(),
-                                new: range.clone(),
-                            }],
-                            wrap_width,
-                            &mut line_wrapper,
-                        )
-                        .await;
-                }
+                let mut line_wrapper = text_system.line_wrapper(font, font_size);
+                let tab_snapshot = new_snapshot.tab_snapshot.clone();
+                let range = TabPoint::zero()..tab_snapshot.max_point();
+                let edits = new_snapshot
+                    .update(
+                        tab_snapshot,
+                        &[TabEdit {
+                            old: range.clone(),
+                            new: range.clone(),
+                        }],
+                        wrap_width,
+                        &mut line_wrapper,
+                    )
+                    .await;
                 (new_snapshot, edits)
             });
 
@@ -245,15 +242,12 @@ impl WrapMap {
                 let (font, font_size) = self.font_with_size.clone();
                 let update_task = cx.background_executor().spawn(async move {
                     let mut edits = Patch::default();
-                    if let Some(mut line_wrapper) =
-                        text_system.line_wrapper(font, font_size).log_err()
-                    {
-                        for (tab_snapshot, tab_edits) in pending_edits {
-                            let wrap_edits = snapshot
-                                .update(tab_snapshot, &tab_edits, wrap_width, &mut line_wrapper)
-                                .await;
-                            edits = edits.compose(&wrap_edits);
-                        }
+                    let mut line_wrapper = text_system.line_wrapper(font, font_size);
+                    for (tab_snapshot, tab_edits) in pending_edits {
+                        let wrap_edits = snapshot
+                            .update(tab_snapshot, &tab_edits, wrap_width, &mut line_wrapper)
+                            .await;
+                        edits = edits.compose(&wrap_edits);
                     }
                     (snapshot, edits)
                 });
@@ -1059,7 +1053,7 @@ mod tests {
         };
         let tab_size = NonZeroU32::new(rng.gen_range(1..=4)).unwrap();
         let font = font("Helvetica");
-        let _font_id = text_system.font_id(&font).unwrap();
+        let _font_id = text_system.font_id(&font);
         let font_size = px(14.0);
 
         log::info!("Tab size: {}", tab_size);
@@ -1086,7 +1080,7 @@ mod tests {
         let tabs_snapshot = tab_map.set_max_expansion_column(32);
         log::info!("TabMap text: {:?}", tabs_snapshot.text());
 
-        let mut line_wrapper = text_system.line_wrapper(font.clone(), font_size).unwrap();
+        let mut line_wrapper = text_system.line_wrapper(font.clone(), font_size);
         let unwrapped_text = tabs_snapshot.text();
         let expected_text = wrap_text(&unwrapped_text, wrap_width, &mut line_wrapper);
 

crates/gpui/src/text_system.rs

@@ -364,28 +364,20 @@ impl TextSystem {
         self.line_layout_cache.start_frame()
     }
 
-    pub fn line_wrapper(
-        self: &Arc<Self>,
-        font: Font,
-        font_size: Pixels,
-    ) -> Result<LineWrapperHandle> {
+    pub fn line_wrapper(self: &Arc<Self>, font: Font, font_size: Pixels) -> LineWrapperHandle {
         let lock = &mut self.wrapper_pool.lock();
-        let font_id = self.font_id(&font)?;
+        let font_id = self.resolve_font(&font);
         let wrappers = lock
             .entry(FontIdWithSize { font_id, font_size })
             .or_default();
-        let wrapper = wrappers.pop().map(anyhow::Ok).unwrap_or_else(|| {
-            Ok(LineWrapper::new(
-                font_id,
-                font_size,
-                self.platform_text_system.clone(),
-            ))
-        })?;
+        let wrapper = wrappers.pop().unwrap_or_else(|| {
+            LineWrapper::new(font_id, font_size, self.platform_text_system.clone())
+        });
 
-        Ok(LineWrapperHandle {
+        LineWrapperHandle {
             wrapper: Some(wrapper),
             text_system: self.clone(),
-        })
+        }
     }
 
     pub fn raster_bounds(&self, params: &RenderGlyphParams) -> Result<Bounds<DevicePixels>> {