Make all HighlightStyle properties optional

Previously, some of those properties such the font weight, style and color would be mandatory: when the theme didn't specify them, Zed would use a default value during deserialization. This meant that those default properties would unconditionally override the base text style, causing a rendering bug when combining syntax highlights with diagnostic styles. This commit fixes that by making `HighlightStyle`s more additive: each property can be set independently and only the properties that theme specifies get overridden in the base text style.
2022-03-15 10:39:43 +01:00
parent 72692f1700
commit fbf7cdf4f2
10 changed files with 149 additions and 146 deletions
@@ -214,7 +214,7 @@ mod tests {
            "Menlo",
            12.,
            Default::default(),
-            None,
+            Default::default(),
            Color::black(),
            cx.font_cache(),
        )
@@ -223,7 +223,7 @@ mod tests {
            "Menlo",
            12.,
            *FontProperties::new().weight(Weight::BOLD),
-            None,
+            Default::default(),
            Color::new(255, 0, 0, 255),
            cx.font_cache(),
        )
@@ -1,4 +1,4 @@
-use std::{ops::Range, sync::Arc};
+use std::{borrow::Cow, ops::Range, sync::Arc};

 use crate::{
    color::Color,
@@ -205,8 +205,6 @@ pub fn layout_highlighted_chunks<'a>(
    max_line_count: usize,
 ) -> Vec<Line> {
    let mut layouts = Vec::with_capacity(max_line_count);
-    let mut prev_font_properties = text_style.font_properties.clone();
-    let mut prev_font_id = text_style.font_id;
    let mut line = String::new();
    let mut styles = Vec::new();
    let mut row = 0;
@@ -225,30 +223,14 @@ pub fn layout_highlighted_chunks<'a>(
            }

            if !line_chunk.is_empty() && !line_exceeded_max_len {
-                let font_properties;
-                let mut color;
-                let underline;
-
-                if let Some(highlight_style) = highlight_style {
-                    font_properties = highlight_style.font_properties;
-                    color = Color::blend(highlight_style.color, text_style.color);
-                    if let Some(fade) = highlight_style.fade_out {
-                        color.fade_out(fade);
-                    }
-                    underline = highlight_style.underline;
+                let text_style = if let Some(style) = highlight_style {
+                    text_style
+                        .clone()
+                        .highlight(style, font_cache)
+                        .map(Cow::Owned)
+                        .unwrap_or_else(|_| Cow::Borrowed(text_style))
                } else {
-                    font_properties = text_style.font_properties;
-                    color = text_style.color;
-                    underline = None;
-                }
-
-                // Avoid a lookup if the font properties match the previous ones.
-                let font_id = if font_properties == prev_font_properties {
-                    prev_font_id
-                } else {
-                    font_cache
-                        .select_font(text_style.font_family_id, &font_properties)
-                        .unwrap_or(text_style.font_id)
+                    Cow::Borrowed(text_style)
                };

                if line.len() + line_chunk.len() > max_line_len {
@@ -264,13 +246,11 @@ pub fn layout_highlighted_chunks<'a>(
                styles.push((
                    line_chunk.len(),
                    RunStyle {
-                        font_id,
-                        color,
-                        underline,
+                        font_id: text_style.font_id,
+                        color: text_style.color,
+                        underline: text_style.underline,
                    },
                ));
-                prev_font_id = font_id;
-                prev_font_properties = font_properties;
            }
        }
    }
@@ -28,13 +28,14 @@ pub struct TextStyle {
    pub font_id: FontId,
    pub font_size: f32,
    pub font_properties: Properties,
-    pub underline: Option<Underline>,
+    pub underline: Underline,
 }

 #[derive(Copy, Clone, Debug, Default, PartialEq)]
 pub struct HighlightStyle {
-    pub color: Color,
-    pub font_properties: Properties,
+    pub color: Option<Color>,
+    pub weight: Option<Weight>,
+    pub italic: Option<bool>,
    pub underline: Option<Underline>,
    pub fade_out: Option<f32>,
 }
@@ -43,7 +44,7 @@ impl Eq for HighlightStyle {}

 #[derive(Copy, Clone, Debug, Default, PartialEq, Eq)]
 pub struct Underline {
-    pub color: Color,
+    pub color: Option<Color>,
    pub thickness: OrderedFloat<f32>,
    pub squiggly: bool,
 }
@@ -80,13 +81,10 @@ struct TextStyleJson {

 #[derive(Deserialize)]
 struct HighlightStyleJson {
-    color: Color,
+    color: Option<Color>,
    weight: Option<WeightJson>,
-    #[serde(default)]
-    italic: bool,
-    #[serde(default)]
-    underline: UnderlineStyleJson,
-    #[serde(default)]
+    italic: Option<bool>,
+    underline: Option<UnderlineStyleJson>,
    fade_out: Option<f32>,
 }

@@ -109,7 +107,7 @@ impl TextStyle {
        font_family_name: impl Into<Arc<str>>,
        font_size: f32,
        font_properties: Properties,
-        underline: Option<Underline>,
+        underline: Underline,
        color: Color,
        font_cache: &FontCache,
    ) -> Result<Self> {
@@ -133,14 +131,31 @@ impl TextStyle {
    }

    pub fn highlight(mut self, style: HighlightStyle, font_cache: &FontCache) -> Result<Self> {
-        if self.font_properties != style.font_properties {
-            self.font_id = font_cache.select_font(self.font_family_id, &style.font_properties)?;
+        let mut font_properties = self.font_properties;
+        if let Some(weight) = style.weight {
+            font_properties.weight(weight);
+        }
+        if let Some(italic) = style.italic {
+            if italic {
+                font_properties.style(Style::Italic);
+            } else {
+                font_properties.style(Style::Normal);
+            }
+        }
+
+        if self.font_properties != font_properties {
+            self.font_id = font_cache.select_font(self.font_family_id, &font_properties)?;
+        }
+        if let Some(color) = style.color {
+            self.color = Color::blend(color, self.color);
        }
-        self.color = Color::blend(style.color, self.color);
        if let Some(factor) = style.fade_out {
            self.color.fade_out(factor);
        }
-        self.underline = style.underline;
+        if let Some(underline) = style.underline {
+            self.underline = underline;
+        }
+
        Ok(self)
    }

@@ -160,7 +175,7 @@ impl TextStyle {
                    json.family,
                    json.size,
                    font_properties,
-                    underline_from_json(json.underline, json.color),
+                    underline_from_json(json.underline),
                    json.color,
                    font_cache,
                )
@@ -214,9 +229,10 @@ impl From<TextStyle> for HighlightStyle {
 impl From<&TextStyle> for HighlightStyle {
    fn from(other: &TextStyle) -> Self {
        Self {
-            color: other.color,
-            font_properties: other.font_properties,
-            underline: other.underline,
+            color: Some(other.color),
+            weight: Some(other.font_properties.weight),
+            italic: Some(other.font_properties.style == Style::Italic),
+            underline: Some(other.underline),
            fade_out: None,
        }
    }
@@ -256,17 +272,38 @@ impl Default for TextStyle {

 impl HighlightStyle {
    fn from_json(json: HighlightStyleJson) -> Self {
-        let font_properties = properties_from_json(json.weight, json.italic);
        Self {
            color: json.color,
-            font_properties,
-            underline: underline_from_json(json.underline, json.color),
+            weight: json.weight.map(weight_from_json),
+            italic: json.italic,
+            underline: json.underline.map(underline_from_json),
            fade_out: json.fade_out,
        }
    }

    pub fn highlight(&mut self, other: HighlightStyle) {
-        self.color = Color::blend(other.color, self.color);
+        match (self.color, other.color) {
+            (Some(self_color), Some(other_color)) => {
+                self.color = Some(Color::blend(other_color, self_color));
+            }
+            (None, Some(other_color)) => {
+                self.color = Some(other_color);
+            }
+            _ => {}
+        }
+
+        if other.weight.is_some() {
+            self.weight = other.weight;
+        }
+
+        if other.italic.is_some() {
+            self.italic = other.italic;
+        }
+
+        if other.underline.is_some() {
+            self.underline = other.underline;
+        }
+
        match (other.fade_out, self.fade_out) {
            (Some(source_fade), None) => self.fade_out = Some(source_fade),
            (Some(source_fade), Some(dest_fade)) => {
@@ -278,20 +315,14 @@ impl HighlightStyle {
            }
            _ => {}
        }
-        self.font_properties = other.font_properties;
-        if other.underline.is_some() {
-            self.underline = other.underline;
-        }
    }
 }

 impl From<Color> for HighlightStyle {
    fn from(color: Color) -> Self {
        Self {
-            color,
-            font_properties: Default::default(),
-            underline: None,
-            fade_out: None,
+            color: Some(color),
+            ..Default::default()
        }
    }
 }
@@ -329,36 +360,40 @@ impl<'de> Deserialize<'de> for HighlightStyle {
        } else {
            Ok(Self {
                color: serde_json::from_value(json).map_err(de::Error::custom)?,
-                font_properties: Properties::new(),
-                underline: None,
-                fade_out: None,
+                ..Default::default()
            })
        }
    }
 }

-fn underline_from_json(json: UnderlineStyleJson, text_color: Color) -> Option<Underline> {
+fn underline_from_json(json: UnderlineStyleJson) -> Underline {
    match json {
-        UnderlineStyleJson::Underlined(false) => None,
-        UnderlineStyleJson::Underlined(true) => Some(Underline {
-            color: text_color,
+        UnderlineStyleJson::Underlined(false) => Underline::default(),
+        UnderlineStyleJson::Underlined(true) => Underline {
+            color: None,
            thickness: 1.0.into(),
            squiggly: false,
-        }),
+        },
        UnderlineStyleJson::UnderlinedWithProperties {
            color,
            thickness,
            squiggly,
-        } => Some(Underline {
-            color: color.unwrap_or(text_color),
+        } => Underline {
+            color,
            thickness: thickness.unwrap_or(1.).into(),
            squiggly,
-        }),
+        },
    }
 }

 fn properties_from_json(weight: Option<WeightJson>, italic: bool) -> Properties {
-    let weight = match weight.unwrap_or(WeightJson::normal) {
+    let weight = weight.map(weight_from_json).unwrap_or_default();
+    let style = if italic { Style::Italic } else { Style::Normal };
+    *Properties::new().weight(weight).style(style)
+}
+
+fn weight_from_json(weight: WeightJson) -> Weight {
+    match weight {
        WeightJson::thin => Weight::THIN,
        WeightJson::extra_light => Weight::EXTRA_LIGHT,
        WeightJson::light => Weight::LIGHT,
@@ -368,9 +403,7 @@ fn properties_from_json(weight: Option<WeightJson>, italic: bool) -> Properties
        WeightJson::bold => Weight::BOLD,
        WeightJson::extra_bold => Weight::EXTRA_BOLD,
        WeightJson::black => Weight::BLACK,
-    };
-    let style = if italic { Style::Italic } else { Style::Normal };
-    *Properties::new().weight(weight).style(style)
+    }
 }

 impl ToJson for Properties {
@@ -425,21 +425,21 @@ mod tests {
        let menlo_regular = RunStyle {
            font_id: fonts.select_font(&menlo, &Properties::new()).unwrap(),
            color: Default::default(),
-            underline: None,
+            underline: Default::default(),
        };
        let menlo_italic = RunStyle {
            font_id: fonts
                .select_font(&menlo, &Properties::new().style(Style::Italic))
                .unwrap(),
            color: Default::default(),
-            underline: None,
+            underline: Default::default(),
        };
        let menlo_bold = RunStyle {
            font_id: fonts
                .select_font(&menlo, &Properties::new().weight(Weight::BOLD))
                .unwrap(),
            color: Default::default(),
-            underline: None,
+            underline: Default::default(),
        };
        assert_ne!(menlo_regular, menlo_italic);
        assert_ne!(menlo_regular, menlo_bold);
@@ -466,13 +466,13 @@ mod tests {
        let zapfino_regular = RunStyle {
            font_id: fonts.select_font(&zapfino, &Properties::new())?,
            color: Default::default(),
-            underline: None,
+            underline: Default::default(),
        };
        let menlo = fonts.load_family("Menlo")?;
        let menlo_regular = RunStyle {
            font_id: fonts.select_font(&menlo, &Properties::new())?,
            color: Default::default(),
-            underline: None,
+            underline: Default::default(),
        };

        let text = "This is, m𐍈re 𐍈r less, Zapfino!𐍈";
@@ -551,7 +551,7 @@ mod tests {
        let style = RunStyle {
            font_id: fonts.select_font(&font_ids, &Default::default()).unwrap(),
            color: Default::default(),
-            underline: None,
+            underline: Default::default(),
        };

        let line = "\u{feff}";
@@ -28,7 +28,7 @@ pub struct TextLayoutCache {
 pub struct RunStyle {
    pub color: Color,
    pub font_id: FontId,
-    pub underline: Option<Underline>,
+    pub underline: Underline,
 }

 impl TextLayoutCache {
@@ -167,7 +167,7 @@ impl<'a> Hash for CacheKeyRef<'a> {
 #[derive(Default, Debug)]
 pub struct Line {
    layout: Arc<LineLayout>,
-    style_runs: SmallVec<[(u32, Color, Option<Underline>); 32]>,
+    style_runs: SmallVec<[(u32, Color, Underline); 32]>,
 }

 #[derive(Default, Debug)]
@@ -283,17 +283,21 @@ impl Line {
                if glyph.index >= run_end {
                    if let Some((run_len, run_color, run_underline)) = style_runs.next() {
                        if let Some((_, underline_style)) = underline {
-                            if *run_underline != Some(underline_style) {
+                            if *run_underline != underline_style {
                                finished_underline = underline.take();
                            }
                        }
-                        if let Some(run_underline) = run_underline {
+                        if run_underline.thickness.into_inner() > 0. {
                            underline.get_or_insert((
                                vec2f(
                                    glyph_origin.x(),
                                    origin.y() + baseline_offset.y() + 0.618 * self.layout.descent,
                                ),
-                                *run_underline,
+                                Underline {
+                                    color: Some(run_underline.color.unwrap_or(*run_color)),
+                                    thickness: run_underline.thickness.into(),
+                                    squiggly: run_underline.squiggly,
+                                },
                            ));
                        }

@@ -301,7 +305,6 @@ impl Line {
                        color = *run_color;
                    } else {
                        run_end = self.layout.len;
-                        color = Color::black();
                        finished_underline = underline.take();
                    }
                }
@@ -315,7 +318,7 @@ impl Line {
                        origin: underline_origin,
                        width: glyph_origin.x() - underline_origin.x(),
                        thickness: underline_style.thickness.into(),
-                        color: underline_style.color,
+                        color: underline_style.color.unwrap(),
                        squiggly: underline_style.squiggly,
                    });
                }
@@ -335,7 +338,7 @@ impl Line {
            cx.scene.push_underline(scene::Underline {
                origin: underline_start,
                width: line_end_x - underline_start.x(),
-                color: underline_style.color,
+                color: underline_style.color.unwrap(),
                thickness: underline_style.thickness.into(),
                squiggly: underline_style.squiggly,
            });
@@ -610,7 +613,7 @@ impl LineWrapper {
                    RunStyle {
                        font_id: self.font_id,
                        color: Default::default(),
-                        underline: None,
+                        underline: Default::default(),
                    },
                )],
            )
@@ -694,7 +697,7 @@ mod tests {
        let normal = RunStyle {
            font_id,
            color: Default::default(),
-            underline: None,
+            underline: Default::default(),
        };
        let bold = RunStyle {
            font_id: font_cache
@@ -707,7 +710,7 @@ mod tests {
                )
                .unwrap(),
            color: Default::default(),
-            underline: None,
+            underline: Default::default(),
        };

        let text = "aa bbb cccc ddddd eeee";