From 85a761cb2b7cb55fb9fca05828b7750a3f42949d Mon Sep 17 00:00:00 2001 From: Martin Fischer Date: Fri, 21 Mar 2025 22:17:42 +0100 Subject: [PATCH] markdown_preview: Fix rendering image not at all or too often (#25592) 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. --- .../markdown_preview/src/markdown_parser.rs | 124 ++++++++++++++---- 1 file changed, 100 insertions(+), 24 deletions(-) diff --git a/crates/markdown_preview/src/markdown_parser.rs b/crates/markdown_preview/src/markdown_parser.rs index f433edf8b3..23ca9c2015 100644 --- a/crates/markdown_preview/src/markdown_parser.rs +++ b/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 = "\