dtourolle/DarkRoom
1
0
Fork 0
Code Issues 39 Pull Requests Actions Packages Projects Releases 20 Wiki Activity

Say which photograph the sliders are pointed at

Browse Source 
Selecting a mask layer silently re-points about thirty controls at that layer's
chain. Same panel, same order, same sliders, different meaning — and the only
thing that said so was a sentence in the panel above, which a photographer
reaching for the exposure slider has no reason to read. An exposure change
lands on the whole frame when it was meant for a face, or the reverse; both are
silent, and both are discovered later. `ui-navigation.md` §1.1 calls it the
dangerous one and it is: the others in that document cost time, this one costs
work.

The remedy is the classic one for a modal fault — make the mode visible — and
the application already had the pattern. Crop arms a canvas interaction, draws
an overlay, gives the column one job and is left by the control that entered
it. Local masking is the same animal built as a peer panel, and that is what
created the ambiguity. So `crop-mode` stops being a bare boolean and becomes
one value of a three-state mode, which is the point: two modes could both be on
before, and now that is not a state the interface can be in rather than one it
is tested against.

**One strip, not two.** The mode control was going to sit beside the group
strip that filters the adjustments, which is two controls above one column
answering the same question — what am I working on. They are one control now,
`Crop · Local │ All · Light · Colour`, which is the shape Lightroom Mobile's
bottom strip has for the same reason. The two halves are different kinds of
state and are drawn differently: a mode is a chip that fills with the accent
when it is on, a group is a word with a rule under it. That difference is what
lets both be read at once, which they routinely are — picking Light while a
mask is selected filters *that layer's* chain and does not leave the mode.
Dropping the scope on a group press would be the same fault coming back from
the other end, and would make Light mean two things depending on where it was
pressed.

The strip stays pinned above the develop column rather than moving to the top
of the canvas as the document proposed. The half that filters the column
belongs to the column, and the photograph is the subject. The canvas keeps one
button, which now names the mode it leaves rather than saying "Done" — that was
unambiguous with one mode and would not be with two — because the column can be
closed on a narrow window and no mode may be inescapable.

Entering a mode is a side effect, so Rust owns it rather than the strip writing
the property: crop drops the zoom, local turns the overlay on, and leaving
clears the selection. That last one is the fix. The "Overlay" and "Select"
toggles are gone because they armed things that are simply what the mode *is* —
a mode that has to be switched on separately is one you can enter and have do
nothing. Escape and the Android back gesture join `back_step` as one
`LeaveMode` rather than a second exit concept, and the mode is left before the
zoom is: it was entered later, and it is the bigger step back.

The heading is where the scope goes. Not a caption beside the panel, the
heading *of* the panel that changed — `ADJUST` becomes the layer's name, the
same string the selected row in the stack shows. That is the difference between
describing a hazard and removing it.

**Handles on the photograph.** A linear or radial mask could be created and
then not moved, so a radial sat at the centre of the frame at its default size
for ever. Three faults stood in the way of drawing one.

The first is that a gradient did not render at all until the model had run. The
rasteriser was built on the way out of `segment` and the array's size was read
*off* the segmentation, so a gradient added to an unsegmented photograph
produced nothing — silently, in the same way exports and thumbnails once did:
the shader still emits the layer's block and the empty placeholder multiplies it
by zero. The proxy size is a property of the photograph. Both are derived from
it now, and deliberately at the same size rather than by coincidence, because a
subject's distance field is sampled against that array.

The second is hit-testing. A handle is drawn in output coordinates and stored
in source ones, and between them lie the crop, the zoom, the pan, the
straightening and the turns. `Framing::source_at` is `wgsl_prologue` evaluated
on the CPU, kept in that file beside it so that keeping the two in step is one
file's problem — a handle mapped through anything less drifts off the mask the
moment the view moves, which is exactly what masks are rasterised in source
space to avoid.

The third is that a drag is a displacement, not a destination. Each handle
answers to the movement of the pointer since the press, applied to where the
mask was when the press landed. Snapping the handle to the pointer instead
jerks it by up to half a touch target on the first press, and the target is
finger-sized because a tablet has no hover to reveal a control and no modifier
to qualify it.

A ramp gets three handles — centre, width, angle. An ellipse gets three too:
centre and one per semi-axis, the major one carrying the direction as well as
the length, because where an axis is put says both. It had a fourth, and it is
gone: standing off the shape by a fixed distance, the rotation arm began
outside the photograph at the size a new radial is created at, so the first
thing anyone saw was a control they could not reach without first shrinking the
mask.

Two faults here were found by looking at the screen rather than at the source,
both of the kind that cannot be found any other way. A `1px` rule with a size
and no position is *centred* by Slint, so the seam between the photograph and
the column was a hairline down the middle of the panel, through the histogram
and every slider under it — twice, once in `app.slint` and once in
`AdjustPanel`. And handing Slint a fresh model for the handles on every pointer
event made the repeater rebuild its items, taking the `TouchArea` holding the
gesture with them: the handle jumped once and then went dead under a finger
that was still down. `develop.rs` carries the same warning about the parameter
rows, where it broke slider drags; the model is rewritten in place now.

The tests worth having are the ones about ambiguity and about the map. That the
same row reads the frame's value, then the layer's, then the frame's again is
§1.1 in one assertion. That dragging a handle onto another gradient's matching
handle *produces* that gradient closes the loop between the two directions of
the framing map, through a view that is cropped, zoomed, panned, straightened
and quarter-turned at once — a one-legged map is invisible when the framing is
neutral, because then both legs are the identity.

Not done here: the histogram still reports the whole frame while the sliders
edit a layer. That disagreement is real and is N3's, which this unblocks. The
strip has room for a Brush entry beside Crop and Local when the painted masks
land in the core, and it needs nothing here but the canvas interaction.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
dtourolle
2026-08-22 13:20:41 +02:00
co-authored by Claude Opus 5
parent c75863c93f
commit 96a7b405c2
10 changed files with 2171 additions and 185 deletions
Download Patch File Download Diff File Expand all files Collapse all files
ui/dr-ui/src/masks_ui.rs
+314 -18
View File
@@ -17,11 +17,40 @@
use std::cell::RefCell;
use std::rc::Rc;
use slint::{ComponentHandle as _, ModelRc, VecModel};
use slint::{ComponentHandle as _, Model as _, ModelRc, VecModel};
use crate::develop::DevelopSession;
use crate::segmentation;
use crate::{sync_rows, AppWindow, MaskRow, ParamRow, SubjectRow};
use crate::{sync_rows, AppWindow, GradientHandle, MaskRow, ParamRow, SubjectRow};
/// What the adjust panel's heading says when the controls are global.
///
/// The panel's own default too, and named because the two have to agree: a
/// literal in both would eventually be a literal in one.
pub(crate) const GLOBAL_SCOPE: &str = "ADJUST";
/// TRACES: FR-DEV-3
/// What the adjust panel is pointed at, for its heading.
///
/// **The layer's name, not the word "adjust".** Selecting a layer re-points
/// every control in that panel at that layer's chain, and the heading is the
/// one piece of text a photographer cannot avoid reading on the way to a
/// slider. Upper case because the heading style is, and it is the *same*
/// string the row in the stack above shows — one name for one thing, so the
/// selected row and the panel it scopes cannot appear to disagree.
pub(crate) fn scope_label(session: &DevelopSession) -> String {
let Some(id) = session.active_mask() else {
return GLOBAL_SCOPE.to_string();
};
session
.mask_layers()
.into_iter()
.find(|(layer_id, ..)| layer_id == id)
.map_or_else(
|| GLOBAL_SCOPE.to_string(),
|(_, label, ..)| label.to_uppercase(),
)
}
/// Push every mask-related property from the session into the window.
pub(crate) fn sync(window: &AppWindow, session: &Rc<RefCell<Option<DevelopSession>>>) {
@@ -30,12 +59,12 @@ pub(crate) fn sync(window: &AppWindow, session: &Rc<RefCell<Option<DevelopSessio
window.set_mask_rows(ModelRc::new(VecModel::<MaskRow>::default()));
window.set_subject_rows(ModelRc::new(VecModel::<SubjectRow>::default()));
window.set_segmented(false);
window.set_editing_mask(false);
window.set_adjust_scope(GLOBAL_SCOPE.into());
clear_handles(window);
window.set_overlay_on(false);
return;
};
let active = s.active_mask().map(|id| id.to_string());
let rows: Vec<MaskRow> = s
.mask_layers()
.into_iter()
@@ -71,7 +100,8 @@ pub(crate) fn sync(window: &AppWindow, session: &Rc<RefCell<Option<DevelopSessio
window.set_subject_rows(ModelRc::new(VecModel::from(subjects)));
window.set_segmented(s.has_segmentation());
window.set_editing_mask(active.is_some());
window.set_adjust_scope(scope_label(s).into());
sync_handles(window, s);
// The overlay is regenerated only when there is one to draw. It is a
// proxy-sized RGBA buffer — a megabyte or so — and rebuilding it on every
@@ -91,6 +121,71 @@ pub(crate) fn sync(window: &AppWindow, session: &Rc<RefCell<Option<DevelopSessio
sync_overlay_view(window, s);
}
/// TRACES: FR-DEV-3 | FR-UI-3
/// Move the canvas handles to where the selected gradient now is.
///
/// # Why this is not `set_gradient_handles(VecModel::from(…))`
///
/// **A fresh model kills the gesture that is moving them.** The handles are a
/// repeater over this model, and handing Slint a new `ModelRc` makes it throw
/// the repeated items away and build new ones — including the `TouchArea`
/// holding the pointer. A drag therefore moved the handle exactly once, on the
/// first pointer event, and then went dead under the finger with the button
/// still down. Seen on screen and invisible in the source; `develop.rs` carries
/// the same warning about the parameter rows, where it broke slider drags.
///
/// So the model is kept and its rows are rewritten in place. Slint updates the
/// existing item rather than replacing it, and the handle stays under the
/// pointer for the whole drag.
///
/// Split out from [`sync`] for a second reason too: a drag emits a pointer
/// event a frame, and rebuilding the mask stack and the subject list on each of
/// them would be a model rewrite per frame for lists that did not change.
pub(crate) fn sync_handles(window: &AppWindow, session: &DevelopSession) {
let next = session.gradient_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, and the point of this function is
// to emit as few of those as the truth allows.
if model.row_data(i).as_ref() != Some(&handle) {
model.set_row_data(i, handle);
}
} else {
model.push(handle);
}
}
window.set_gradient_handles(model.into());
}
/// The handles' model, held for the life of the process.
///
/// One shared identity, for the reason [`sync_handles`] gives. A thread-local
/// because the interface is single-threaded and this is the same shape
/// `develop.rs` uses for its shared empty models.
fn handle_model() -> Rc<VecModel<GradientHandle>> {
thread_local! {
static HANDLES: Rc<VecModel<GradientHandle>> = Rc::new(VecModel::default());
}
HANDLES.with(Clone::clone)
}
/// Take the handles off the canvas, emptying the held model rather than
/// replacing it — see [`sync_handles`] for why the identity is kept.
fn clear_handles(window: &AppWindow) {
let model = handle_model();
while model.row_count() > 0 {
model.remove(model.row_count() - 1);
}
window.set_gradient_handles(model.into());
}
/// Push the overlay's clip rectangle and angle.
///
/// Separate from [`sync`] because it is called from the render path too: a pan
@@ -163,23 +258,61 @@ pub(crate) fn wire(
});
}
// --- the overlay and the picking mode ---------------------------------
// --- dragging a gradient on the photograph ----------------------------
//
// The geometry the gesture started from, held for its duration.
//
// **A drag is applied to where the mask was when the press landed**, not
// to where it was one frame ago. Accumulating frame by frame would let the
// clamps compound — a radius dragged past its limit and back would not
// return to where it started — and would make the result depend on how
// many events the pointer happened to deliver.
let dragging: Rc<RefCell<Option<dr_pipeline::mask::MaskSource>>> = Rc::new(RefCell::new(None));
{
let weak = window.as_weak();
let session = session.clone();
window.on_overlay_toggled(move |on| {
let redraw = redraw.clone();
let dragging = dragging.clone();
window.on_gradient_handle_dragged(move |role, from_x, from_y, to_x, to_y| {
let Some(w) = weak.upgrade() else { return };
if let Some(s) = session.borrow_mut().as_mut() {
s.set_overlay(on);
let origin = dragging.borrow().clone();
let started = session.borrow_mut().as_mut().and_then(|s| {
s.drag_gradient_handle(role, origin.as_ref(), (from_x, from_y), (to_x, to_y))
});
if started.is_none() {
return;
}
sync(&w, &session);
*dragging.borrow_mut() = started;
// Only the handles, not the whole panel: nothing in the mask stack
// or the subject list changed, and rewriting those models on every
// frame of a drag is work for no difference. See `sync_handles` for
// the sharper reason — a full `sync` would also take the gesture
// out from under the finger.
if let Some(s) = session.borrow().as_ref() {
sync_handles(&w, s);
}
redraw(&w);
});
}
{
let weak = window.as_weak();
window.on_region_picking_toggled(move |on| {
let Some(w) = weak.upgrade() else { return };
w.set_region_picking(on);
let session = session.clone();
let dragging = dragging.clone();
window.on_gradient_handle_released(move || {
// Forgotten on release, so the next gesture measures from wherever
// this one left the mask rather than from where this one began.
if dragging.borrow_mut().take().is_none() {
// A press with no movement. Nothing changed, so recording a
// step would put an identical snapshot on the undo stack.
return;
}
if let Some(s) = session.borrow_mut().as_mut() {
s.commit_gradient_drag();
}
if let Some(w) = weak.upgrade() {
w.set_can_undo(session.borrow().as_ref().is_some_and(|s| s.can_undo()));
w.set_can_redo(session.borrow().as_ref().is_some_and(|s| s.can_redo()));
}
});
}
@@ -382,15 +515,178 @@ pub(crate) fn wire(
/// Clear the panel when the open image changes.
///
/// Its own function rather than a call to [`sync`] with an empty session,
/// because the *window* state has to be reset too: picking mode and the
/// overlay are properties of looking at one photograph, and carrying them to
/// the next one leaves a crosshair over an image with no region map behind it.
/// because the *window* state has to be reset too: the overlay and the scope
/// are properties of looking at one photograph, and carrying them to the next
/// one would draw a region map over an image that has none and name a heading
/// after a layer that is not there. Picking is not among them any more — it
/// follows the view mode, which `reset_view_state` returns to `photo`.
pub(crate) fn reset(window: &AppWindow) {
window.set_region_picking(false);
window.set_overlay_on(false);
window.set_segmenting(false);
window.set_segmented(false);
window.set_mask_rows(ModelRc::new(VecModel::<MaskRow>::default()));
window.set_subject_rows(ModelRc::new(VecModel::<SubjectRow>::default()));
window.set_editing_mask(false);
window.set_adjust_scope(GLOBAL_SCOPE.into());
clear_handles(window);
}
#[cfg(test)]
mod tests {
use super::*;
/// A session over a flat frame. No segmentation, which is deliberate: a
/// gradient needs none, and the tests below are about scope rather than
/// about what the model found.
fn session() -> Option<DevelopSession> {
let ctx = pollster::block_on(dr_gpu::GpuContext::new_headless()).ok()?;
let rgba: Vec<u8> = (0..32 * 32).flat_map(|_| [128u8, 128, 128, 255]).collect();
DevelopSession::open_rgb(&ctx, &rgba, 32, 32, dr_types::Orientation::NORMAL).ok()
}
/// TRACES: FR-DEV-3 | FR-UI-1
/// The fault this pass exists for, stated as a test.
///
/// Selecting a mask layer re-points every control in the adjust panel at
/// that layer's chain. Before this, the only thing that said so was a
/// caption in a *different* panel, which a photographer reaching for the
/// exposure slider has no reason to read. The heading of the panel that
/// changed now carries the answer, so the two cannot be read apart — and
/// this asserts they cannot come apart either.
#[test]
fn the_heading_says_which_chain_the_controls_are_pointed_at() {
let Some(mut s) = session() else {
eprintln!("no adapter; skipping");
return;
};
assert_eq!(scope_label(&s), GLOBAL_SCOPE, "nothing selected");
let id = s
.add_gradient_mask(false)
.expect("a gradient needs no model");
assert_ne!(
scope_label(&s),
GLOBAL_SCOPE,
"adding a layer selects it, so the panel is already scoped to it \
and must already say so"
);
// And the name is the one the row in the stack shows. Two names for
// one layer would let the selected row and the panel it scopes appear
// to disagree.
let row = s
.mask_layers()
.into_iter()
.find(|(layer_id, ..)| *layer_id == id)
.expect("the layer is in the stack");
assert_eq!(scope_label(&s), row.1.to_uppercase());
s.set_active_mask(None);
assert_eq!(
scope_label(&s),
GLOBAL_SCOPE,
"clearing the selection must put the heading back, or the panel \
would go on naming a layer it is no longer editing"
);
}
/// TRACES: FR-UI-5
/// Leaving local mode is what clears the selection, and this is the half
/// of it that can be tested without a window.
///
/// The mode handler in `lib.rs` calls `set_active_mask(None)`; what has to
/// be true afterwards is that the controls are global *and say so*. A mode
/// that was left with a layer still selected would leave thirty sliders
/// pointed at a region of the photograph with nothing on screen saying it.
#[test]
fn clearing_the_selection_returns_the_rows_to_the_whole_photograph() {
let Some(mut s) = session() else {
eprintln!("no adapter; skipping");
return;
};
// The scope is invisible in the *shape* of the panel — a layer holds
// the same chain the frame does, so both produce the same rows in the
// same order. It is only visible in what those rows read, which is
// precisely why the fault was silent: the panel looks identical either
// way and means something different.
//
// Addressed by index rather than by name, because no part of the
// frontend may route by a parameter's identity (FR-DEV-3a).
let first = s.rows()[0].clone();
s.set_param(first.op_index, first.param_index, first.maximum);
assert_eq!(s.rows()[0].value, first.maximum, "the global chain moved");
// Adding a layer selects it, so the same row is now the layer's.
s.add_gradient_mask(true).expect("gradient");
assert_eq!(
s.rows()[0].value,
first.default_value,
"the same control, pointed somewhere else and reading its own \
value — the whole hazard, in one row"
);
s.set_active_mask(None);
assert_eq!(
s.rows()[0].value,
first.maximum,
"and leaving the layer puts the frame's value back"
);
assert_eq!(scope_label(&s), GLOBAL_SCOPE);
}
/// TRACES: FR-UI-3
/// The handles' model keeps one identity for the life of the process.
///
/// **This is what makes a drag last longer than one frame.** The handles
/// are a repeater over this model, and a *new* `ModelRc` makes Slint throw
/// the repeated items away and build fresh ones — taking the `TouchArea`
/// that holds the pointer with them. The symptom is precise and was seen
/// on screen before it was understood: the handle jumps once, on the first
/// pointer event, and then sits dead under a finger that is still down.
///
/// It cannot be asserted through a window without a Slint backend, so it is
/// asserted where it is decided. Every path that touches the handles —
/// `sync_handles` and `clear_handles` — goes through this one model.
#[test]
fn the_handles_are_one_model_rewritten_rather_than_a_new_one_each_time() {
assert!(
Rc::ptr_eq(&handle_model(), &handle_model()),
"a fresh model per sync destroys the gesture that is moving the \
handles"
);
}
/// TRACES: FR-DEV-3 | FR-UI-3
/// A gradient offers handles; a mask with nothing to drag offers none.
///
/// This is the panel's whole test for whether to draw anything on the
/// canvas, so it is worth pinning: handles over a subject mask would
/// suggest an outline that cannot be moved can be.
#[test]
fn only_a_selected_gradient_puts_handles_on_the_canvas() {
let Some(mut s) = session() else {
eprintln!("no adapter; skipping");
return;
};
assert!(s.gradient_handles().is_empty(), "nothing selected");
s.add_gradient_mask(false).expect("linear");
assert_eq!(s.gradient_handles().len(), 3, "centre, width and rotation");
s.add_gradient_mask(true).expect("radial");
assert_eq!(
s.gradient_handles().len(),
3,
"centre and two semi-axes — the major one carries the angle, so \
an ellipse needs no fourth handle to say it twice"
);
s.set_active_mask(None);
assert!(
s.gradient_handles().is_empty(),
"a gradient nobody has selected is not being edited"
);
}
}