-
Notifications
You must be signed in to change notification settings - Fork 927
fix(docx): strip hardcoded numbering prefix from list item text #80
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -13,7 +13,7 @@ use crate::shared::blockstyle::{BlockStyle, StyledRun}; | |||||||
| use crate::shared::delta::rebase_emphasis; | ||||||||
| use crate::shared::fields::{FieldFrame, field_result}; | ||||||||
| use crate::shared::header::resolve_header_rows; | ||||||||
| use crate::shared::list::{ListEntry, ListKey, flush_list}; | ||||||||
| use crate::shared::list::{ListEntry, ListKey, MarkerKind, flush_list}; | ||||||||
| use crate::shared::text::{clean_text, is_xml_space}; | ||||||||
| use std::cell::RefCell; | ||||||||
| use std::collections::HashMap; | ||||||||
|
|
@@ -172,11 +172,97 @@ fn collect_blocks( | |||||||
| Ok(()) | ||||||||
| } | ||||||||
|
|
||||||||
| fn strip_list_num_prefix( | ||||||||
| blocks: &mut Vec<Block>, | ||||||||
| marker: MarkerKind, | ||||||||
| number: u64, | ||||||||
| label: &Option<String>, | ||||||||
| ) { | ||||||||
| // Bullets don't carry hardened number prefixes in OOXML. | ||||||||
| if !marker.ordered() { | ||||||||
| return; | ||||||||
| } | ||||||||
|
|
||||||||
| // The text the renderer will place before the paragraph. | ||||||||
| let num_text = label.clone().unwrap_or_else(|| marker.label(number)); | ||||||||
| // A list item's pieces may start with block attachments (text boxes, | ||||||||
| // rules, ...) before the paragraph carrying the inline text, so locate | ||||||||
| // the first paragraph rather than assuming it is blocks[0]. | ||||||||
| let para_idx = blocks.iter().position(|b| matches!(b, Block::Paragraph(_))); | ||||||||
| let Some(para_idx) = para_idx else { return }; | ||||||||
| let Block::Paragraph(inlines) = &mut blocks[para_idx] else { unreachable!() }; | ||||||||
|
|
||||||||
| // Flatten enough leading text inlines to decide whether the prefix matches. | ||||||||
| let mut flat = String::new(); | ||||||||
| let mut lead = 0usize; | ||||||||
| for inv in inlines.iter() { | ||||||||
| match inv { | ||||||||
| Inline::Text { text, .. } => { | ||||||||
| flat.push_str(text); | ||||||||
| lead += 1; | ||||||||
| } | ||||||||
| Inline::LineBreak | Inline::Anchor(_) => lead += 1, | ||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: A hard break can be silently removed from list content because Prompt for AI agents
Suggested change
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not a bug. Scanning skips LineBreak in |
||||||||
| _ => break, | ||||||||
| } | ||||||||
| if flat.len() >= num_text.len() + 1 { | ||||||||
| break; | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| // Must start with `num_text` followed by a separator: an exact match with | ||||||||
| // nothing after it is content that happens to equal the prefix, not a | ||||||||
| // hardened prefix over content we should strip. | ||||||||
| let sep = match flat.strip_prefix(&num_text) { | ||||||||
| Some(r) if r.starts_with(' ') || r.starts_with('\t') => 1, | ||||||||
| _ => return, | ||||||||
| }; | ||||||||
| let mut strip = num_text.len() + sep; | ||||||||
|
|
||||||||
| // Drain the leading inlines and re-insert the survivors. | ||||||||
| let tail: Vec<Inline> = inlines.split_off(lead); | ||||||||
| let mut head = Vec::with_capacity(inlines.len()); | ||||||||
| for inv in std::mem::take(inlines) { | ||||||||
| if strip == 0 { | ||||||||
| head.push(inv); | ||||||||
| continue; | ||||||||
| } | ||||||||
| match inv { | ||||||||
| Inline::Text { text, style } => { | ||||||||
| if text.len() <= strip { | ||||||||
| strip -= text.len(); | ||||||||
| } else { | ||||||||
| let t = text[strip..].trim_start().to_string(); | ||||||||
| strip = 0; | ||||||||
| if !t.is_empty() { | ||||||||
| head.push(Inline::Text { text: t, style }); | ||||||||
| } | ||||||||
| } | ||||||||
| } | ||||||||
| // Anchors and line breaks carry structural meaning (intra-document link | ||||||||
| // targets, source line boundaries), so preserve them instead of | ||||||||
| // consuming them as part of the prefix region. | ||||||||
| Inline::LineBreak | Inline::Anchor(_) => head.push(inv), | ||||||||
| other => { | ||||||||
| strip = 0; | ||||||||
| head.push(other); | ||||||||
| } | ||||||||
| } | ||||||||
| } | ||||||||
| head.extend(tail); | ||||||||
| *inlines = head; | ||||||||
|
|
||||||||
| // Don't leave a ghost paragraph with no visible content. | ||||||||
| if inlines.is_empty() { | ||||||||
| blocks.remove(para_idx); | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| fn emit_paragraph(kind: ParaKind, pieces: Vec<Piece>, blocks: &mut Vec<Block>, runs: &mut Runs) { | ||||||||
| match kind { | ||||||||
| ParaKind::ListItem { ilvl, key, number, label } => { | ||||||||
| runs.styled.flush(blocks); | ||||||||
| let item = pieces_into_blocks(pieces); | ||||||||
| let mut item = pieces_into_blocks(pieces); | ||||||||
| strip_list_num_prefix(&mut item, key.marker, number, &label); | ||||||||
| runs.list.push(ListEntry { level: ilvl, key, number, label, blocks: item }); | ||||||||
| } | ||||||||
| ParaKind::Styled(style) => { | ||||||||
|
|
@@ -986,4 +1072,73 @@ mod tests { | |||||||
| assert_eq!(before, "before"); | ||||||||
| assert_eq!(after, "after"); | ||||||||
| } | ||||||||
|
|
||||||||
| // -- strip_list_num_prefix ------------------------------------------- | ||||||||
|
|
||||||||
| fn strip(blocks: &mut Vec<Block>, marker: MarkerKind, n: u64, label: &Option<String>) -> String { | ||||||||
| strip_list_num_prefix(blocks, marker, n, label); | ||||||||
| match blocks.first() { | ||||||||
| Some(Block::Paragraph(inlines)) => crate::model::inlines_to_plain_text(inlines), | ||||||||
| _ => String::new(), | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| #[test] | ||||||||
| fn strip_prefix_ordered() { | ||||||||
| let cases = [ | ||||||||
| (vec![Inline::plain("1. text")], MarkerKind::Decimal, 1, None, "text"), | ||||||||
| (vec![Inline::plain("1.\ttext")], MarkerKind::Decimal, 1, None, "text"), | ||||||||
| (vec![Inline::plain("a. text")], MarkerKind::LowerAlpha, 1, None, "text"), | ||||||||
| (vec![Inline::plain("iv. text")], MarkerKind::LowerRoman, 4, None, "text"), | ||||||||
| ]; | ||||||||
| for (inlines, marker, n, label, expected) in cases { | ||||||||
| let mut blocks = vec![Block::Paragraph(inlines)]; | ||||||||
| assert_eq!(strip(&mut blocks, marker, n, &label), expected); | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| #[test] | ||||||||
| fn strip_prefix_composite_label() { | ||||||||
| let mut blocks = vec![Block::Paragraph(vec![Inline::plain("1.1) text")])]; | ||||||||
| assert_eq!(strip(&mut blocks, MarkerKind::Decimal, 1, &Some("1.1)".into())), "text"); | ||||||||
| } | ||||||||
|
|
||||||||
| #[test] | ||||||||
| fn strip_prefix_split_inlines() { | ||||||||
| let mut blocks = vec![Block::Paragraph(vec![ | ||||||||
| Inline::plain("1."), Inline::plain(" "), Inline::plain("text"), | ||||||||
| ])]; | ||||||||
| assert_eq!(strip(&mut blocks, MarkerKind::Decimal, 1, &None), "text"); | ||||||||
|
|
||||||||
| let mut blocks = vec![Block::Paragraph(vec![ | ||||||||
| Inline::plain("1. "), Inline::plain("content"), | ||||||||
| ])]; | ||||||||
| assert_eq!(strip(&mut blocks, MarkerKind::Decimal, 1, &None), "content"); | ||||||||
| } | ||||||||
|
|
||||||||
| #[test] | ||||||||
| fn strip_prefix_no_match() { | ||||||||
| // Text that doesn't start with the expected prefix. | ||||||||
| let mut blocks = vec![Block::Paragraph(vec![Inline::plain("text")])]; | ||||||||
| assert_eq!(strip(&mut blocks, MarkerKind::Decimal, 1, &None), "text"); | ||||||||
|
|
||||||||
| // Starts with number but no separator after it. | ||||||||
| let mut blocks = vec![Block::Paragraph(vec![Inline::plain("1.text")])]; | ||||||||
| assert_eq!(strip(&mut blocks, MarkerKind::Decimal, 1, &None), "1.text"); | ||||||||
|
|
||||||||
| // Resolved number differs from the hardcoded prefix. | ||||||||
| let mut blocks = vec![Block::Paragraph(vec![Inline::plain("1. text")])]; | ||||||||
| assert_eq!(strip(&mut blocks, MarkerKind::Decimal, 3, &None), "1. text"); | ||||||||
|
|
||||||||
| // Bullets short-circuit via `!marker.ordered()`. | ||||||||
| let mut blocks = vec![Block::Paragraph(vec![Inline::plain("- text")])]; | ||||||||
| assert_eq!(strip(&mut blocks, MarkerKind::Bullet, 0, &None), "- text"); | ||||||||
| } | ||||||||
|
|
||||||||
| #[test] | ||||||||
| fn strip_prefix_empty_blocks() { | ||||||||
| let mut blocks: Vec<Block> = Vec::new(); | ||||||||
| strip_list_num_prefix(&mut blocks, MarkerKind::Decimal, 1, &None); | ||||||||
| assert!(blocks.is_empty()); | ||||||||
| } | ||||||||
| } | ||||||||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: The new first-paragraph lookup can target the wrong paragraph when a preceding text box is present. Because text-box content is converted into
Block::Paragraph(viawalk_drawing->parse_blocks, andpieces_into_blocksalways produces a Paragraph), a list item ordered as[textbox attachment, item text]yields[Paragraph(textbox…), Paragraph("1. text")], andposition(Paragraph)matches the text box's paragraph. That both skips stripping the item's real prefix (so the stated text-box-first goal isn't met) and risks stripping text-box content whose first line coincidentally starts with the resolved label. Consider locating the paragraph whose leading inlines actually match the expected prefix, or tracking which blocks derive from the item's own runs vs. text-box attachments, rather than taking the first Paragraph.Prompt for AI agents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is pre-existing, not from Fix 1. The
old
blocks.first_mut()and the newposition(Paragraph)have the samebehavior when a textbox Paragraph comes first. Fix 1 only deals with
non-Paragraph blocks (Rule, Image) blocking the item. Disambiguating
textbox paragraphs would be a separate change.