Turn the ratio lock with the photograph when it is turned
A quarter turn carries the crop with it — that is what makes turning a photograph keep its composition rather than sliding the selection onto a different part of the picture. So a rect locked to 16:9 comes out of the turn at 9:16, of a frame whose axes have also swapped, and the lock was left claiming landscape over a portrait rect. The next drag would then snap it back upright and undo what the turn had just done. The orientation switch now turns with it, on odd numbers of quarters. `Original` is deliberately excluded, and getting that wrong flips it twice: it is resolved against the framed size every time it is asked for, and the turn has already swapped that frame's axes — so it has turned by the time anything asks. `turns_with_the_frame` is the one predicate that separates the two cases, with a test that pins both. `CropAspect` arrived without tests of its own; it has them now, including the round trip this fixes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -644,6 +644,23 @@ impl CropAspect {
|
||||
!matches!(self, Self::Free | Self::Fixed(1, 1))
|
||||
}
|
||||
|
||||
/// TRACES: FR-DEV-3
|
||||
/// Whether a quarter turn of the frame has to flip the orientation switch
|
||||
/// to leave this ratio describing the same shape.
|
||||
///
|
||||
/// A quarter turn carries the crop with it — that is what makes turning a
|
||||
/// photograph keep its composition — so a rect locked to 16:9 comes out of
|
||||
/// the turn at 9:16, and the switch has to agree or the next drag would
|
||||
/// snap the crop back and undo the turn's effect on it.
|
||||
///
|
||||
/// `Original` is deliberately *not* included, and getting that wrong flips
|
||||
/// it twice. It is resolved against the framed size every time it is
|
||||
/// asked for, and a quarter turn swaps that frame's axes — so it has
|
||||
/// already turned by the time anything asks.
|
||||
pub fn turns_with_the_frame(self) -> bool {
|
||||
matches!(self, Self::Fixed(w, h) if w != h)
|
||||
}
|
||||
|
||||
/// Width over height in output pixels, or `None` where nothing is locked.
|
||||
///
|
||||
/// `frame` is the framed size the crop is measured against — the turned
|
||||
@@ -3848,6 +3865,102 @@ mod tests {
|
||||
use super::*;
|
||||
use dr_pipeline::EditGraph;
|
||||
|
||||
// --- the crop ratio lock ---------------------------------------------
|
||||
|
||||
#[test]
|
||||
fn a_locked_ratio_is_resolved_in_output_pixels() {
|
||||
// 3:2 means three pixels across to two down, whatever shape the frame
|
||||
// it is being cut out of happens to be.
|
||||
let landscape = CropAspect::Fixed(3, 2);
|
||||
assert_eq!(landscape.ratio((6000, 4000), false), Some(1.5));
|
||||
assert_eq!(landscape.ratio((4000, 6000), false), Some(1.5));
|
||||
// Stood on its short edge.
|
||||
assert_eq!(landscape.ratio((6000, 4000), true), Some(2.0 / 3.0));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn the_frames_own_ratio_follows_the_frame() {
|
||||
// What separates `Original` from naming the same numbers: it is right
|
||||
// on the next photograph from another body, and after a quarter turn.
|
||||
let a = CropAspect::Original;
|
||||
assert_eq!(a.ratio((6000, 4000), false), Some(1.5));
|
||||
assert_eq!(a.ratio((4000, 6000), false), Some(2.0 / 3.0));
|
||||
assert_eq!(a.ratio((5000, 5000), false), Some(1.0));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn free_locks_nothing() {
|
||||
assert_eq!(CropAspect::Free.ratio((6000, 4000), false), None);
|
||||
assert_eq!(CropAspect::Free.ratio((6000, 4000), true), None);
|
||||
assert!(!CropAspect::Free.has_orientation());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_square_has_no_second_orientation() {
|
||||
// Turning it would be a control that visibly does nothing, so the
|
||||
// switch is disabled and the flag is ignored either way.
|
||||
let square = CropAspect::Fixed(1, 1);
|
||||
assert!(!square.has_orientation());
|
||||
assert_eq!(square.ratio((6000, 4000), true), Some(1.0));
|
||||
assert_eq!(square.ratio((6000, 4000), false), Some(1.0));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn only_a_named_ratio_has_to_be_turned_with_the_frame() {
|
||||
// The distinction that stops `Original` being flipped twice: a quarter
|
||||
// turn swaps the frame's axes, so a ratio resolved *against* the frame
|
||||
// has already turned by the time anything asks it.
|
||||
assert!(CropAspect::Fixed(16, 9).turns_with_the_frame());
|
||||
assert!(CropAspect::Fixed(3, 2).turns_with_the_frame());
|
||||
assert!(!CropAspect::Original.turns_with_the_frame());
|
||||
assert!(!CropAspect::Free.turns_with_the_frame());
|
||||
assert!(!CropAspect::Fixed(1, 1).turns_with_the_frame());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn a_quarter_turn_leaves_a_locked_crop_the_shape_it_already_was() {
|
||||
// The whole reason the switch is flipped on a quarter turn. A crop
|
||||
// locked to 16:9 is carried through the turn by `rotate_crop`, coming
|
||||
// out at 9:16 of a frame whose axes have also swapped — so the lock
|
||||
// must now read as portrait, or the next drag would snap the crop back
|
||||
// upright and undo what the turn did to the composition.
|
||||
let (fw, fh) = (6000u32, 4000u32);
|
||||
let aspect = CropAspect::Fixed(16, 9);
|
||||
let before = CropRect::default().with_aspect(
|
||||
fw,
|
||||
fh,
|
||||
aspect.ratio((fw, fh), false).unwrap(),
|
||||
(0.5, 0.5),
|
||||
);
|
||||
|
||||
let after = rotate_crop(before, 1);
|
||||
let (tw, th) = (fh, fw);
|
||||
let got = (after.width * tw as f32) / (after.height * th as f32);
|
||||
let want = aspect.ratio((tw, th), true).unwrap();
|
||||
assert!(
|
||||
(got / want - 1.0).abs() < 1e-3,
|
||||
"turned crop is {got}, the flipped lock says {want}"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn every_offered_ratio_has_a_name_and_a_place() {
|
||||
// The chips are drawn from this list, so a duplicate would light two
|
||||
// at once and an empty label would draw a blank button.
|
||||
let mut seen = Vec::new();
|
||||
for a in CropAspect::CHOICES {
|
||||
assert!(!a.label().is_empty(), "{a:?} has no label");
|
||||
assert!(!seen.contains(&a), "{a:?} is offered twice");
|
||||
seen.push(a);
|
||||
}
|
||||
assert_eq!(
|
||||
CropAspect::CHOICES[0],
|
||||
CropAspect::Free,
|
||||
"free is the default"
|
||||
);
|
||||
assert_eq!(CropAspect::default(), CropAspect::Free);
|
||||
}
|
||||
|
||||
/// TRACES: FR-DSP-1 | AC-8
|
||||
/// Copy a displayed frame back to the CPU, for assertions and nothing else.
|
||||
///
|
||||
|
||||
@@ -2636,11 +2636,23 @@ pub fn run(paths: Vec<PathBuf>) -> Result<()> {
|
||||
let weak = window.as_weak();
|
||||
let session = session.clone();
|
||||
let redraw = redraw.clone();
|
||||
let crop_aspect = crop_aspect.clone();
|
||||
let crop_portrait = crop_portrait.clone();
|
||||
window.on_rotate_quarters(move |turns| {
|
||||
let Some(w) = weak.upgrade() else { return };
|
||||
if let Some(s) = session.borrow_mut().as_mut() {
|
||||
s.rotate_quarters(turns);
|
||||
}
|
||||
// TRACES: FR-DEV-3
|
||||
// The crop travels with the frame, so a rect locked to 16:9 comes
|
||||
// out of the turn at 9:16. The switch has to agree, or the next
|
||||
// drag would snap it back upright and undo what the turn did to
|
||||
// the composition. An even number of turns lands where it started.
|
||||
let aspect = crop_aspect.get();
|
||||
if aspect.turns_with_the_frame() && turns.rem_euclid(2) != 0 {
|
||||
crop_portrait.set(!crop_portrait.get());
|
||||
sync_crop_aspect(&w, &crop_aspect, &crop_portrait);
|
||||
}
|
||||
sync_framing(&w, &session);
|
||||
redraw(&w);
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user