From 7f524d2fd0c485d91d7c658373dddb7fcb5e7712 Mon Sep 17 00:00:00 2001 From: Duncan Tourolle Date: Mon, 17 Aug 2026 09:03:34 +0200 Subject: [PATCH] Give a mis-drag a way back MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Develop edits now save themselves to a sidecar the moment you leave the image, so until this there was no way to undo one — the mistake was persisted and the only recourse was to remember the old number. The history is a stack of snapshots, because the edit graph is already plain data: `Preset::capture` reduces it to what differs from default and `Preset::apply` puts it back, so undo is those two calls and nothing else. A command object per action, with an inverse beside it, would have been a second thing every operation had to register — and operations are declared in YAML precisely so that a new one needs no code written for it. A snapshot cannot fall behind them. The interesting part is coalescing. A slider drag emits an event per frame and must be one step, not forty. Nothing in the interface reports a gesture boundary — the same wall the render coalescing hit, and it is answered the same way rather than by threading a "finger is down" out of every slider, curve point and crop handle. What stands in for the boundary is the control plus recency: changes to the same control within 700 ms amend one step. Which control is "the same" is asked of the graph, not listed: an operation whose declared presentation claims a parameter is one where a single gesture moves several — a curve point carries an x and a y — so those coalesce as one widget. Nothing in the history names the tone curve. The compromise, and it is a real one: a control let go of and picked up again within the window is one step rather than two. Buying the other answer costs a gesture-boundary signal on every control, which is more surface than the difference is worth. The stack is bounded at 64 states for NFR-RES-1 — a develop session stays open for hours. Sixty-four rather than a byte cap: what is being bounded is steps a photographer would want back, and a byte cap would give the elaborate edit the shallowest history, which is exactly backwards. The session owns its history and every mutator records into it, so the callbacks in `lib.rs` cannot change the edit and forget to — with a dozen generic callbacks that would have been one press of undo away from wrong every time a control was added. Opening a photograph makes its stored edit the floor rather than a step: it is not work done in this sitting, and an undo reaching behind it would discard a previous session's edit and then save that on the way out. Not yet done, from FR-DEV-5: history is per-session and in memory, and there are no named snapshots. What mattered was that a saved mis-drag had no way back at all. Co-Authored-By: Claude Opus 5 --- core/dr-pipeline/src/history.rs | 609 ++++++++++++++++++++++++++++++++ core/dr-pipeline/src/lib.rs | 2 + ui/dr-ui/src/develop.rs | 67 +++- ui/dr-ui/src/lib.rs | 52 +++ ui/dr-ui/ui/app.slint | 51 +++ 5 files changed, 779 insertions(+), 2 deletions(-) create mode 100644 core/dr-pipeline/src/history.rs diff --git a/core/dr-pipeline/src/history.rs b/core/dr-pipeline/src/history.rs new file mode 100644 index 0000000..cb5a9e3 --- /dev/null +++ b/core/dr-pipeline/src/history.rs @@ -0,0 +1,609 @@ +//! TRACES: FR-DEV-5 +//! Stepping an edit backwards, and forwards again. +//! +//! # A stack of snapshots, not a stack of commands +//! +//! The edit graph is plain data, and [`Preset::capture`] already reduces it to +//! the values that differ from default. So a history is a list of those, and +//! undo is [`Preset::apply`] at full scope — the same two calls the clipboard +//! and the sidecar are built from. +//! +//! The alternative, a command per action with an inverse beside it, would be a +//! second thing every operation had to register. Operations are *declared* +//! (FR-DEV-3c): a new node is a YAML file and appears in the panel with no +//! code written for it, and it must appear in the history on the same terms. +//! A snapshot knows nothing about which operations exist, so it cannot fall +//! behind them. +//! +//! # What makes forty events one step +//! +//! A drag emits a change per frame. Recorded naively that is forty undo steps, +//! thirty-nine of which are positions the photographer's finger passed +//! through rather than decisions they made — and undo would rewind a gesture +//! at forty presses to the millimetre. +//! +//! Nothing reports a gesture boundary. No slider, curve point or crop handle +//! says "the finger is down", and threading that out of every control would be +//! a lot of surface for a bookkeeping concern — this is the same conclusion +//! `dr-ui`'s render coalescing reached, and it stands in the same place. What +//! stands in for the boundary here is **the control plus recency**: two +//! changes to the same control within [`COALESCE_WINDOW`] amend one step, and +//! anything else opens a new one. A control the user let go of and came back +//! to a second later is one step rather than two, and that is the honest cost +//! of not having the boundary; a pause long enough to be a decision is not. +//! +//! Which control is "the same control" is [`Edit`], and it is asked of the +//! graph rather than hardcoded: an operation whose +//! [`Presentation`](crate::Presentation) claims a parameter is an operation +//! where one gesture moves several at once — a curve +//! point carries an x and a y — so those coalesce as a single widget. Nothing +//! here names the tone curve. +//! +//! # What this is not, yet +//! +//! FR-DEV-5 also asks for history persisted with the catalog and named +//! snapshots. This is per-session and in memory: closing the image forgets it. +//! The gap that mattered was that an automatically saved mis-drag had no way +//! back at all, and that is what this closes. + +use std::time::{Duration, Instant}; + +use crate::descriptor::{OpId, ParamId}; +use crate::graph::EditGraph; +use crate::preset::{Preset, Scope}; + +/// TRACES: FR-DEV-5 | NFR-RES-1 +/// How many states are held, the current one included. +/// +/// Bounded because memory is a stated budget (NFR-RES-1) and a develop session +/// can be open for hours. A snapshot costs only what the edit moved off +/// default — a heavily worked frame with the colour mixer engaged is on the +/// order of a hundred entries, so the whole stack is a few hundred kilobytes +/// at worst and a few kilobytes in practice. +/// +/// Sixty-four rather than a number derived from a byte budget: the thing being +/// bounded is *steps a photographer would want back*, and past a few dozen the +/// answer is a preset or a reset rather than more presses of undo. A byte cap +/// would make the depth depend on how complicated the edit is, which is +/// exactly backwards — the elaborate edit is the one worth stepping through. +pub const DEPTH: usize = 64; + +/// TRACES: FR-DEV-5 +/// How long the same control may keep amending its own step. +/// +/// Above the pause a finger makes mid-drag — a slider is often held still +/// while the sharp frame catches up, and `dr-ui` waits 120 ms before drawing +/// it — and below the pause that reads as having finished and thought again. +pub const COALESCE_WINDOW: Duration = Duration::from_millis(700); + +/// TRACES: FR-DEV-5 +/// Which control a change came from, for deciding whether two changes are one +/// gesture. +/// +/// Not "what changed" — the snapshot already carries that. This exists purely +/// so that consecutive changes can be told apart from one continuing change. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Edit { + /// One parameter's own control, dragged. + Param(OpId, ParamId), + /// A whole operation, where one gesture moves several of its parameters at + /// once — a curve point, or a crop rectangle's four edges. + Op(OpId), + /// A change with no gesture behind it: a reset, a paste, a flip, a quarter + /// turn. Never coalesces, not even with an identical one, because two + /// clicks are two decisions however quickly they follow each other. + Discrete, +} + +impl Edit { + /// The key a change to one parameter coalesces under. + /// + /// A parameter a [`Presentation`](crate::Presentation) claims is not + /// dragged on its own — the widget owning it moves its siblings in the + /// same gesture — so the key is the operation. Asked of the graph rather + /// than listed here, so an operation that declares a compound widget gets + /// the right undo granularity by declaring it (FR-DEV-3c). + pub fn for_param(graph: &EditGraph, op: OpId, param: ParamId) -> Self { + let grouped = graph + .capabilities() + .into_iter() + .any(|cap| cap.id == op && cap.presentation.is_some_and(|p| p.params.contains(¶m))); + if grouped { + Self::Op(op) + } else { + Self::Param(op, param) + } + } + + /// Whether a second change to this same control continues the first. + fn is_gesture(self) -> bool { + !matches!(self, Self::Discrete) + } +} + +/// TRACES: FR-DEV-5 +/// One image's undo stack. +/// +/// Holds *states*, not differences: `states[cursor]` is what the graph shows +/// now, everything before it is where undo goes, and everything after it is +/// where redo goes. `states` is never empty — the state the history was opened +/// on is the floor, and undo stops there rather than at nothing. +pub struct History { + states: Vec, + cursor: usize, + /// What produced `states[cursor]`, and when — the pair that decides + /// whether the next change amends it. `None` means the current step is + /// closed: nothing may be folded into it. + last: Option<(Edit, Instant)>, +} + +impl History { + /// Start from the state `graph` is in. + pub fn new(graph: &EditGraph) -> Self { + Self { + states: vec![Preset::capture(graph)], + cursor: 0, + last: None, + } + } + + /// Forget everything and take `graph` as the new floor. + /// + /// What opening an image does. A photograph's stored edit is not something + /// the user did in this session, so undo must not reach behind it and + /// silently discard work from a previous one. + pub fn reset(&mut self, graph: &EditGraph) { + *self = Self::new(graph); + } + + /// Note that `edit` has just changed `graph`. + /// + /// Returns whether a step was opened or amended — `false` when the graph + /// holds what it already held, which happens whenever a control re-emits + /// its current value or a clamp swallows a movement. + pub fn record(&mut self, graph: &EditGraph, edit: Edit) -> bool { + self.record_at(graph, edit, Instant::now()) + } + + /// [`Self::record`] with the clock supplied, so coalescing can be tested + /// without sleeping. + pub fn record_at(&mut self, graph: &EditGraph, edit: Edit, at: Instant) -> bool { + let state = Preset::capture(graph); + if self.states.get(self.cursor) == Some(&state) { + return false; + } + + if self.coalesces(edit, at) { + if let Some(top) = self.states.get_mut(self.cursor) { + *top = state; + self.last = Some((edit, at)); + return true; + } + } + + // A new step abandons the redo tail: the future that was undone away + // is no longer reachable from here, and keeping it would let redo jump + // to a state this one was never derived from. + self.states.truncate(self.cursor + 1); + self.states.push(state); + self.cursor = self.states.len().saturating_sub(1); + + // Oldest first, so what is lost is the part furthest from where the + // user is working. + let excess = self.states.len().saturating_sub(DEPTH); + if excess > 0 { + self.states.drain(..excess); + self.cursor = self.cursor.saturating_sub(excess); + } + + self.last = Some((edit, at)); + true + } + + /// Whether `edit` at `at` continues the step already on top. + fn coalesces(&self, edit: Edit, at: Instant) -> bool { + // Never over the floor. The state the image opened in is what the + // first undo has to return to, and folding the first change into it + // would make that state unreachable. + if self.cursor == 0 { + return false; + } + let Some((last, when)) = self.last else { + return false; + }; + edit.is_gesture() && last == edit && at.saturating_duration_since(when) <= COALESCE_WINDOW + } + + pub fn can_undo(&self) -> bool { + self.cursor > 0 + } + + pub fn can_redo(&self) -> bool { + self.cursor + 1 < self.states.len() + } + + /// Step `graph` back one, returning whether there was anywhere to go. + /// + /// Applied at [`Scope::Everything`]: framing is as undoable as colour, and + /// a crop drag the user wants back is one of the likelier reasons to + /// reach for this. The view survives, because [`Preset::apply`] preserves + /// it — an undo that also jumped the viewport would read as navigation. + pub fn undo(&mut self, graph: &mut EditGraph) -> bool { + let Some(target) = self.cursor.checked_sub(1) else { + return false; + }; + self.restore(graph, target) + } + + /// Step `graph` forward one, returning whether there was anywhere to go. + pub fn redo(&mut self, graph: &mut EditGraph) -> bool { + self.restore(graph, self.cursor + 1) + } + + fn restore(&mut self, graph: &mut EditGraph, target: usize) -> bool { + let Some(state) = self.states.get(target) else { + return false; + }; + state.apply(graph, Scope::Everything); + self.cursor = target; + // Closes the current step. Without this, a slider moved immediately + // after an undo would fold into the step it was just undone out of — + // and the state the user had just recovered would be overwritten by + // the very edit they made from it. + self.last = None; + true + } + + /// How many states are held, the current one included. + /// + /// Never zero. Reported rather than inferred so a caller can say how far + /// back it can go without walking the stack. + pub fn depth(&self) -> usize { + self.states.len() + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::framing; + use crate::ops::{curve, exposure, saturation}; + use crate::CropRect; + + fn at(base: Instant, ms: u64) -> Instant { + base + Duration::from_millis(ms) + } + + /// A slider dragged from `from` to `to` in `steps`, one event per frame at + /// 60 Hz — what the coalescing has to survive. + fn drag(h: &mut History, g: &mut EditGraph, base: Instant, from: f32, to: f32, steps: u32) { + for i in 1..=steps { + let v = from + (to - from) * (i as f32 / steps as f32); + g.set_param(exposure::ID, exposure::EXPOSURE, v); + h.record_at( + g, + Edit::for_param(g, exposure::ID, exposure::EXPOSURE), + at(base, u64::from(i) * 16), + ); + } + } + + #[test] + fn a_whole_drag_of_one_slider_is_a_single_undo_step() { + // The reason this type is not a plain stack. A drag emits an event per + // frame; recorded one apiece, undo would rewind the gesture in + // millimetres and forty presses would not reach the start of it. + let mut g = EditGraph::default_chain(); + let mut h = History::new(&g); + let base = Instant::now(); + + drag(&mut h, &mut g, base, 0.0, 1.5, 40); + + assert_eq!(h.depth(), 2, "the drag should have added exactly one state"); + assert!(h.undo(&mut g)); + assert_eq!(g.param(exposure::ID, exposure::EXPOSURE), Some(0.0)); + assert!(!h.can_undo(), "undo went behind the state it opened in"); + } + + #[test] + fn a_pause_ends_the_gesture_so_the_next_move_is_its_own_step() { + // The other half of coalescing. Without a time bound, every change a + // photographer ever made to the exposure slider — across the whole + // session — would collapse into one step, and undo would throw away an + // hour of decisions in a single press. + let mut g = EditGraph::default_chain(); + let mut h = History::new(&g); + let base = Instant::now(); + + g.set_param(exposure::ID, exposure::EXPOSURE, 1.0); + h.record_at(&g, Edit::Param(exposure::ID, exposure::EXPOSURE), base); + + g.set_param(exposure::ID, exposure::EXPOSURE, 2.0); + h.record_at( + &g, + Edit::Param(exposure::ID, exposure::EXPOSURE), + at(base, COALESCE_WINDOW.as_millis() as u64 + 1), + ); + + assert_eq!(h.depth(), 3); + assert!(h.undo(&mut g)); + assert_eq!(g.param(exposure::ID, exposure::EXPOSURE), Some(1.0)); + } + + #[test] + fn two_controls_moved_in_quick_succession_are_two_steps() { + // Coalescing keys on the control, not on time alone. Were it time + // alone, reaching straight from exposure to saturation would fuse two + // unrelated decisions and undo would take back the one the user did + // not ask about. + let mut g = EditGraph::default_chain(); + let mut h = History::new(&g); + let base = Instant::now(); + + g.set_param(exposure::ID, exposure::EXPOSURE, 1.0); + h.record_at(&g, Edit::Param(exposure::ID, exposure::EXPOSURE), base); + g.set_param(saturation::ID, saturation::SATURATION, 30.0); + h.record_at( + &g, + Edit::Param(saturation::ID, saturation::SATURATION), + at(base, 16), + ); + + assert!(h.undo(&mut g)); + assert_eq!(g.param(saturation::ID, saturation::SATURATION), Some(0.0)); + assert_eq!( + g.param(exposure::ID, exposure::EXPOSURE), + Some(1.0), + "undoing the saturation must not take the exposure with it" + ); + } + + #[test] + fn a_curve_point_drag_is_one_step_although_it_moves_two_parameters() { + // A curve point is dragged in x and y together, so the events + // alternate between two parameters. Keyed per parameter, *every* event + // would look like a new control and the coalescing would never fire — + // which is the case that made the key ask the graph about the + // operation's presentation rather than assume one parameter per + // widget. + let mut g = EditGraph::default_chain(); + let mut h = History::new(&g); + let base = Instant::now(); + + for i in 1..=20u32 { + let t = i as f32 / 40.0; + for (param, value) in [(curve::P1_X, 0.25 + t * 0.1), (curve::P1_Y, 0.25 + t * 0.3)] { + g.set_param(curve::ID, param, value); + h.record_at( + &g, + Edit::for_param(&g, curve::ID, param), + at(base, u64::from(i) * 16), + ); + } + } + + assert_eq!(h.depth(), 2, "the curve drag fragmented into steps"); + } + + #[test] + fn a_crop_drag_is_one_step_and_undo_gives_the_composition_back() { + // Framing is undoable on the same terms as colour. A crop is the edit + // a mis-drag destroys most visibly, and it is stored as four + // parameters moved by one gesture — so it is keyed on the operation. + let mut g = EditGraph::default_chain(); + let mut h = History::new(&g); + let base = Instant::now(); + + for i in 1..=15u32 { + let w = 1.0 - i as f32 * 0.04; + g.set_crop(CropRect { + x: 0.0, + y: 0.0, + width: w, + height: w, + }); + h.record_at(&g, Edit::Op(framing::ID), at(base, u64::from(i) * 16)); + } + + assert_eq!(h.depth(), 2); + assert!(h.undo(&mut g)); + assert!( + g.crop().is_full(), + "the frame did not come back: {:?}", + g.crop() + ); + } + + #[test] + fn a_reset_never_folds_into_the_change_before_it() { + // `Discrete` must not coalesce even with itself. Two resets in quick + // succession — the geometry section then the panel — are two actions, + // and folding them would make one press of undo restore neither + // completely. + let mut g = EditGraph::default_chain(); + let mut h = History::new(&g); + let base = Instant::now(); + + g.set_param(exposure::ID, exposure::EXPOSURE, 1.0); + h.record_at(&g, Edit::Discrete, base); + g.set_param(saturation::ID, saturation::SATURATION, 20.0); + h.record_at(&g, Edit::Discrete, at(base, 5)); + + assert_eq!(h.depth(), 3); + } + + #[test] + fn a_change_that_moves_nothing_opens_no_step() { + // A slider re-emitting its current value, or a drag pushed past a + // clamp so the graph stops moving. Recorded anyway, undo would need + // several presses to visibly do anything, and holding a slider against + // its end stop would quietly consume the whole history. + let mut g = EditGraph::default_chain(); + let mut h = History::new(&g); + + g.set_param(exposure::ID, exposure::EXPOSURE, 99.0); + assert!(h.record(&g, Edit::Discrete)); + // Clamped at the descriptor's ceiling, so this asks for no movement. + g.set_param(exposure::ID, exposure::EXPOSURE, 120.0); + assert!(!h.record(&g, Edit::Discrete)); + assert_eq!(h.depth(), 2); + } + + #[test] + fn redo_puts_back_exactly_what_undo_took() { + // Checked over the whole chain rather than the parameter that moved: + // undo restores by *replacing* the graph's state, so an operation that + // survived the round trip only because it happened to be neutral would + // pass a narrower assertion. + let mut g = EditGraph::default_chain(); + let mut h = History::new(&g); + + g.set_param(exposure::ID, exposure::EXPOSURE, 1.25); + g.set_param(saturation::ID, saturation::SATURATION, -40.0); + g.set_param(framing::ID, framing::ANGLE, 3.0); + h.record(&g, Edit::Discrete); + let after = Preset::capture(&g); + + assert!(h.undo(&mut g)); + assert!(h.redo(&mut g)); + assert_eq!(Preset::capture(&g), after); + assert!(!h.can_redo()); + } + + #[test] + fn editing_after_an_undo_discards_the_future() { + // The branch the user chose not to take must not be reachable. Left + // in place, redo would jump to a state derived from an edit that no + // longer exists — values from a version of the photograph that was + // never on screen. + let mut g = EditGraph::default_chain(); + let mut h = History::new(&g); + + g.set_param(exposure::ID, exposure::EXPOSURE, 1.0); + h.record(&g, Edit::Discrete); + assert!(h.undo(&mut g)); + + g.set_param(saturation::ID, saturation::SATURATION, 25.0); + h.record(&g, Edit::Discrete); + + assert!(!h.can_redo()); + assert_eq!(h.depth(), 2); + } + + #[test] + fn an_edit_straight_after_an_undo_does_not_overwrite_what_was_recovered() { + // Undo closes the open step. Without that, moving the same slider + // again within the coalescing window would amend the step the undo had + // just stepped out of, and the recovered state would be lost with no + // action having discarded it. + let mut g = EditGraph::default_chain(); + let mut h = History::new(&g); + let base = Instant::now(); + + g.set_param(exposure::ID, exposure::EXPOSURE, 1.0); + h.record_at(&g, Edit::Param(exposure::ID, exposure::EXPOSURE), base); + g.set_param(exposure::ID, exposure::EXPOSURE, 2.0); + h.record_at( + &g, + Edit::Param(exposure::ID, exposure::EXPOSURE), + at(base, 2000), + ); + + assert!(h.undo(&mut g)); + assert_eq!(g.param(exposure::ID, exposure::EXPOSURE), Some(1.0)); + + g.set_param(exposure::ID, exposure::EXPOSURE, 1.1); + h.record_at( + &g, + Edit::Param(exposure::ID, exposure::EXPOSURE), + at(base, 2010), + ); + + assert!(h.undo(&mut g)); + assert_eq!( + g.param(exposure::ID, exposure::EXPOSURE), + Some(1.0), + "the state the undo recovered was folded away" + ); + } + + #[test] + fn the_stack_is_bounded_and_forgets_the_oldest_first() { + // NFR-RES-1. A develop session stays open for hours, and an unbounded + // stack would grow with every gesture for all of them. What it drops + // is the far end, because that is the part furthest from where the + // user is working. + let mut g = EditGraph::default_chain(); + let mut h = History::new(&g); + let base = Instant::now(); + + for i in 1..=(DEPTH as u32 * 2) { + g.set_param(exposure::ID, exposure::EXPOSURE, i as f32 * 0.01); + h.record_at(&g, Edit::Discrete, at(base, u64::from(i) * 1000)); + } + + assert_eq!(h.depth(), DEPTH); + // Every step still present is reachable, and the walk stops at the + // floor rather than running off the end. + let mut steps = 0; + while h.undo(&mut g) { + steps += 1; + } + assert_eq!(steps, DEPTH - 1); + } + + #[test] + fn reopening_an_image_does_not_leave_the_previous_ones_history_behind() { + // A session is reused across photographs. Were the stack carried over, + // one press of undo on a freshly opened frame would apply the previous + // frame's edit to it — the paste nobody asked for. + let mut g = EditGraph::default_chain(); + let mut h = History::new(&g); + g.set_param(exposure::ID, exposure::EXPOSURE, 1.0); + h.record(&g, Edit::Discrete); + + let mut next = EditGraph::default_chain(); + next.set_param(saturation::ID, saturation::SATURATION, 10.0); + h.reset(&next); + + assert!(!h.can_undo()); + assert!(!h.can_redo()); + assert_eq!(h.depth(), 1); + } + + #[test] + fn undo_leaves_the_view_where_the_user_was_looking() { + // Zoom is navigation, not an edit — the same rule `Preset::apply` + // keeps for a paste. An undo that refitted the frame would read as + // having moved the photograph rather than having taken back a change. + let mut g = EditGraph::default_chain(); + let mut h = History::new(&g); + g.set_param(exposure::ID, exposure::EXPOSURE, 1.0); + h.record(&g, Edit::Discrete); + + g.framing_mut().set_view(CropRect { + x: 0.25, + y: 0.25, + width: 0.25, + height: 0.25, + }); + assert!(h.undo(&mut g)); + assert!(g.framing().is_zoomed(), "the undo threw away the viewport"); + } + + #[test] + fn the_key_for_an_ungrouped_parameter_is_the_parameter_itself() { + // The complement of the curve case: most operations are plain sliders, + // and keying those on the operation would fuse two of an operation's + // sliders — temperature and tint — into one undo step. + let g = EditGraph::default_chain(); + assert_eq!( + Edit::for_param(&g, exposure::ID, exposure::EXPOSURE), + Edit::Param(exposure::ID, exposure::EXPOSURE) + ); + assert_eq!( + Edit::for_param(&g, curve::ID, curve::P1_X), + Edit::Op(curve::ID) + ); + } +} diff --git a/core/dr-pipeline/src/lib.rs b/core/dr-pipeline/src/lib.rs index cd0bc28..1e22af6 100644 --- a/core/dr-pipeline/src/lib.rs +++ b/core/dr-pipeline/src/lib.rs @@ -34,6 +34,7 @@ pub mod descriptor; pub mod framing; pub mod graph; +pub mod history; pub mod lens; pub mod operation; pub mod ops; @@ -46,6 +47,7 @@ pub use descriptor::{ }; pub use framing::{CropRect, Framing}; pub use graph::{EditGraph, OpCapability, ParamCapability}; +pub use history::{Edit, History}; pub use lens::{compose_warps, ComposedWarp, Warp}; pub use operation::{ compose, compose_with_framing, Affects, ComposedShader, Helper, Operation, Uniform, diff --git a/ui/dr-ui/src/develop.rs b/ui/dr-ui/src/develop.rs index f03f4d3..e03bcee 100644 --- a/ui/dr-ui/src/develop.rs +++ b/ui/dr-ui/src/develop.rs @@ -13,8 +13,8 @@ use dr_decode::RawImage; use dr_gpu::{AdjustPass, DemosaicedImage, Demosaicer, GpuContext}; use dr_pipeline::ops::curve; use dr_pipeline::{ - CropRect, EditGraph, OpCapability, OpId, ParamId, ParamKind, Presentation, Preset, Scope, Unit, - WidgetKind, + CropRect, Edit, EditGraph, History, OpCapability, OpId, ParamId, ParamKind, Presentation, + Preset, Scope, Unit, WidgetKind, }; use crate::labels; @@ -23,6 +23,15 @@ use crate::ParamRow; /// A loaded image plus its edit state. pub struct DevelopSession { graph: EditGraph, + /// TRACES: FR-DEV-5 + /// Undo, kept beside the graph rather than in the window. + /// + /// Every mutator below records into it, so a caller cannot change the edit + /// and forget to. That is the whole reason it lives here: the callbacks in + /// `lib.rs` are generic by construction and there are a dozen of them, and + /// a history the *call sites* had to remember would be one press of undo + /// away from wrong every time a control is added. + history: History, demosaiced: DemosaicedImage, adjust: AdjustPass, } @@ -73,8 +82,10 @@ impl DevelopSession { ) -> Self { let mut graph = EditGraph::default_chain(); graph.set_orientation(orientation); + let history = History::new(&graph); Self { graph, + history, demosaiced, adjust: AdjustPass::new(ctx), } @@ -450,6 +461,9 @@ impl DevelopSession { for p in &cap.params { self.graph.set_param(cap.id, p.id, p.default); } + // One step, though it moved every parameter the operation has: the + // user pressed one button. + self.history.record(&self.graph, Edit::Discrete); } /// Reset a curve, which is to reset its operation. @@ -471,6 +485,8 @@ impl DevelopSession { return; }; self.graph.set_param(op, param, value); + let edit = Edit::for_param(&self.graph, op, param); + self.history.record(&self.graph, edit); } /// Return one parameter to its default. @@ -487,10 +503,12 @@ impl DevelopSession { .map(|p| p.default) .unwrap_or(0.0); self.graph.set_param(op, param, default); + self.history.record(&self.graph, Edit::Discrete); } pub fn reset_all(&mut self) { self.graph.reset(); + self.history.record(&self.graph, Edit::Discrete); } fn lookup(&self, op_index: i32, param_index: i32) -> Option<(OpId, ParamId)> { @@ -654,6 +672,10 @@ impl DevelopSession { /// Set the crop rectangle, in fractions of the source. pub fn set_crop(&mut self, rect: CropRect) { self.graph.set_crop(rect); + // Keyed on the operation, not on a parameter: one drag of one handle + // moves the origin and the extent together. + self.history + .record(&self.graph, Edit::Op(dr_pipeline::framing::ID)); } pub fn crop(&self) -> CropRect { @@ -673,6 +695,7 @@ impl DevelopSession { self.graph.set_crop(rotate_crop(crop, turns)); } self.graph.rotate_quarters(turns); + self.history.record(&self.graph, Edit::Discrete); } /// Straightening, in degrees. Positive turns the image clockwise. @@ -697,6 +720,7 @@ impl DevelopSession { dr_pipeline::framing::FLIP_H, f32::from(u8::from(!h)), ); + self.history.record(&self.graph, Edit::Discrete); } pub fn toggle_flip_v(&mut self) { @@ -706,6 +730,7 @@ impl DevelopSession { dr_pipeline::framing::FLIP_V, f32::from(u8::from(!v)), ); + self.history.record(&self.graph, Edit::Discrete); } /// Set the straightening angle, in degrees. @@ -715,6 +740,10 @@ impl DevelopSession { dr_pipeline::framing::ANGLE, degrees, ); + self.history.record( + &self.graph, + Edit::Param(dr_pipeline::framing::ID, dr_pipeline::framing::ANGLE), + ); } /// Whether the framing currently changes the image — what lights the @@ -737,6 +766,7 @@ impl DevelopSession { let view = self.graph.framing().view(); self.graph.framing_mut().reset(); self.graph.framing_mut().set_view(view); + self.history.record(&self.graph, Edit::Discrete); } /// How far the viewport is zoomed in: 1.0 fits the frame, 4.0 is 4×. @@ -859,6 +889,10 @@ impl DevelopSession { /// values the sliders are showing, and nothing here pushes them. pub fn apply_settings(&mut self, preset: &Preset, scope: Scope) { preset.apply(&mut self.graph, scope); + // A paste is undoable, and is the action most in need of it: it + // replaces everything in scope at once, so getting it wrong costs more + // than any single control can. + self.history.record(&self.graph, Edit::Discrete); } /// TRACES: FR-CAT-8 @@ -870,6 +904,35 @@ impl DevelopSession { /// orientation survives it, since that was never an edit. pub fn apply_version(&mut self, version: &dr_pipeline::Version) { version.apply(&mut self.graph); + // The stored edit becomes the floor rather than a step. It is not + // something the user did in this sitting, and an undo that reached + // behind it would discard a previous session's work in one press — + // then persist that on the way out, since saving is automatic. + self.history.reset(&self.graph); + } + + /// TRACES: FR-DEV-5 + /// Step the edit back one, returning whether anything moved. + /// + /// The panel must be rebuilt from [`Self::rows`] afterwards, for the same + /// reason a paste must: this moves values the controls are showing and + /// nothing here pushes them. + pub fn undo(&mut self) -> bool { + self.history.undo(&mut self.graph) + } + + /// TRACES: FR-DEV-5 + /// Step the edit forward one, returning whether anything moved. + pub fn redo(&mut self) -> bool { + self.history.redo(&mut self.graph) + } + + pub fn can_undo(&self) -> bool { + self.history.can_undo() + } + + pub fn can_redo(&self) -> bool { + self.history.can_redo() } } diff --git a/ui/dr-ui/src/lib.rs b/ui/dr-ui/src/lib.rs index d7acf76..5871583 100644 --- a/ui/dr-ui/src/lib.rs +++ b/ui/dr-ui/src/lib.rs @@ -256,6 +256,11 @@ fn reset_view_state(window: &AppWindow) { window.set_flip_h(false); window.set_flip_v(false); window.set_framing_modified(false); + // A photograph that failed to decode has no session, so nothing below + // will speak for it — and the buttons would otherwise keep offering the + // previous image's history. + window.set_can_undo(false); + window.set_can_redo(false); } /// Push the framing back to the geometry panel. @@ -961,6 +966,15 @@ pub fn run(paths: Vec) -> Result<()> { Rc::new(move |window: &AppWindow, draft: bool| { let mut slot = session.borrow_mut(); let Some(s) = slot.as_mut() else { return }; + + // TRACES: FR-DEV-5 + // Whether undo has anywhere to go, pushed from here because every + // edit ends in a redraw and nothing else is on all of their paths: + // the parameter callbacks sync rows, the framing ones sync the + // geometry panel, and a paste arrives through neither. + window.set_can_undo(s.can_undo()); + window.set_can_redo(s.can_redo()); + let (mut w, mut h) = *viewport.borrow(); // **Half resolution while the gesture is still moving.** @@ -1505,6 +1519,44 @@ pub fn run(paths: Vec) -> Result<()> { }); } + // ---- undo and redo (FR-DEV-5) --------------------------------------- + // + // Thin, because the history lives in the session and every mutator there + // records into it — see `DevelopSession::history`. What is left for the + // interface is the refresh a paste also needs: the controls are showing + // values that have just moved underneath them. + // + // `can-undo` and `can-redo` are not set here; `render_now` pushes them on + // every redraw, which is every path that can change them. + { + let weak = window.as_weak(); + let session = session.clone(); + let redraw = redraw.clone(); + let rows = rows.clone(); + window.on_undo(move || { + let Some(w) = weak.upgrade() else { return }; + let stepped = session.borrow_mut().as_mut().is_some_and(|s| s.undo()); + if stepped { + sync_rows(&w, &rows, &session); + redraw(&w); + } + }); + } + { + let weak = window.as_weak(); + let session = session.clone(); + let redraw = redraw.clone(); + let rows = rows.clone(); + window.on_redo(move || { + let Some(w) = weak.upgrade() else { return }; + let stepped = session.borrow_mut().as_mut().is_some_and(|s| s.redo()); + if stepped { + sync_rows(&w, &rows, &session); + redraw(&w); + } + }); + } + // ---- zoom, pan and crop --------------------------------------------- // // Zoom and pan are viewing state and touch no parameter, so unlike the diff --git a/ui/dr-ui/ui/app.slint b/ui/dr-ui/ui/app.slint index 1be7c1e..dce4e65 100644 --- a/ui/dr-ui/ui/app.slint +++ b/ui/dr-ui/ui/app.slint @@ -33,11 +33,16 @@ component StatusBar inherits Rectangle { /// dialogue: an export that succeeded needs no acknowledging, and one /// that failed needs its reason where the retry is. in property export-status; + /// Whether the edit can be stepped either way (FR-DEV-5). + in property can-undo: false; + in property can-redo: false; callback back-to-library(); callback open-settings(); callback toggle-panel(); callback export-image(); + callback undo(); + callback redo(); // 44px and `surface`, the same bar the library and settings draw. // @@ -86,6 +91,25 @@ component StatusBar inherits Rectangle { Caption { text: root.layout-class; } + // Undo and redo, in the strip rather than in the develop column: the + // panel can be put away, and the one control that takes back a + // mis-drag must not go away with it. Disabled rather than hidden, so + // the pair keeps its place and the keyboard shortcut has something + // visible to correspond to. + Button { + text: "Undo"; + enabled: root.can-undo; + y: (parent.height - self.height) / 2; + clicked => { root.undo(); } + } + + Button { + text: "Redo"; + enabled: root.can-redo; + y: (parent.height - self.height) / 2; + clicked => { root.redo(); } + } + // Show or hide the develop column. On a tablet the panel is 280px of a // screen that is mostly photograph, and the whole point of opening an // image is to look at it — so being able to put the instruments away @@ -253,6 +277,16 @@ export component AppWindow inherits Window { callback straighten-changed(float); /// Crop, angle, rotation and flips back to neutral, leaving colour alone. callback framing-reset(); + + // --- undo and redo (FR-DEV-5) --- + // + // Rust owns whether there is anywhere to step, because "anywhere" is a + // position in a stack of snapshots this side never sees. A whole drag is + // one step; the coalescing that makes it so lives in `dr-pipeline`. + in property can-undo: false; + in property can-redo: false; + callback undo(); + callback redo(); /// Scroll-to-zoom: factor, and the anchor in fractions of the visible area. callback zoom-at(float, float, float); callback pan-by(float, float); @@ -1015,10 +1049,14 @@ in property panel-visible: true; export-label: root.export-label; export-busy: root.export-busy; export-status: root.export-status; + can-undo: root.can-undo; + can-redo: root.can-redo; back-to-library() => { root.back-to-library(); } open-settings() => { root.settings-open(); } toggle-panel() => { root.toggle-panel(); } export-image() => { root.export-image(); } + undo() => { root.undo(); } + redo() => { root.redo(); } } HorizontalLayout { @@ -1371,6 +1409,19 @@ in property panel-visible: true; init => { self.focus(); } key-pressed(event) => { + // Ctrl+Z and Ctrl+Shift+Z (FR-DEV-5). Both cases + // of the letter, because the logical key that + // reaches us carries the shift: holding it for + // redo turns "z" into "Z". + if (event.modifiers.control + && (event.text == "z" || event.text == "Z")) { + if (event.modifiers.shift) { + root.redo(); + } else { + root.undo(); + } + return accept; + } if (event.text == Key.RightArrow || event.text == " ") { root.next-image(); return accept;