Fix panic when copying smart quotes in MarkdownElement (#29285)

Antonio Scandurra created

Release Notes:

- Fixed a panic that could sometimes happen when copying text in the
agent.

Change summary

crates/gpui/src/elements/text.rs |   9 ++
crates/markdown/src/markdown.rs  | 124 +++++++++++++++++++++++++++++++--
2 files changed, 123 insertions(+), 10 deletions(-)

Detailed changes

crates/gpui/src/elements/text.rs 🔗

@@ -278,6 +278,7 @@ impl IntoElement for StyledText {
 pub struct TextLayout(Rc<RefCell<Option<TextLayoutInner>>>);
 
 struct TextLayoutInner {
+    len: usize,
     lines: SmallVec<[WrappedLine; 1]>,
     line_height: Pixels,
     wrap_width: Option<Pixels>,
@@ -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

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<usize>) {
         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<Self>) -> 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<Vec<(usize, usize)>>) {
+        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
+                );
+            }
+        }
+    }
+}