From fbf7cdf4f2b6845f8ef20963e9874026f86ca249 Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Tue, 15 Mar 2022 10:39:43 +0100 Subject: [PATCH] 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. --- crates/editor/src/display_map.rs | 2 +- crates/editor/src/editor.rs | 63 ++++----- crates/editor/src/element.rs | 12 +- crates/gpui/examples/text.rs | 4 +- crates/gpui/src/elements/label.rs | 4 +- crates/gpui/src/elements/text.rs | 42 ++---- crates/gpui/src/fonts.rs | 125 +++++++++++------- crates/gpui/src/platform/mac/fonts.rs | 12 +- crates/gpui/src/text_layout.rs | 25 ++-- crates/project_symbols/src/project_symbols.rs | 6 +- 10 files changed, 149 insertions(+), 146 deletions(-) diff --git a/crates/editor/src/display_map.rs b/crates/editor/src/display_map.rs index 5e18ad0b64..dbadbb386e 100644 --- a/crates/editor/src/display_map.rs +++ b/crates/editor/src/display_map.rs @@ -1174,7 +1174,7 @@ mod tests { for chunk in snapshot.chunks(rows, true) { let color = chunk .syntax_highlight_id - .and_then(|id| id.style(theme).map(|s| s.color)); + .and_then(|id| id.style(theme)?.color); if let Some((last_chunk, last_color)) = chunks.last_mut() { if color == *last_color { last_chunk.push_str(chunk.text); diff --git a/crates/editor/src/editor.rs b/crates/editor/src/editor.rs index 59f344b543..eab6e2c1dd 100644 --- a/crates/editor/src/editor.rs +++ b/crates/editor/src/editor.rs @@ -615,11 +615,7 @@ impl CompletionsMenu { .with_highlights(combine_syntax_and_fuzzy_match_highlights( &completion.label.text, style.text.color.into(), - styled_runs_for_code_label( - &completion.label, - style.text.color, - &style.syntax, - ), + styled_runs_for_code_label(&completion.label, &style.syntax), &mat.positions, )) .contained() @@ -5716,7 +5712,7 @@ fn build_style( font_id, font_size, font_properties, - underline: None, + underline: Default::default(), }, placeholder_text: None, theme, @@ -5938,7 +5934,7 @@ pub fn combine_syntax_and_fuzzy_match_highlights( for (range, mut syntax_highlight) in syntax_ranges.chain([(usize::MAX..0, Default::default())]) { - syntax_highlight.font_properties.weight(Default::default()); + syntax_highlight.weight = None; // Add highlights for any fuzzy match characters before the next // syntax highlight range. @@ -5949,7 +5945,7 @@ pub fn combine_syntax_and_fuzzy_match_highlights( match_indices.next(); let end_index = char_ix_after(match_index, text); let mut match_style = default_style; - match_style.font_properties.weight(fonts::Weight::BOLD); + match_style.weight = Some(fonts::Weight::BOLD); result.push((match_index..end_index, match_style)); } @@ -5981,7 +5977,7 @@ pub fn combine_syntax_and_fuzzy_match_highlights( } let mut match_style = syntax_highlight; - match_style.font_properties.weight(fonts::Weight::BOLD); + match_style.weight = Some(fonts::Weight::BOLD); result.push((match_index..end_index, match_style)); offset = end_index; } @@ -6000,16 +5996,12 @@ pub fn combine_syntax_and_fuzzy_match_highlights( pub fn styled_runs_for_code_label<'a>( label: &'a CodeLabel, - default_color: Color, syntax_theme: &'a theme::SyntaxTheme, ) -> impl 'a + Iterator, HighlightStyle)> { - const MUTED_OPACITY: usize = 165; - - let mut muted_default_style = HighlightStyle { - color: default_color, + let fade_out = HighlightStyle { + fade_out: Some(0.35), ..Default::default() }; - muted_default_style.color.a = ((default_color.a as usize * MUTED_OPACITY) / 255) as u8; let mut prev_end = label.filter_range.end; label @@ -6023,12 +6015,12 @@ pub fn styled_runs_for_code_label<'a>( return Default::default(); }; let mut muted_style = style.clone(); - muted_style.color.a = ((style.color.a as usize * MUTED_OPACITY) / 255) as u8; + muted_style.highlight(fade_out); let mut runs = SmallVec::<[(Range, HighlightStyle); 3]>::new(); if range.start >= label.filter_range.end { if range.start > prev_end { - runs.push((prev_end..range.start, muted_default_style)); + runs.push((prev_end..range.start, fade_out)); } runs.push((range.clone(), muted_style)); } else if range.end <= label.filter_range.end { @@ -6040,7 +6032,7 @@ pub fn styled_runs_for_code_label<'a>( prev_end = cmp::max(prev_end, range.end); if ix + 1 == label.runs.len() && label.text.len() > prev_end { - runs.push((prev_end..label.text.len(), muted_default_style)); + runs.push((prev_end..label.text.len(), fade_out)); } runs @@ -8995,20 +8987,19 @@ mod tests { #[test] fn test_combine_syntax_and_fuzzy_match_highlights() { let string = "abcdefghijklmnop"; - let default = HighlightStyle::default(); let syntax_ranges = [ ( 0..3, HighlightStyle { - color: Color::red(), - ..default + color: Some(Color::red()), + ..Default::default() }, ), ( 4..8, HighlightStyle { - color: Color::green(), - ..default + color: Some(Color::green()), + ..Default::default() }, ), ]; @@ -9016,7 +9007,7 @@ mod tests { assert_eq!( combine_syntax_and_fuzzy_match_highlights( &string, - default, + Default::default(), syntax_ranges.into_iter(), &match_indices, ), @@ -9024,38 +9015,38 @@ mod tests { ( 0..3, HighlightStyle { - color: Color::red(), - ..default + color: Some(Color::red()), + ..Default::default() }, ), ( 4..5, HighlightStyle { - color: Color::green(), - font_properties: *fonts::Properties::default().weight(fonts::Weight::BOLD), - ..default + color: Some(Color::green()), + weight: Some(fonts::Weight::BOLD), + ..Default::default() }, ), ( 5..6, HighlightStyle { - color: Color::green(), - ..default + color: Some(Color::green()), + ..Default::default() }, ), ( 6..8, HighlightStyle { - color: Color::green(), - font_properties: *fonts::Properties::default().weight(fonts::Weight::BOLD), - ..default + color: Some(Color::green()), + weight: Some(fonts::Weight::BOLD), + ..Default::default() }, ), ( 8..9, HighlightStyle { - font_properties: *fonts::Properties::default().weight(fonts::Weight::BOLD), - ..default + weight: Some(fonts::Weight::BOLD), + ..Default::default() }, ), ] diff --git a/crates/editor/src/element.rs b/crates/editor/src/element.rs index 582619199a..6b1a6a0326 100644 --- a/crates/editor/src/element.rs +++ b/crates/editor/src/element.rs @@ -397,7 +397,7 @@ impl EditorElement { RunStyle { font_id, color: style.background, - underline: None, + underline: Default::default(), }, )], )) @@ -555,7 +555,7 @@ impl EditorElement { RunStyle { font_id: style.text.font_id, color: Color::black(), - underline: None, + underline: Default::default(), }, )], ) @@ -596,7 +596,7 @@ impl EditorElement { RunStyle { font_id: style.text.font_id, color, - underline: None, + underline: Default::default(), }, )], ))); @@ -644,7 +644,7 @@ impl EditorElement { RunStyle { font_id: placeholder_style.font_id, color: placeholder_style.color, - underline: None, + underline: Default::default(), }, )], ) @@ -669,7 +669,7 @@ impl EditorElement { let diagnostic_style = super::diagnostic_style(severity, true, style); let diagnostic_highlight = HighlightStyle { underline: Some(Underline { - color: diagnostic_style.message.text.color, + color: Some(diagnostic_style.message.text.color), thickness: 1.0.into(), squiggly: true, }), @@ -1237,7 +1237,7 @@ fn layout_line( RunStyle { font_id: style.text.font_id, color: Color::black(), - underline: None, + underline: Default::default(), }, )], ) diff --git a/crates/gpui/examples/text.rs b/crates/gpui/examples/text.rs index fb4772d11c..cc849a5e62 100644 --- a/crates/gpui/examples/text.rs +++ b/crates/gpui/examples/text.rs @@ -62,7 +62,7 @@ impl gpui::Element for TextElement { .select_font(family, &Default::default()) .unwrap(), color: Color::default(), - underline: None, + underline: Default::default(), }; let bold = RunStyle { font_id: cx @@ -76,7 +76,7 @@ impl gpui::Element for TextElement { ) .unwrap(), color: Color::default(), - underline: None, + underline: Default::default(), }; let text = "Hello world!"; diff --git a/crates/gpui/src/elements/label.rs b/crates/gpui/src/elements/label.rs index 20180c380d..e13dea3725 100644 --- a/crates/gpui/src/elements/label.rs +++ b/crates/gpui/src/elements/label.rs @@ -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(), ) diff --git a/crates/gpui/src/elements/text.rs b/crates/gpui/src/elements/text.rs index 707bad55e6..1c5fa7046a 100644 --- a/crates/gpui/src/elements/text.rs +++ b/crates/gpui/src/elements/text.rs @@ -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 { 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; } } } diff --git a/crates/gpui/src/fonts.rs b/crates/gpui/src/fonts.rs index c0995b6808..b0cea3d75b 100644 --- a/crates/gpui/src/fonts.rs +++ b/crates/gpui/src/fonts.rs @@ -28,13 +28,14 @@ pub struct TextStyle { pub font_id: FontId, pub font_size: f32, pub font_properties: Properties, - pub underline: Option, + pub underline: Underline, } #[derive(Copy, Clone, Debug, Default, PartialEq)] pub struct HighlightStyle { - pub color: Color, - pub font_properties: Properties, + pub color: Option, + pub weight: Option, + pub italic: Option, pub underline: Option, pub fade_out: Option, } @@ -43,7 +44,7 @@ impl Eq for HighlightStyle {} #[derive(Copy, Clone, Debug, Default, PartialEq, Eq)] pub struct Underline { - pub color: Color, + pub color: Option, pub thickness: OrderedFloat, pub squiggly: bool, } @@ -80,13 +81,10 @@ struct TextStyleJson { #[derive(Deserialize)] struct HighlightStyleJson { - color: Color, + color: Option, weight: Option, - #[serde(default)] - italic: bool, - #[serde(default)] - underline: UnderlineStyleJson, - #[serde(default)] + italic: Option, + underline: Option, fade_out: Option, } @@ -109,7 +107,7 @@ impl TextStyle { font_family_name: impl Into>, font_size: f32, font_properties: Properties, - underline: Option, + underline: Underline, color: Color, font_cache: &FontCache, ) -> Result { @@ -133,14 +131,31 @@ impl TextStyle { } pub fn highlight(mut self, style: HighlightStyle, font_cache: &FontCache) -> Result { - 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 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 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 { +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, 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, 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 { diff --git a/crates/gpui/src/platform/mac/fonts.rs b/crates/gpui/src/platform/mac/fonts.rs index 2453660abc..1706134f67 100644 --- a/crates/gpui/src/platform/mac/fonts.rs +++ b/crates/gpui/src/platform/mac/fonts.rs @@ -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}"; diff --git a/crates/gpui/src/text_layout.rs b/crates/gpui/src/text_layout.rs index 25f1cb82c0..a866ea0a85 100644 --- a/crates/gpui/src/text_layout.rs +++ b/crates/gpui/src/text_layout.rs @@ -28,7 +28,7 @@ pub struct TextLayoutCache { pub struct RunStyle { pub color: Color, pub font_id: FontId, - pub underline: Option, + pub underline: Underline, } impl TextLayoutCache { @@ -167,7 +167,7 @@ impl<'a> Hash for CacheKeyRef<'a> { #[derive(Default, Debug)] pub struct Line { layout: Arc, - style_runs: SmallVec<[(u32, Color, Option); 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"; diff --git a/crates/project_symbols/src/project_symbols.rs b/crates/project_symbols/src/project_symbols.rs index dd253d886e..bfd204671f 100644 --- a/crates/project_symbols/src/project_symbols.rs +++ b/crates/project_symbols/src/project_symbols.rs @@ -284,11 +284,7 @@ impl ProjectSymbolsView { &settings.theme.selector.item }; let symbol = &self.symbols[string_match.candidate_id]; - let syntax_runs = styled_runs_for_code_label( - &symbol.label, - style.label.text.color, - &settings.theme.editor.syntax, - ); + let syntax_runs = styled_runs_for_code_label(&symbol.label, &settings.theme.editor.syntax); let mut path = symbol.path.to_string_lossy(); if show_worktree_root_name {