Merge PR #2280: #2253: crop ratios keep the orientation asked for

docs/parity-checklist.md: LR-CROP-ASPECT keeps #2253's orientation note and #991's fresh-crop lock
docs/whats-new.md: 2026-10-18 entry placed between 2026-10-19 and 2026-10-15

crates/engine/src/cmd/develop.rs

@@ -77,13 +77,22 @@ fn aspect_tuple(c: &str, x: f64, y: f64) -> Result<(u32, u32)> {
     Ok((a as u32, b as u32))
 }
 
-fn set_aspect(d: &mut DevelopSettings, w: f64, h: f64, aspect: Option<(u32, u32)>) {
+/// How a new aspect ratio is turned (issue #2253).
+#[derive(Clone, Copy, PartialEq)]
+enum Turn {
+    /// As the crop is now: a named ratio ("2x3", "original"…) lands landscape on a landscape crop
+    /// and portrait on a portrait one (after X), as in Lightroom. A square crop follows the image.
+    LikeCrop,
+    /// As given: a custom `[w, h]` taller than wide is a portrait crop.
+    AsGiven,
+}
+
+fn set_aspect(d: &mut DevelopSettings, w: f64, h: f64, aspect: Option<(u32, u32)>, turn: Turn) {
+    let r = d.crop.geometry.rect_px(w, h);
+    let landscape = if (r.width() - r.height()).abs() > 1e-6 * r.width().max(r.height()) { r.width() > r.height() } else { w >= h };
+    let aspect = aspect.map(|(aw, ah)| if turn == Turn::LikeCrop && (aw >= ah) != landscape { (ah, aw) } else { (aw, ah) });
     d.crop.aspect = aspect;
-    let a = aspect.map(|(aw, ah)| {
-        // orientation of the crop follows the image (landscape image → landscape crop)
-        let (aw, ah) = (aw as f64, ah as f64);
-        if (w >= h) == (aw >= ah) { aw / ah } else { ah / aw }
-    });
+    let a = aspect.map(|(aw, ah)| aw as f64 / ah as f64);
     let angle = d.crop.geometry.angle;
     d.crop.geometry = crop_fit_angle(w, h, angle, a);
 }
@@ -628,11 +637,19 @@ pub fn specs() -> Vec<CommandSpec> {
             "Crop Aspect",
             [],
             None,
-            "{aspect: \"original\"|\"free\"|\"current\" (lock the crop's present shape)|\"toggle\" (lock ↔ free)|\"1x1\"|\"4x5\"|\"8.5x11\"|\"5x7\"|\"2x3\"|\"4x3\"|\"16x9\"|\"16x10\"|[w,h]}",
+            "{aspect: \"original\"|\"free\"|\"current\" (lock the crop's present shape)|\"toggle\" (lock ↔ free)|\"1x1\"|\"4x5\"|\"8.5x11\"|\"5x7\"|\"2x3\"|\"4x3\"|\"16x9\"|\"16x10\"|[w,h], orientation?: \"crop\"|\"given\"} — a named ratio takes the crop's present orientation (landscape, or portrait after X); [w,h] is used as given (taller than wide → portrait); `orientation` overrides either",
             has_active,
             |s, p| {
                 let id = active(s, "crop.aspect")?;
                 let (w, h) = crop_dims(s, id);
+                // checked first, whatever the aspect: a bad value is an error on every path
+                let turn = match p.get("orientation") {
+                    Some(Value::String(o)) if o == "crop" => Turn::LikeCrop,
+                    Some(Value::String(o)) if o == "given" => Turn::AsGiven,
+                    Some(_) => return Err(bad("crop.aspect", "orientation is \"crop\" or \"given\"")),
+                    None if matches!(p.get("aspect"), Some(Value::Array(_))) => Turn::AsGiven,
+                    None => Turn::LikeCrop,
+                };
                 let locked = s.develop_of(id).is_some_and(|d| d.crop.aspect.is_some());
                 let lock_current = match p.get("aspect").and_then(Value::as_str) {
                     Some("current") => true,
@@ -667,7 +684,7 @@ pub fn specs() -> Vec<CommandSpec> {
                     if aspect.is_none() {
                         d.crop.aspect = None;
                     } else {
-                        set_aspect(d, w, h, aspect);
+                        set_aspect(d, w, h, aspect, turn);
                     }
                     Ok(())
                 })

crates/engine/src/lib.rs

@@ -1299,6 +1299,8 @@ mod tests_constrain_crop;
 #[cfg(test)]
 mod tests_crop_angle;
 #[cfg(test)]
+mod tests_crop_aspect;
+#[cfg(test)]
 mod tests_denoise;
 #[cfg(test)]
 mod tests_export;

crates/engine/src/tests_crop_aspect.rs

@@ -0,0 +1,86 @@
+//! Issue #2253: a crop ratio keeps the orientation asked for. A named ratio ("2x3", "4x5",
+//! "original") takes the crop's present orientation, landscape or portrait after X, as in
+//! Lightroom; a custom `[w, h]` is used as given, so 2 × 3 is a portrait crop.
+
+use serde_json::{Value, json};
+
+use crate::Session;
+
+/// The demo photo, landscape, with its crop's width and height in pixels.
+fn crop_px(s: &Session) -> (f64, f64) {
+    let p = s.catalog.photo(s.active().unwrap()).unwrap();
+    let (w, h) = (p.width as f64, p.height as f64);
+    let (w, h) = if p.develop.orientation.swaps_axes() { (h, w) } else { (w, h) };
+    let r = p.develop.crop.geometry.rect_px(w, h);
+    (r.width(), r.height())
+}
+
+fn landscape_photo() -> Session {
+    let mut s = Session::with_demo();
+    let id = s.catalog.photos().find(|p| p.width > p.height && !p.develop.orientation.swaps_axes()).map(|p| p.id).unwrap();
+    s.selection = crate::Selection { ids: vec![id], active: Some(id) };
+    s
+}
+
+fn aspect(s: &mut Session, p: Value) {
+    s.execute("crop.aspect", &p).unwrap();
+}
+
+fn ratio(s: &Session) -> f64 {
+    let (w, h) = crop_px(s);
+    w / h
+}
+
+#[test]
+fn a_named_ratio_follows_the_crop_and_x_keeps_it_portrait() {
+    let mut s = landscape_photo();
+    aspect(&mut s, json!({"aspect": "2x3"}));
+    assert!((ratio(&s) - 1.5).abs() < 0.01, "landscape crop: {}", ratio(&s));
+    s.execute("crop.rotateAspect", &json!({})).unwrap();
+    assert!((ratio(&s) - 2.0 / 3.0).abs() < 0.01, "X turns it portrait: {}", ratio(&s));
+    // another named ratio after X stays portrait (it used to turn back to the image's landscape)
+    aspect(&mut s, json!({"aspect": "4x5"}));
+    assert!((ratio(&s) - 0.8).abs() < 0.01, "still portrait: {}", ratio(&s));
+    aspect(&mut s, json!({"aspect": "16x9"}));
+    assert!((ratio(&s) - 9.0 / 16.0).abs() < 0.01, "{}", ratio(&s));
+}
+
+#[test]
+fn a_custom_ratio_is_used_as_given() {
+    let mut s = landscape_photo();
+    aspect(&mut s, json!({"aspect": [2, 3]}));
+    assert!((ratio(&s) - 2.0 / 3.0).abs() < 0.01, "custom 2 × 3 is portrait: {}", ratio(&s));
+    aspect(&mut s, json!({"aspect": [3, 2]}));
+    assert!((ratio(&s) - 1.5).abs() < 0.01, "{}", ratio(&s));
+}
+
+/// The window reapplies the last ratio to an untouched photo with `orientation: "crop"`: on a
+/// landscape photo it lands landscape, whatever photo it was chosen on.
+#[test]
+fn orientation_overrides_and_is_checked() {
+    let mut s = landscape_photo();
+    aspect(&mut s, json!({"aspect": [2, 3], "orientation": "crop"}));
+    assert!((ratio(&s) - 1.5).abs() < 0.01, "{}", ratio(&s));
+    aspect(&mut s, json!({"aspect": "2x3", "orientation": "given"}));
+    assert!((ratio(&s) - 2.0 / 3.0).abs() < 0.01, "{}", ratio(&s));
+    let e = s.execute("crop.aspect", &json!({"aspect": "2x3", "orientation": "sideways"})).unwrap_err();
+    assert!(e.to_string().contains("orientation"), "{e}");
+}
+
+/// A bad `orientation` is refused on every path, the lock-as-is ones ("current", an unlocked
+/// "toggle") included, and whether it is a wrong word or not a string at all; nothing changes.
+#[test]
+fn a_bad_orientation_is_refused_before_anything_is_locked() {
+    let mut s = landscape_photo();
+    let before = s.develop_of(s.selection.active.unwrap()).unwrap().crop.clone();
+    for aspect in [json!("current"), json!("toggle"), json!("2x3"), json!([2, 3]), json!("free")] {
+        for orientation in [json!("sideways"), json!(1), json!(true), json!(null)] {
+            let e = s.execute("crop.aspect", &json!({"aspect": aspect, "orientation": orientation})).unwrap_err();
+            assert!(e.to_string().contains("orientation"), "{aspect} {orientation}: {e}");
+            assert_eq!(s.develop_of(s.selection.active.unwrap()).unwrap().crop, before, "{aspect} {orientation}: untouched");
+        }
+    }
+    // the valid words still lock as is
+    aspect(&mut s, json!({"aspect": "current", "orientation": "crop"}));
+    assert!(s.develop_of(s.selection.active.unwrap()).unwrap().crop.aspect.is_some());
+}

crates/ui-egui/src/lib.rs

@@ -727,7 +727,8 @@ impl LightcraftApp {
         if self.session.interaction.is_some()
             && self.session.active().and_then(|id| self.session.develop_of(id)).is_some_and(|d| d.crop == lightcraft_develop::Crop::default())
             && aspect != serde_json::json!("free")
-            && let Err(e) = self.session.execute("crop.aspect", &serde_json::json!({"aspect": aspect}))
+            // the remembered ratio, turned like the new photo's crop (as a named ratio is)
+            && let Err(e) = self.session.execute("crop.aspect", &serde_json::json!({"aspect": aspect, "orientation": "crop"}))
         {
             // A remembered value the command refuses (a hand-edited settings file) would refuse
             // every crop drag on an untouched photo: forget it and let this drag go ahead.

docs/parity-checklist.md

@@ -250,7 +250,7 @@ differences and missing bindings are in [ui-parity.md](ui-parity.md#shortcuts).
 | Id | Feature | Tier | Status | Evidence | Missing / notes |
 |---|---|---|---|---|---|
 | LR-CROP-RECT | Crop rectangle | P0 | ✅ | `cmd:crop.set`, `cmd:crop.drag`, `crates/geom/src/crop_drag.rs`, `crates/ui-egui/src/panels/detail.rs` | double-click inside the box applies the crop (like Return) |
-| LR-CROP-ASPECT | Aspect ratios | P0 | ✅ | `cmd:crop.aspect`, `cmd:crop.rotateAspect`, `crates/ui-egui/src/panels/right.rs` | menu with presets + custom W × H, visible Lock toggle (also A), button names the active ratio; every handle honours the lock, holding Shift while dragging a handle keeps the frame's proportions for that drag (issue #614), and the drag stops at the image edge instead of shifting the frame (issue #295); "Original" is the photo's own shape (no separate As Shot); the last chosen aspect is saved as a crop tool default, resolving Original per photo and preserving existing crops; opening Crop or browsing never edits a photo: the default applies only when a crop gesture starts, within its single undo step; a crop starts locked to the photo's own shape until the Lock is switched off once, then as last set (#991) |
+| LR-CROP-ASPECT | Aspect ratios | P0 | ✅ | `cmd:crop.aspect`, `cmd:crop.rotateAspect`, `crates/ui-egui/src/panels/right.rs` | menu with presets + custom W × H (a preset takes the crop's orientation, portrait after X; a custom ratio is used as entered, issue #2253), visible Lock toggle (also A), button names the active ratio; every handle honours the lock, holding Shift while dragging a handle keeps the frame's proportions for that drag (issue #614), and the drag stops at the image edge instead of shifting the frame (issue #295); "Original" is the photo's own shape (no separate As Shot); the last chosen aspect is saved as a crop tool default, resolving Original per photo and preserving existing crops; opening Crop or browsing never edits a photo: the default applies only when a crop gesture starts, within its single undo step; a crop starts locked to the photo's own shape until the Lock is switched off once, then as last set (#991) |
 | LR-CROP-STRAIGHTEN | Straighten tool | P0 | ✅ | `cmd:crop.straighten`, `cmd:crop.autoStraighten`, `crates/ui-egui/src/panels/detail.rs` (`rotate_cursor`, `angle_readout`), `crates/ui-egui/src/panels/right.rs` (Angle field), `crates/ui-egui/src/tests_crop_rotate.rs` | Straighten Tool button: drag along a horizon/vertical; double-click or Auto levels automatically. Dragging outside the crop box rotates it: there the pointer is a curved double arrow that bulges away from the side of the box it is on, diagonally off a corner and following a rotated box (`rotate_cursor_turn`, tested by `rotate_cursor_faces_away_from_the_crop_box`) (with ⌘ held, the straighten crosshair), and while rotating the angle shows next to it, kept on the canvas (issue #534); an exact angle goes in the Angle field under Straighten (`cropAngleField`: applies on Return or leaving it, Esc keeps the old angle, takes a decimal comma; a drag on it is one undo step) or by clicking the Straighten value (its hover says so). Every way of setting the angle (rotate handle, Straighten slider, Angle field, `crop.straighten`, `crop.set {angle}`, and the generic `develop.set` / `develop.adjust` / `develop.quickAdjust` / `develop.resetControl` on `crop.angle`, as MCP, the control channel and `lightcraft-cli render --set` use them) keeps the crop if it still fits, else shrinks it about its own centre, keeping its aspect, until it fits; the full photo (or a crop an earlier turn shrank to the largest of its aspect) stays the largest, so it grows back towards 0° (issue #1388), and no other crop ever grows (issue #1794: a crop dragged in from a corner turned into nearly the whole photo) (`CropGeometry::with_angle`, `crates/engine/src/tests_crop_angle.rs`; before issue #742 the generic path left the box, and the export, past the photo with smeared edge pixels in the corners). The dimming outside the box is the photo minus the (rotated) box, drawn as one mesh (`outside_crop_mesh`), so rotating never dims wedges inside the box or doubles them outside (issue #731) |
 | LR-CROP-AUTO | Auto straighten | P1 | ✅ | `cmd:crop.autoStraighten` | crop-angle leveling from detected horizon/plumb lines (consensus required) |
 | LR-CROP-ANGLE | Angle slider | P0 | ✅ | `ctl:crop.angle`, `crates/ui-egui/src/panels/right.rs` | the Straighten slider; an exact angle can also be typed in the Angle field under it (see LR-CROP-STRAIGHTEN); setting `crop.angle` through `develop.set` fits the crop inside the photo as Straighten does (issue #742) |

docs/whats-new.md

@@ -6,6 +6,13 @@
 - With several locations removed from Local, "Show N hidden locations" now lists them, so each comes back on its own
   (Restore All is there too). The row used to bring every hidden location back at once (issue #2114).
 
+## 2026-10-18
+
+### Portrait crop ratios stay portrait
+- A crop ratio no longer flips to the photo's orientation (issue #2253): after X turns the crop portrait, picking
+  another ratio (4 × 5, 2 × 3…) keeps it portrait, as in Lightroom, and a Custom ratio taller than wide gives a
+  portrait crop.
+
 ## 2026-10-15
 
 ### Constrain Crop tightens the crop you see