From 6441099a67c6c6a16aa4bdef884aa64f5e562cc8 Mon Sep 17 00:00:00 2001 From: Finn Evers Date: Wed, 10 Sep 2025 18:56:49 +0200 Subject: [PATCH] gpui: Fix blending of colors in `HighlightStyle::highlight` (#37666) Whilst looking into adding support for RainbowBrackes, we stumbled upon this: Whereas for all properties during this blending, we take the value of `other` if it is set, for the color we actually take `self.color` instead of `other.color` if `self.color` is at full opacity. `Hsla::blend` returns the latter color if it is at full opacity, which seems wrong for this case. Hence, this PR swaps these. Will not merge before the next release, to ensure that we don't break something somewhere unexpected. Release Notes: - N/A --- crates/editor/src/display_map.rs | 101 ++++++----- .../src/display_map/custom_highlights.rs | 10 +- crates/editor/src/display_map/inlay_map.rs | 6 +- crates/editor/src/editor.rs | 3 +- crates/gpui/src/style.rs | 163 +++++++++++++----- crates/language/src/buffer.rs | 8 +- 6 files changed, 180 insertions(+), 111 deletions(-) diff --git a/crates/editor/src/display_map.rs b/crates/editor/src/display_map.rs index fd4e762fb200e0c9dd19f71d4148e8bf7d9bc7bc..7b2bd2eb419b1436b3f55f88e1904028e25f33d6 100644 --- a/crates/editor/src/display_map.rs +++ b/crates/editor/src/display_map.rs @@ -703,9 +703,8 @@ impl<'a> HighlightedChunk<'a> { }), ..Default::default() }; - let invisible_style = if let Some(mut style) = style { - style.highlight(invisible_highlight); - style + let invisible_style = if let Some(style) = style { + style.highlight(invisible_highlight) } else { invisible_highlight }; @@ -726,9 +725,8 @@ impl<'a> HighlightedChunk<'a> { }), ..Default::default() }; - let invisible_style = if let Some(mut style) = style { - style.highlight(invisible_highlight); - style + let invisible_style = if let Some(style) = style { + style.highlight(invisible_highlight) } else { invisible_highlight }; @@ -962,62 +960,59 @@ impl DisplaySnapshot { }, ) .flat_map(|chunk| { - let mut highlight_style = chunk + let highlight_style = chunk .syntax_highlight_id .and_then(|id| id.style(&editor_style.syntax)); - if let Some(chunk_highlight) = chunk.highlight_style { - // For color inlays, blend the color with the editor background - let mut processed_highlight = chunk_highlight; - if chunk.is_inlay - && let Some(inlay_color) = chunk_highlight.color - { - // Only blend if the color has transparency (alpha < 1.0) - if inlay_color.a < 1.0 { - let blended_color = editor_style.background.blend(inlay_color); - processed_highlight.color = Some(blended_color); - } - } - - if let Some(highlight_style) = highlight_style.as_mut() { - highlight_style.highlight(processed_highlight); - } else { - highlight_style = Some(processed_highlight); + let chunk_highlight = chunk.highlight_style.map(|chunk_highlight| { + HighlightStyle { + // For color inlays, blend the color with the editor background + // if the color has transparency (alpha < 1.0) + color: chunk_highlight.color.map(|color| { + if chunk.is_inlay && !color.is_opaque() { + editor_style.background.blend(color) + } else { + color + } + }), + ..chunk_highlight } - } - - let mut diagnostic_highlight = HighlightStyle::default(); + }); - if let Some(severity) = chunk.diagnostic_severity.filter(|severity| { - self.diagnostics_max_severity - .into_lsp() - .is_some_and(|max_severity| severity <= &max_severity) - }) { - if chunk.is_unnecessary { - diagnostic_highlight.fade_out = Some(editor_style.unnecessary_code_fade); - } - if chunk.underline - && editor_style.show_underlines - && !(chunk.is_unnecessary && severity > lsp::DiagnosticSeverity::WARNING) - { - let diagnostic_color = super::diagnostic_style(severity, &editor_style.status); - diagnostic_highlight.underline = Some(UnderlineStyle { - color: Some(diagnostic_color), - thickness: 1.0.into(), - wavy: true, - }); - } - } + let diagnostic_highlight = chunk + .diagnostic_severity + .filter(|severity| { + self.diagnostics_max_severity + .into_lsp() + .is_some_and(|max_severity| severity <= &max_severity) + }) + .map(|severity| HighlightStyle { + fade_out: chunk + .is_unnecessary + .then_some(editor_style.unnecessary_code_fade), + underline: (chunk.underline + && editor_style.show_underlines + && !(chunk.is_unnecessary && severity > lsp::DiagnosticSeverity::WARNING)) + .then(|| { + let diagnostic_color = + super::diagnostic_style(severity, &editor_style.status); + UnderlineStyle { + color: Some(diagnostic_color), + thickness: 1.0.into(), + wavy: true, + } + }), + ..Default::default() + }); - if let Some(highlight_style) = highlight_style.as_mut() { - highlight_style.highlight(diagnostic_highlight); - } else { - highlight_style = Some(diagnostic_highlight); - } + let style = [highlight_style, chunk_highlight, diagnostic_highlight] + .into_iter() + .flatten() + .reduce(|acc, highlight| acc.highlight(highlight)); HighlightedChunk { text: chunk.text, - style: highlight_style, + style, is_tab: chunk.is_tab, is_inlay: chunk.is_inlay, replacement: chunk.renderer.map(ChunkReplacement::Renderer), diff --git a/crates/editor/src/display_map/custom_highlights.rs b/crates/editor/src/display_map/custom_highlights.rs index 0fb2a893d918159100105727bd46218fc584299e..e4af453f894f5d4afeeed990f2233f1ec1b5dc76 100644 --- a/crates/editor/src/display_map/custom_highlights.rs +++ b/crates/editor/src/display_map/custom_highlights.rs @@ -150,11 +150,11 @@ impl<'a> Iterator for CustomHighlightsChunks<'a> { ..chunk.clone() }; if !self.active_highlights.is_empty() { - let mut highlight_style = HighlightStyle::default(); - for active_highlight in self.active_highlights.values() { - highlight_style.highlight(*active_highlight); - } - prefix.highlight_style = Some(highlight_style); + prefix.highlight_style = self + .active_highlights + .values() + .copied() + .reduce(|acc, active_highlight| acc.highlight(active_highlight)); } Some(prefix) } diff --git a/crates/editor/src/display_map/inlay_map.rs b/crates/editor/src/display_map/inlay_map.rs index e00ffdbf2c6ed7ee7288b2371481cb1f1b28bc92..62c16f5a1e453d5e9cc60e63804cbe611781aa16 100644 --- a/crates/editor/src/display_map/inlay_map.rs +++ b/crates/editor/src/display_map/inlay_map.rs @@ -385,9 +385,9 @@ impl<'a> Iterator for InlayChunks<'a> { next_inlay_highlight_endpoint = usize::MAX; } else { next_inlay_highlight_endpoint = range.end - offset_in_inlay.0; - highlight_style - .get_or_insert_with(Default::default) - .highlight(*style); + highlight_style = highlight_style + .map(|highlight| highlight.highlight(*style)) + .or_else(|| Some(*style)); } } else { next_inlay_highlight_endpoint = usize::MAX; diff --git a/crates/editor/src/editor.rs b/crates/editor/src/editor.rs index 217b3d4bf3c342051f56e1e817df10979aac1fcd..46f28f078978a60c99f53ff384efcf30214a00a2 100644 --- a/crates/editor/src/editor.rs +++ b/crates/editor/src/editor.rs @@ -23817,8 +23817,7 @@ pub fn styled_runs_for_code_label<'a>( } else { return Default::default(); }; - let mut muted_style = style; - muted_style.highlight(fade_out); + let muted_style = style.highlight(fade_out); let mut runs = SmallVec::<[(Range, HighlightStyle); 3]>::new(); if range.start >= label.filter_range.end { diff --git a/crates/gpui/src/style.rs b/crates/gpui/src/style.rs index 5b69ce7fa6eb06affc2f77c0d1bdfbe4165c206a..53ca6508bc94e96033d0929a674ce59e8206ba04 100644 --- a/crates/gpui/src/style.rs +++ b/crates/gpui/src/style.rs @@ -886,43 +886,32 @@ impl HighlightStyle { } /// Blend this highlight style with another. /// Non-continuous properties, like font_weight and font_style, are overwritten. - pub fn highlight(&mut self, other: HighlightStyle) { - match (self.color, other.color) { - (Some(self_color), Some(other_color)) => { - self.color = Some(Hsla::blend(other_color, self_color)); - } - (None, Some(other_color)) => { - self.color = Some(other_color); - } - _ => {} - } - - if other.font_weight.is_some() { - self.font_weight = other.font_weight; - } - - if other.font_style.is_some() { - self.font_style = other.font_style; - } - - if other.background_color.is_some() { - self.background_color = other.background_color; - } - - if other.underline.is_some() { - self.underline = other.underline; - } - - if other.strikethrough.is_some() { - self.strikethrough = other.strikethrough; - } - - match (other.fade_out, self.fade_out) { - (Some(source_fade), None) => self.fade_out = Some(source_fade), - (Some(source_fade), Some(dest_fade)) => { - self.fade_out = Some((dest_fade * (1. + source_fade)).clamp(0., 1.)); - } - _ => {} + #[must_use] + pub fn highlight(self, other: HighlightStyle) -> Self { + Self { + color: other + .color + .map(|other_color| { + if let Some(color) = self.color { + color.blend(other_color) + } else { + other_color + } + }) + .or(self.color), + font_weight: other.font_weight.or(self.font_weight), + font_style: other.font_style.or(self.font_style), + background_color: other.background_color.or(self.background_color), + underline: other.underline.or(self.underline), + strikethrough: other.strikethrough.or(self.strikethrough), + fade_out: other + .fade_out + .map(|source_fade| { + self.fade_out + .map(|dest_fade| (dest_fade * (1. + source_fade)).clamp(0., 1.)) + .unwrap_or(source_fade) + }) + .or(self.fade_out), } } } @@ -987,10 +976,11 @@ pub fn combine_highlights( while let Some((endpoint_ix, highlight_id, is_start)) = endpoints.peek() { let prev_index = mem::replace(&mut ix, *endpoint_ix); if ix > prev_index && !active_styles.is_empty() { - let mut current_style = HighlightStyle::default(); - for highlight_id in &active_styles { - current_style.highlight(highlights[*highlight_id]); - } + let current_style = active_styles + .iter() + .fold(HighlightStyle::default(), |acc, highlight_id| { + acc.highlight(highlights[*highlight_id]) + }); return Some((prev_index..ix, current_style)); } @@ -1306,10 +1296,95 @@ impl From for taffy::style::Position { #[cfg(test)] mod tests { - use crate::{blue, green, red, yellow}; + use crate::{blue, green, px, red, yellow}; use super::*; + #[test] + fn test_basic_highlight_style_combination() { + let style_a = HighlightStyle::default(); + let style_b = HighlightStyle::default(); + let style_a = style_a.highlight(style_b); + assert_eq!( + style_a, + HighlightStyle::default(), + "Combining empty styles should not produce a non-empty style." + ); + + let mut style_b = HighlightStyle { + color: Some(red()), + strikethrough: Some(StrikethroughStyle { + thickness: px(2.), + color: Some(blue()), + }), + fade_out: Some(0.), + font_style: Some(FontStyle::Italic), + font_weight: Some(FontWeight(300.)), + background_color: Some(yellow()), + underline: Some(UnderlineStyle { + thickness: px(2.), + color: Some(red()), + wavy: true, + }), + }; + let expected_style = style_b; + + let style_a = style_a.highlight(style_b); + assert_eq!( + style_a, expected_style, + "Blending an empty style with another style should return the other style" + ); + + let style_b = style_b.highlight(Default::default()); + assert_eq!( + style_b, expected_style, + "Blending a style with an empty style should not change the style." + ); + + let mut style_c = expected_style; + + let style_d = HighlightStyle { + color: Some(blue().alpha(0.7)), + strikethrough: Some(StrikethroughStyle { + thickness: px(4.), + color: Some(crate::red()), + }), + fade_out: Some(0.), + font_style: Some(FontStyle::Oblique), + font_weight: Some(FontWeight(800.)), + background_color: Some(green()), + underline: Some(UnderlineStyle { + thickness: px(4.), + color: None, + wavy: false, + }), + }; + + let expected_style = HighlightStyle { + color: Some(red().blend(blue().alpha(0.7))), + strikethrough: Some(StrikethroughStyle { + thickness: px(4.), + color: Some(red()), + }), + // TODO this does not seem right + fade_out: Some(0.), + font_style: Some(FontStyle::Oblique), + font_weight: Some(FontWeight(800.)), + background_color: Some(green()), + underline: Some(UnderlineStyle { + thickness: px(4.), + color: None, + wavy: false, + }), + }; + + let style_c = style_c.highlight(style_d); + assert_eq!( + style_c, expected_style, + "Blending styles should blend properties where possible and override all others" + ); + } + #[test] fn test_combine_highlights() { assert_eq!( @@ -1337,14 +1412,14 @@ mod tests { ( 1..2, HighlightStyle { - color: Some(green()), + color: Some(blue()), ..Default::default() } ), ( 2..3, HighlightStyle { - color: Some(green()), + color: Some(blue()), font_style: Some(FontStyle::Italic), ..Default::default() } diff --git a/crates/language/src/buffer.rs b/crates/language/src/buffer.rs index 496f9d60d1ad405f82ab522e9050560ad4e64bf4..363040e2fc6e72098ab3b4176b226fb02e72d411 100644 --- a/crates/language/src/buffer.rs +++ b/crates/language/src/buffer.rs @@ -635,13 +635,13 @@ impl HighlightedTextBuilder { self.text.push_str(chunk.text); let end = self.text.len(); - if let Some(mut highlight_style) = chunk + if let Some(highlight_style) = chunk .syntax_highlight_id .and_then(|id| id.style(syntax_theme)) { - if let Some(override_style) = override_style { - highlight_style.highlight(override_style); - } + let highlight_style = override_style.map_or(highlight_style, |override_style| { + highlight_style.highlight(override_style) + }); self.highlights.push((start..end, highlight_style)); } else if let Some(override_style) = override_style { self.highlights.push((start..end, override_style));