From 9a1334e04d99a6b5dd3c432b4ec2a2f8fbdce0a2 Mon Sep 17 00:00:00 2001 From: FredApps <153173854+FredApps@users.noreply.github.com> Date: Sun, 1 Nov 2026 14:47:07 +0100 Subject: [PATCH] fix(edit): preserve image scopes and content ownership Move editor crop scopes with grouped images while retaining fixed author clips and folding repeated transforms. Arrange keeps relocated original artwork in its own stream beside an added item, so later edits or deletion cannot discard it; unusual shared owned streams are refused. Refuse nested marked-content extraction when its tags cannot be preserved. Synthetic regressions and the headless group-move, undo/redo, save/reopen, arrange, delete and render lifecycle fail before these fixes and pass afterward. Co-Authored-By: Codex GPT-6.1 --- crates/automation/tests/it/automation.rs | 38 +++++++++ crates/edit/src/images.rs | 89 +++++++++++++++++++- crates/edit/src/tests.rs | 103 +++++++++++++++++++++++ 3 files changed, 229 insertions(+), 1 deletion(-) diff --git a/crates/automation/tests/it/automation.rs b/crates/automation/tests/it/automation.rs index da3d662248..0abf597b44 100644 --- a/crates/automation/tests/it/automation.rs +++ b/crates/automation/tests/it/automation.rs @@ -10304,3 +10304,41 @@ fn simple_rule_scan_drawn(content: &[u8]) -> Vec { } write_full(&doc, &SaveOptions::default()).unwrap() } + +#[test] +fn editor_crop_group_move_and_arrange_survive_added_item_deletion_through_tools() { + let dir = workdir("editor-crop-group-arrange"); + let mut a = auto(&dir); + let source = ok(&mut a, "doc_open", json!({"path":"a.pdf"}))["doc"].as_u64().unwrap(); + ok(&mut a, "doc_export_images", json!({"doc":source,"folder":"src","dpi":18,"pages":[1]})); + let doc = ok(&mut a, "doc_create", json!({"from":"images","paths":["src/a_page_1.png"]}))["doc"].as_u64().unwrap(); + ok(&mut a, "image_edit", json!({"doc":doc,"page":1,"image":1,"action":"move","rect":[5,5,25,35]})); + ok(&mut a, "image_edit", json!({"doc":doc,"page":1,"image":1,"action":"crop","rect":[10,10,20,30]})); + ok(&mut a, "page_add_text", json!({"doc":doc,"page":1,"text":"Movable label","rect":[2,40,45,55],"size":6})); + let inventory = ok(&mut a, "object_list", json!({"doc":doc,"page":1})); + let objects: Vec<_> = inventory["objects"].as_array().unwrap().iter().map(|o| json!({"kind":o["kind"],"index":o["index"]})).collect(); + assert_eq!(objects.len(), 2); + let moved = ok(&mut a, "object_move", json!({"doc":doc,"page":1,"objects":objects,"offset":[3,4]})); + assert_eq!(moved["moved"], 2); + let picture = ok(&mut a, "page_images", json!({"doc":doc,"page":1}))["images"][0].clone(); + assert_eq!(picture["rect"], json!([8.0, 9.0, 28.0, 39.0])); + assert_eq!(picture["visible_rect"], json!([13.0, 14.0, 23.0, 34.0])); + assert!(page_text(&mut a, doc)[0].contains("Movable label")); + ok(&mut a, "edit_undo", json!({"doc":doc})); + assert_eq!(ok(&mut a, "page_images", json!({"doc":doc,"page":1}))["images"][0]["visible_rect"], json!([10.0, 10.0, 20.0, 30.0])); + ok(&mut a, "edit_redo", json!({"doc":doc})); + ok(&mut a, "doc_save", json!({"doc":doc,"path":"grouped.pdf"})); + let reopened = ok(&mut a, "doc_open", json!({"path":"grouped.pdf"}))["doc"].as_u64().unwrap(); + assert_eq!(ok(&mut a, "page_images", json!({"doc":reopened,"page":1}))["images"][0]["visible_rect"], picture["visible_rect"]); + ok(&mut a, "image_edit", json!({"doc":reopened,"page":1,"image":1,"action":"bring_to_front"})); + assert_eq!(ok(&mut a, "content_list", json!({"doc":reopened}))["count"], 1); + ok(&mut a, "content_delete", json!({"doc":reopened,"page":1,"index":1})); + ok(&mut a, "doc_save", json!({"doc":reopened,"path":"independent.pdf"})); + let saved = ok(&mut a, "doc_open", json!({"path":"independent.pdf"}))["doc"].as_u64().unwrap(); + let after = ok(&mut a, "page_images", json!({"doc":saved,"page":1})); + assert_eq!(after["count"], 1); + assert_eq!(after["images"][0]["rect"], picture["rect"]); + assert_eq!(after["images"][0]["visible_rect"], picture["visible_rect"]); + let rendered = a.call("page_render", &json!({"doc":saved,"page":1,"dpi":72})).unwrap(); + assert!(rendered.iter().any(|c| matches!(c, Content::Png {data,width,height} if !data.is_empty() && *width > 0 && *height > 0))); +} diff --git a/crates/edit/src/images.rs b/crates/edit/src/images.rs index fd2cce8612..4b8f619b48 100644 --- a/crates/edit/src/images.rs +++ b/crates/edit/src/images.rs @@ -1221,6 +1221,18 @@ fn arrange_image( // a new structure element and MCID, so we refuse to rearrange it to avoid leaving the image untagged. // If the sequence holds only this image, the whole marked unit is moved intact. let marked_unit = ops.get(target_unit.start).is_some_and(|o| o.is("BDC") || o.is("BMC")); + let marked_depth = ops.iter().take(target).fold(0usize, |depth, op| { + if op.is("BDC") || op.is("BMC") { + depth.saturating_add(1) + } else if op.is("EMC") { + depth.saturating_sub(1) + } else { + depth + } + }); + if marked_depth > usize::from(marked_unit) { + return Err(EditError::Invalid("cannot rearrange an image out of a nested marked-content sequence".into())); + } if marked_unit && has_other_paint { return Err(EditError::Invalid("cannot rearrange an image that shares a marked-content sequence with other content".into())); } @@ -1234,6 +1246,25 @@ fn arrange_image( (ops.get(dest_unit.start).ok_or_else(missing)?.span.start, dest_unit.start) }; + // An added item owns its entire stream: editing or deleting it replaces that stream. + // Insert ordinary artwork next to the complete owned unit in a separate stream instead. + let destination_piece = if insert_after { + dest_unit.end.checked_sub(1).and_then(|i| ops.get(i)).map(|op| pieces.piece_of(op.span.end.saturating_sub(1))) + } else { + ops.get(dest_unit.start).map(|op| pieces.piece_of(op.span.start)) + }; + let owned_destination = destination_piece + .filter(|piece| all.get(*piece).is_some_and(|(obj, _)| doc.resolve(obj).as_dict().is_some_and(|d| d.get(b"PCAdded").is_some()))); + if let Some(piece) = owned_destination { + let whole_unit = ops.iter().enumerate().all(|(index, op)| { + let (first, last) = pieces.pieces_of(op); + if dest_unit.contains(&index) { first == piece && last == piece } else { piece < first || piece > last } + }); + if !whole_unit || img.stream == piece { + return Err(EditError::Invalid("cannot arrange an image inside an added item's content stream".into())); + } + } + let (leaking_ops, has_leaking_clip) = scan_leaking_ops(&ops, rem_start, rem_end); let res = p.dict.get(b"Resources").map(|r| doc.resolve(r)).and_then(|r| r.as_dict().cloned()).unwrap_or_default(); @@ -1390,6 +1421,23 @@ fn arrange_image( Vec::new() }; + if let Some(piece) = owned_destination { + let changed = pieces.splice([(target_span, removal_replacement)]); + let contents = crate::text::rewritten_contents(doc, all, changed); + let mut list = match contents { + Object::Array(list) => list, + one => vec![one], + }; + let insertion = piece.saturating_add(usize::from(insert_after)); + if insertion > list.len() { + return Err(EditError::Invalid("the destination content stream is missing".into())); + } + let relocated = doc.add(Object::Stream(pdfcraft_cos::Stream::flate(Dict::new(), &serialized))); + list.insert(insertion, Object::Ref(relocated)); + doc.update_dict(p.obj, |d| d.set(b"Contents".to_vec(), Object::Array(list)))?; + return Ok(()); + } + let mut edits = Vec::new(); if dest_pos >= target_span.end { edits.push((target_span, removal_replacement)); @@ -1813,6 +1861,38 @@ pub(crate) fn translate_images(doc: &mut Document, page: usize, indexes: &[usize } // Images alone in their own `q re W n cm Do Q` take the clip with them. let mut clips = std::collections::BTreeMap::new(); + let mut crop_starts = std::collections::BTreeMap::new(); + let mut crop_ends = std::collections::BTreeSet::new(); + let mut folds = std::collections::BTreeMap::new(); + for image in images.iter().filter(|image| image.has_editor_crop()) { + let Some(&(_, _, offset)) = plans.get(&image.op) else { continue }; + let Some((start, end, placement)) = image_scopes(&ops, image.op, "moved")?.transform_scope(image.op) else { + return Err(EditError::Invalid("the image crop cannot be moved independently".into())); + }; + let inverse = placement.invert().ok_or_else(|| EditError::Invalid("a selected image has no area".into()))?; + let local = placement.then(&Matrix::translate(offset[0], offset[1])).then(&inverse); + if !local.0.iter().all(|v| v.is_finite() && v.abs() <= 1e12) { + return Err(EditError::Invalid("the image move is too large".into())); + } + let leading_transform = || { + let at = start.checked_add(1)?; + if start >= end || !ops.get(start)?.is("q") || !ops.get(at)?.is("cm") { + return None; + } + ops.get(at)?.nums::<6>().map(|m| (at, Matrix(m))) + }; + if let Some((at, cm)) = enclosing_transform(&ops, start, end).or_else(leading_transform) { + let folded = local.then(&cm); + if !folded.0.iter().all(|v| v.is_finite() && v.abs() <= 1e12) { + return Err(EditError::Invalid("the image move is too large".into())); + } + folds.insert(at, folded); + } else { + crop_starts.insert(start, local); + crop_ends.insert(end); + } + plans.remove(&image.op); + } for index in 0..ops.len() { let Some(&(_, placement, offset)) = plans.get(&index) else { continue }; let Some((at, cm)) = exclusive_clip(&ops, index) else { continue }; @@ -1833,7 +1913,6 @@ pub(crate) fn translate_images(doc: &mut Document, page: usize, indexes: &[usize }; } // An image already alone in `q cm Do Q` moves by changing that `cm`, not by nesting another. - let mut folds = std::collections::BTreeMap::new(); for index in 0..ops.len() { let Some(&(movement, _, _)) = plans.get(&index) else { continue }; let Some((at, cm)) = enclosing_transform(&ops, index, index) else { continue }; @@ -1848,6 +1927,14 @@ pub(crate) fn translate_images(doc: &mut Document, page: usize, indexes: &[usize return Err(EditError::Invalid("a selected image operation is missing".into())); } let data = crate::text::splice(&pieces, &ops, |index| { + if let Some(matrix) = crop_starts.remove(&index) { + return (vec![Op::new("q", vec![]), Op::new("cm", matrix.0.map(Object::Real).to_vec())], true); + } + if crop_ends.remove(&index) { + let mut out: Vec<_> = ops.get(index).cloned().into_iter().collect(); + out.push(Op::new("Q", vec![])); + return (out, false); + } if let Some((c, keep)) = clips.remove(&index) { return (vec![Op::new("cm", c.0.map(Object::Real).to_vec())], keep); } diff --git a/crates/edit/src/tests.rs b/crates/edit/src/tests.rs index 6d3f5da2b6..2e4b8ebbde 100644 --- a/crates/edit/src/tests.rs +++ b/crates/edit/src/tests.rs @@ -10066,3 +10066,106 @@ fn arrange_restores_initial_rendering_intent_from_ri_and_gs() { ); } } + +#[test] +fn editor_crop_moves_with_repeated_group_translation() { + let mut doc = image_page(); + let caption_rect = text::text_lines(&doc, 0).unwrap()[0].rect; + crop(&mut doc, 0, 0, [150.0, 120.0, 250.0, 170.0]).unwrap(); + let start = images::page_images(&doc, 0).unwrap()[0].clone(); + let mut count = 0; + for iteration in 0..20 { + images::translate_images(&mut doc, 0, &[0], [1.0, -2.0]).unwrap(); + let ops = pdfcraft_content::parse(&page_content_bytes(&doc, 0)).ops; + if iteration == 0 { + count = ops.len(); + } + assert_eq!(ops.len(), count, "repeated moves fold the same transform"); + } + let shifted = |r: [f64; 4]| [r[0] + 20.0, r[1] - 40.0, r[2] + 20.0, r[3] - 40.0]; + let doc = reopen(&doc); + let after = &images::page_images(&doc, 0).unwrap()[0]; + assert!(close(after.rect, shifted(start.rect))); + assert!( + close(after.visible_rect.unwrap(), shifted(start.visible_rect.unwrap())), + "the editor crop moves with its pixels: {:?}", + after.visible_rect + ); + assert_eq!(text::text_lines(&doc, 0).unwrap()[0].text, "Caption"); + assert_eq!(text::text_lines(&doc, 0).unwrap()[0].rect, caption_rect); +} + +#[test] +fn arranging_original_image_keeps_it_independent_of_added_items() { + for update in [false, true] { + let mut doc = image_page(); + let caption_rect = text::text_lines(&doc, 0).unwrap()[0].rect; + add_content( + &mut doc, + 0, + &Content::Text(AddedText { rect: [20.0, 250.0, 250.0, 290.0], text: "Added caption".into(), ..AddedText::default() }), + ) + .unwrap(); + arrange(&mut doc, 0, 0, images::ArrangeOrder::BringToFront).unwrap(); + let mut doc = reopen(&doc); + assert_eq!(list_added(&doc).len(), 1, "the added item stays editable"); + if update { + update_content( + &mut doc, + 0, + 0, + &Content::Text(AddedText { rect: [20.0, 250.0, 250.0, 290.0], text: "Updated caption".into(), ..AddedText::default() }), + ) + .unwrap(); + } else { + delete_content(&mut doc, 0, 0).unwrap(); + } + let doc = reopen(&doc); + assert_eq!(images::page_images(&doc, 0).unwrap().len(), 1, "editing/deleting an added item must preserve arranged original artwork"); + assert_eq!(images::page_images(&doc, 0).unwrap()[0].rect, [100.0, 100.0, 300.0, 200.0]); + let text = text::text_lines(&doc, 0).unwrap().into_iter().map(|line| line.text).collect::>(); + assert!(text.iter().any(|line| line == "Caption"), "the original caption survives"); + assert_eq!(text.iter().any(|line| line == "Updated caption"), update); + assert_eq!(text::text_lines(&doc, 0).unwrap().iter().find(|line| line.text == "Caption").unwrap().rect, caption_rect); + } +} + +#[test] +fn arranging_nested_marked_image_refuses_without_losing_tags() { + for marked in ["/Figure <> BDC", "/Figure BMC"] { + let mut doc = image_page_with(&format!( + "q {marked} 100 0 0 100 50 50 cm /Im0 Do EMC BT /F1 12 Tf 20 700 Td (Text) Tj ET Q BT /F1 12 Tf 20 720 Td (Outside) Tj ET" + )); + let before = page_content_bytes(&doc, 0); + let error = arrange(&mut doc, 0, 0, images::ArrangeOrder::BringToFront).unwrap_err(); + assert!(error.to_string().contains("marked-content"), "{error}"); + assert_eq!(page_content_bytes(&doc, 0), before); + assert_eq!(page_content_bytes(&reopen(&doc), 0), before); + } +} + +#[test] +fn editor_crop_group_move_keeps_an_outer_author_clip_fixed() { + let mut doc = image_page_with("0 0 220 300 re W n q 200 0 0 100 100 100 cm /Im0 Do Q BT /F1 12 Tf 10 250 Td (Caption) Tj ET"); + crop(&mut doc, 0, 0, [150.0, 120.0, 210.0, 170.0]).unwrap(); + let mut operators = 0; + for i in 0..20 { + images::translate_images(&mut doc, 0, &[0], [1.0, -2.0]).unwrap(); + let count = pdfcraft_content::parse(&page_content_bytes(&doc, 0)).ops.len(); + if i == 0 { + operators = count; + } + assert_eq!(count, operators, "moves under an inherited clip must fold their transform"); + } + let doc = reopen(&doc); + let image = &images::page_images(&doc, 0).unwrap()[0]; + assert!(close(image.rect, [120.0, 60.0, 320.0, 160.0])); + assert!( + close(image.visible_rect.unwrap(), [170.0, 80.0, 220.0, 130.0]), + "the editor crop moves but its author's outer clip stays fixed: {:?}", + image.visible_rect + ); + let bytes = page_content_bytes(&doc, 0); + assert!(String::from_utf8_lossy(&bytes).starts_with("0 0 220 300 re W n")); + assert_eq!(text::text_lines(&doc, 0).unwrap()[0].text, "Caption"); +}