From 76689a563a96e0b3be142feb4bc2645e9ad17557 Mon Sep 17 00:00:00 2001 From: wstock <1394687+wstock@users.noreply.github.com> Date: Thu, 15 Oct 2026 06:51:16 +0000 Subject: [PATCH] Crop: a fresh crop starts locked to the photo's own shape (#991) With no aspect ever chosen, every crop started free, so the Lock had to be switched on photo after photo. Nothing chosen yet now means locked to Original, as Lightroom starts; switching the Lock off once is remembered as free, and an explicit ratio is remembered as before. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_011K1peyrh5MdZ6ttHkVq8Gh --- crates/ui-egui/src/headless.rs | 8 +++-- crates/ui-egui/src/lib.rs | 8 +++-- crates/ui-egui/src/state.rs | 3 +- crates/ui-egui/src/tests_masking.rs | 49 +++++++++++++++++++++++++++++ docs/parity-checklist.md | 2 +- 5 files changed, 63 insertions(+), 7 deletions(-) diff --git a/crates/ui-egui/src/headless.rs b/crates/ui-egui/src/headless.rs index 6178990b0..5e9dbb981 100644 --- a/crates/ui-egui/src/headless.rs +++ b/crates/ui-egui/src/headless.rs @@ -2629,12 +2629,14 @@ mod tests { h.request("ui.key", json!({"key": "O", "shift": true}), t); assert_eq!(h.app.ui.crop_overlay_orient, 1); assert_eq!(h.app.ui.mask_overlay, mask_overlay, "O doesn't toggle the mask overlay while cropping"); - let locked = |h: &Headless| h.app.session.develop_of(h.app.session.active().unwrap()).unwrap().crop.aspect.is_some(); - assert!(!locked(&h)); - h.request("ui.key", json!({"key": "A"}), t); + // an untouched photo starts locked to its own shape (#991): A unlocks, A again locks + let locked = |h: &Headless| h.app.crop_tool_aspect().is_some(); assert!(locked(&h)); h.request("ui.key", json!({"key": "A"}), t); assert!(!locked(&h)); + assert_eq!(h.app.ui.crop_default_aspect, Some(json!("free"))); + h.request("ui.key", json!({"key": "A"}), t); + assert!(locked(&h)); assert!(!h.app.ui.visualize_spots); } diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index bb290bd86..4b1e2eb46 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -696,7 +696,10 @@ impl LightcraftApp { if d.crop != lightcraft_develop::Crop::default() { return d.crop.aspect; } - match self.ui.crop_default_aspect.as_ref()? { + // nothing chosen yet: locked to the photo's own shape, as Lightroom starts (#991); one + // Lock toggle (remembered as "free") unlocks it for good + let remembered = self.ui.crop_default_aspect.clone().unwrap_or_else(|| Value::String("original".into())); + match &remembered { Value::String(s) if s == "original" => { let p = self.session.catalog.photo(photo)?; let (w, h) = (p.width.max(1).saturating_mul(100), p.height.max(1).saturating_mul(100)); @@ -715,9 +718,10 @@ impl LightcraftApp { /// Called only after a crop gesture begins, inside its undo transaction. pub(crate) fn begin_crop(&mut self) -> Result<(), String> { + // nothing chosen yet: the photo's own shape, as `crop_tool_aspect` shows it + let aspect = self.ui.crop_default_aspect.clone().unwrap_or_else(|| serde_json::json!("original")); 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()) - && 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})) { diff --git a/crates/ui-egui/src/state.rs b/crates/ui-egui/src/state.rs index f8c8cc0d4..7268d8a2b 100644 --- a/crates/ui-egui/src/state.rs +++ b/crates/ui-egui/src/state.rs @@ -699,7 +699,8 @@ pub struct UiState { /// Mirroring of the triangle / spiral crop guides (0..4). pub crop_overlay_orient: u8, pub crop_overlay: CropOverlay, - /// Last explicitly chosen aspect, retained across photos and app launches. + /// Last explicitly chosen aspect, retained across photos and app launches; `None` (nothing + /// chosen yet) crops locked to the photo's own shape, as Lightroom starts (#991). pub crop_default_aspect: Option, pub show_filenames: bool, /// What the square grid's caption shows: `filename`, `exposure` (shutter · aperture · ISO · diff --git a/crates/ui-egui/src/tests_masking.rs b/crates/ui-egui/src/tests_masking.rs index bbea5356e..5ffc8d165 100644 --- a/crates/ui-egui/src/tests_masking.rs +++ b/crates/ui-egui/src/tests_masking.rs @@ -539,6 +539,55 @@ fn crop_drag_activates_the_remembered_ratio_and_undo_restores_the_photo() { assert_eq!(develop(&h), before); } +/// Issue #991: with no aspect ever chosen, a crop starts locked to the photo's own shape (Lightroom's +/// default), so a handle drag keeps the proportions; after the Lock is switched off once, the next +/// photo's crop starts free again. +#[test] +fn a_fresh_crop_starts_locked_to_original_until_the_lock_is_switched_off() { + let mut h = detail("panel.crop"); + h.app.ui.crop_default_aspect = None; + h.settle(SETTLE); + let before = develop(&h); + assert!(before.crop.aspect.is_none()); + let p = h.app.session.catalog.photo(h.app.session.active().unwrap()).unwrap().clone(); + let own = (p.width * 100, p.height * 100); + assert_eq!(h.app.crop_tool_aspect(), Some(own), "the Lock shows engaged on an untouched photo"); + let drag = json!({"events": [ + {"kind": "down", "x": 0.995, "y": 0.995}, + {"kind": "drag", "x": 0.85, "y": 0.85}, + {"kind": "drag", "x": 0.75, "y": 0.75}, + {"kind": "up", "x": 0.75, "y": 0.75} + ]}); + let r = h.request("ui.pointer", drag.clone(), T); + assert_eq!(r["ok"], true, "{r}"); + h.settle(SETTLE); + assert_eq!(develop(&h).crop.aspect, Some(own), "the drag kept the photo's proportions"); + assert_eq!(h.app.ui.crop_default_aspect, None, "nothing was chosen explicitly"); + exec(&mut h, "edit.undo", json!({})); + assert_eq!(develop(&h), before); + // Lock off once: remembered as free, and from then on the same drag crops without a ratio + let mut h = detail("panel.crop"); + h.app.ui.crop_default_aspect = None; + h.settle(SETTLE); + h.app.run("crop.aspect", json!({"aspect": "toggle"})).unwrap(); + h.settle(SETTLE); + assert_eq!(h.app.ui.crop_default_aspect, Some(json!("free"))); + assert_eq!(h.app.crop_tool_aspect(), None); + let before = develop(&h); + // a rotating drag outside the box (any crop drag starts cropping); the release on its own + for events in [ + json!([{"kind": "down", "x": -0.05, "y": 0.2}, {"kind": "drag", "x": -0.05, "y": 0.25}, {"kind": "drag", "x": -0.05, "y": 0.3}]), + json!([{"kind": "up", "x": -0.05, "y": 0.3}]), + ] { + let r = h.request("ui.pointer", json!({ "events": events }), T); + assert_eq!(r["ok"], true, "{r}"); + h.settle(SETTLE); + } + assert_ne!(develop(&h).crop, before.crop, "the drag cropped"); + assert_eq!(develop(&h).crop.aspect, None, "and left the ratio free"); + let _ = drag; +} + /// A remembered ratio the crop command refuses (a hand-edited settings file) is forgotten, and the /// drag still crops: it must not block every crop of an untouched photo. #[test] diff --git a/docs/parity-checklist.md b/docs/parity-checklist.md index 95dcd2aef..4f1ab2a64 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, 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) |