markdown_preview: Fix rendering image not at all or too often (#25592)

Martin Fischer created 9 months ago

Before MarkdownParagraphChunk::Image was pushed for every Text event if
we're currently inside an image. This was wrong since pulldown-cmark
parses `![](foo)` as:

Start(Image { link_type: Inline, dest_url: "foo", title: "", id: "" })
    End(Image)

If there is no alt text, no Text event is emitted. Which caused images
without any alt text not to be rendered at all.

For alt texts containing inline formatting this was even more obviously
broken since e.g. `![foo *bar* baz](foo)` gets parsed as:

Start(Image { link_type: Inline, dest_url: "foo", title: "", id: "" })
      Text(Borrowed("foo "))
      Start(Emphasis)
        Text(Borrowed("bar"))
      End(Emphasis)
      Text(Borrowed(" baz"))
    End(Image)

which for this example caused the image to appear 3 times in the
preview.

This commit fixes these two bugs which have existed since the
introduction of the image previews in
96854c68eadd1dd72aa379368fde3cea791498a4.

Release Notes:

- Fixed images in the markdown preview appearing not at all or too
often.

Change summary

crates/markdown_preview/src/markdown_parser.rs | 124 ++++++++++++++++---
1 file changed, 100 insertions(+), 24 deletions(-)

Detailed changes

crates/markdown_preview/src/markdown_parser.rs

@@ -314,29 +314,6 @@ impl<'a> MarkdownParser<'a> {
                             ));
                         }
                     }
-                    if let Some(image) = image.as_mut() {
-                        text.truncate(text.len() - t.len());
-                        image.set_alt_text(t.to_string().into());
-                        if !text.is_empty() {
-                            let parsed_regions = MarkdownParagraphChunk::Text(ParsedMarkdownText {
-                                source_range: source_range.clone(),
-                                contents: text.clone(),
-                                highlights: highlights.clone(),
-                                region_ranges: region_ranges.clone(),
-                                regions: regions.clone(),
-                            });
-                            text = String::new();
-                            highlights = vec![];
-                            region_ranges = vec![];
-                            regions = vec![];
-                            markdown_text_like.push(parsed_regions);
-                        }
-
-                        let parsed_image = MarkdownParagraphChunk::Image(image.clone());
-                        markdown_text_like.push(parsed_image);
-                        style = MarkdownHighlightStyle::default();
-                        style.underline = true;
-                    }
                 }
                 Event::Code(t) => {
                     text.push_str(t.as_ref());
@@ -367,6 +344,20 @@ impl<'a> MarkdownParser<'a> {
                         );
                     }
                     Tag::Image { dest_url, .. } => {
+                        if !text.is_empty() {
+                            let parsed_regions = MarkdownParagraphChunk::Text(ParsedMarkdownText {
+                                source_range: source_range.clone(),
+                                contents: text.clone(),
+                                highlights: highlights.clone(),
+                                region_ranges: region_ranges.clone(),
+                                regions: regions.clone(),
+                            });
+                            text = String::new();
+                            highlights = vec![];
+                            region_ranges = vec![];
+                            regions = vec![];
+                            markdown_text_like.push(parsed_regions);
+                        }
                         image = Image::identify(
                             dest_url.to_string(),
                             source_range.clone(),
@@ -386,7 +377,12 @@ impl<'a> MarkdownParser<'a> {
                         link = None;
                     }
                     TagEnd::Image => {
-                        image = None;
+                        if let Some(mut image) = image.take() {
+                            if !text.is_empty() {
+                                image.alt_text = Some(std::mem::take(&mut text).into());
+                            }
+                            markdown_text_like.push(MarkdownParagraphChunk::Image(image));
+                        }
                     }
                     TagEnd::Paragraph => {
                         self.cursor += 1;
@@ -931,6 +927,86 @@ mod tests {
         );
     }
 
+    #[gpui::test]
+    async fn test_image_without_alt_text() {
+        let parsed = parse("![](http://example.com/foo.png)").await;
+
+        let paragraph = if let ParsedMarkdownElement::Paragraph(text) = &parsed.children[0] {
+            text
+        } else {
+            panic!("Expected a paragraph");
+        };
+        assert_eq!(
+            paragraph[0],
+            MarkdownParagraphChunk::Image(Image {
+                source_range: 0..31,
+                link: Link::Web {
+                    url: "http://example.com/foo.png".to_string(),
+                },
+                alt_text: None,
+            },)
+        );
+    }
+
+    #[gpui::test]
+    async fn test_image_with_alt_text_containing_formatting() {
+        let parsed = parse("![foo *bar* baz](http://example.com/foo.png)").await;
+
+        let ParsedMarkdownElement::Paragraph(chunks) = &parsed.children[0] else {
+            panic!("Expected a paragraph");
+        };
+        assert_eq!(
+            chunks,
+            &[MarkdownParagraphChunk::Image(Image {
+                source_range: 0..44,
+                link: Link::Web {
+                    url: "http://example.com/foo.png".to_string(),
+                },
+                alt_text: Some("foo bar baz".into()),
+            }),],
+        );
+    }
+
+    #[gpui::test]
+    async fn test_images_with_text_in_between() {
+        let parsed = parse(
+            "![foo](http://example.com/foo.png)\nLorem Ipsum\n![bar](http://example.com/bar.png)",
+        )
+        .await;
+
+        let chunks = if let ParsedMarkdownElement::Paragraph(text) = &parsed.children[0] {
+            text
+        } else {
+            panic!("Expected a paragraph");
+        };
+        assert_eq!(
+            chunks,
+            &vec![
+                MarkdownParagraphChunk::Image(Image {
+                    source_range: 0..81,
+                    link: Link::Web {
+                        url: "http://example.com/foo.png".to_string(),
+                    },
+                    alt_text: Some("foo".into()),
+                }),
+                MarkdownParagraphChunk::Text(ParsedMarkdownText {
+                    source_range: 0..81,
+                    contents: " Lorem Ipsum ".to_string(),
+                    highlights: Vec::new(),
+                    region_ranges: Vec::new(),
+                    regions: Vec::new(),
+                }),
+                MarkdownParagraphChunk::Image(Image {
+                    source_range: 0..81,
+                    link: Link::Web {
+                        url: "http://example.com/bar.png".to_string(),
+                    },
+                    alt_text: Some("bar".into()),
+                })
+            ]
+        );
+    }
+
     #[gpui::test]
     async fn test_header_only_table() {
         let markdown = "\