From 81f02a6947190fcaa27339ab69a6bae9c444fda2 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 18 Oct 2026 22:48:56 +0000 Subject: [PATCH] #2253: crop ratios keep the orientation asked for set_aspect always turned the ratio to the image's orientation, so a Custom ratio taller than wide came out landscape on a landscape photo, and a ratio picked after X (crop.rotateAspect) turned the crop back. A named ratio now takes the crop's present orientation (landscape, or portrait after X; a square crop follows the image), a custom [w, h] is used as given, and crop.aspect takes orientation: "crop" | "given" to choose. The window reapplies the remembered ratio to an untouched photo with orientation "crop", as before. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01AbegtYaPuwqZ81pGfELsFK --- crates/engine/src/cmd/develop.rs | 32 +++++++++--- crates/engine/src/lib.rs | 2 + crates/engine/src/tests_crop_aspect.rs | 68 ++++++++++++++++++++++++++ crates/ui-egui/src/lib.rs | 3 +- docs/parity-checklist.md | 2 +- docs/whats-new.md | 7 +++ 6 files changed, 104 insertions(+), 10 deletions(-) create mode 100644 crates/engine/src/tests_crop_aspect.rs diff --git a/crates/engine/src/cmd/develop.rs b/crates/engine/src/cmd/develop.rs index e3d59c567..38bf7a3da 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,7 +637,7 @@ 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")?; @@ -647,6 +656,13 @@ pub fn specs() -> Vec { Ok(()) }); } + let turn = match p.get("orientation").and_then(Value::as_str) { + Some("crop") => Turn::LikeCrop, + Some("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 aspect = match p.get("aspect") { Some(Value::String(a)) if a == "toggle" => None, Some(Value::String(a)) if a == "free" => None, @@ -667,7 +683,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 25e6ef52f..676544258 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -1251,6 +1251,8 @@ mod tests_color; #[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..3cfee0c05 --- /dev/null +++ b/crates/engine/src/tests_crop_aspect.rs @@ -0,0 +1,68 @@ +//! 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}"); +} diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index bb290bd86..83603e038 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -719,7 +719,8 @@ impl LightcraftApp { && self.session.active().and_then(|id| self.session.develop_of(id)).is_some_and(|d| d.crop == lightcraft_develop::Crop::default()) && let Some(aspect) = self.ui.crop_default_aspect.clone() && 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 4e0531793..3ebda6d38 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 | +| 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 | | 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 2f02ffb91..d99edeba3 100644 --- a/docs/whats-new.md +++ b/docs/whats-new.md @@ -1,5 +1,12 @@ # What's new in LightCraft +## 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-14 ### Model downloads time out on a name that doesn't resolve