Put the repair tool on the photograph

A third chip beside Crop and Local, and the mode strip's own comment
predicted the shape: a mode that arms a gesture on the canvas and scopes
the column. Click a mark to cover it, drag the disc to move the repair,
drag the source circle to say where the patch comes from, Delete to remove
it. The source starts two and a half radii towards the middle of the
frame, which is FR-DEV-8's automatic placement in its cheap form — dust
sits on skies and skies are smooth, so it is usually right and always one
drag from fixed.

Two things are drawn deliberately. The circles are the size the repairs
actually are, because whether a disc covers a speck is the whole judgement
being made and a fixed-size dot would say nothing about it; the reach
around them is padded to a touch target so a spot on a dust mark can still
be picked up on a phone. And only the selected repair shows its source: a
dusty sky carries a dozen, and two dozen circles with nothing saying which
belongs to which is less information rather than more.

The panel edits what is stored while the canvas draws what is mapped, and
the two are pushed separately for that reason — a slider deriving its
value from the drawn radius would move differently at different zoom
levels. It is also the one panel built from SliderRow rather than a live
track: a repair has no OpId to coalesce a drag under, so a row that fires
once per gesture is what keeps undo one step per decision.

Verified as far as this environment allows: the strip renders and the
column re-scopes, photographed under XWayland. Synthetic clicks do not
reach this application, so the gestures are as-written rather than
as-felt, and docs/spot-removal.md says so.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
2026-08-26 20:58:13 +02:00
co-authored by Claude Opus 5
parent 2958444835
commit 8ab9440190
9 changed files with 1088 additions and 54 deletions
+239
View File
@@ -533,6 +533,15 @@ pub struct DevelopSession {
/// "make these three subjects a stop darker" is one gesture rather than
/// three. Order is insertion order and nothing reads it, only membership.
active_masks: Vec<String>,
/// TRACES: FR-DEV-8
/// Which repair the panel is describing, if any.
///
/// Interface state and not part of the edit, exactly as `active_masks` is:
/// it changes no pixel, it is not in the sidecar, and it is not on the undo
/// stack. One at a time rather than a set — a repair is eight numbers and
/// there is no gesture that usefully moves several at once, where three
/// masked layers really can share a slider drag.
selected_spot: Option<String>,
/// Whether to draw the false-coloured region overlay.
show_overlay: bool,
/// Which attribute the panel is filtered to, or all of them.
@@ -619,6 +628,7 @@ impl DevelopSession {
subjects: None,
subject_key: 0,
active_masks: Vec::new(),
selected_spot: None,
show_overlay: false,
active_tab: None,
curve_channel: 0,
@@ -1897,6 +1907,235 @@ impl DevelopSession {
self.history.record(&self.graph, Edit::Discrete);
}
// --- repairs (FR-DEV-8) ------------------------------------------------
/// TRACES: FR-DEV-8
/// Cover what is at `(x, y)`, in fractions of the shown image.
///
/// The click arrives in *output* coordinates — where the photograph
/// currently sits on screen — and a repair is stored against the
/// photograph, so it goes through `Framing::source_at`: the same map the
/// shader applies, run backwards. Anything less would put the repair where
/// the pointer was rather than where the mark is, and the two agree only at
/// fit-to-window with no crop.
///
/// Returns the new repair's id, or `None` when the set is full. Selecting
/// it is deliberate: the control that changes its size is in the column,
/// and a photographer who has just placed a spot too small should find that
/// control already pointed at it.
pub fn place_spot(&mut self, x: f32, y: f32) -> Option<String> {
let (sw, sh) = self.demosaiced.size();
let centre = self.graph.framing().source_at((x, y), sw, sh);
// Outside the photograph entirely — the letterbox margin, or a drag
// that ended off the edge. Placing a repair there would put a disc
// somewhere the user cannot see and cannot pick up again.
if !(0.0..=1.0).contains(&centre.0) || !(0.0..=1.0).contains(&centre.1) {
return None;
}
let aspect = sw.max(1) as f32 / sh.max(1) as f32;
let radius = dr_pipeline::spot::DEFAULT_RADIUS;
let offset = dr_pipeline::Spot::default_offset(centre, radius, aspect);
let id = self
.graph
.spots_mut()
.place(dr_pipeline::Spot::new(centre, offset, radius))?;
self.selected_spot = Some(id.clone());
self.history.record(&self.graph, Edit::Discrete);
Some(id)
}
/// TRACES: FR-DEV-8 | FR-UI-3
/// Every repair as a circle on the shown image, plus the source circle of
/// the selected one.
///
/// # Why only the selected repair shows its source
///
/// A dusty sky carries a dozen repairs. Two dozen circles with nothing
/// saying which source belongs to which disc is not more information, it is
/// less — and there is no room on a phone for a connector between each
/// pair. The selection is what disambiguates them, which is also why a
/// press on a repair selects it before the drag begins.
///
/// Recomputed per redraw rather than cached, for the reason
/// [`Self::gradient_handles`] gives: the answer changes with the *view*,
/// and a pan moves every circle while touching no edit.
pub fn spot_handles(&self) -> Vec<crate::SpotHandle> {
let (sw, sh) = self.demosaiced.size();
let framing = self.graph.framing();
let aspect = sw.max(1) as f32 / sh.max(1) as f32;
// The shown image's own shape, which is not the source's once the frame
// has been cropped or turned. A radius is reported against its height,
// so this is what converts the x half of the mapped offset.
let (ow, oh) = self.graph.output_size(sw, sh);
let shown_aspect = ow.max(1) as f32 / oh.max(1) as f32;
let mut handles = Vec::new();
for spot in self.graph.spots().spots() {
let selected = self.selected_spot.as_deref() == Some(spot.id.as_str());
let centre = framing.output_at(spot.centre, sw, sh);
// The radius, mapped rather than scaled: a point one radius above
// the centre goes through the same map, and the distance between
// the two answers is the radius as drawn. The x half is multiplied
// by the shown aspect because the two axes are normalised by
// different lengths, and a circle measured in mixed units is an
// ellipse.
let rim = framing.output_at((spot.centre.0, spot.centre.1 + spot.radius), sw, sh);
let radius = ((rim.0 - centre.0) * shown_aspect).hypot(rim.1 - centre.1);
handles.push(crate::SpotHandle {
id: spot.id.clone().into(),
role: crate::SpotRole::Destination,
x: centre.0,
y: centre.1,
radius,
selected,
enabled: spot.enabled,
});
if selected {
let source = framing.output_at(spot.source(aspect), sw, sh);
handles.push(crate::SpotHandle {
id: spot.id.clone().into(),
role: crate::SpotRole::Source,
x: source.0,
y: source.1,
radius,
selected: true,
enabled: spot.enabled,
});
}
}
handles
}
/// TRACES: FR-DEV-8
/// Drag one circle of one repair, from `press` to `now`, both in fractions
/// of the shown image.
///
/// Dragging the disc moves the whole repair and carries its source along —
/// what a photographer means by nudging a spot. Dragging the source moves
/// the source alone, which is the override FR-DEV-8 asks for over the
/// automatic placement.
///
/// `origin` is the repair as it stood when the gesture began; the caller
/// holds it for the duration and hands it back, so a drag is applied to
/// that rather than accumulated frame by frame — the rule
/// [`Self::drag_gradient_handle`] states, for the same reasons.
pub fn drag_spot(
&mut self,
id: &str,
role: crate::SpotRole,
origin: Option<&dr_pipeline::Spot>,
press: (f32, f32),
now: (f32, f32),
) -> Option<dr_pipeline::Spot> {
let (sw, sh) = self.demosaiced.size();
let framing = *self.graph.framing();
let aspect = sw.max(1) as f32 / sh.max(1) as f32;
let start = match origin {
Some(spot) => spot.clone(),
None => self.graph.spots().get(id)?.clone(),
};
// The displacement in source coordinates. Affine, so a movement is a
// movement: the map may be run on the two endpoints and subtracted,
// which is what makes a drag on a rotated photograph move the repair in
// the direction the finger went.
let from = framing.source_at(press, sw, sh);
let to = framing.source_at(now, sw, sh);
let moved = (to.0 - from.0, to.1 - from.1);
let spot = self.graph.spots_mut().get_mut(id)?;
match role {
crate::SpotRole::Destination => {
spot.set_centre((start.centre.0 + moved.0, start.centre.1 + moved.1));
}
// In frame units, because that is what an offset is stored in — and
// the x half of a normalised displacement is short by the aspect.
crate::SpotRole::Source => {
spot.set_offset((start.offset.0 + moved.0 * aspect, start.offset.1 + moved.1));
}
}
// Nothing recorded here: a drag delivers a pointer event a frame, and
// one history step apiece would make undo walk the gesture back pixel
// by pixel. Recorded once, on release.
Some(start)
}
/// 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);
}
/// Which repair the column is describing.
pub fn selected_spot(&self) -> Option<&dr_pipeline::Spot> {
let id = self.selected_spot.as_deref()?;
self.graph.spots().get(id)
}
pub fn selected_spot_id(&self) -> Option<&str> {
self.selected_spot.as_deref()
}
/// Choose a repair, or `None` to describe none.
///
/// An id the graph no longer holds selects nothing rather than being kept:
/// the circle that offered it is stale by the time the press lands, and a
/// selection pointing at a deleted repair would leave the column describing
/// something that is not on the photograph.
pub fn select_spot(&mut self, id: Option<&str>) {
self.selected_spot = id
.filter(|id| self.graph.spots().get(id).is_some())
.map(str::to_string);
}
/// TRACES: FR-DEV-8
/// Take a repair off the photograph, returning whether one went.
pub fn remove_spot(&mut self, id: &str) -> bool {
if self.graph.spots_mut().remove(id).is_none() {
return false;
}
if self.selected_spot.as_deref() == Some(id) {
self.selected_spot = None;
}
self.history.record(&self.graph, Edit::Discrete);
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,
/// 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.
pub fn set_selected_spot<F>(&mut self, change: F) -> bool
where
F: FnOnce(&mut dr_pipeline::Spot),
{
let Some(id) = self.selected_spot.clone() else {
return false;
};
let Some(spot) = self.graph.spots_mut().get_mut(&id) else {
return false;
};
change(spot);
self.history.record(&self.graph, Edit::Discrete);
true
}
/// How many repairs this photograph carries.
pub fn spot_count(&self) -> usize {
self.graph.spots().len()
}
/// The selected layer's mask rule, for a caller that has to remember what
/// a gesture started from.
pub fn active_mask_source(&self) -> Option<MaskSource> {
+37
View File
@@ -40,6 +40,7 @@ mod segmentation;
mod settings_store;
mod settings_ui;
mod sidecar_cache;
mod spots_ui;
mod trash;
use std::cell::RefCell;
@@ -292,6 +293,11 @@ fn reset_view_state(window: &AppWindow) {
// overlay or the crosshair to the next one would offer a selection of
// regions that are not in the picture on screen.
masks_ui::reset(window);
// TRACES: FR-DEV-8
// The repairs belong to one photograph too. The next one's arrive with its
// sidecar a moment later, and until they do the canvas must not be showing
// the last one's.
spots_ui::reset(window);
}
/// Push the framing back to the geometry panel.
@@ -1279,6 +1285,18 @@ pub fn run(paths: Vec<PathBuf>) -> Result<()> {
// those ends in a redraw.
masks_ui::sync_overlay_view(window, s);
// TRACES: FR-DEV-8
// And where the repairs are drawn, for the same reason: a pan or a
// zoom moves every circle while touching no repair, and a circle
// left where the mark used to be is worse than no circle at all.
spots_ui::sync_handles(window, s);
// The column's own numbers, pushed from here as well so that a
// photograph opened with repairs already on it arrives with the
// panel describing them — the sidecar lands after the callbacks
// have all been installed, and a redraw is the one path every
// arrival takes.
spots_ui::sync_panel(window, s);
let (mut w, mut h) = *viewport.borrow();
// **Half resolution while the gesture is still moving.**
@@ -1951,6 +1969,7 @@ pub fn run(paths: Vec<PathBuf>) -> Result<()> {
}
masks_ui::wire(&window, &session, &rows, &redraw);
spots_ui::wire(&window, &session, redraw.clone());
{
let weak = window.as_weak();
@@ -2166,14 +2185,32 @@ pub fn run(paths: Vec<PathBuf>) -> Result<()> {
w.set_crop_h(c.height);
}
ViewMode::Local => s.set_overlay(true),
// TRACES: FR-DEV-8
// Nothing to arm: the circles are drawn whenever there are
// repairs, and what the mode changes is whether a click on
// the photograph makes another one. Leaving the mask
// selection behind would re-point the column at a layer's
// chain while the canvas is showing repairs, which is the
// fault this whole strip exists to prevent.
ViewMode::Spots => {
s.set_overlay(false);
s.set_active_mask(None);
}
ViewMode::Photo => {
s.set_overlay(false);
s.set_active_mask(None);
// A repair stays on the photograph; only the *selection*
// goes, so the source circle does not hang about over a
// frame nobody is repairing any more.
s.select_spot(None);
}
}
}
w.set_view_mode(mode);
masks_ui::sync(&w, &session);
if let Some(s) = session.borrow().as_ref() {
spots_ui::sync_handles(&w, s);
}
// The scope may have just changed, so the panel below is now
// describing a different chain.
sync_rows(&w, &rows, &session);
+292
View File
@@ -0,0 +1,292 @@
//! TRACES: FR-DEV-8
//! Wiring the repair tool to the develop session.
//!
//! Translation only, exactly as [`crate::masks_ui`] is: circles out in one
//! direction, gestures in the other, and every decision in
//! [`crate::develop::DevelopSession`]. What this module does decide is when the
//! canvas is redrawn and when the panel is rebuilt, and those are the two
//! things that go wrong quietly here — see [`sync_handles`] for the one that
//! took a gesture out from under the finger.
use std::cell::RefCell;
use std::rc::Rc;
use slint::{ComponentHandle as _, Model as _, ModelRc, VecModel};
use crate::develop::DevelopSession;
use crate::{AppWindow, SpotHandle};
/// TRACES: FR-DEV-8 | FR-UI-3
/// Move the circles to where the repairs now are.
///
/// # Why this is not `set_spot_handles(VecModel::from(…))`
///
/// **A fresh model kills the gesture that is moving them.** The circles are a
/// repeater over this model, and handing Slint a new `ModelRc` makes it throw
/// the repeated items away and build new ones — the `TouchArea` holding the
/// pointer included. The drag then dies under the finger with the button still
/// down. `masks_ui::sync_handles` carries the same warning, and `develop.rs`
/// carries it about the parameter rows, where it broke slider drags: this is
/// the third place the same mistake is available, which is why it is written
/// down in all three.
///
/// So the model is kept and its rows are rewritten in place.
pub(crate) fn sync_handles(window: &AppWindow, session: &DevelopSession) {
let next = session.spot_handles();
let model = handle_model();
while model.row_count() > next.len() {
model.remove(model.row_count() - 1);
}
for (i, handle) in next.into_iter().enumerate() {
if i < model.row_count() {
// Only where it actually moved: an unchanged row written back is
// still a change notification.
if model.row_data(i).as_ref() != Some(&handle) {
model.set_row_data(i, handle);
}
} else {
model.push(handle);
}
}
window.set_spot_handles(ModelRc::from(model));
window.set_selected_spot(session.selected_spot_id().unwrap_or_default().into());
}
/// The circles' model, held for the life of the process — one shared identity,
/// for the reason [`sync_handles`] gives.
fn handle_model() -> Rc<VecModel<SpotHandle>> {
thread_local! {
static HANDLES: Rc<VecModel<SpotHandle>> = Rc::new(VecModel::default());
}
HANDLES.with(Clone::clone)
}
/// Take the repairs off the canvas between photographs.
///
/// Emptied rather than replaced, keeping the model's identity for the reason
/// [`sync_handles`] gives.
pub(crate) fn reset(window: &AppWindow) {
let model = handle_model();
while model.row_count() > 0 {
model.remove(model.row_count() - 1);
}
window.set_spot_handles(ModelRc::from(model));
window.set_selected_spot(Default::default());
window.set_spot_count(0);
}
/// Install the repair tool's callbacks.
pub(crate) fn wire(
window: &AppWindow,
session: &Rc<RefCell<Option<DevelopSession>>>,
redraw: Rc<dyn Fn(&AppWindow)>,
) {
// --- placing ----------------------------------------------------------
{
let weak = window.as_weak();
let session = session.clone();
let redraw = redraw.clone();
window.on_spot_placed(move |x, y| {
let Some(w) = weak.upgrade() else { return };
let placed = session
.borrow_mut()
.as_mut()
.and_then(|s| s.place_spot(x, y));
if placed.is_none() {
// The letterbox margin, or a full set. Neither is an error and
// neither should clear the selection: a near-miss that threw
// away what the column was describing would read as hostile.
return;
}
if let Some(s) = session.borrow().as_ref() {
sync_handles(&w, s);
sync_panel(&w, s);
}
sync_undo(&w, &session);
redraw(&w);
});
}
// --- dragging ---------------------------------------------------------
//
// The repair as it stood when the press landed, held for the gesture. A
// drag is applied to *that* rather than accumulated frame by frame: the
// clamps would otherwise compound, so a source dragged past its limit and
// back would not return to where it started, and the result would depend on
// how many pointer events the platform happened to deliver.
let dragging: Rc<RefCell<Option<dr_pipeline::Spot>>> = Rc::new(RefCell::new(None));
{
let weak = window.as_weak();
let session = session.clone();
let redraw = redraw.clone();
let dragging = dragging.clone();
window.on_spot_handle_dragged(move |id, role, from_x, from_y, to_x, to_y| {
let Some(w) = weak.upgrade() else { return };
let origin = dragging.borrow().clone();
let started = session.borrow_mut().as_mut().and_then(|s| {
s.drag_spot(
id.as_str(),
role,
origin.as_ref(),
(from_x, from_y),
(to_x, to_y),
)
});
if started.is_none() {
return;
}
*dragging.borrow_mut() = started;
// Only the circles. Rebuilding the panel on every frame of a drag
// is work for no difference, and a full rebuild would take the
// gesture out from under the finger — see `sync_handles`.
if let Some(s) = session.borrow().as_ref() {
sync_handles(&w, s);
}
redraw(&w);
});
}
{
let weak = window.as_weak();
let session = session.clone();
let dragging = dragging.clone();
window.on_spot_handle_released(move || {
// Forgotten on release, so the next gesture measures from wherever
// this one left the repair.
if dragging.borrow_mut().take().is_none() {
// A press with no movement — a selection, not a drag. Recording
// a step would put an identical snapshot on the undo stack.
return;
}
if let Some(s) = session.borrow_mut().as_mut() {
s.commit_spot_drag();
}
if let Some(w) = weak.upgrade() {
sync_undo(&w, &session);
}
});
}
// --- the column's controls --------------------------------------------
//
// One closure shape, four callbacks: each hands the session a change to
// make to whichever repair is selected, and the session decides whether
// there is one. Reading the selection here instead would put the same
// `Option` check in four places and let them disagree.
macro_rules! control {
($install:ident, $apply:expr) => {{
let weak = window.as_weak();
let session = session.clone();
let redraw = redraw.clone();
window.$install(move |value| {
let Some(w) = weak.upgrade() else { return };
let changed = session
.borrow_mut()
.as_mut()
.is_some_and(|s| s.set_selected_spot(|spot| $apply(spot, value)));
if !changed {
return;
}
if let Some(s) = session.borrow().as_ref() {
sync_handles(&w, s);
sync_panel(&w, s);
}
sync_undo(&w, &session);
redraw(&w);
});
}};
}
control!(on_spot_radius_changed, |spot: &mut dr_pipeline::Spot, v| {
spot.set_radius(v)
});
control!(
on_spot_feather_changed,
|spot: &mut dr_pipeline::Spot, v| { spot.set_feather(v) }
);
control!(
on_spot_opacity_changed,
|spot: &mut dr_pipeline::Spot, v| { spot.set_opacity(v) }
);
control!(on_spot_mode_picked, |spot: &mut dr_pipeline::Spot, i| {
spot.mode = if i == 1 {
dr_pipeline::SpotMode::Clone
} else {
dr_pipeline::SpotMode::Heal
}
});
// --- selecting and removing -------------------------------------------
{
let weak = window.as_weak();
let session = session.clone();
let redraw = redraw.clone();
window.on_spot_selected(move |id| {
let Some(w) = weak.upgrade() else { return };
if let Some(s) = session.borrow_mut().as_mut() {
s.select_spot(Some(id.as_str()));
}
if let Some(s) = session.borrow().as_ref() {
sync_handles(&w, s);
sync_panel(&w, s);
}
// The canvas changes — the selected repair gains its source circle
// — but no pixel does, so this is a repaint of the overlay rather
// than of the photograph. `redraw` is the only hook there is, and
// it is cheap enough: the render caches on the invalidation key,
// which a selection does not move.
redraw(&w);
});
}
{
let weak = window.as_weak();
let session = session.clone();
let redraw = redraw.clone();
window.on_spot_removed(move |id| {
let Some(w) = weak.upgrade() else { return };
let removed = session
.borrow_mut()
.as_mut()
.is_some_and(|s| s.remove_spot(id.as_str()));
if !removed {
return;
}
if let Some(s) = session.borrow().as_ref() {
sync_handles(&w, s);
sync_panel(&w, s);
}
sync_undo(&w, &session);
redraw(&w);
});
}
}
/// TRACES: FR-DEV-8
/// Push the selected repair's settings into the column.
///
/// Separate from [`sync_handles`] because they answer different questions and
/// change at different times: the circles move on every frame of a drag and on
/// every pan, and these move when a repair is selected or a control is used.
pub(crate) fn sync_panel(window: &AppWindow, session: &DevelopSession) {
window.set_spot_count(session.spot_count() as i32);
if let Some(spot) = session.selected_spot() {
window.set_spot_radius(spot.radius);
window.set_spot_feather(spot.feather);
window.set_spot_opacity(spot.opacity);
window.set_spot_mode(match spot.mode {
dr_pipeline::SpotMode::Heal => 0,
dr_pipeline::SpotMode::Clone => 1,
});
}
}
/// Push whether undo and redo have anywhere to go.
///
/// Every repair is a history step, so every one of them moves these — and a
/// disabled undo button after an edit that *is* undoable is the kind of small
/// lie that stops people trusting the button at all.
fn sync_undo(window: &AppWindow, session: &Rc<RefCell<Option<DevelopSession>>>) {
window.set_can_undo(session.borrow().as_ref().is_some_and(|s| s.can_undo()));
window.set_can_redo(session.borrow().as_ref().is_some_and(|s| s.can_redo()));
}