From 09db31288a16d05a465545fa50be5d0f88a5959f Mon Sep 17 00:00:00 2001 From: Antonio Scandurra Date: Wed, 23 Apr 2025 17:17:27 +0200 Subject: [PATCH] Fix panic when copying smart quotes in MarkdownElement (#29285) Release Notes: - Fixed a panic that could sometimes happen when copying text in the agent. --- crates/gpui/src/elements/text.rs | 9 +++ crates/markdown/src/markdown.rs | 124 ++++++++++++++++++++++++++++--- 2 files changed, 123 insertions(+), 10 deletions(-) diff --git a/crates/gpui/src/elements/text.rs b/crates/gpui/src/elements/text.rs index 04e43f5b09..eb6809c69e 100644 --- a/crates/gpui/src/elements/text.rs +++ b/crates/gpui/src/elements/text.rs @@ -278,6 +278,7 @@ impl IntoElement for StyledText { pub struct TextLayout(Rc>>); struct TextLayoutInner { + len: usize, lines: SmallVec<[WrappedLine; 1]>, line_height: Pixels, wrap_width: Option, @@ -349,6 +350,7 @@ impl TextLayout { } else { text.clone() }; + let len = text.len(); let Some(lines) = window .text_system() @@ -363,6 +365,7 @@ impl TextLayout { else { element_state.0.borrow_mut().replace(TextLayoutInner { lines: Default::default(), + len: 0, line_height, wrap_width, size: Some(Size::default()), @@ -380,6 +383,7 @@ impl TextLayout { element_state.0.borrow_mut().replace(TextLayoutInner { lines, + len, line_height, wrap_width, size: Some(size), @@ -544,6 +548,11 @@ impl TextLayout { self.0.borrow().as_ref().unwrap().line_height } + /// The UTF-8 length of the underlying text. + pub fn len(&self) -> usize { + self.0.borrow().as_ref().unwrap().len + } + /// The text for this layout. pub fn text(&self) -> String { self.0 diff --git a/crates/markdown/src/markdown.rs b/crates/markdown/src/markdown.rs index 556a48135b..5fad779da1 100644 --- a/crates/markdown/src/markdown.rs +++ b/crates/markdown/src/markdown.rs @@ -1027,21 +1027,21 @@ impl Element for MarkdownElement { _ => log::debug!("unsupported markdown tag end: {:?}", tag), }, MarkdownEvent::Text => { - builder.push_text(&parsed_markdown.source[range.clone()], range.start); + builder.push_text(&parsed_markdown.source[range.clone()], range.clone()); } MarkdownEvent::SubstitutedText(text) => { - builder.push_text(text, range.start); + builder.push_text(text, range.clone()); } MarkdownEvent::Code => { builder.push_text_style(self.style.inline_code.clone()); - builder.push_text(&parsed_markdown.source[range.clone()], range.start); + builder.push_text(&parsed_markdown.source[range.clone()], range.clone()); builder.pop_text_style(); } MarkdownEvent::Html => { - builder.push_text(&parsed_markdown.source[range.clone()], range.start); + builder.push_text(&parsed_markdown.source[range.clone()], range.clone()); } MarkdownEvent::InlineHtml => { - builder.push_text(&parsed_markdown.source[range.clone()], range.start); + builder.push_text(&parsed_markdown.source[range.clone()], range.clone()); } MarkdownEvent::Rule => { builder.push_div( @@ -1054,8 +1054,8 @@ impl Element for MarkdownElement { ); builder.pop_div() } - MarkdownEvent::SoftBreak => builder.push_text(" ", range.start), - MarkdownEvent::HardBreak => builder.push_text("\n", range.start), + MarkdownEvent::SoftBreak => builder.push_text(" ", range.clone()), + MarkdownEvent::HardBreak => builder.push_text("\n", range.clone()), _ => log::error!("unsupported markdown event {:?}", event), } } @@ -1383,13 +1383,13 @@ impl MarkdownElementBuilder { }); } - fn push_text(&mut self, text: &str, source_index: usize) { + fn push_text(&mut self, text: &str, source_range: Range) { self.pending_line.source_mappings.push(SourceMapping { rendered_index: self.pending_line.text.len(), - source_index, + source_index: source_range.start, }); self.pending_line.text.push_str(text); - self.current_source_index = source_index + text.len(); + self.current_source_index = source_range.end; if let Some(Some(language)) = self.code_block_stack.last() { let mut offset = 0; @@ -1466,6 +1466,10 @@ struct RenderedLine { impl RenderedLine { fn rendered_index_for_source_index(&self, source_index: usize) -> usize { + if source_index >= self.source_end { + return self.layout.len(); + } + let mapping = match self .source_mappings .binary_search_by_key(&source_index, |probe| probe.source_index) @@ -1477,6 +1481,10 @@ impl RenderedLine { } fn source_index_for_rendered_index(&self, rendered_index: usize) -> usize { + if rendered_index >= self.layout.len() { + return self.source_end; + } + let mapping = match self .source_mappings .binary_search_by_key(&rendered_index, |probe| probe.rendered_index) @@ -1649,3 +1657,99 @@ impl RenderedText { .find(|link| link.source_range.contains(&source_index)) } } + +#[cfg(test)] +mod tests { + use super::*; + use gpui::{TestAppContext, size}; + + #[gpui::test] + fn test_mappings(cx: &mut TestAppContext) { + // Formatting. + assert_mappings( + &render_markdown("He*l*lo", cx), + vec![vec![(0, 0), (1, 1), (2, 3), (3, 5), (4, 6), (5, 7)]], + ); + + // Multiple lines. + assert_mappings( + &render_markdown("Hello\n\nWorld", cx), + vec![ + vec![(0, 0), (1, 1), (2, 2), (3, 3), (4, 4), (5, 5)], + vec![(0, 7), (1, 8), (2, 9), (3, 10), (4, 11), (5, 12)], + ], + ); + + // Multi-byte characters. + assert_mappings( + &render_markdown("αβγ\n\nδεζ", cx), + vec![ + vec![(0, 0), (2, 2), (4, 4), (6, 6)], + vec![(0, 8), (2, 10), (4, 12), (6, 14)], + ], + ); + + // Smart quotes. + assert_mappings(&render_markdown("\"", cx), vec![vec![(0, 0), (3, 1)]]); + assert_mappings( + &render_markdown("\"hey\"", cx), + vec![vec![(0, 0), (3, 1), (4, 2), (5, 3), (6, 4), (9, 5)]], + ); + } + + fn render_markdown(markdown: &str, cx: &mut TestAppContext) -> RenderedText { + struct TestWindow; + + impl Render for TestWindow { + fn render(&mut self, _: &mut Window, _: &mut Context) -> impl IntoElement { + div() + } + } + + let (_, cx) = cx.add_window_view(|_, _| TestWindow); + let markdown = cx.new(|cx| Markdown::new(markdown.to_string().into(), None, None, cx)); + cx.run_until_parked(); + let (rendered, _) = cx.draw( + Default::default(), + size(px(600.0), px(600.0)), + |_window, _cx| MarkdownElement::new(markdown, MarkdownStyle::default()), + ); + rendered.text + } + + #[track_caller] + fn assert_mappings(rendered: &RenderedText, expected: Vec>) { + assert_eq!(rendered.lines.len(), expected.len(), "line count mismatch"); + for (line_ix, line_mappings) in expected.into_iter().enumerate() { + let line = &rendered.lines[line_ix]; + + assert!( + line.source_mappings.windows(2).all(|mappings| { + mappings[0].source_index < mappings[1].source_index + && mappings[0].rendered_index < mappings[1].rendered_index + }), + "line {} has duplicate mappings: {:?}", + line_ix, + line.source_mappings + ); + + for (rendered_ix, source_ix) in line_mappings { + assert_eq!( + line.source_index_for_rendered_index(rendered_ix), + source_ix, + "line {}, rendered_ix {}", + line_ix, + rendered_ix + ); + + assert_eq!( + line.rendered_index_for_source_index(source_ix), + rendered_ix, + "line {}, source_ix {}", + line_ix, + source_ix + ); + } + } + } +}