List the steps, and let a photographer step straight to one
Build and test / Desktop (Linux) (push) Successful in 19m30s
Build and test / Layer separation (push) Successful in 25s
🐳 Android image / Build and push (push) Successful in 1s
Build and test / android-image (push) Successful in 1s
Traceability / Requirement traces (push) Successful in 23s
Build and test / Android (aarch64) (push) Failing after 33m5s
Build and test / Desktop (Linux) (push) Successful in 19m30s
Build and test / Layer separation (push) Successful in 25s
🐳 Android image / Build and push (push) Successful in 1s
Build and test / android-image (push) Successful in 1s
Traceability / Requirement traces (push) Successful in 23s
Build and test / Android (aarch64) (push) Failing after 33m5s
Undo answers "take back the last thing", which is the question asked about a mistake just noticed. It is the wrong instrument for one noticed six adjustments later: eight presses, each changing the picture, with no way to see how far back the mistake is without passing through it. A step is a whole state, so arriving from six away costs what arriving from one does — which is what makes a row worth making clickable rather than decorative. `Edit::Discrete` had to go for the list to be worth drawing. Seventeen call sites recorded the same anonymous step, which is fine for deciding whether two changes are one gesture and useless for a panel: seventeen rows reading "Discrete" is not a history. Every variant now carries enough to name itself, and the compiler enumerated the sites that had to start saying so. A step that moved a parameter is still named out of the descriptor, so an operation added as a YAML declaration appears in the history correctly named with nothing written for it (FR-DEV-3c). Choosing a film stock was not undoable at all. The pick went straight to `choose_film`, which nothing on the history's path ever sees. `pick_film` records it, and is separate because the same call is also how a *restored* edit gets its tables back — recording that would push a step for the undo the photographer had just asked for. The list is rebuilt off a revision rather than off every redraw. A drag ends in a redraw per frame while folding into one step, so the unconditional version would tear down and recreate every row sixty times a second to arrive back at the list already on screen. The counter is process-wide: a per-instance one starts every photograph at the same number, so a frontend holding "the revision I last drew" would keep the previous image's steps on screen — invisible while every image opens with one identical row, and a wrong-photograph bug the moment persisted history means it does not. The step names that no descriptor can supply are constants with a roll, and a test walks the roll rather than a second copy of it. `resolve` splits so that "is this catalogued?" can be asked: `derive` turns `history.mask_toggled` into "Mask Toggled", which names a field rather than an act and, being perfectly readable, is a mistake nobody would look at twice. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
+156
-28
@@ -1336,7 +1336,8 @@ impl DevelopSession {
|
||||
layer.set_param(id, param, default);
|
||||
}
|
||||
}
|
||||
self.history.record(&self.graph, Edit::Discrete);
|
||||
self.history
|
||||
.record(&self.graph, Edit::Action(labels::step::RESET_OP));
|
||||
return;
|
||||
}
|
||||
for p in &cap.params {
|
||||
@@ -1344,7 +1345,8 @@ impl DevelopSession {
|
||||
}
|
||||
// One step, though it moved every parameter the operation has: the
|
||||
// user pressed one button.
|
||||
self.history.record(&self.graph, Edit::Discrete);
|
||||
self.history
|
||||
.record(&self.graph, Edit::Action(labels::step::RESET_OP));
|
||||
}
|
||||
|
||||
/// Reset a curve, which is to reset its operation.
|
||||
@@ -1400,12 +1402,14 @@ impl DevelopSession {
|
||||
.map(|p| p.default)
|
||||
.unwrap_or(0.0);
|
||||
self.graph.set_param(op, param, default);
|
||||
self.history.record(&self.graph, Edit::Discrete);
|
||||
self.history
|
||||
.record(&self.graph, Edit::Action(labels::step::RESET_PARAM));
|
||||
}
|
||||
|
||||
pub fn reset_all(&mut self) {
|
||||
self.graph.reset();
|
||||
self.history.record(&self.graph, Edit::Discrete);
|
||||
self.history
|
||||
.record(&self.graph, Edit::Action(labels::step::RESET_ALL));
|
||||
}
|
||||
|
||||
/// Rasterise the current mask stack, if there is one.
|
||||
@@ -1981,7 +1985,8 @@ impl DevelopSession {
|
||||
/// Called on the pointer's release rather than on each move, which is what
|
||||
/// makes a drag one decision in the undo stack however many frames it took.
|
||||
pub fn commit_gradient_drag(&mut self) {
|
||||
self.history.record(&self.graph, Edit::Discrete);
|
||||
self.history
|
||||
.record(&self.graph, Edit::Action(labels::step::MASK_MOVED));
|
||||
}
|
||||
|
||||
// --- repairs (FR-DEV-8) ------------------------------------------------
|
||||
@@ -2019,7 +2024,8 @@ impl DevelopSession {
|
||||
.spots_mut()
|
||||
.place(dr_pipeline::Spot::new(centre, offset, radius))?;
|
||||
self.selected_spot = Some(id.clone());
|
||||
self.history.record(&self.graph, Edit::Discrete);
|
||||
self.history
|
||||
.record(&self.graph, Edit::Action(labels::step::SPOT_PLACED));
|
||||
Some(id)
|
||||
}
|
||||
|
||||
@@ -2145,7 +2151,8 @@ impl DevelopSession {
|
||||
|
||||
/// A repair's drag finished: one history step for the whole gesture.
|
||||
pub fn commit_spot_drag(&mut self) {
|
||||
self.history.record(&self.graph, Edit::Discrete);
|
||||
self.history
|
||||
.record(&self.graph, Edit::Action(labels::step::SPOT_MOVED));
|
||||
}
|
||||
|
||||
/// Which repair the column is describing.
|
||||
@@ -2179,17 +2186,18 @@ impl DevelopSession {
|
||||
if self.selected_spot.as_deref() == Some(id) {
|
||||
self.selected_spot = None;
|
||||
}
|
||||
self.history.record(&self.graph, Edit::Discrete);
|
||||
self.history
|
||||
.record(&self.graph, Edit::Action(labels::step::SPOT_REMOVED));
|
||||
true
|
||||
}
|
||||
|
||||
/// TRACES: FR-DEV-8
|
||||
/// Change one of the selected repair's settings.
|
||||
///
|
||||
/// Coalesced under [`Edit::Discrete`] like the drags are, rather than under
|
||||
/// a parameter key: a repair is not an operation and has no `OpId` to
|
||||
/// coalesce by, so a slider drag over it records a step per movement unless
|
||||
/// the caller debounces. `SliderRow` fires once per completed gesture,
|
||||
/// Recorded as a named [`Edit::Action`] rather than under a parameter key:
|
||||
/// a repair is not an operation and has no `OpId` to name or coalesce by,
|
||||
/// so a slider drag over it records a step per movement unless the caller
|
||||
/// debounces. `SliderRow` fires once per completed gesture,
|
||||
/// which is what makes that acceptable here and is why this is the one
|
||||
/// panel in the application built from that row rather than from a live
|
||||
/// track.
|
||||
@@ -2204,7 +2212,8 @@ impl DevelopSession {
|
||||
return false;
|
||||
};
|
||||
change(spot);
|
||||
self.history.record(&self.graph, Edit::Discrete);
|
||||
self.history
|
||||
.record(&self.graph, Edit::Action(labels::step::SPOT));
|
||||
true
|
||||
}
|
||||
|
||||
@@ -2290,7 +2299,8 @@ impl DevelopSession {
|
||||
return None;
|
||||
}
|
||||
self.active_masks = vec![id.clone()];
|
||||
self.history.record(&self.graph, Edit::Discrete);
|
||||
self.history
|
||||
.record(&self.graph, Edit::Action(labels::step::MASK_ADDED));
|
||||
Some(id)
|
||||
}
|
||||
|
||||
@@ -2319,28 +2329,32 @@ impl DevelopSession {
|
||||
return None;
|
||||
}
|
||||
self.active_masks = vec![id.clone()];
|
||||
self.history.record(&self.graph, Edit::Discrete);
|
||||
self.history
|
||||
.record(&self.graph, Edit::Action(labels::step::MASK_ADDED));
|
||||
Some(id)
|
||||
}
|
||||
|
||||
pub fn remove_mask(&mut self, id: &str) {
|
||||
if self.graph.masks_mut().remove(id).is_some() {
|
||||
self.active_masks.retain(|a| a != id);
|
||||
self.history.record(&self.graph, Edit::Discrete);
|
||||
self.history
|
||||
.record(&self.graph, Edit::Action(labels::step::MASK_REMOVED));
|
||||
}
|
||||
}
|
||||
|
||||
pub fn set_mask_enabled(&mut self, id: &str, enabled: bool) {
|
||||
if let Some(layer) = self.graph.masks_mut().get_mut(id) {
|
||||
layer.enabled = enabled;
|
||||
self.history.record(&self.graph, Edit::Discrete);
|
||||
self.history
|
||||
.record(&self.graph, Edit::Action(labels::step::MASK_TOGGLED));
|
||||
}
|
||||
}
|
||||
|
||||
pub fn set_mask_invert(&mut self, id: &str, invert: bool) {
|
||||
if let Some(layer) = self.graph.masks_mut().get_mut(id) {
|
||||
layer.invert = invert;
|
||||
self.history.record(&self.graph, Edit::Discrete);
|
||||
self.history
|
||||
.record(&self.graph, Edit::Action(labels::step::MASK_INVERTED));
|
||||
}
|
||||
}
|
||||
|
||||
@@ -2349,7 +2363,7 @@ impl DevelopSession {
|
||||
if let Some(layer) = self.graph.masks_mut().get_mut(id) {
|
||||
layer.feather = feather.clamp(0.0, 1.0);
|
||||
self.history
|
||||
.record(&self.graph, Edit::Op(OpId("mask-feather")));
|
||||
.record(&self.graph, Edit::Control(labels::step::MASK_FEATHER));
|
||||
}
|
||||
}
|
||||
|
||||
@@ -2360,7 +2374,8 @@ impl DevelopSession {
|
||||
};
|
||||
if let Some(layer) = self.graph.masks_mut().get_mut(id) {
|
||||
layer.falloff = falloff;
|
||||
self.history.record(&self.graph, Edit::Discrete);
|
||||
self.history
|
||||
.record(&self.graph, Edit::Action(labels::step::MASK_FALLOFF));
|
||||
}
|
||||
}
|
||||
|
||||
@@ -2376,7 +2391,8 @@ impl DevelopSession {
|
||||
if morphology != Morphology::None && layer.morph_radius <= 0.0 {
|
||||
layer.morph_radius = 0.006;
|
||||
}
|
||||
self.history.record(&self.graph, Edit::Discrete);
|
||||
self.history
|
||||
.record(&self.graph, Edit::Action(labels::step::MASK_MORPHOLOGY));
|
||||
}
|
||||
}
|
||||
|
||||
@@ -2384,7 +2400,7 @@ impl DevelopSession {
|
||||
if let Some(layer) = self.graph.masks_mut().get_mut(id) {
|
||||
layer.morph_radius = radius.clamp(0.0, 1.0);
|
||||
self.history
|
||||
.record(&self.graph, Edit::Op(OpId("mask-morph")));
|
||||
.record(&self.graph, Edit::Control(labels::step::MASK_MORPH));
|
||||
}
|
||||
}
|
||||
|
||||
@@ -2439,7 +2455,7 @@ impl DevelopSession {
|
||||
// one decision however many values it passes through. `Discrete`
|
||||
// would put every intermediate position on the undo stack.
|
||||
self.history
|
||||
.record(&self.graph, Edit::Op(OpId("mask-opacity")));
|
||||
.record(&self.graph, Edit::Control(labels::step::MASK_OPACITY));
|
||||
}
|
||||
}
|
||||
|
||||
@@ -2704,6 +2720,25 @@ impl DevelopSession {
|
||||
/// is the sync case — a sidecar written on a device with a stock this one
|
||||
/// lacks — and rendering it as *some other* film would be worse than
|
||||
/// rendering it plainly.
|
||||
/// TRACES: FR-DEV-3f | FR-DEV-5
|
||||
/// Choose a stock **as the photographer just did**, and record the step.
|
||||
///
|
||||
/// Separate from [`Self::choose_film`] because that call has two very
|
||||
/// different callers. Picking Portra from the list is an edit and belongs
|
||||
/// in the history; the same call made while *restoring* an edit — opening
|
||||
/// a photograph, or stepping to a history row that names a stock — is the
|
||||
/// second half of putting a state back, and recording it would push a step
|
||||
/// for the undo the photographer had just asked for.
|
||||
///
|
||||
/// Choosing a stock was not undoable at all before this existed: the pick
|
||||
/// went straight to `choose_film`, which nothing on the history's path
|
||||
/// ever sees.
|
||||
pub fn pick_film(&mut self, stock: Option<&str>, print: bool) {
|
||||
self.choose_film(stock, print);
|
||||
self.history
|
||||
.record(&self.graph, Edit::Action(labels::step::FILM));
|
||||
}
|
||||
|
||||
pub fn choose_film(&mut self, stock: Option<&str>, print: bool) {
|
||||
let Some(stock) = stock else {
|
||||
self.set_film(None);
|
||||
@@ -2929,7 +2964,8 @@ impl DevelopSession {
|
||||
self.graph.set_crop(rotate_crop(crop, turns));
|
||||
}
|
||||
self.graph.rotate_quarters(turns);
|
||||
self.history.record(&self.graph, Edit::Discrete);
|
||||
self.history
|
||||
.record(&self.graph, Edit::Action(labels::step::ROTATE));
|
||||
}
|
||||
|
||||
/// Straightening, in degrees. Positive turns the image clockwise.
|
||||
@@ -2954,7 +2990,8 @@ impl DevelopSession {
|
||||
dr_pipeline::framing::FLIP_H,
|
||||
f32::from(u8::from(!h)),
|
||||
);
|
||||
self.history.record(&self.graph, Edit::Discrete);
|
||||
self.history
|
||||
.record(&self.graph, Edit::Action(labels::step::FLIP_H));
|
||||
}
|
||||
|
||||
pub fn toggle_flip_v(&mut self) {
|
||||
@@ -2964,7 +3001,8 @@ impl DevelopSession {
|
||||
dr_pipeline::framing::FLIP_V,
|
||||
f32::from(u8::from(!v)),
|
||||
);
|
||||
self.history.record(&self.graph, Edit::Discrete);
|
||||
self.history
|
||||
.record(&self.graph, Edit::Action(labels::step::FLIP_V));
|
||||
}
|
||||
|
||||
/// Set the straightening angle, in degrees.
|
||||
@@ -3000,7 +3038,8 @@ 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);
|
||||
self.history
|
||||
.record(&self.graph, Edit::Action(labels::step::RESET_FRAMING));
|
||||
}
|
||||
|
||||
/// How far the viewport is zoomed in: 1.0 fits the frame, 4.0 is 4×.
|
||||
@@ -3137,7 +3176,8 @@ impl DevelopSession {
|
||||
// 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);
|
||||
self.history
|
||||
.record(&self.graph, Edit::Action(labels::step::PASTE));
|
||||
}
|
||||
|
||||
/// TRACES: FR-CAT-8
|
||||
@@ -3233,6 +3273,94 @@ impl DevelopSession {
|
||||
pub fn can_redo(&self) -> bool {
|
||||
self.history.can_redo()
|
||||
}
|
||||
|
||||
/// TRACES: FR-DEV-5 | FR-DEV-7
|
||||
/// Every step this photograph has been through, newest first.
|
||||
///
|
||||
/// Newest first because the list is consulted to take back something just
|
||||
/// done, not browsed chronologically — the order `dr_catalog::trash`
|
||||
/// settled on for the same question. It also keeps the interesting end
|
||||
/// against the heading, so a stack sixty-four deep does not put the step
|
||||
/// the photographer is looking for at the bottom of a long scroll.
|
||||
///
|
||||
/// The reversal happens here rather than in the core, which returns the
|
||||
/// stack in stack order and stamps each row with its own index — so
|
||||
/// nothing on this side does arithmetic to turn a row back into a step.
|
||||
pub fn history_rows(&self) -> Vec<crate::HistoryRow> {
|
||||
let mut rows: Vec<_> = self
|
||||
.history
|
||||
.entries(&self.graph)
|
||||
.into_iter()
|
||||
.map(|entry| crate::HistoryRow {
|
||||
index: entry.index as i32,
|
||||
label: labels::resolve(entry.label.0).into(),
|
||||
current: entry.current,
|
||||
// Everything past the mark is a future the photographer
|
||||
// stepped out of. Still listed, because it is still reachable
|
||||
// by redo and hiding it would make redo arrive somewhere the
|
||||
// panel never mentioned — but drawn as the branch it is.
|
||||
undone: entry.index > self.history.cursor(),
|
||||
})
|
||||
.collect();
|
||||
rows.reverse();
|
||||
rows
|
||||
}
|
||||
|
||||
/// TRACES: FR-DEV-5 | FR-DEV-7
|
||||
/// Step straight to one row of [`Self::history_rows`].
|
||||
///
|
||||
/// Takes the row's own `index`, not its position in that list.
|
||||
/// TRACES: FR-DEV-5
|
||||
/// A number that changes exactly when [`Self::history_rows`] would.
|
||||
///
|
||||
/// The panel is rebuilt off this rather than every redraw: a drag ends in
|
||||
/// a redraw per frame and changes no row, and pushing a fresh model makes
|
||||
/// the toolkit tear down and recreate every one of them.
|
||||
pub fn history_revision(&self) -> u64 {
|
||||
self.history.revision()
|
||||
}
|
||||
|
||||
pub fn go_to_history(&mut self, index: i32) -> bool {
|
||||
let Ok(index) = usize::try_from(index) else {
|
||||
return false;
|
||||
};
|
||||
let step = self.history.go_to(&mut self.graph, index);
|
||||
self.settle(&step);
|
||||
step.moved()
|
||||
}
|
||||
|
||||
/// TRACES: FR-DEV-5
|
||||
/// What undo would take back, and what redo would put back.
|
||||
///
|
||||
/// Named on the buttons rather than left to the bare verb. "Undo" asks the
|
||||
/// photographer to remember what they last did, which after a run of small
|
||||
/// adjustments is exactly what they have stopped tracking — and it is the
|
||||
/// moment they are least willing to press a button and find out.
|
||||
///
|
||||
/// Empty when there is nowhere to go, so the caller falls back to the verb
|
||||
/// alone rather than printing a label for a disabled control.
|
||||
pub fn undo_label(&self) -> String {
|
||||
// Undo takes back the step the graph is *standing on*, so the row to
|
||||
// name is the current one — not the one it will land on.
|
||||
self.step_name(self.history.cursor(), self.history.can_undo())
|
||||
}
|
||||
|
||||
/// TRACES: FR-DEV-5
|
||||
pub fn redo_label(&self) -> String {
|
||||
self.step_name(self.history.cursor() + 1, self.history.can_redo())
|
||||
}
|
||||
|
||||
fn step_name(&self, index: usize, offered: bool) -> String {
|
||||
if !offered {
|
||||
return String::new();
|
||||
}
|
||||
self.history
|
||||
.entries(&self.graph)
|
||||
.into_iter()
|
||||
.find(|e| e.index == index)
|
||||
.map(|e| labels::resolve(e.label.0))
|
||||
.unwrap_or_default()
|
||||
}
|
||||
}
|
||||
|
||||
/// Re-express a crop rect after the frame it is measured against turns.
|
||||
|
||||
Reference in New Issue
Block a user