From d67e720eed1ff5b88e2dbe1d7af41696388e4be6 Mon Sep 17 00:00:00 2001 From: Combiler01 Date: Sat, 31 Oct 2026 19:20:44 +0530 Subject: [PATCH] fix: address CodeRabbit review feedback on transforms, formatting, and UI --- crates/engine/src/edit/run.rs | 22 ++++++++----- crates/engine/src/tests.rs | 35 +++++++++++++++++++++ crates/ui-egui/src/app.rs | 4 +-- crates/ui-egui/src/canvas.rs | 8 +++-- crates/ui-egui/src/panels/format_text_ui.rs | 15 +++++++-- crates/ui-egui/src/tools/content_ui.rs | 2 +- crates/ui-egui/src/tools/edit_text_ui.rs | 21 +++++++++++-- 7 files changed, 90 insertions(+), 17 deletions(-) diff --git a/crates/engine/src/edit/run.rs b/crates/engine/src/edit/run.rs index 056b28d766..9a80e6882d 100644 --- a/crates/engine/src/edit/run.rs +++ b/crates/engine/src/edit/run.rs @@ -309,7 +309,12 @@ fn run_edit_page_image(doc: &mut pdfcraft_cos::Document, page: usize, index: usi }; let added_items: Vec<(usize, pdfcraft_edit::Added)> = pdfcraft_edit::list_added(doc).into_iter().filter(|a| a.page == page).enumerate().collect(); - let matched_added = if let Some(st_ref) = target_stream_obj { + let matched_added = if let (Some(st_ref), Some(target_ref)) = (target_stream_obj, img.object) { + added_items + .iter() + .find(|(_, a)| a.obj == st_ref && matches!(&a.content, AddedContent::Image(ai) if ai.image == target_ref)) + .or_else(|| added_items.iter().find(|(_, a)| matches!(&a.content, AddedContent::Image(ai) if ai.image == target_ref))) + } else if let Some(st_ref) = target_stream_obj { added_items.iter().find(|(_, a)| a.obj == st_ref) } else if let Some(target_ref) = img.object { added_items.iter().find(|(_, a)| matches!(&a.content, AddedContent::Image(ai) if ai.image == target_ref)) @@ -384,29 +389,32 @@ fn run_edit_page_image(doc: &mut pdfcraft_cos::Document, page: usize, index: usi let is_perpendicular = t_display[0].abs() < 1e-5 && t_display[3].abs() < 1e-5; if is_parallel { - if t_display[0] < 0.0 { + let (toggle_h, toggle_v) = + if ai.rotation % 2 == 1 { (t_display[3] < 0.0, t_display[0] < 0.0) } else { (t_display[0] < 0.0, t_display[3] < 0.0) }; + if toggle_h { ai.flip_h = !ai.flip_h; } - if t_display[3] < 0.0 { + if toggle_v { ai.flip_v = !ai.flip_v; } ai.rect = bbox(t_display, ai.rect); } else if is_perpendicular { let b_pos = t_display[1] > 0.0; let c_pos = t_display[2] > 0.0; - let (quarters_ccw, flip_h, flip_v) = match (b_pos, c_pos) { + let (quarters_ccw, disp_flip_h, disp_flip_v) = match (b_pos, c_pos) { (true, false) => (1, false, false), (false, true) => (3, false, false), (true, true) => (1, false, true), (false, false) => (1, true, false), }; - ai.rotation = (ai.rotation + quarters_ccw) % 4; - if flip_h { + let (toggle_h, toggle_v) = if ai.rotation % 2 == 1 { (disp_flip_v, disp_flip_h) } else { (disp_flip_h, disp_flip_v) }; + if toggle_h { ai.flip_h = !ai.flip_h; } - if flip_v { + if toggle_v { ai.flip_v = !ai.flip_v; } + ai.rotation = (ai.rotation + quarters_ccw) % 4; ai.rect = bbox(t_display, ai.rect); } else { return Err(EditError::Edit(pdfcraft_edit::EditError::Invalid( diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index d5550437b0..3b8152235f 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -9493,6 +9493,41 @@ fn transform_on_added_image_folds_quarter_turns_and_rejects_non_axis_aligned() { assert!(res_skew.unwrap_err().to_string().contains("non-axis-aligned")); } +#[test] +fn test_added_image_transform_reflections_with_rotation() { + let make_png = |rgb: [u8; 3]| { + let mut png = Vec::new(); + { + let mut enc = png::Encoder::new(&mut png, 2, 2); + enc.set_color(png::ColorType::Rgb); + let data = [rgb[0], rgb[1], rgb[2], rgb[0], rgb[1], rgb[2], rgb[0], rgb[1], rgb[2], rgb[0], rgb[1], rgb[2]]; + enc.write_header().unwrap().write_image_data(&data).unwrap(); + } + Arc::new(png) + }; + let png = make_png([255, 0, 0]); + + let (mut s, id) = session_with(1); + s.apply(id, Edit::AddImage { page: 0, rect: Some([100.0, 100.0, 200.0, 150.0]), name: "img.png".into(), bytes: png }).unwrap(); + + // Rotate by 90° CW (quarter turns CCW: rotation = 3): + s.apply(id, Edit::EditPageImage { page: 0, index: 0, change: ImageEdit::Rotate(1) }).unwrap(); + let cos = pdfcraft_cos::Document::open(s.get(id).unwrap().bytes.clone()).unwrap(); + let AddedContent::Image(ai) = &pdfcraft_edit::list_added(&cos)[0].content else { panic!() }; + assert_eq!(ai.rotation, 3); + assert!(!ai.flip_h); + assert!(!ai.flip_v); + + // Apply a horizontal flip in display space (parallel transform with m0 = -1): + // For rotation = 3 (odd rotation), display horizontal flip corresponds to local vertical flip (flip_v): + s.apply(id, Edit::EditPageImage { page: 0, index: 0, change: ImageEdit::Transform([-1.0, 0.0, 0.0, 1.0, 300.0, 0.0]) }).unwrap(); + let cos2 = pdfcraft_cos::Document::open(s.get(id).unwrap().bytes.clone()).unwrap(); + let AddedContent::Image(ai2) = &pdfcraft_edit::list_added(&cos2)[0].content else { panic!() }; + assert_eq!(ai2.rotation, 3); + assert!(!ai2.flip_h, "flip_h should NOT be toggled for horizontal flip when rotation is 90°"); + assert!(ai2.flip_v, "flip_v should be toggled for horizontal flip when rotation is 90°"); +} + /// ISO 32000-2 Table 192, 12.7.5.2.3 and Table 228 (Comb): a check box, a push button, a comb /// field the file leaves to the viewer. The page as displayed and printed as an image shows the /// check box's caption symbol in its text colour, the button's grey face and the comb's cells in diff --git a/crates/ui-egui/src/app.rs b/crates/ui-egui/src/app.rs index f53857c245..19ca9c1091 100644 --- a/crates/ui-egui/src/app.rs +++ b/crates/ui-egui/src/app.rs @@ -372,7 +372,7 @@ impl PdfCraftApp { if !self.is_tool_pinned(id) { self.pinned_tools.push(id.to_string()); if let Some(g) = pdfcraft_engine::catalog::group(id) { - self.notify_fmt("Pinned {tool} to Home", &[("tool", g.label)]); + self.notify_fmt("Pinned {tool} to Home", &[("tool", tl!(g.label))]); } return true; } @@ -384,7 +384,7 @@ impl PdfCraftApp { if let Some(pos) = self.pinned_tools.iter().position(|t| t == id) { self.pinned_tools.remove(pos); if let Some(g) = pdfcraft_engine::catalog::group(id) { - self.notify_fmt("Unpinned {tool} from Home", &[("tool", g.label)]); + self.notify_fmt("Unpinned {tool} from Home", &[("tool", tl!(g.label))]); } } } diff --git a/crates/ui-egui/src/canvas.rs b/crates/ui-egui/src/canvas.rs index 8fe38fdd8e..1319e00871 100644 --- a/crates/ui-egui/src/canvas.rs +++ b/crates/ui-egui/src/canvas.rs @@ -310,8 +310,12 @@ impl ImageObb { /// Resizes the OBB proportionally along its local axes using projected mouse delta, /// locking the diagonally opposite corner as the fixed anchor point. pub fn scale_proportional(&self, handle: ImageHandle, delta: Vec2, min_size: f32) -> Self { - let u_vec = vec2(self.u_axis.0 as f32, self.u_axis.1 as f32); - let v_vec = vec2(self.v_axis.0 as f32, self.v_axis.1 as f32); + let u_diff = self.s10 - self.s00; + let v_diff = self.s01 - self.s00; + let u_len = u_diff.length(); + let v_len = v_diff.length(); + let u_vec = if u_len > 1e-4 { u_diff / u_len } else { vec2(1.0, 0.0) }; + let v_vec = if v_len > 1e-4 { v_diff / v_len } else { vec2(0.0, 1.0) }; let delta_u = delta.dot(u_vec); let delta_v = delta.dot(v_vec); let min_s = min_size.max(5.0); diff --git a/crates/ui-egui/src/panels/format_text_ui.rs b/crates/ui-egui/src/panels/format_text_ui.rs index cfce824941..636a6d1897 100644 --- a/crates/ui-egui/src/panels/format_text_ui.rs +++ b/crates/ui-egui/src/panels/format_text_ui.rs @@ -231,8 +231,9 @@ pub(crate) fn panel( ui.add_space(4.0); // Size and colour | bullets, numbering and alignment. + let mut size_committed = false; row(ui, |ui| { - font_size_input(ui, &mut look.size, fonts.mixed_size); + size_committed = font_size_input(ui, &mut look.size, fonts.mixed_size); let swatch = colour_button(ui, t, look.color); egui::Popup::menu(&swatch).close_behavior(egui::PopupCloseBehavior::CloseOnClickOutside).show(|ui| { if let Some(picked) = crate::comments::swatch_grid(ui, Some(look.color)) { @@ -351,7 +352,7 @@ pub(crate) fn panel( number_with_presets(ui, spacing, &CHAR_SPACINGS, half, (label.id, tl!("Character spacing"))); }); - out.changed = (look.clone(), extras.as_deref().copied()) != before; + out.changed = size_committed || (look.clone(), extras.as_deref().copied()) != before; ui.add_space(6.0); ui.separator(); out @@ -440,7 +441,8 @@ fn colour_button(ui: &mut egui::Ui, t: &Tokens, color: [f64; 3]) -> egui::Respon /// Commit a typed size once (Enter or leaving the field), rather than one undo step per key. /// The bounds match AddedText validation, so the panel also accepts coloured text below 8 pt. -fn font_size_input(ui: &mut egui::Ui, size: &mut f64, mixed: bool) { +fn font_size_input(ui: &mut egui::Ui, size: &mut f64, mixed: bool) -> bool { + let mut committed = false; ui.scope(|ui| { ui.spacing_mut().item_spacing.x = 2.0; let id = ui.make_persistent_id("font-size-input"); @@ -459,6 +461,9 @@ fn font_size_input(ui: &mut egui::Ui, size: &mut f64, mixed: bool) { response.surrender_focus(); } else if response.lost_focus() || (response.has_focus() && ui.input(|i| i.key_pressed(egui::Key::Enter))) { if let Some(value) = typed_font_size(&text) { + if mixed || *size != value { + committed = true; + } *size = value; text = size.to_string(); } else { @@ -470,6 +475,9 @@ fn font_size_input(ui: &mut egui::Ui, size: &mut f64, mixed: bool) { egui::Popup::menu(&list).show(|ui| { for value in SIZES { if ui.selectable_label(*size == value, format!("{value} pt")).clicked() { + if mixed || *size != value { + committed = true; + } *size = value; ui.close(); } @@ -480,6 +488,7 @@ fn font_size_input(ui: &mut egui::Ui, size: &mut f64, mixed: bool) { } ui.data_mut(|d| d.insert_temp(id, (size.to_bits(), text))); }); + committed } fn typed_font_size(text: &str) -> Option { diff --git a/crates/ui-egui/src/tools/content_ui.rs b/crates/ui-egui/src/tools/content_ui.rs index 7e52502530..b005b21262 100644 --- a/crates/ui-egui/src/tools/content_ui.rs +++ b/crates/ui-egui/src/tools/content_ui.rs @@ -1048,7 +1048,7 @@ impl crate::PdfCraftApp { } let command_recent = self .last_command_frame - .is_some_and(|(f, t)| self.frame_count.saturating_sub(f) <= 10 || (inp.time > 0.0 && (inp.time - t).abs() < 0.35)); + .is_some_and(|(f, t)| if inp.time > 0.0 { (inp.time - t).abs() < 0.35 } else { self.frame_count.saturating_sub(f) <= 10 }); let command_active = modifiers.command || command_recent; if !*pressed && !self.v_pressed && command_active { released = true; diff --git a/crates/ui-egui/src/tools/edit_text_ui.rs b/crates/ui-egui/src/tools/edit_text_ui.rs index ee259fd2c3..a2c30e8e03 100644 --- a/crates/ui-egui/src/tools/edit_text_ui.rs +++ b/crates/ui-egui/src/tools/edit_text_ui.rs @@ -77,8 +77,10 @@ pub fn toggle_bullet_list(text: &mut String) -> bool { new_text.push('\n'); } let trimmed = line.trim_start(); + let indent = &line[..line.len() - trimmed.len()]; if all_bulleted { if let Some(rest) = trimmed.strip_prefix("• ").or_else(|| trimmed.strip_prefix("- ")) { + new_text.push_str(indent); new_text.push_str(rest); } else { new_text.push_str(line); @@ -86,8 +88,9 @@ pub fn toggle_bullet_list(text: &mut String) -> bool { } else if trimmed.is_empty() || trimmed.starts_with("• ") { new_text.push_str(line); } else { + new_text.push_str(indent); new_text.push_str("• "); - new_text.push_str(line); + new_text.push_str(trimmed); } } if has_trailing_crlf { @@ -126,12 +129,14 @@ pub fn toggle_numbered_list(text: &mut String) -> bool { new_text.push('\n'); } let t = line.trim_start(); + let indent = &line[..line.len() - t.len()]; if all_numbered { if let Some(dot_idx) = t.find('.') && dot_idx > 0 && t[..dot_idx].chars().all(|c| c.is_ascii_digit()) && t[dot_idx..].starts_with(". ") { + new_text.push_str(indent); new_text.push_str(t.get(dot_idx + 2..).unwrap_or("")); continue; } @@ -140,6 +145,7 @@ pub fn toggle_numbered_list(text: &mut String) -> bool { new_text.push_str(line); } else { use std::fmt::Write as _; + new_text.push_str(indent); let _ = write!(new_text, "{num}. "); new_text.push_str(t); num += 1; @@ -697,7 +703,6 @@ impl LineEditor { self.look0 = self.look.clone(); self.extras0 = self.extras; self.original = self.text.clone(); - self.selection = None; // The page now has this text: the box's history restarts, even when the format left the // text as it was, so a Redo from before can't put undone typing back on top of it. self.typing_from = None; @@ -1717,6 +1722,12 @@ C", &picked assert!(toggle_bullet_list(&mut text)); assert!(!is_bulleted(&text)); assert_eq!(text, "First line\nSecond line\n\nThird line"); + + let mut indented = " First line\n Second line".to_string(); + assert!(toggle_bullet_list(&mut indented)); + assert_eq!(indented, " • First line\n • Second line"); + assert!(toggle_bullet_list(&mut indented)); + assert_eq!(indented, " First line\n Second line"); } #[test] @@ -1731,6 +1742,12 @@ C", &picked assert!(toggle_numbered_list(&mut text)); assert!(!is_numbered(&text)); assert_eq!(text, "Item A\nItem B\n\nItem C"); + + let mut indented = " Item A\n Item B".to_string(); + assert!(toggle_numbered_list(&mut indented)); + assert_eq!(indented, " 1. Item A\n 2. Item B"); + assert!(toggle_numbered_list(&mut indented)); + assert_eq!(indented, " Item A\n Item B"); } #[test]