Merge pull request #7739 from davidmann4/fix/crop-swap-button-turns-frame

Crop: the options-bar swap button turns the frame instead of shrinking it (#7738)

crates/ui-egui/src/panels/options_bar.rs

@@ -474,9 +474,11 @@ fn selection_specific_options(app: &mut PhotocraftApp, ui: &mut egui::Ui, tool:
             // Ratios, sizes, saved presets, New / Delete Crop Preset… (#2443, #1919).
             crate::crop_presets::dropdown(app, ui);
             let o = &mut app.ui.tool_options;
+            let mut turn = false;
             let mut ratio_base = None;
             if o.crop_ratio == crate::crop_size::WHR {
-                crate::crop_size::fields(o, ui);
+                // ⇄ turns the frame with W and H, as with a ratio.
+                turn = crate::crop_size::fields(o, ui);
             } else {
                 // Keep partial edits in ToolOptions so the first side survives a frame;
                 // Original Ratio shows the document's numbers, as Photoshop does.
@@ -488,6 +490,7 @@ fn selection_specific_options(app: &mut PhotocraftApp, ui: &mut egui::Ui, tool:
                 let (cw, ch) = (fw.changed(), fh.changed());
                 if swap {
                     std::mem::swap(&mut rw, &mut rh);
+                    turn = true;
                 }
                 if cw || ch || swap {
                     crate::crop_size::set_custom_ratio_fields(o, rw, rh);
@@ -526,6 +529,9 @@ fn selection_specific_options(app: &mut PhotocraftApp, ui: &mut egui::Ui, tool:
                 &[(false, crate::crop_ui::fill_label(delete_cropped_pixels)), (true, tl!("Content-Aware Fill"))],
                 150.0,
             );
+            if turn {
+                crate::crop_ui::turn_frame(app);
+            }
             if crate::crop_ui::preset_ratio(app) != before_ratio {
                 match ratio_base {
                     Some(base) => crate::crop_ui::apply_ratio_from(app, base),

crates/ui-egui/src/tools/crop_size.rs

@@ -315,11 +315,15 @@ fn text_field(ui: &mut egui::Ui, id: &str, value: &mut String, hint: &str, width
 }
 
 /// The W x H x Resolution fields of the options bar: W, ⇄, H, the resolution and its unit.
-pub fn fields(o: &mut ToolOptions, ui: &mut egui::Ui) {
+/// Returns true when ⇄ swapped W and H, so the Crop tool turns its frame with them.
+pub fn fields(o: &mut ToolOptions, ui: &mut egui::Ui) -> bool {
     text_field(ui, "crop-width", &mut o.crop_width, tl!("Width"), 64.0, normalize_length);
-    if crate::icons::button(ui, "arrow-left-right", 22.0, false, tl!("Swaps height and width")).clicked() {
+    // Always swaps (Perspective Crop shows the fields without the mode); true in W x H x Resolution
+    // mode, where the Crop tool turns its frame with W and H.
+    let swapped = crate::icons::button(ui, "arrow-left-right", 22.0, false, tl!("Swaps height and width")).clicked() && {
         swap_fields(o);
-    }
+        o.crop_ratio == WHR
+    };
     text_field(ui, "crop-height", &mut o.crop_height, tl!("Height"), 64.0, normalize_length);
     text_field(ui, "crop-resolution", &mut o.crop_resolution, tl!("Resolution"), 64.0, normalize_resolution);
     let units = [(PX_PER_IN.to_string(), "px/in"), (PX_PER_CM.to_string(), "px/cm")];
@@ -331,6 +335,7 @@ pub fn fields(o: &mut ToolOptions, ui: &mut egui::Ui) {
             o.crop_resolution = crate::widgets::fmt_num2(v);
         }
     }
+    swapped
 }
 
 #[cfg(test)]

crates/ui-egui/src/tools/crop_ui.rs

@@ -884,6 +884,18 @@ pub fn swapped(r: [f64; 4]) -> [f64; 4] {
     [c[0] - hh, c[1] - hw, c[0] + hh, c[1] + hw]
 }
 
+/// The options bar's ⇄ with a ratio set: the frame turns with the ratio about its centre, as X
+/// does, so fitting the swapped ratio afterwards doesn't shrink it (#7738).
+pub(crate) fn turn_frame(app: &mut PhotocraftApp) {
+    // A square frame doesn't change, so an untouched default frame stays one (and isn't
+    // committed on leaving the tool).
+    if let Some(r) = app.ui.crop_rect.filter(|_| takes_keys(app) && preset_ratio(app).is_some())
+        && swapped(r) != r
+    {
+        keyed(app, swapped(r));
+    }
+}
+
 /// The options-bar ratio `key` with its sides swapped (what the bar's ⇄ button does), `None` when
 /// no ratio is set; "original" becomes the document's size turned on its side.
 pub fn swapped_ratio(key: &str, doc_w: f64, doc_h: f64) -> Option<String> {
@@ -903,8 +915,11 @@ pub fn swap_orientation(app: &mut PhotocraftApp) -> bool {
     {
         app.ui.tool_options.crop_ratio = k;
     }
-    // The frame turns first: fitting the swapped ratio inside the old frame would shrink it.
-    keyed(app, swapped(r));
+    // The frame turns first: fitting the swapped ratio inside the old frame would shrink it. A
+    // square frame doesn't change, so an untouched default frame stays one.
+    if swapped(r) != r {
+        keyed(app, swapped(r));
+    }
     if preset_ratio(app).is_some() {
         apply_ratio(app);
     } else {

crates/ui-egui/src/tools/crop_ui/tests.rs

@@ -1288,6 +1288,94 @@ fn x_swaps_the_frame_orientation_not_the_colours() {
     assert!(!swap_orientation(&mut app));
 }
 
+/// #7738: with a ratio set, the options bar's ⇄ turns the frame with the ratio, as X does,
+/// instead of fitting the swapped ratio inside the old frame; a second click brings it back.
+#[test]
+fn the_swap_button_turns_the_ratio_frame() {
+    use egui_kittest::kittest::Queryable;
+    let a = app(SampleType::U8);
+    let mut h = swap_button_harness(a, "16:9", [60.0, 27.5, 140.0, 72.5]);
+    let near = |a: Option<[f64; 4]>, b: [f64; 4]| a.is_some_and(|a| a.iter().zip(b).all(|(x, y)| (x - y).abs() < 1e-9));
+    h.get_by_label("Swaps height and width").click();
+    h.run_steps(2);
+    assert!(near(h.state().ui.crop_rect, [77.5, 10.0, 122.5, 90.0]), "45 x 80, not shrunk: {:?}", h.state().ui.crop_rect);
+    h.get_by_label("Swaps height and width").click();
+    h.run_steps(2);
+    assert!(near(h.state().ui.crop_rect, [60.0, 27.5, 140.0, 72.5]), "and back: {:?}", h.state().ui.crop_rect);
+}
+
+/// With W x H x Resolution the bar's ⇄ swaps W and H and turns the frame with them, as X
+/// does, instead of fitting the turned size inside the old frame.
+#[test]
+fn the_swap_button_turns_a_size_preset_frame() {
+    use egui_kittest::kittest::Queryable;
+    let whr = |a: &mut PhotocraftApp| {
+        let o = &mut a.ui.tool_options;
+        (o.crop_width, o.crop_height, o.crop_resolution) = ("1600 px".into(), "900 px".into(), "72".into());
+    };
+    let mut a = app(SampleType::U8);
+    whr(&mut a);
+    let mut h = swap_button_harness(a, crate::crop_size::WHR, [60.0, 27.5, 140.0, 72.5]);
+    let near = |a: Option<[f64; 4]>, b: [f64; 4]| a.is_some_and(|a| a.iter().zip(b).all(|(x, y)| (x - y).abs() < 1e-9));
+    h.get_by_label("Swaps height and width").click();
+    h.run_steps(2);
+    let o = &h.state().ui.tool_options;
+    assert_eq!((o.crop_ratio.as_str(), o.crop_width.as_str(), o.crop_height.as_str()), (crate::crop_size::WHR, "900 px", "1600 px"));
+    assert!(near(h.state().ui.crop_rect, [77.5, 10.0, 122.5, 90.0]), "45 x 80, not shrunk: {:?}", h.state().ui.crop_rect);
+    h.get_by_label("Swaps height and width").click();
+    h.run_steps(2);
+    assert!(near(h.state().ui.crop_rect, [60.0, 27.5, 140.0, 72.5]), "and back: {:?}", h.state().ui.crop_rect);
+    // X does the same.
+    let mut a = app(SampleType::U8);
+    whr(&mut a);
+    a.ui.tool_options.crop_ratio = crate::crop_size::WHR.into();
+    a.ui.crop_rect = Some([60.0, 27.5, 140.0, 72.5]);
+    assert!(swap_orientation(&mut a));
+    assert_eq!((a.ui.tool_options.crop_width.as_str(), a.ui.tool_options.crop_height.as_str()), ("900 px", "1600 px"));
+    assert!(near(a.ui.crop_rect, [77.5, 10.0, 122.5, 90.0]), "X: 45 x 80, not shrunk: {:?}", a.ui.crop_rect);
+}
+
+/// A square default frame doesn't turn, so ⇄ leaves it untouched: not edited, and leaving the
+/// tool drops it instead of cropping.
+#[test]
+fn the_swap_button_leaves_a_square_default_frame_untouched() {
+    use egui_kittest::kittest::Queryable;
+    let mut a = app(SampleType::U8);
+    a.crop.default_frame = true;
+    a.crop.editing = false;
+    let mut h = swap_button_harness(a, "1:1", [50.0, 0.0, 150.0, 100.0]);
+    h.get_by_label("Swaps height and width").click();
+    h.run_steps(2);
+    assert_eq!(h.state().ui.crop_rect, Some([50.0, 0.0, 150.0, 100.0]));
+    assert!(h.state().crop.default_frame && !h.state().crop.editing, "still the untouched default frame");
+    // X, the same swap from the keyboard.
+    let mut a = app(SampleType::U8);
+    a.crop.default_frame = true;
+    a.crop.editing = false;
+    a.crop.frame_for = a.session.active_index().zip(a.session.active()).map(|(i, st)| (i, st.doc.size.width, st.doc.size.height));
+    a.ui.tool_options.crop_ratio = "1:1".into();
+    a.ui.crop_rect = Some([50.0, 0.0, 150.0, 100.0]);
+    assert!(swap_orientation(&mut a));
+    assert_eq!(a.ui.crop_rect, Some([50.0, 0.0, 150.0, 100.0]));
+    assert!(a.crop.default_frame && !a.crop.editing, "X keeps it untouched too");
+}
+
+fn swap_button_harness(mut a: PhotocraftApp, ratio: &str, rect: [f64; 4]) -> egui_kittest::Harness<'static, PhotocraftApp> {
+    a.ui.tool_options.crop_ratio = ratio.into();
+    a.ui.crop_rect = Some(rect);
+    let mut h = egui_kittest::Harness::builder().with_size(egui::vec2(1600.0, 60.0)).build_ui_state(
+        |ui, app: &mut PhotocraftApp| {
+            if ui.ctx().fonts(|f| f.families().contains(&egui::FontFamily::Name("medium".into()))) {
+                crate::panels::options_bar(app, ui);
+            }
+        },
+        a,
+    );
+    PhotocraftApp::setup_context(&h.ctx, crate::theme::ThemeKind::ProMedium);
+    h.run_steps(4);
+    h
+}
+
 /// #1919: W × H beside the pointer while the frame is drawn or resized (its own size when
 /// turned), not while it moves or turns or between gestures.
 #[test]

crates/ui-egui/tests/it/crop_default_mode.rs

@@ -138,18 +138,17 @@ fn the_swap_button_and_x_swap_the_ratio_about_the_centre() {
     let before = mid(screen_box(&h));
     h.get_by_label("Swaps height and width").click();
     h.run_steps(3);
+    // Both turn the frame instead of fitting the turned ratio inside it (#7057, #7738).
     let button = h.state().ui.crop_rect.unwrap();
     assert_eq!(h.state().ui.tool_options.crop_ratio, "2:3");
-    assert!(((button[2] - button[0]) / (button[3] - button[1]) - 2.0 / 3.0).abs() < 1e-10);
-    assert!(button[0] >= original[0] && button[2] <= original[2] && button[1] >= original[1] && button[3] <= original[3]);
+    assert_eq!(button, [70.0, -20.0, 230.0, 220.0]);
     assert!(mid(screen_box(&h)).distance(before) < 0.01);
     h.state_mut().ui.crop_rect = Some(original);
     h.state_mut().ui.tool_options.crop_ratio = "3:2".into();
     h.run_steps(3);
     h.key_press(Key::X);
     h.run_steps(3);
-    // X turns the frame instead of fitting the turned ratio inside it (#7057).
-    assert_eq!(h.state().ui.crop_rect, Some([70.0, -20.0, 230.0, 220.0]));
+    assert_eq!(h.state().ui.crop_rect, Some(button));
     assert!(mid(screen_box(&h)).distance(before) < 0.01);
 }