From afbc1ed1b73ced9b96d81e68f91d1e5bffcc7ff6 Mon Sep 17 00:00:00 2001 From: Combiler01 Date: Tue, 27 Oct 2026 15:30:53 +0530 Subject: [PATCH] fix: address CodeRabbit review recommendations across edit, engine, and ui --- crates/automation/src/tools.rs | 2 +- crates/edit/src/images.rs | 36 +++--- crates/edit/src/tests.rs | 116 +++++++++--------- crates/engine/src/lib.rs | 2 +- crates/engine/src/pictures.rs | 12 ++ crates/engine/src/text_fonts.rs | 3 +- crates/fonts/src/embedding.rs | 9 ++ crates/ui-egui/src/canvas/image_pick.rs | 1 + crates/ui-egui/src/tools/content_ui.rs | 60 ++++++++- crates/ui-egui/src/tools/edit_text_ui.rs | 44 +++++-- .../ui-egui/src/tools/edit_text_ui/image.rs | 6 +- 11 files changed, 204 insertions(+), 87 deletions(-) diff --git a/crates/automation/src/tools.rs b/crates/automation/src/tools.rs index c7616c1eaa..6226047330 100644 --- a/crates/automation/src/tools.rs +++ b/crates/automation/src/tools.rs @@ -1337,7 +1337,7 @@ pub fn tools() -> Vec { .ro() .cmd("edit.edit_text") .with(schema(json!({ "doc": doc(), "page": { "type": "integer", "minimum": 1 } }), &["doc", "page"])), - t("image_edit", "Edit an image", "Change one of a page's images (number from page_images): action move (rect: new full placement box in top-left-origin points, based on page_images.rect), crop (rect: the part to keep, top-left-origin points; the pixels stay in place and page_images.visible_rect shows the result; a later move carries the crop along), rotate (quarters clockwise, default 1), rotate_by (degrees: any angle clockwise about the visible centre; the box in page_images then becomes the turned image's bounding box), flip_horizontal, flip_vertical, replace (path: an image file, drawn in the same place) or delete. Undoable. The selected image crop transforms with its pixels, isolating shared path scopes while retaining neighboring painting. Text-outline clips return a clear error. Rotation/flip use the visible centre. Form artwork supports move/resize/rotate/flip/delete as a group; replace and image_save only support raster images.") + t("image_edit", "Edit an image", "Change one of a page's images (number from page_images): action move (rect: new full placement box in top-left-origin points, based on page_images.rect), crop (rect: the part to keep, top-left-origin points; the pixels stay in place and page_images.visible_rect shows the result; a later move carries the crop along), remove_crop (restore the original uncropped visible rectangle), rotate (quarters clockwise, default 1), rotate_by (degrees: any angle clockwise about the visible centre; the box in page_images then becomes the turned image's bounding box), flip_horizontal, flip_vertical, replace (path: an image file, drawn in the same place), delete, or arrange: bring_to_front, send_to_back, bring_forward, send_backward (reorder stacking order among content on the page; errors when the image is already at the requested limit; arranging an added image converts it into an ordinary page image). Undoable. The selected image crop transforms with its pixels, isolating shared path scopes while retaining neighboring painting. Text-outline clips return a clear error. Rotation/flip use the visible centre. Form artwork supports move/resize/rotate/flip/delete as a group; replace and image_save only support raster images.") .cmd("edit.edit_text") .with(schema( json!({ diff --git a/crates/edit/src/images.rs b/crates/edit/src/images.rs index 3272663c3c..b860348acd 100644 --- a/crates/edit/src/images.rs +++ b/crates/edit/src/images.rs @@ -1488,9 +1488,10 @@ fn extract_image_drawing_ops( _target: usize, _scope_matrix: Matrix, _draw: Matrix, -) -> Result, EditError> { +) -> Result<(Vec, bool), EditError> { let is_path = |o: &Op| matches!(o.op.as_slice(), b"m" | b"l" | b"c" | b"v" | b"y" | b"h" | b"re"); let mut drawing = Vec::new(); + let mut removed_clip = false; let mut i = start.saturating_add(1); while i < end { let Some(op) = ops.get(i) else { break }; @@ -1500,6 +1501,7 @@ fn extract_image_drawing_ops( k = k.saturating_add(1); } if ops.get(k).is_some_and(|o| o.is("W") || o.is("W*")) && ops.get(k.saturating_add(1)).is_some_and(|o| o.is("n")) { + removed_clip = true; i = k.saturating_add(2); continue; } @@ -1507,7 +1509,7 @@ fn extract_image_drawing_ops( drawing.push(op.clone()); i = i.saturating_add(1); } - Ok(drawing) + Ok((drawing, removed_clip)) } /// Remove any private clipping path / crop on image `index` on `page`. @@ -1522,20 +1524,24 @@ pub fn remove_image_crop(doc: &mut Document, page: usize, index: usize) -> Resul if !ops.get(target).is_some_and(|op| op.is("Do")) { return Err(EditError::Invalid("the image operation could not be located".into())); } - if let Some((start, end, scope_matrix)) = image_exclusive_scope(&ops, target)? { - let drawing_ops = extract_image_drawing_ops(&ops, start, end, target, scope_matrix, Matrix(img.draw_matrix))?; - let start_op = ops.get(start).ok_or_else(|| EditError::Invalid("the image operation is missing".into()))?; - let end_op = ops.get(end).ok_or_else(|| EditError::Invalid("the image operation is missing".into()))?; - let range = start_op.span.start..end_op.span.end; - let mut unit_ops = Vec::new(); - unit_ops.push(Op::new("q", vec![])); - unit_ops.extend(drawing_ops); - unit_ops.push(Op::new("Q", vec![])); - let serialized = serialize_ops(&unit_ops); - let changed = pieces.splice([(range, serialized)]); - let contents = crate::text::rewritten_contents(doc, &all, changed); - doc.update_dict(p.obj, |d| d.set(b"Contents".to_vec(), contents))?; + let Some((start, end, scope_matrix)) = image_exclusive_scope(&ops, target)? else { + return Err(EditError::Invalid("the image has no crop to remove".into())); + }; + let (drawing_ops, removed_clip) = extract_image_drawing_ops(&ops, start, end, target, scope_matrix, Matrix(img.draw_matrix))?; + if !removed_clip { + return Err(EditError::Invalid("the image has no crop to remove".into())); } + let start_op = ops.get(start).ok_or_else(|| EditError::Invalid("the image operation is missing".into()))?; + let end_op = ops.get(end).ok_or_else(|| EditError::Invalid("the image operation is missing".into()))?; + let range = start_op.span.start..end_op.span.end; + let mut unit_ops = Vec::new(); + unit_ops.push(Op::new("q", vec![])); + unit_ops.extend(drawing_ops); + unit_ops.push(Op::new("Q", vec![])); + let serialized = serialize_ops(&unit_ops); + let changed = pieces.splice([(range, serialized)]); + let contents = crate::text::rewritten_contents(doc, &all, changed); + doc.update_dict(p.obj, |d| d.set(b"Contents".to_vec(), contents))?; Ok(()) } diff --git a/crates/edit/src/tests.rs b/crates/edit/src/tests.rs index 48d9aa43fa..aaf5a78e54 100644 --- a/crates/edit/src/tests.rs +++ b/crates/edit/src/tests.rs @@ -3107,6 +3107,9 @@ fn image_arrange_stacking_order_and_remove_crop() { let doc = reopen(&doc); let img_after = &images::page_images(&doc, 0).unwrap()[0]; assert_eq!(img_after.visible_rect, Some(img_after.rect)); + // Removing crop again when there is no crop is refused. + let mut doc = doc; + assert!(images::remove_image_crop(&mut doc, 0, 0).is_err()); // Test crop_image let uncropped_content = "q 200 0 0 100 100 100 cm /Im0 Do Q"; @@ -4424,7 +4427,10 @@ fn arrange_and_crop_image_in_second_content_stream_succeeds() { images::crop_image(&mut doc, 0, 0, [210.0, 210.0, 290.0, 290.0]).unwrap(); let after_crop = images::page_images(&reopen(&doc), 0).unwrap(); assert_eq!(after_crop.len(), 2); - assert!(after_crop[0].clip_rect.is_some() || after_crop[0].visible_rect.is_some()); + assert_eq!(after_crop[0].name, "Im1"); + assert!(close(after_crop[0].visible_rect.unwrap(), [210.0, 210.0, 290.0, 290.0])); + assert_eq!(after_crop[1].name, "Im0"); + assert_eq!(after_crop[1].visible_rect, Some(after_crop[1].rect)); } /// One page whose font draws a few Hebrew letters (ToUnicode): A ב, B ד, C י, D ו, E ק, F ש, @@ -7598,66 +7604,66 @@ fn reading_keeps_dotted_leader_entries_whole_and_in_order() { "Ordinary body text. Still the same paragraph.", ] ); +} - #[cfg(test)] - mod proptests { - use super::*; - use proptest::prelude::*; +#[cfg(test)] +mod proptests { + use super::*; + use proptest::prelude::*; - proptest! { + proptest! { - #[test] - fn prop_is_entire_page_wrapped_arbitrary_bytes(bytes in proptest::collection::vec(any::(), 0..2048)) { - let pieces = pdfcraft_content::Pieces::join(&[&bytes]); - let ops = pieces.parse().ops; - let _ = images::is_entire_page_wrapped(&ops); + #[test] + fn prop_is_entire_page_wrapped_arbitrary_bytes(bytes in proptest::collection::vec(any::(), 0..2048)) { + let pieces = pdfcraft_content::Pieces::join(&[&bytes]); + let ops = pieces.parse().ops; + let _ = images::is_entire_page_wrapped(&ops); + } + + #[test] + fn prop_content_units_arbitrary_bytes( + bytes in proptest::collection::vec(any::(), 0..2048), + start in 0usize..50, + end in 0usize..50, + ) { + let pieces = pdfcraft_content::Pieces::join(&[&bytes]); + let ops = pieces.parse().ops; + let scan_start = start.min(ops.len()); + let scan_end = end.min(ops.len()).max(scan_start); + let units = images::content_units(&ops, scan_start, scan_end); + for u in units { + prop_assert!(u.start <= u.end); + prop_assert!(u.end <= ops.len()); } + } - #[test] - fn prop_content_units_arbitrary_bytes( - bytes in proptest::collection::vec(any::(), 0..2048), - start in 0usize..50, - end in 0usize..50, - ) { - let pieces = pdfcraft_content::Pieces::join(&[&bytes]); - let ops = pieces.parse().ops; - let scan_start = start.min(ops.len()); - let scan_end = end.min(ops.len()).max(scan_start); - let units = images::content_units(&ops, scan_start, scan_end); - for u in units { - prop_assert!(u.start <= u.end); - prop_assert!(u.end <= ops.len()); - } - } + #[test] + fn prop_arrange_image_arbitrary_malformed( + index in 0usize..10, + order_idx in 0u8..4, + bytes in proptest::collection::vec(any::(), 0..1024), + ) { + let order = match order_idx { + 0 => images::ArrangeOrder::BringToFront, + 1 => images::ArrangeOrder::SendToBack, + 2 => images::ArrangeOrder::BringForward, + _ => images::ArrangeOrder::SendBackward, + }; + let mut doc = image_page_with(&String::from_utf8_lossy(&bytes)); + let _ = images::arrange_image(&mut doc, 0, index, order); + } - #[test] - fn prop_arrange_image_arbitrary_malformed( - index in 0usize..10, - order_idx in 0u8..4, - bytes in proptest::collection::vec(any::(), 0..1024), - ) { - let order = match order_idx { - 0 => images::ArrangeOrder::BringToFront, - 1 => images::ArrangeOrder::SendToBack, - 2 => images::ArrangeOrder::BringForward, - _ => images::ArrangeOrder::SendBackward, - }; - let mut doc = image_page_with(&String::from_utf8_lossy(&bytes)); - let _ = images::arrange_image(&mut doc, 0, index, order); - } - - #[test] - fn prop_crop_image_malformed_inputs( - x1 in -500.0f64..500.0, - y1 in -500.0f64..500.0, - x2 in -500.0f64..500.0, - y2 in -500.0f64..500.0, - bytes in proptest::collection::vec(any::(), 0..1024), - ) { - let mut doc = image_page_with(&String::from_utf8_lossy(&bytes)); - let _ = images::crop_image(&mut doc, 0, 0, [x1, y1, x2, y2]); - let _ = images::remove_image_crop(&mut doc, 0, 0); - } + #[test] + fn prop_crop_image_malformed_inputs( + x1 in -500.0f64..500.0, + y1 in -500.0f64..500.0, + x2 in -500.0f64..500.0, + y2 in -500.0f64..500.0, + bytes in proptest::collection::vec(any::(), 0..1024), + ) { + let mut doc = image_page_with(&String::from_utf8_lossy(&bytes)); + let _ = images::crop_image(&mut doc, 0, 0, [x1, y1, x2, y2]); + let _ = images::remove_image_crop(&mut doc, 0, 0); } } } diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index bd0b9af817..7c1aa40a36 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -133,7 +133,7 @@ pub use history::Label; use layers::{apply_layer_state, set_default_layer_state, use_layer_choices}; pub use open::{OpenContext, PreparedDocument}; pub use page_filter::{PageOrientation, PageParity, filter_pages}; -pub use pictures::MarkFile; +pub use pictures::{MarkFile, image_dimensions}; use pictures::{embed_image, fit_picture, mark_source}; use protection::unrestricted; pub use protection::{Changes, Printing, Protection, SecuritySummary}; diff --git a/crates/engine/src/pictures.rs b/crates/engine/src/pictures.rs index b29941720a..d426fd9f33 100644 --- a/crates/engine/src/pictures.rs +++ b/crates/engine/src/pictures.rs @@ -57,3 +57,15 @@ pub(crate) fn mark_source(doc: &mut pdfcraft_cos::Document, f: &MarkFile) -> Res Ok(pdfcraft_edit::MarkSource { xobject, size, image: true }) } } + +/// The dimensions of an image from its bytes (JPEG, PNG, TIFF), without decoding the full raster. +pub fn image_dimensions(bytes: &[u8]) -> Option<(u32, u32)> { + let format = image::guess_format(bytes).ok()?; + let mut reader = image::ImageReader::with_format(std::io::Cursor::new(bytes), format); + let mut limits = image::Limits::default(); + limits.max_alloc = Some(64_000_000 * 4); + reader.limits(limits); + let decoder = reader.into_decoder().ok()?; + let (w, h) = image::ImageDecoder::dimensions(&decoder); + if w == 0 || h == 0 { None } else { Some((w, h)) } +} diff --git a/crates/engine/src/text_fonts.rs b/crates/engine/src/text_fonts.rs index 8fc1275ed7..5cc43c4081 100644 --- a/crates/engine/src/text_fonts.rs +++ b/crates/engine/src/text_fonts.rs @@ -58,7 +58,8 @@ pub fn discover_budgeted(roots: &[PathBuf], mut budget: u64) -> Vec { if meta.is_dir() && depth < 4 { if let Ok(children) = std::fs::read_dir(&path) { let remaining = 4096usize.saturating_sub(entries.saturating_add(pending.len())); - let mut sorted_children: Vec<_> = children.take(remaining).filter_map(Result::ok).map(|p| p.path()).collect(); + // Best effort: unreadable entries are skipped and do not count against the entry allowance. + let mut sorted_children: Vec<_> = children.filter_map(Result::ok).take(remaining).map(|p| p.path()).collect(); sorted_children.sort(); pending.extend(sorted_children.into_iter().rev().map(|p| (p, depth + 1))); } diff --git a/crates/fonts/src/embedding.rs b/crates/fonts/src/embedding.rs index 866ab7c0b1..fcaed67581 100644 --- a/crates/fonts/src/embedding.rs +++ b/crates/fonts/src/embedding.rs @@ -701,6 +701,15 @@ pub fn subset_truetype(bytes: &[u8], used_gids: &[u16], postscript_name: &str) - if let Some(os2) = map.get(b"OS/2") { tables.push((*b"OS/2", os2.to_vec())); } + if let Some(cvt) = map.get(b"cvt ") { + tables.push((*b"cvt ", cvt.to_vec())); + } + if let Some(fpgm) = map.get(b"fpgm") { + tables.push((*b"fpgm", fpgm.to_vec())); + } + if let Some(prep) = map.get(b"prep") { + tables.push((*b"prep", prep.to_vec())); + } tables.push((*b"post", subset_post)); tables.sort_by_key(|t| t.0); diff --git a/crates/ui-egui/src/canvas/image_pick.rs b/crates/ui-egui/src/canvas/image_pick.rs index 4823cb4531..cede9b5365 100644 --- a/crates/ui-egui/src/canvas/image_pick.rs +++ b/crates/ui-egui/src/canvas/image_pick.rs @@ -204,6 +204,7 @@ impl PdfCraftApp { let copied = file.and_then(|(ext, bytes)| { let name = format!("{stem} p{} img{}.{ext}", page.saturating_add(1), index.saturating_add(1)); self.put_file_on_clipboard(&name, &bytes)?; + self.last_copied_image = None; self.last_copied_file = Some((name, bytes)); self.last_copied_image_rect = None; Ok(ext) diff --git a/crates/ui-egui/src/tools/content_ui.rs b/crates/ui-egui/src/tools/content_ui.rs index 1c6df76300..fe45142dc2 100644 --- a/crates/ui-egui/src/tools/content_ui.rs +++ b/crates/ui-egui/src/tools/content_ui.rs @@ -800,6 +800,7 @@ impl crate::PdfCraftApp { if self.system_clipboard { return Err("the web version can't put images on the clipboard yet; use Save Image As".to_string()); } + self.last_copied_file = None; self.last_copied_image = Some((w, h, rgba)); self.last_copied_image_rect = None; Ok((w, h)) @@ -922,8 +923,14 @@ impl crate::PdfCraftApp { if modifiers.command { self.last_command_time = Some(inp.time); } - let command_active = modifiers.command || self.last_command_time.is_some(); - released |= !*pressed && !self.v_pressed && command_active; + let command_recent = self.last_command_time.is_some_and(|t| (inp.time - t).abs() < 0.35); + let command_active = modifiers.command || command_recent; + if !*pressed { + if !self.v_pressed && command_active { + released = true; + } + self.last_command_time = None; + } self.v_pressed = *pressed; } } @@ -1043,7 +1050,12 @@ impl crate::PdfCraftApp { let target_rect = if let Some(r) = rect { Some(r) } else if let Some(at) = at { - let (raw_w, raw_h) = self.last_copied_image.as_ref().map(|(w, h, _)| (*w as f64, *h as f64)).unwrap_or((100.0, 100.0)); + let (raw_w, raw_h) = self + .last_copied_image + .as_ref() + .map(|(w, h, _)| (*w as f64, *h as f64)) + .or_else(|| pdfcraft_engine::image_dimensions(&bytes).map(|(w, h)| (w as f64, h as f64))) + .unwrap_or((100.0, 100.0)); let (pw, ph) = self.session.get(id).and_then(|d| d.info.pages.get(page)).map(|p| (p.width as f64, p.height as f64)).unwrap_or((612.0, 792.0)); let k = ((pw * 0.8) / raw_w.max(1.0)).min((ph * 0.8) / raw_h.max(1.0)).min(1.0); @@ -1357,4 +1369,46 @@ mod tests { assert_eq!(rows(width + 5.0), 2, "the saved text wraps here, so the editor must too"); assert_eq!(rows(width + 10.0), 1, "and keeps the line when the spacing fits"); } + + #[test] + fn ctrl_c_followed_by_plain_v_release_does_not_paste() { + let mut app = crate::PdfCraftApp::default(); + let ctx = egui::Context::default(); + + // 1. User presses Ctrl+C at time 0.0 + let mut out = ctx.run_ui( + egui::RawInput { + time: Some(0.0), + events: vec![egui::Event::Key { + key: egui::Key::C, + physical_key: None, + pressed: true, + repeat: false, + modifiers: egui::Modifiers::COMMAND, + }], + ..Default::default() + }, + |_| {}, + ); + out.textures_delta.clear(); + assert!(!app.paste_key_released(&ctx)); + + // 2. Later at time 1.0, user releases V with Modifiers::NONE + let mut out = ctx.run_ui( + egui::RawInput { + time: Some(1.0), + events: vec![egui::Event::Key { + key: egui::Key::V, + physical_key: None, + pressed: false, + repeat: false, + modifiers: egui::Modifiers::NONE, + }], + ..Default::default() + }, + |_| {}, + ); + out.textures_delta.clear(); + assert!(!app.paste_key_released(&ctx), "Ctrl+C followed by plain V release must not trigger paste"); + } } diff --git a/crates/ui-egui/src/tools/edit_text_ui.rs b/crates/ui-egui/src/tools/edit_text_ui.rs index 289df1fb68..f47ebcedd5 100644 --- a/crates/ui-egui/src/tools/edit_text_ui.rs +++ b/crates/ui-egui/src/tools/edit_text_ui.rs @@ -57,6 +57,8 @@ pub fn is_numbered(text: &str) -> bool { } pub fn toggle_bullet_list(text: &mut String) -> bool { + let has_trailing_crlf = text.ends_with("\r\n"); + let has_trailing_newline = text.ends_with('\n'); let lines: Vec<&str> = text.lines().collect(); if lines.is_empty() { return false; @@ -84,6 +86,11 @@ pub fn toggle_bullet_list(text: &mut String) -> bool { new_text.push_str(line); } } + if has_trailing_crlf { + new_text.push_str("\r\n"); + } else if has_trailing_newline { + new_text.push('\n'); + } if new_text != *text { *text = new_text; true @@ -93,6 +100,8 @@ pub fn toggle_bullet_list(text: &mut String) -> bool { } pub fn toggle_numbered_list(text: &mut String) -> bool { + let has_trailing_crlf = text.ends_with("\r\n"); + let has_trailing_newline = text.ends_with('\n'); let lines: Vec<&str> = text.lines().collect(); if lines.is_empty() { return false; @@ -132,6 +141,11 @@ pub fn toggle_numbered_list(text: &mut String) -> bool { num += 1; } } + if has_trailing_crlf { + new_text.push_str("\r\n"); + } else if has_trailing_newline { + new_text.push('\n'); + } if new_text != *text { *text = new_text; true @@ -652,20 +666,34 @@ impl LineEditor { self.look.size = sz; self.look0.size = sz; } else { - self.look.size = 0.0; - self.look0.size = 0.0; + if self.look.size <= 0.0 { + self.look.size = 12.0; + } + self.look0.size = self.look.size; } if let Some(f) = fmt.family { self.look.family = f; self.look0.family = f; } - if let Some(b) = fmt.bold { - self.look.bold = b; - self.look0.bold = b; + match fmt.bold { + Some(b) => { + self.look.bold = b; + self.look0.bold = b; + } + None => { + self.look.bold = false; + self.look0.bold = false; + } } - if let Some(it) = fmt.italic { - self.look.italic = it; - self.look0.italic = it; + match fmt.italic { + Some(it) => { + self.look.italic = it; + self.look0.italic = it; + } + None => { + self.look.italic = false; + self.look0.italic = false; + } } if let Some(c) = fmt.color { self.look.color = c; diff --git a/crates/ui-egui/src/tools/edit_text_ui/image.rs b/crates/ui-egui/src/tools/edit_text_ui/image.rs index 7d1be61a4d..219bd55e8e 100644 --- a/crates/ui-egui/src/tools/edit_text_ui/image.rs +++ b/crates/ui-egui/src/tools/edit_text_ui/image.rs @@ -1170,7 +1170,7 @@ pub(crate) fn image_context_menu( *action = Some(ImageAction::Redact(page, hit)); close(ui); } - if ui.add_enabled(raster, egui::Button::new("Export image")).clicked() { + if ui.add_enabled(raster, egui::Button::new(tl!("Export image"))).clicked() { *action = Some(ImageAction::Save(page, hit)); close(ui); } @@ -1182,8 +1182,8 @@ pub(crate) fn image_context_menu( close(ui); } - ui.menu_button("Edit image using", |ui| { - ui.add_enabled(false, egui::Button::new("Default image editor")); + ui.menu_button(tl!("Edit image using"), |ui| { + ui.add_enabled(false, egui::Button::new(tl!("Default image editor"))); }); if ui.button(tl!("Recognize Text…")).clicked() {