From e4476b007dc3665735672bab2f7774c69b12be0f Mon Sep 17 00:00:00 2001 From: Combiler01 Date: Fri, 23 Oct 2026 13:08:57 +0530 Subject: [PATCH] Fix:Added Image don't follow the Refactored Edit_image --- crates/edit/src/images.rs | 145 ++++++++++++++++++++++++------------- crates/edit/src/tests.rs | 43 ++++++++--- crates/engine/src/lib.rs | 128 ++++++++++++-------------------- crates/engine/src/tests.rs | 16 ++++ 4 files changed, 189 insertions(+), 143 deletions(-) diff --git a/crates/edit/src/images.rs b/crates/edit/src/images.rs index 058d94e08c..723c79a8c0 100644 --- a/crates/edit/src/images.rs +++ b/crates/edit/src/images.rs @@ -986,49 +986,6 @@ pub fn arrange_image(doc: &mut Document, page: usize, index: usize, order: Arran let Some(target_idx) = units.iter().position(|u| u.start <= target && target < u.end) else { return Err(EditError::Invalid("the image content unit could not be located".into())); }; - if units.len() <= 1 { - return Err(EditError::Invalid("the page has only one content unit to arrange".into())); - } - - let (dest_unit, insert_after) = match order { - ArrangeOrder::BringToFront => { - if target_idx == units.len().saturating_sub(1) { - return Err(EditError::Invalid("the image is already in front".into())); - } - let Some(u) = units.last() else { - return Err(EditError::Invalid("the image is already in front".into())); - }; - (u, true) - } - ArrangeOrder::SendToBack => { - if target_idx == 0 { - return Err(EditError::Invalid("the image is already at the back".into())); - } - let Some(u) = units.first() else { - return Err(EditError::Invalid("the image is already at the back".into())); - }; - (u, false) - } - ArrangeOrder::BringForward => { - if target_idx == units.len().saturating_sub(1) { - return Err(EditError::Invalid("the image is already in front".into())); - } - let Some(u) = units.get(target_idx.saturating_add(1)) else { - return Err(EditError::Invalid("the image is already in front".into())); - }; - (u, true) - } - ArrangeOrder::SendBackward => { - if target_idx == 0 { - return Err(EditError::Invalid("the image is already at the back".into())); - } - let Some(u) = target_idx.checked_sub(1).and_then(|i| units.get(i)) else { - return Err(EditError::Invalid("the image is already at the back".into())); - }; - (u, false) - } - }; - let Some(target_unit) = units.get(target_idx) else { return Ok(()) }; let has_other_paint = ops.iter().enumerate().any(|(i, op)| { i >= target_unit.start @@ -1039,6 +996,68 @@ pub fn arrange_image(doc: &mut Document, page: usize, index: usize, order: Arran b"Do" | b"Tj" | b"TJ" | b"'" | b"\"" | b"sh" | b"BI" | b"S" | b"s" | b"f" | b"F" | b"f*" | b"B" | b"B*" | b"b" | b"b*" ) }); + if units.len() <= 1 && !has_other_paint { + return Err(EditError::Invalid("the page has only one content unit to arrange".into())); + } + + let (dest_unit, insert_after) = match order { + ArrangeOrder::BringToFront => { + if target_idx == units.len().saturating_sub(1) { + if has_other_paint { + (target_unit, true) + } else { + return Err(EditError::Invalid("the image is already in front".into())); + } + } else { + let Some(u) = units.last() else { + return Err(EditError::Invalid("the image is already in front".into())); + }; + (u, true) + } + } + ArrangeOrder::SendToBack => { + if target_idx == 0 { + if has_other_paint { + (target_unit, false) + } else { + return Err(EditError::Invalid("the image is already at the back".into())); + } + } else { + let Some(u) = units.first() else { + return Err(EditError::Invalid("the image is already at the back".into())); + }; + (u, false) + } + } + ArrangeOrder::BringForward => { + if target_idx == units.len().saturating_sub(1) { + if has_other_paint { + (target_unit, true) + } else { + return Err(EditError::Invalid("the image is already in front".into())); + } + } else { + let Some(u) = units.get(target_idx.saturating_add(1)) else { + return Err(EditError::Invalid("the image is already in front".into())); + }; + (u, true) + } + } + ArrangeOrder::SendBackward => { + if target_idx == 0 { + if has_other_paint { + (target_unit, false) + } else { + return Err(EditError::Invalid("the image is already at the back".into())); + } + } else { + let Some(u) = target_idx.checked_sub(1).and_then(|i| units.get(i)) else { + return Err(EditError::Invalid("the image is already at the back".into())); + }; + (u, false) + } + } + }; // A marked-content sequence that holds other content too moves only the image out of the // sequence, wrapping it in its own tag at the destination so it is not grouped with the @@ -1085,6 +1104,19 @@ pub fn arrange_image(doc: &mut Document, page: usize, index: usize, order: Arran (op.span.start, dest_unit.start) }; + let mut q_depth = 0usize; + let mut leaking_non_ctm_ops = Vec::new(); + for op in ops.get(rem_start..rem_end).unwrap_or_default() { + match op.op.as_slice() { + b"q" => q_depth = q_depth.saturating_add(1), + b"Q" => q_depth = q_depth.saturating_sub(1), + b"rg" | b"RG" | b"g" | b"G" | b"k" | b"K" | b"w" | b"J" | b"j" | b"M" | b"d" | b"ri" | b"i" | b"gs" | b"cs" | b"CS" if q_depth == 0 => { + leaking_non_ctm_ops.push(op.clone()); + } + _ => {} + } + } + let res = p.dict.get(b"Resources").map(|r| doc.resolve(r)).and_then(|r| r.as_dict().cloned()).unwrap_or_default(); let ext_gstates = res.get(b"ExtGState").map(|g| doc.resolve(g)).and_then(|g| g.as_dict().cloned()).unwrap_or_default(); @@ -1201,7 +1233,7 @@ pub fn arrange_image(doc: &mut Document, page: usize, index: usize, order: Arran } let mut unit_ops = Vec::new(); - let marked_open = if marked_unit { ops.get(target_unit.start).cloned() } else { None }; + let marked_open = if marked_unit && !has_other_paint { ops.get(target_unit.start).cloned() } else { None }; if let Some(open) = &marked_open { unit_ops.push(open.clone()); } @@ -1265,8 +1297,12 @@ pub fn arrange_image(doc: &mut Document, page: usize, index: usize, order: Arran serialized.push(b'\n'); let leaking = if let Some(inv) = ctm_before_removal.invert() { ctm_after_removal.then(&inv) } else { Matrix::IDENTITY }; - let removal_replacement = if leaking != Matrix::IDENTITY { - let leak_ops = vec![Op::new("cm", leaking.0.map(pdfcraft_content::num).to_vec())]; + let mut leak_ops = Vec::new(); + if leaking != Matrix::IDENTITY { + leak_ops.push(Op::new("cm", leaking.0.map(pdfcraft_content::num).to_vec())); + } + leak_ops.extend(leaking_non_ctm_ops); + let removal_replacement = if !leak_ops.is_empty() { let mut s = Vec::new(); s.push(b'\n'); s.extend_from_slice(&serialize_ops(&leak_ops)); @@ -1710,18 +1746,23 @@ fn crop_image_impl( .chain([Op::new("h", vec![]), Op::new("W", vec![]), Op::new("n", vec![])]) .collect() }; - // The image's own earlier crop (a clipping path right after the scope's `q`) gives way to - // the new one, so a crop can grow again as well as shrink. + // The image's own earlier crop (a clipping path right after the scope's `q`, skipping any + // initial `cm` view transform) gives way to the new one, so a crop can grow again as well as shrink. + let mut clip_start = start + 1; + while ops.get(clip_start).is_some_and(|o| o.is("cm")) { + clip_start += 1; + } let old_clip: Option> = (start != end) .then(|| { - let mut k = start + 1; + let mut k = clip_start; while ops.get(k).is_some_and(|o| matches!(o.op.as_slice(), b"m" | b"l" | b"c" | b"v" | b"y" | b"h" | b"re")) { k += 1; } - (k > start + 1 && ops.get(k).is_some_and(|o| matches!(o.op.as_slice(), b"W" | b"W*")) && ops.get(k + 1).is_some_and(|o| o.is("n"))) - .then_some(start + 1..k + 2) + (k > clip_start && ops.get(k).is_some_and(|o| matches!(o.op.as_slice(), b"W" | b"W*")) && ops.get(k + 1).is_some_and(|o| o.is("n"))) + .then_some(clip_start..k + 2) }) .flatten(); + let insert_at = old_clip.as_ref().map_or(clip_start, |r| r.start); let changed = crate::text::splice(&pieces, &ops, |index| { if start == end && index == target { // No scope of its own yet: one around the drawing, with the crop inside. @@ -1730,7 +1771,7 @@ fn crop_image_impl( out.extend(ops.get(index).cloned()); out.push(Op::new("Q", vec![])); (out, false) - } else if start != end && index == start + 1 { + } else if start != end && index == insert_at { // Inside the scope's `q`, before whatever it draws (in place of its old crop). (clip.clone(), old_clip.is_none()) } else if old_clip.as_ref().is_some_and(|r| r.contains(&index)) { diff --git a/crates/edit/src/tests.rs b/crates/edit/src/tests.rs index ec6cda76af..4e0c056bab 100644 --- a/crates/edit/src/tests.rs +++ b/crates/edit/src/tests.rs @@ -2852,8 +2852,8 @@ fn image_arrange_moves_a_bmc_sequence_as_a_whole() { assert!(body < art && art < img && img < emc, "text first, then the whole artifact sequence: {s}"); } -/// An image that shares its marked-content sequence with other content keeps its tag and properties -/// (e.g. /Figure with MCID) when arranged, keeping the remaining content in its original tag. +/// An image that shares its marked-content sequence with other content keeps the remaining +/// content in its original tag, while moving only the image without duplicating the MCID. #[test] fn image_arrange_splits_a_shared_marked_sequence_into_own_tag() { let content = @@ -2867,17 +2867,15 @@ fn image_arrange_splits_a_shared_marked_sequence_into_own_tag() { let caption = s.find("(Caption)").expect("caption preserved"); let emc1 = s.find("EMC").expect("first EMC"); assert!(fig < caption && caption < emc1, "caption stays in Figure sequence: {s}"); - // The moved image is placed after Body, wrapped in its own sequence retaining the Figure tag and MCID. + // The moved image is placed after Body, without duplicating the structural MCID tag (ISO 32000-1 ยง 14.7.4.2). let body = s.find("(Body)").expect("body text"); - let fig2 = s.rfind("/Figure <> BDC").expect("image retains Figure tag and MCID"); let img = s.find("/Im0 Do").expect("image moved"); - let emc2 = s.rfind("EMC").expect("second EMC"); - assert!(emc1 < body && body < fig2 && fig2 < img && img < emc2, "image moved after body in own tag: {s}"); - assert_ne!(fig, fig2); + assert!(emc1 < body && body < img, "image moved after body: {s}"); + assert_eq!(s.matches("/Figure <> BDC").count(), 1, "MCID must not be duplicated"); assert_eq!(images::page_images(&doc, 0).unwrap().len(), 1); } -/// Send backward also splits out the image while keeping its tag when it shares its sequence with text, +/// Send backward also splits out the image without duplicating the MCID when it shares its sequence with text, /// placing it before the preceding unit and keeping the remaining content tagged. #[test] fn image_arrange_send_backward_splits_shared_marked_sequence() { @@ -2891,11 +2889,34 @@ fn image_arrange_send_backward_splits_shared_marked_sequence() { let img = s.find("/Im0 Do").expect("image"); let cell = s.find("(Cell)").expect("cell text"); assert!(img < header && header < cell, "image sent backward before header, while cell stays: {s}"); - let table = s.rfind("/Table <> BDC").expect("table tag preserved"); - let table_first = s.find("/Table <> BDC").expect("moved image table tag"); + let table = s.find("/Table <> BDC").expect("table tag preserved"); let emc = s.rfind("EMC").expect("EMC"); - assert_ne!(table_first, table); assert!(table < cell && cell < emc, "cell text stays in table sequence: {s}"); + assert_eq!(s.matches("/Table <> BDC").count(), 1, "MCID must not be duplicated"); +} + +#[test] +fn arrange_preserves_color_leaking_out_of_marked_content() { + let content = "/Figure <> BDC 1 0 0 rg q 100 0 0 100 0 0 cm /Im0 Do Q EMC BT /F1 12 Tf 72 700 Td (Text) Tj ET"; + let mut doc = image_page_with(content); + let res = images::arrange_image(&mut doc, 0, 0, images::ArrangeOrder::BringToFront); + if res.is_ok() { + let bytes = page_content_bytes(&doc, 0); + let s = String::from_utf8_lossy(&bytes); + let text_pos = s.find("(Text) Tj").unwrap(); + assert!(s[..text_pos].contains("1 0 0 rg") || s[..text_pos].contains("1 0 0 RG")); + } +} + +#[test] +fn arrange_shared_tagged_does_not_duplicate_mcid() { + let content = "/Figure <> BDC q 100 0 0 100 0 0 cm /Im0 Do Q 0 0 m 10 10 l S EMC"; + let mut doc = image_page_with(content); + images::arrange_image(&mut doc, 0, 0, images::ArrangeOrder::SendToBack).unwrap(); + let bytes = page_content_bytes(&doc, 0); + let s = String::from_utf8_lossy(&bytes); + let mcid_count = s.matches("/MCID 0").count(); + assert_eq!(mcid_count, 1, "MCID 0 must not be duplicated in the content stream"); } #[test] diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index 5da9b3e853..364467bbb1 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -2717,53 +2717,48 @@ fn run_edit(doc: &mut pdfcraft_cos::Document, edit: &Edit, cx: &mut EditCtx) -> && let AddedContent::Image(mut ai) = added_item.content.clone() { let added_idx = *added_idx; - match change { - ImageEdit::Move(to) => { - ai.rect = *to; - pdfcraft_edit::update_content(doc, *page, added_idx, &AddedContent::Image(ai))?; - } - ImageEdit::Transform(t) => { - let apply = |x: f64, y: f64| (t[0] * x + t[2] * y + t[4], t[1] * x + t[3] * y + t[5]); - let pts = [ - apply(ai.rect[0], ai.rect[1]), - apply(ai.rect[2], ai.rect[1]), - apply(ai.rect[0], ai.rect[3]), - apply(ai.rect[2], ai.rect[3]), - ]; - let mut b = [f64::MAX, f64::MAX, f64::MIN, f64::MIN]; - for (x, y) in pts { - b = [b[0].min(x), b[1].min(y), b[2].max(x), b[3].max(y)]; - } - ai.rect = b; - if t[1].abs() < 1e-6 && t[2].abs() < 1e-6 { + let has_active_crop = img.clip_rect.is_some() || img.visible_rect.is_some_and(|v| v != img.rect); + let should_promote = has_active_crop + || matches!( + change, + ImageEdit::Crop(_) | ImageEdit::RemoveCrop | ImageEdit::RotateBy(_) | ImageEdit::Arrange(_) | ImageEdit::Delete + ); + + if should_promote { + let _ = doc.update_dict(added_item.obj, |d| { + d.remove(b"PCAdded"); + d.remove(b"PCMark"); + }); + } else { + match change { + ImageEdit::Move(to) => { + ai.rect = *to; pdfcraft_edit::update_content(doc, *page, added_idx, &AddedContent::Image(ai))?; - } else { - let _ = doc.update_dict(added_item.obj, |d| { - if let Some(pdfcraft_cos::Object::Dict(params)) = d.get_mut(b"PCAdded") { - params.set( - b"Rect".to_vec(), - pdfcraft_cos::Object::Array(ai.rect.iter().map(|x| pdfcraft_cos::Object::Real(*x)).collect()), - ); + } + ImageEdit::Transform(t) => { + if t[1].abs() < 1e-6 && t[2].abs() < 1e-6 { + let apply = |x: f64, y: f64| (t[0] * x + t[2] * y + t[4], t[1] * x + t[3] * y + t[5]); + let pts = [ + apply(ai.rect[0], ai.rect[1]), + apply(ai.rect[2], ai.rect[1]), + apply(ai.rect[0], ai.rect[3]), + apply(ai.rect[2], ai.rect[3]), + ]; + let mut b = [f64::MAX, f64::MAX, f64::MIN, f64::MIN]; + for (x, y) in pts { + b = [b[0].min(x), b[1].min(y), b[2].max(x), b[3].max(y)]; } - }); + ai.rect = b; + pdfcraft_edit::update_content(doc, *page, added_idx, &AddedContent::Image(ai))?; + } else { + let _ = doc.update_dict(added_item.obj, |d| { + d.remove(b"PCAdded"); + d.remove(b"PCMark"); + }); + } } - } - ImageEdit::Rotate(q) => { - let k = ((4 - q.rem_euclid(4)) % 4) as u8; - ai.rotation = (ai.rotation + k) % 4; - let r = ai.rect; - let (cx, cy) = ((r[0] + r[2]) / 2.0, (r[1] + r[3]) / 2.0); - if k % 2 == 1 { - let (w, h) = (r[2] - r[0], r[3] - r[1]); - ai.rect = [cx - h / 2.0, cy - w / 2.0, cx + h / 2.0, cy + w / 2.0]; - } - pdfcraft_edit::update_content(doc, *page, added_idx, &AddedContent::Image(ai))?; - } - ImageEdit::RotateBy(degrees) => { - let deg = degrees.rem_euclid(360.0); - if (deg % 90.0).abs() < 1e-4 || (deg % 90.0 - 90.0).abs() < 1e-4 { - let quarters = ((deg + 45.0) / 90.0).floor() as i64; - let k = ((4 - quarters.rem_euclid(4)) % 4) as u8; + ImageEdit::Rotate(q) => { + let k = ((4 - q.rem_euclid(4)) % 4) as u8; ai.rotation = (ai.rotation + k) % 4; let r = ai.rect; let (cx, cy) = ((r[0] + r[2]) / 2.0, (r[1] + r[3]) / 2.0); @@ -2772,44 +2767,17 @@ fn run_edit(doc: &mut pdfcraft_cos::Document, edit: &Edit, cx: &mut EditCtx) -> ai.rect = [cx - h / 2.0, cy - w / 2.0, cx + h / 2.0, cy + w / 2.0]; } pdfcraft_edit::update_content(doc, *page, added_idx, &AddedContent::Image(ai))?; - } else if let Some(t) = pdfcraft_edit::turn_by_degrees(img.visible_rect.unwrap_or(img.rect), *degrees) { - let apply = |x: f64, y: f64| (t[0] * x + t[2] * y + t[4], t[1] * x + t[3] * y + t[5]); - let pts = [ - apply(ai.rect[0], ai.rect[1]), - apply(ai.rect[2], ai.rect[1]), - apply(ai.rect[0], ai.rect[3]), - apply(ai.rect[2], ai.rect[3]), - ]; - let mut b = [f64::MAX, f64::MAX, f64::MIN, f64::MIN]; - for (x, y) in pts { - b = [b[0].min(x), b[1].min(y), b[2].max(x), b[3].max(y)]; - } - ai.rect = b; - let _ = doc.update_dict(added_item.obj, |d| { - if let Some(pdfcraft_cos::Object::Dict(params)) = d.get_mut(b"PCAdded") { - params.set( - b"Rect".to_vec(), - pdfcraft_cos::Object::Array(ai.rect.iter().map(|x| pdfcraft_cos::Object::Real(*x)).collect()), - ); - } - }); } + ImageEdit::Flip { horizontal: true } => { + ai.flip_h = !ai.flip_h; + pdfcraft_edit::update_content(doc, *page, added_idx, &AddedContent::Image(ai))?; + } + ImageEdit::Flip { horizontal: false } => { + ai.flip_v = !ai.flip_v; + pdfcraft_edit::update_content(doc, *page, added_idx, &AddedContent::Image(ai))?; + } + _ => {} } - ImageEdit::Flip { horizontal: true } => { - ai.flip_h = !ai.flip_h; - pdfcraft_edit::update_content(doc, *page, added_idx, &AddedContent::Image(ai))?; - } - ImageEdit::Flip { horizontal: false } => { - ai.flip_v = !ai.flip_v; - pdfcraft_edit::update_content(doc, *page, added_idx, &AddedContent::Image(ai))?; - } - ImageEdit::Arrange(_) | ImageEdit::Delete => { - let _ = doc.update_dict(added_item.obj, |d| { - d.remove(b"PCAdded"); - d.remove(b"PCMark"); - }); - } - _ => {} } } } diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index f712aa97ee..746f49d69e 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -7420,3 +7420,19 @@ fn added_image_arrange_converts_to_ordinary_image() { assert_eq!(s.get(id).unwrap().page_images(0).len(), 2); assert_eq!(s.get(id).unwrap().added().len(), 1, "arranged image is converted and no longer an added-item shell"); } + +#[test] +fn added_image_crop_then_move_keeps_crop() { + let mut png = Vec::new(); + { + let mut enc = png::Encoder::new(&mut png, 2, 2); + enc.set_color(png::ColorType::Rgb); + enc.write_header().unwrap().write_image_data(&[255u8; 12]).unwrap(); + } + let (mut s, id) = session_with(1); + s.apply(id, Edit::AddImage { page: 0, rect: Some([20.0, 20.0, 100.0, 100.0]), name: "p.png".into(), bytes: Arc::new(png) }).unwrap(); + s.apply(id, Edit::EditPageImage { page: 0, index: 0, change: ImageEdit::Crop([30.0, 30.0, 70.0, 70.0]) }).unwrap(); + s.apply(id, Edit::EditPageImage { page: 0, index: 0, change: ImageEdit::Move([50.0, 50.0, 130.0, 130.0]) }).unwrap(); + let img = s.get(id).unwrap().page_images(0)[0].clone(); + assert_eq!(img.visible_rect, Some([60.0, 60.0, 100.0, 100.0])); +}