From 8218dea5bdb5c41de365855df0c277600559fbd4 Mon Sep 17 00:00:00 2001 From: FredApps <153173854+FredApps@users.noreply.github.com> Date: Wed, 4 Nov 2026 17:43:46 +0100 Subject: [PATCH] Fix #5559 follow-ups: generated list prefix, hidden-text objects, turned clipped lines, kept-markup checks once per export - update_generated keeps what precedes a wrapped list's field (r4468529243) - from_doc keeps objects in hidden text when hidden text is written (r4469018820) - clipped cell lines in a turned frame are drawn once, clipped, turned (r4469018900) - kept::while_writing works out reply map and page fingerprint once per export (r4468529227) - odt_charts fixture declares its namespaces (r4468529259) - guard test for character styles based on a missing parent (r4469018843, not a bug) --- crates/doc/src/kept.rs | 87 ++++++++++++++++++- crates/engine/src/cmd/citations/generated.rs | 48 ++++++++-- crates/engine/tests/it/odt_charts.rs | 3 +- crates/formats/src/model/from_doc.rs | 10 ++- .../formats/src/odt/tests_more/char_styles.rs | 18 ++++ crates/formats/src/odt/tests_more/chars.rs | 16 ++++ crates/layout/src/display/page.rs | 13 ++- crates/layout/src/tests/textbox.rs | 42 +++++++++ 8 files changed, 222 insertions(+), 15 deletions(-) diff --git a/crates/doc/src/kept.rs b/crates/doc/src/kept.rs index cc474565d6..ee0d3e9a1a 100644 --- a/crates/doc/src/kept.rs +++ b/crates/doc/src/kept.rs @@ -263,7 +263,9 @@ impl KeptXml { } /// Whether `doc`'s page setup is still as it was read. pub fn pages_as_read(&self, doc: &crate::Document) -> bool { - self.pages_fp.is_some_and(|fp| fp == pages_fingerprint(doc)) + let Some(fp) = self.pages_fp else { return false }; + let now = in_scope(doc, |sc| *sc.pages.get_or_insert_with(|| pages_fingerprint(doc))).unwrap_or_else(|| pages_fingerprint(doc)); + fp == now } /// Whether `doc`'s style definitions are still as they were read. pub fn styles_as_read(&self, doc: &crate::Document) -> bool { @@ -384,11 +386,62 @@ const MAX_REPLIES: usize = 10_000; /// What comment `root` is with its replies (and theirs): each one's author, date, state and /// text, and how many they are. pub fn comments_fingerprint(doc: &crate::Document, root: u32) -> (u64, usize) { - family_fingerprint(doc, root, &replies(doc)) + family_fingerprint(doc, root, &scoped_replies(doc)) +} + +/// What a writer asks of a document's kept markup for many paragraphs, worked out once while +/// [`while_writing`] it: each comment's replies, and its page setup's fingerprint. Without it a +/// document of C comments took C² steps (and its page setup serialized once per note) to save. +struct Scope { + /// The document's address: it can't move or change while borrowed for the writing. + doc: usize, + replies: Option>>>, + pages: Option, +} + +thread_local! { + static SCOPE: std::cell::RefCell> = const { std::cell::RefCell::new(None) }; +} + +#[cfg(test)] +thread_local! { + /// How many times [`replies`] was built (tests). + static REPLY_BUILDS: std::cell::Cell = const { std::cell::Cell::new(0) }; +} + +/// Puts the scope before it back when dropped (when `f` returns or unwinds). +struct Restore(Option); + +impl Drop for Restore { + fn drop(&mut self) { + let prev = self.0.take(); + SCOPE.with(|s| *s.borrow_mut() = prev); + } +} + +/// Run `f`, which writes `doc`, with what the kept markup asks of `doc` worked out at most once. +pub fn while_writing(doc: &crate::Document, f: impl FnOnce() -> R) -> R { + let key = std::ptr::from_ref(doc) as usize; + let prev = SCOPE.with(|s| s.replace(Some(Scope { doc: key, replies: None, pages: None }))); + let _restore = Restore(prev); + f() +} + +/// The scope for `doc`, if it is being written: `f` of it. +fn in_scope(doc: &crate::Document, f: impl FnOnce(&mut Scope) -> R) -> Option { + let key = std::ptr::from_ref(doc) as usize; + SCOPE.with(|s| s.borrow_mut().as_mut().filter(|sc| sc.doc == key).map(f)) +} + +/// [`replies`], built once while `doc` is being written. +fn scoped_replies(doc: &crate::Document) -> std::rc::Rc>> { + in_scope(doc, |sc| sc.replies.get_or_insert_with(|| std::rc::Rc::new(replies(doc))).clone()).unwrap_or_else(|| std::rc::Rc::new(replies(doc))) } /// Each comment's replies, by the comment's id. fn replies(doc: &crate::Document) -> HashMap> { + #[cfg(test)] + REPLY_BUILDS.with(|n| n.set(n.get() + 1)); let mut out: HashMap> = HashMap::new(); for (id, c) in &doc.comments { if let Some(p) = c.parent { @@ -694,4 +747,34 @@ mod tests { assert_eq!(k.group(p), Some((1, 1))); assert_eq!(k.story_fps.get(&id), Some(&story_fingerprint(&doc, id))); } + + /// Writing a document checks each kept paragraph's comments against what was read: the + /// comments' replies are looked up once for the whole document, not once per comment. + #[test] + fn writing_many_comments_looks_their_replies_up_once() { + use crate::para::InlineObject; + let n = 50u32; + let text = (0..n).map(|i| format!("Para {i}")).collect::>().join("\n"); + let mut doc = crate::Document::from_text(&text); + for i in 0..n { + let part = doc.add_part(crate::PartKind::Comment, vec![crate::para_block(Paragraph::with_text("Note", Default::default()))]); + doc.comments.insert(i, crate::Comment { parent: None, part, ..Default::default() }); + doc.para_mut(crate::StoryRef::Body, &crate::Path::top(i as usize)) + .unwrap() + .insert_object(0, InlineObject::CommentStart { id: i }, &Default::default()) + .unwrap(); + } + let mut k = KeptXml { format: "odt".into(), ..Default::default() }; + for i in 0..n { + k.comment_fps.insert(i, comments_fingerprint(&doc, i).0); + } + let xml = "x"; + let before = REPLY_BUILDS.with(std::cell::Cell::get); + let all = while_writing(&doc, || doc.body.iter().filter_map(|b| b.as_para()).all(|p| k.comments_as_read(&doc, p, xml))); + assert!(all, "every paragraph's comment is as read"); + assert_eq!(REPLY_BUILDS.with(std::cell::Cell::get) - before, 1); + // Outside a writing, nothing is kept between calls. + assert!(doc.body.first().and_then(|b| b.as_para()).is_some_and(|p| k.comments_as_read(&doc, p, xml))); + assert_eq!(REPLY_BUILDS.with(std::cell::Cell::get) - before, 2); + } } diff --git a/crates/engine/src/cmd/citations/generated.rs b/crates/engine/src/cmd/citations/generated.rs index 5600533c58..9105c84206 100644 --- a/crates/engine/src/cmd/citations/generated.rs +++ b/crates/engine/src/cmd/citations/generated.rs @@ -72,7 +72,7 @@ const MAX_WRAPPED: usize = 10_000; /// A generated list (index, bibliography, table of figures or authorities) held as a complex /// field that starts at body block `i` and runs over its entries: its field code and the block /// it ends in. `None` for anything else (a TOC has its own update). -fn wrapped_list(s: &Session, i: usize) -> Option<(String, usize, usize)> { +fn wrapped_list(s: &Session, i: usize) -> Option<(String, usize, usize, usize)> { let p = s.doc.body.get(i)?.as_para()?; let (k, off, instr) = p.objects_with_offsets().enumerate().find_map(|(k, (off, o))| match o { InlineObject::FieldStart { instr, .. } => { @@ -97,7 +97,7 @@ fn wrapped_list(s: &Session, i: usize) -> Option<(String, usize, usize)> { InlineObject::FieldEnd => { depth = depth.saturating_sub(1); if depth == 0 { - return Some((instr, j, at + wordcraft_doc::para::OBJ.len_utf8())); + return Some((instr, off, j, at + wordcraft_doc::para::OBJ.len_utf8())); } } _ => {} @@ -198,12 +198,13 @@ pub fn update_generated(s: &mut Session) -> Result<(), CmdError> { // field's own paragraph, as WordCraft inserts one, its old entries rebuilt below. // What follows its closing field end, and its first paragraph's properties and section // break, are the document's and stay. - if let Some((instr, last, end)) = wrapped_list(s, i) { + if let Some((instr, start, last, end)) = wrapped_list(s, i) { // The entries between its first and last paragraphs go. for _ in i.saturating_add(1)..last { s.doc.remove_block(StoryRef::Body, &Path::top(i + 1))?; } - // Its first paragraph keeps only the field; its last (or, ending in its first, the + // Its first paragraph keeps what comes before the field (bookmarks, other fields) + // and the field; its last (or, ending in its first, the // same) loses what is up to and through the field end, and goes when nothing is left // of it and it ends no section. let first_end = if last > i { @@ -217,15 +218,15 @@ pub fn update_generated(s: &mut Session) -> Result<(), CmdError> { end }; let p = s.doc.para_mut(StoryRef::Body, &Path::top(i))?; - p.delete(0, first_end)?; + p.delete(start, first_end)?; p.insert_object( - 0, + start, InlineObject::Field { instr, result: String::new(), locked: false, dirty: false, code: false, form: String::new() }, &Default::default(), )?; // What followed the field end in its own paragraph comes after the list, in a // paragraph of its own that takes the section break. - let field = wordcraft_doc::para::OBJ.len_utf8(); + let field = start + wordcraft_doc::para::OBJ.len_utf8(); if p.len() > field { let rest = p.split_off(field)?; s.doc.insert_block(StoryRef::Body, &Path::top(i + 1), Block::Para(rest))?; @@ -432,7 +433,7 @@ mod tests { p.insert_object(n, InlineObject::FieldEnd, &props).unwrap(); } let s = Session::new(d); - assert_eq!(wrapped_list(&s, 0).map(|(instr, last, _)| (instr, last)), Some(("BIBLIOGRAPHY".to_string(), 1))); + assert_eq!(wrapped_list(&s, 0).map(|(instr, _, last, _)| (instr, last)), Some(("BIBLIOGRAPHY".to_string(), 1))); } /// A wrapped list as a session holds it: the field starting paragraph `first`'s text, ending @@ -487,4 +488,35 @@ mod tests { assert_eq!(t.get(1).map(String::as_str), Some(" Then this"), "{t:?}"); assert_eq!(t.last().map(String::as_str), Some("Below")); } + + /// What comes before a wrapped list's field in its first paragraph (a bookmark, an empty + /// `PAGE` field) stays there, before the rebuilt list's field. + #[test] + fn what_comes_before_a_wrapped_lists_field_stays() { + let mut s = wrapped("Old one\nOld two\nBelow", 0, 1, "Old two".len()); + let props = CharProps::default(); + if let Ok(p) = s.doc.para_mut(StoryRef::Body, &Path::top(0)) { + p.insert_object(0, InlineObject::FieldEnd, &props).unwrap(); + p.insert_object(0, InlineObject::FieldStart { instr: "PAGE".into(), locked: false, dirty: false }, &props).unwrap(); + p.insert_object(0, InlineObject::BookmarkStart { name: "Sources".into() }, &props).unwrap(); + } + update_generated(&mut s).unwrap(); + let first = s.doc.body.first().and_then(|b| b.as_para()); + let kinds: Vec<&str> = first + .map(|p| { + p.objects + .iter() + .map(|o| match o { + InlineObject::BookmarkStart { .. } => "bookmark", + InlineObject::FieldStart { instr, .. } if instr == "PAGE" => "page", + InlineObject::FieldEnd => "end", + InlineObject::Field { instr, .. } if instr.starts_with("BIBLIOGRAPHY") => "list", + _ => "other", + }) + .collect() + }) + .unwrap_or_default(); + assert_eq!(kinds, ["bookmark", "page", "end", "list"]); + assert_eq!(texts(&s), ["", "Below"]); + } } diff --git a/crates/engine/tests/it/odt_charts.rs b/crates/engine/tests/it/odt_charts.rs index fbe0e09d41..76292398a4 100644 --- a/crates/engine/tests/it/odt_charts.rs +++ b/crates/engine/tests/it/odt_charts.rs @@ -15,7 +15,8 @@ fn odt() -> Vec { let content = format!( r#"Sales "# ); - let chart = r#"VisitsSiteMonTueWed"#; + // A well-formed chart part, its prefixes declared as LibreOffice writes them. + let chart = r#"VisitsSiteMonTueWed"#; let mut z = zip::ZipWriter::new(std::io::Cursor::new(Vec::new())); let o = zip::write::SimpleFileOptions::default().compression_method(zip::CompressionMethod::Stored); for (name, data) in [("mimetype", "application/vnd.oasis.opendocument.text"), ("content.xml", content.as_str()), ("Object 1/content.xml", chart)] diff --git a/crates/formats/src/model/from_doc.rs b/crates/formats/src/model/from_doc.rs index 230c90eb9b..1a8351cd81 100644 --- a/crates/formats/src/model/from_doc.rs +++ b/crates/formats/src/model/from_doc.rs @@ -494,6 +494,7 @@ fn flow_paras_in(doc: &Document, p: &Paragraph, opts: FlowOpts) -> Vec { let hidden = props.hidden.unwrap_or(false) || props.del.is_some(); // Formats that keep hidden text get it, marked hidden. let hidden_text = (props.hidden.unwrap_or(false) && !opts.chars) || (props.del.is_some() && !tracked); + let hidden_obj = (props.hidden.unwrap_or(false) && !opts.chars) || props.del.is_some(); let mut f = run_fmt(doc, props, in_code, opts); if tracked { (f.ins, f.del) = rev_marks(doc, props); @@ -541,8 +542,9 @@ fn flow_paras_in(doc: &Document, p: &Paragraph, opts: FlowOpts) -> Vec { let obj = p.objects.get(k); k += 1; control.interrupt(obj, &mut out, &f); - // Hidden text, or a hidden drawing: not in the copy. - if hidden || obj.and_then(InlineObject::frame).is_some_and(|(_, _, f)| f.hidden) { + // Hidden text a format leaves out, a deleted object, or a hidden drawing: not in the + // copy (a format keeping hidden text keeps its objects too, as it keeps the text). + if hidden_obj || obj.and_then(InlineObject::frame).is_some_and(|(_, _, f)| f.hidden) { continue; } let from = out.inlines.len(); @@ -1146,7 +1148,9 @@ pub fn from_doc_tracked(doc: &Document) -> Flow { /// [`from_doc`] keeping what `opts` asks for. pub fn from_doc_with(doc: &Document, opts: FlowOpts) -> Flow { - flow_of(doc, opts) + // Paragraphs kept as read are checked against the document's comments and page setup: + // worked out once for the whole document, not once per paragraph. + wordcraft_doc::kept::while_writing(doc, || flow_of(doc, opts)) } /// `blocks` of `doc` (a header, footer or note) as flow blocks, keeping what `opts` asks for. diff --git a/crates/formats/src/odt/tests_more/char_styles.rs b/crates/formats/src/odt/tests_more/char_styles.rs index 628d5d2935..13325dd042 100644 --- a/crates/formats/src/odt/tests_more/char_styles.rs +++ b/crates/formats/src/odt/tests_more/char_styles.rs @@ -71,3 +71,21 @@ fn what_a_character_style_turns_off_stays_off() { let plain = s.split(r#"style:name="Plain""#).nth(1).and_then(|r| r.split("").next()).unwrap_or("missing"); assert!(plain.contains(r#"fo:font-weight="normal""#) && plain.contains(r#"fo:font-style="normal""#), "{plain}"); } + +#[test] +fn a_character_style_based_on_a_missing_one_keeps_its_own_look() { + // "Orphan" names a parent the file doesn't define; "Tilted" comes after it. + let styles = format!( + r#""# + ); + let content = format!( + r#"aa bb"# + ); + let d = import(&package(&content, Some(&styles))).unwrap(); + let r = runs(&d); + assert_eq!( + r.iter().map(|x| (x.0.as_str(), x.1.as_deref(), x.2, x.3)).collect::>(), + [("aa", Some("Orphan"), true, false), ("bb", Some("Tilted"), false, true)] + ); + assert!(d.styles.get("Nowhere").is_none(), "no style for the missing parent"); +} diff --git a/crates/formats/src/odt/tests_more/chars.rs b/crates/formats/src/odt/tests_more/chars.rs index 2c5ebcc48f..46c1bed9c9 100644 --- a/crates/formats/src/odt/tests_more/chars.rs +++ b/crates/formats/src/odt/tests_more/chars.rs @@ -268,3 +268,19 @@ fn emphasis_marks_are_read_and_written() { let back: Vec> = back.body[0].as_para().unwrap().runs.iter().map(|r| r.props.emphasis).collect(); assert_eq!(back, [Some(E::Dot), Some(E::Comma), Some(E::Circle), Some(E::UnderDot), Some(E::Dot), None]); } + +#[test] +fn objects_in_hidden_text_are_kept() { + // Hidden text is written, marked hidden: so are the bookmarks and fields it holds. + let hidden = CharProps { hidden: Some(true), ..Default::default() }; + let mut p = Paragraph::with_text("Shown. Hidden words.", CharProps::default()); + let n = p.len(); + p.format(7, n, &|c| c.hidden = Some(true)).unwrap(); + p.insert_object(14, wordcraft_doc::para::InlineObject::BookmarkStart { name: "Secret".into() }, &hidden).unwrap(); + let mut d = Document::new(); + d.body = vec![para_block(p)]; + let back = import(&export(&d).unwrap()).unwrap(); + let p = back.body[0].as_para().unwrap(); + assert!(p.text.contains("Hidden"), "{:?}", p.text); + assert!(p.objects.iter().any(|o| matches!(o, wordcraft_doc::para::InlineObject::BookmarkStart { name } if name == "Secret")), "{:?}", p.objects); +} diff --git a/crates/layout/src/display/page.rs b/crates/layout/src/display/page.rs index 6c6c9b69b2..007e11a73a 100644 --- a/crates/layout/src/display/page.rs +++ b/crates/layout/src/display/page.rs @@ -133,8 +133,11 @@ fn stories_display( if hidden.as_ref().is_some_and(|h| h.hides(it)) { continue; } + // A line in a clipped cell is drawn in its clip; in a turned text box, with the box's + // other lines (clipped there), so it is drawn once. if let Placed::Lines { story, path, .. } = it && let Some(rect) = cell_clip(clips, *story, &path.0) + && turned_frame(&frames, it).is_none() { let mut inner = Vec::new(); item(doc, it, opts, authors, a, &mut inner); @@ -166,7 +169,15 @@ fn stories_display( if let Some((_, rect, spin)) = frames.get(k) { let mut inner = Vec::new(); for line in items.iter().filter(|i| turned_frame(&frames, i) == Some(k)) { - item(doc, line, opts, authors, a, &mut inner); + if let Placed::Lines { story, path, .. } = line + && let Some(rect) = cell_clip(clips, *story, &path.0) + { + let mut clipped = Vec::new(); + item(doc, line, opts, authors, a, &mut clipped); + inner.push(Draw::Clip { rect, items: clipped }); + } else { + item(doc, line, opts, authors, a, &mut inner); + } } super::objects::spun(*spin, *rect, inner, out); } diff --git a/crates/layout/src/tests/textbox.rs b/crates/layout/src/tests/textbox.rs index e9074a57b4..10054faee7 100644 --- a/crates/layout/src/tests/textbox.rs +++ b/crates/layout/src/tests/textbox.rs @@ -358,3 +358,45 @@ fn a_turned_linked_box_turns_only_its_own_lines() { let items: Vec = l.pages[0].items.iter().flat_map(|it| shown.draws(it, &Default::default())).collect(); assert_eq!(turned_texts(&items), (true, true)); } + +#[test] +fn a_clipped_line_in_a_turned_box_is_drawn_once_turned_and_clipped() { + // Lines in a cell cut off at its edge (an exact-height row) inside a turned text box: drawn + // once, clipped, inside the box's turn (it was drawn upright and clipped, and again turned). + let mut d = Document::from_text("one"); + let text = "Words in the box."; + let float = wordcraft_doc::para::Float { wrap: wordcraft_doc::para::Wrap::Square, x: 20.0, y: 100.0, rot: 30.0, ..Default::default() }; + let b = text_box_with(&mut d, 0, text, 144.0, 200.0, TextBody::default(), float); + let l = lay(&d); + let mut page = l.pages[0].clone(); + let in_box: Vec = page + .items + .iter() + .enumerate() + .filter(|(_, it)| matches!(it, Placed::Lines { story: StoryRef::Part(s), .. } if *s == b)) + .map(|(i, _)| i) + .collect(); + // Its lines taken as ones in a clipped cell (table 5, row 0, cell 0). + let Some(Placed::Lines { path, .. }) = in_box.first().and_then(|&i| page.items.get_mut(i)) else { panic!("the box has no lines") }; + *path = wordcraft_doc::Path(vec![5, 0, 0, 0]); + let rect = Rect::new(0.0, 0.0, 1000.0, 1000.0); + page.items.push(Placed::Cell { + rect, + covered: false, + table: wordcraft_doc::Path(vec![5]), + row: 0, + cell: 0, + story: StoryRef::Part(b), + clip: true, + }); + let draws = display::page_display(&d, &page, &display::DisplayOptions::default()); + let upright_clips = draws.iter().filter(|x| matches!(x, display::Draw::Clip { .. })).count(); + let turned_clips: usize = draws + .iter() + .map(|x| match x { + display::Draw::Rotated { items, .. } => items.iter().filter(|g| matches!(g, display::Draw::Clip { .. })).count(), + _ => 0, + }) + .sum(); + assert_eq!((upright_clips, turned_clips), (0, 1)); +}