fix: address CodeRabbit review recommendations across edit, engine, and ui

crates/automation/src/tools.rs

@@ -1337,7 +1337,7 @@ pub fn tools() -> Vec<ToolDef> {
             .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!({

crates/edit/src/images.rs

@@ -1488,9 +1488,10 @@ fn extract_image_drawing_ops(
     _target: usize,
     _scope_matrix: Matrix,
     _draw: Matrix,
-) -> Result<Vec<Op>, EditError> {
+) -> Result<(Vec<Op>, 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(())
 }
 

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::<u8>(), 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::<u8>(), 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::<u8>(), 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::<u8>(), 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::<u8>(), 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::<u8>(), 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::<u8>(), 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::<u8>(), 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);
         }
     }
 }

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};

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)) }
+}

crates/engine/src/text_fonts.rs

@@ -58,7 +58,8 @@ pub fn discover_budgeted(roots: &[PathBuf], mut budget: u64) -> Vec<FontFile> {
         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)));
             }

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);

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)

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");
+    }
 }

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;

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() {