diff --git a/crates/engine/src/cmd/develop.rs b/crates/engine/src/cmd/develop.rs index e3d59c567..e4dc9e3fd 100644 --- a/crates/engine/src/cmd/develop.rs +++ b/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 { "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 { if aspect.is_none() { d.crop.aspect = None; } else { - set_aspect(d, w, h, aspect); + set_aspect(d, w, h, aspect, turn); } Ok(()) }) diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index c046df639..0bd5c03d7 100644 --- a/crates/engine/src/lib.rs +++ b/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; diff --git a/crates/engine/src/tests_crop_aspect.rs b/crates/engine/src/tests_crop_aspect.rs new file mode 100644 index 000000000..1b8698691 --- /dev/null +++ b/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()); +} diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index c19e85018..b953e1a85 100644 --- a/crates/ui-egui/src/lib.rs +++ b/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. diff --git a/docs/parity-checklist.md b/docs/parity-checklist.md index fa3b26f4e..2a5670b5a 100644 --- a/docs/parity-checklist.md +++ b/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) | diff --git a/docs/whats-new.md b/docs/whats-new.md index f6bec2f8d..3a9033456 100644 --- a/docs/whats-new.md +++ b/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