From eca16e6bd50f6261e9af557809eb842d199a56fe Mon Sep 17 00:00:00 2001 From: alehel Date: Sun, 18 Oct 2026 16:24:14 +0200 Subject: [PATCH] fix(edit): backdrop copies keep the stream dictionary and every slot Review of #2815 (CodeRabbit): - A content stream whose bytes live in an external file (`/F`) draws those, not its inline bytes, so it is never taken for a backdrop. - The copy keeps the original stream dictionary, unknown entries included, less `Length` and `DL` (Flate re-encodes it); it had started from an empty dictionary. - Every `/Contents` slot that lists the original stream gets the copy: a page listing the same stream twice painted the white fill again over the background. - The Background dialog starts with replacing on each time it opens. Update restores backdrops before re-applying, so a replaced page shows no fill and no checkbox, and a choice left off from an earlier dialog hid the background again unseen. Co-Authored-By: Claude Opus 5.5 --- crates/edit/src/backdrop.rs | 29 +++++++++++--- crates/edit/src/backdrop_tests.rs | 52 ++++++++++++++++++++++++++ crates/ui-egui/src/commands.rs | 1 + crates/ui-egui/src/lib.rs | 3 ++ crates/ui-egui/src/marks_ui.rs | 12 ++++++ crates/ui-egui/tests/ui_group/marks.rs | 16 +++++--- 6 files changed, 102 insertions(+), 11 deletions(-) diff --git a/crates/edit/src/backdrop.rs b/crates/edit/src/backdrop.rs index 967a8f9121..86924b8cb7 100644 --- a/crates/edit/src/backdrop.rs +++ b/crates/edit/src/backdrop.rs @@ -80,7 +80,7 @@ pub(crate) fn replace(doc: &mut Document, page: &pdfcraft_model::Page) -> Result let Some((Backdrop::Fill, Some(found))) = scan(doc, page) else { return Ok(false) }; let edited = found.pieces.splice([(found.op.clone(), b"n".to_vec())]); let piece = found.pieces.piece_of(found.op.start); - let (Some(&(slot, original)), Some(bytes)) = (found.refs.get(piece), edited.get(piece)) else { return Ok(false) }; + let (Some(&(_, original)), Some(bytes)) = (found.refs.get(piece), edited.get(piece)) else { return Ok(false) }; // Where the operator sits in that piece: the joined offset less the pieces before it. let before: usize = (0..piece).filter_map(|i| edited.get(i)).map(|p| p.len() + 1).sum(); let at = found.op.start.saturating_sub(before); @@ -89,14 +89,27 @@ pub(crate) fn replace(doc: &mut Document, page: &pdfcraft_model::Page) -> Result note.set(b"Original".to_vec(), Object::Ref(original)); note.set(b"At".to_vec(), Object::Int(i64::try_from(at).unwrap_or(i64::MAX))); note.set(b"Len".to_vec(), Object::Int(i64::try_from(len).unwrap_or(i64::MAX))); - let mut dict = Dict::new(); + // The copy keeps the original's dictionary (unknown entries included) less what describes + // the old encoding; `scan` refused streams whose bytes live in an external file (`/F`). + let Object::Stream(source) = &*doc.get(original) else { return Ok(false) }; + let mut dict = source.dict.clone(); + for key in [&b"Length"[..], b"DL", b"PCBackdrop"] { + dict.remove(key); + } dict.set(b"PCBackdrop".to_vec(), Object::Dict(note)); let copy = doc.add(Object::Stream(Stream::flate(dict, bytes))); + // Every slot that draws the original: a page may list the same stream twice, and a second + // copy of the fill would cover the background again. let mut list = crate::contents(doc, page)?; - let Some(entry) = list.get_mut(slot) else { return Ok(false) }; - *entry = Object::Ref(copy); - doc.update_dict(page.obj, |d| d.set(b"Contents".to_vec(), Object::Array(list)))?; - Ok(true) + let mut replaced = false; + for entry in list.iter_mut().filter(|o| matches!(o, Object::Ref(r) if *r == original)) { + *entry = Object::Ref(copy); + replaced = true; + } + if replaced { + doc.update_dict(page.obj, |d| d.set(b"Contents".to_vec(), Object::Array(list)))?; + } + Ok(replaced) } /// Put back the original content streams [`replace`] copied on `page`, where the copy is still @@ -158,6 +171,10 @@ fn scan(doc: &Document, p: &pdfcraft_model::Page) -> Option<(Backdrop, Option>".to_string(), + "<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 300 200] >>".into(), + "<< /Type /Page /Parent 2 0 R /Contents [4 0 R] >>".into(), + stream("/F (content.bin)", &format!("1 g 0 0 300 200 re f {TEXT}")), + ]; + let mut doc = build(&objs); + assert_eq!(page_backdrop(&doc, 0).unwrap(), None, "the inline bytes are not what the page draws"); + let before = streams(&doc); + add_background(&mut doc, &[0], &red(), false).unwrap(); + assert_eq!(streams(&doc)[1..], before[..]); +} + +#[test] +fn the_backdrop_copy_keeps_the_original_dictionary_less_its_old_encoding() { + let objs = vec![ + "<< /Type /Catalog /Pages 2 0 R >>".to_string(), + "<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 300 200] >>".into(), + "<< /Type /Page /Parent 2 0 R /Contents [4 0 R] >>".into(), + stream("/Vendor (keep me) /DL 9999", &format!("1 g 0 0 300 200 re f {TEXT}")), + ]; + let mut doc = build(&objs); + add_background(&mut doc, &[0], &red(), false).unwrap(); + let doc = reopen(&doc); + let p = &pdfcraft_model::pages(&doc)[0]; + let Some(Object::Array(list)) = p.dict.get(b"Contents").map(|c| (*doc.resolve(c)).clone()) else { panic!("contents") }; + let Object::Stream(copy) = &*doc.resolve(&list[1]) else { panic!("a stream") }; + assert!(copy.dict.get(b"Vendor").is_some(), "unknown entries survive"); + assert!(copy.dict.get(b"DL").is_none(), "the old decoded length no longer holds"); + assert!(copy.dict.get(b"PCBackdrop").is_some()); + assert_eq!(String::from_utf8_lossy(©.decoded().unwrap()), format!("1 g 0 0 300 200 re n {TEXT}")); +} + +#[test] +fn a_stream_listed_twice_is_replaced_in_both_slots_and_restored_in_both() { + let mut doc = build(&[ + "<< /Type /Catalog /Pages 2 0 R >>".to_string(), + "<< /Type /Pages /Kids [3 0 R] /Count 1 /MediaBox [0 0 300 200] >>".into(), + "<< /Type /Page /Parent 2 0 R /Contents [4 0 R 4 0 R] >>".into(), + stream("", &format!("q 1 g 0 0 300 200 re f Q {TEXT}")), + ]); + let original = streams(&doc); + add_background(&mut doc, &[0], &red(), false).unwrap(); + let s = streams(&doc); + assert_eq!(s.len(), 3, "{s:?}"); + assert!(s[1..].iter().all(|c| c.contains("re n") && !c.contains("re f")), "no slot still paints the white fill: {s:?}"); + remove_marks(&mut doc, &[0], MarkKind::Background).unwrap(); + assert_eq!(streams(&doc), original); +} diff --git a/crates/ui-egui/src/commands.rs b/crates/ui-egui/src/commands.rs index 832ce7cfbd..d32aedeef9 100644 --- a/crates/ui-egui/src/commands.rs +++ b/crates/ui-egui/src/commands.rs @@ -371,6 +371,7 @@ impl PdfCraftApp { K::Background }; self.marks_draft.replace = id.ends_with(".update"); + self.marks_draft.opened(kind); self.dialog = Some(Dialog::Marks(kind)); } "edit.bates" => { diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 725bcad4b0..3e817d2717 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -2557,6 +2557,9 @@ impl PdfCraftApp { } "none" => None, _ => Some(Dialog::About), + }; + if let Some(Dialog::Marks(kind)) = self.dialog { + self.marks_draft.opened(kind); } } ("tools", _) => self.all_tools_expanded = value != "collapsed", diff --git a/crates/ui-egui/src/marks_ui.rs b/crates/ui-egui/src/marks_ui.rs index c838f7861e..a555b437e2 100644 --- a/crates/ui-egui/src/marks_ui.rs +++ b/crates/ui-egui/src/marks_ui.rs @@ -136,6 +136,18 @@ impl BackdropCheck { } } +impl MarksDraft { + /// The dialog for `kind` is opening. Other settings carry over from last time, but replacing + /// a page's own backdrop starts on: Update restores backdrops before it re-applies, so a + /// page whose backdrop was replaced shows none and the checkbox stays hidden, and a choice + /// left off from an earlier dialog would hide the background again unseen. + pub fn opened(&mut self, kind: MarkKind) { + if kind == MarkKind::Background { + self.bg.replace_backdrop = true; + } + } +} + /// "1 page" or "{n} pages", in a message. fn pages_phrase(n: usize, one: &str, many: &str) -> String { if n == 1 { one.to_string() } else { crate::i18n::fmt(many, &[("n", &n.to_string())]) } diff --git a/crates/ui-egui/tests/ui_group/marks.rs b/crates/ui-egui/tests/ui_group/marks.rs index 3a9864ea47..20b8a41a19 100644 --- a/crates/ui-egui/tests/ui_group/marks.rs +++ b/crates/ui-egui/tests/ui_group/marks.rs @@ -148,18 +148,24 @@ fn the_background_dialog_offers_to_replace_a_white_backdrop_and_warns_about_scan assert_eq!(backdrop(&h, 0), Some(pdfcraft_engine::Backdrop::Fill)); assert!(h.state_mut().execute("edit.undo")); h.run_steps(2); - // Ticked, it is replaced; the scan is reported, not touched. + // Reopened, the choice starts on again; ticked, the backdrop is replaced and the scan is + // reported, not touched. h.state_mut().execute("edit.background"); h.run_steps(3); - h.get_by_label(replace).click(); - h.run_steps(2); - assert!(h.state().marks_draft.bg.replace_backdrop); + h.get_by_label(replace); + assert!(h.state().marks_draft.bg.replace_backdrop, "on again when the dialog opens"); h.get_by_label("OK").click(); h.run_steps(3); assert_eq!(backdrop(&h, 0), None); assert_eq!(backdrop(&h, 1), Some(pdfcraft_engine::Backdrop::Image)); - // Nothing left to replace: the offer is gone. + // Nothing left to replace: the offer is gone. A choice left off from an earlier dialog + // doesn't carry into Update, which restores the backdrop first and would otherwise keep it + // with no checkbox shown. + h.state_mut().marks_draft.bg.replace_backdrop = false; h.state_mut().execute("edit.background.update"); h.run_steps(3); assert!(h.query_by_label(replace).is_none()); + h.get_by_label("OK").click(); + h.run_steps(3); + assert_eq!(backdrop(&h, 0), None, "Update keeps the backdrop replaced"); }