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

Let framing say it wants the canvas, instead of the panel knowing

Browse Source 
The generated panel opened with a special case:

    if op.id == dr_pipeline::framing::ID { continue; }

and a paragraph explaining that framing's eight parameters are eight bad
controls — four crop edges you would have to type coordinates into, a "rotate"
slider running 0..3, two switches — so `GeometryPanel` presents them as the
gestures they are instead.

Every word of that is true, and none of it was the frontend's to know. It is a
fact about the operation, and ARCH §4.3a is explicit that a frontend deciding
things by *naming a stage* is the boundary being crossed: a second frontend
would have had to learn the same special case, and nothing in the capability
output said why it existed.

Framing now declares a `Presentation` preferring `WidgetKind::CropOverlay`,
with a demand of two-dimensional dragging and — deliberately — no precise
pointing, since FR-UI-7 grows the handles to the modality and a crop is
forgiving. The widget owns all eight parameters rather than only the rect: a
frontend taking this on takes the whole framing control surface, and leaving
rotation and the flips behind would scatter them into the generated panel
underneath a crop control that already exists.

The panel's rule is now general. `supported` answers whether a kind is
implemented *anywhere* — drawn in the panel like the tone curve, or hosted on
the canvas like the crop — and `is_on_canvas` settles which afterwards, so the
two cannot disagree about the same kind. Any stage preferring an on-canvas
widget is skipped, with nothing named. A frontend that implements neither still
gets the eight sliders: tedious, complete, and the guarantee the whole hint
mechanism rests on.

**The tests were asserting against the wrong thing.** `rows_of` was a
hand-written simulation of the row generator, complete with its own copy of the
framing skip, so the suite was checking a second implementation kept in step by
hand. It was not in step: giving framing a presentation changed the real panel
and the simulation disagreed, which is exactly how a green suite hides a
regression. It now calls `rows_from`, and `rows_of_unfiltered` is gone.

`framing_is_not_generated_as_sliders` survives but asserts by routing rather
than by counting — no row may carry framing's capability index — so it cannot
be satisfied by two miscounts cancelling out. Alongside it, an invented stage
preferring a widget this frontend lacks, falling back to one the canvas hosts,
must also be skipped: if that ever needs a name added to pass, the special case
has grown back.

Not included: moving the crop overlay's markup out of app.slint into
controls.slint. It is cosmetic next to the above and app.slint is in another
session's working set.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
dtourolle
2026-08-16 23:29:23 +02:00
co-authored by Claude Opus 5
parent 0a331c717e
commit 69b12e327f
3 changed files with 192 additions and 109 deletions
Download Patch File Download Diff File Expand all files Collapse all files
ui/dr-ui/src/develop.rs
+136 -104
View File
@@ -88,19 +88,28 @@ impl DevelopSession {
}
}
/// Whether this frontend has an implementation of `widget`.
/// Whether this frontend has an implementation of `widget` **anywhere**.
///
/// The single source of truth for what the panel can draw. A function rather
/// than a constant list so that a kind whose availability depends on something
/// — a canvas-hosted widget needs a canvas — has one obvious place to say so.
pub(crate) fn draws(widget: WidgetKind) -> bool {
/// "Anywhere" is doing real work: a widget may be drawn in the panel, as the
/// tone curve is, or hosted on the canvas, as the crop is. Both count as
/// implemented, and the difference is settled afterwards by
/// [`WidgetKind::is_on_canvas`] rather than by two separate lists that could
/// disagree about the same kind.
///
/// A kind answering `false` here is not an error — the operation's parameters
/// are ordinary scalars, so it falls back to sliders and stays fully editable
/// (ARCH §4.3a).
pub(crate) fn supported(widget: WidgetKind) -> bool {
match widget {
// Drawn in the panel.
WidgetKind::ToneCurve => true,
// Not implemented here. The `is_on_canvas` kinds additionally need a
// host the panel does not have: the canvas draws those, and the
// panel's job is the affordance that turns them on.
// Hosted on the canvas: the overlay is drawn over the photograph and
// the panel contributes `GeometryPanel`, the affordance that turns it
// on.
WidgetKind::CropOverlay => true,
// Not implemented. Listed rather than caught by a wildcard so the next
// kind added to the core surfaces here as a compile error.
WidgetKind::ColourWheel
| WidgetKind::CropOverlay
| WidgetKind::GradientHandle
| WidgetKind::BrushMask
| WidgetKind::WhitePoint => false,
@@ -118,19 +127,6 @@ pub(crate) fn draws(widget: WidgetKind) -> bool {
pub(crate) fn rows_from(caps: &[OpCapability]) -> Vec<ParamRow> {
let mut rows = Vec::new();
for (op_index, op) in caps.iter().enumerate() {
// Framing has a panel of its own.
//
// The one place this side names a stage, and the exception proves
// the rule: every *other* operation is rendered from its
// descriptor alone. Framing is skipped because its parameters are
// not sliders in any useful sense — four crop edges are dragged on
// the photograph and a quarter turn is a button — so it is
// presented by `GeometryPanel` instead of generated here. Emitting
// both would show the same eight values twice, in one good control
// surface and one bad one.
if op.id == dr_pipeline::framing::ID {
continue;
}
// Where this operation's rows begin. The panel groups by walking
// back to it, so it has to be taken before any row is pushed.
let group_head = rows.len();
@@ -141,31 +137,50 @@ pub(crate) fn rows_from(caps: &[OpCapability]) -> Vec<ParamRow> {
if let Some(presentation) = &op.presentation {
// **The widget registry, and the only one.**
//
// `choose` walks the operation's preference list and hands
// back the first entry this frontend implements (ARCH §4.3a).
// Everything not listed here is unimplemented by definition,
// so an operation asking for a colour wheel gets sliders
// rather than an error — which is the designed behaviour, not
// a gap: every parameter is an individually addressable
// scalar, so the edit still works.
//
// The `match` inside is exhaustive on purpose. Adding a
// `WidgetKind` to the core stops this compiling until someone
// has decided, here, whether this frontend draws it.
let row = presentation.choose(draws).and_then(|widget| match widget {
WidgetKind::ToneCurve => curve_row(op_index, group_head, op, presentation),
// Named but not drawn here. Listed rather than caught
// by a wildcard so that the next kind added to the
// core surfaces as a compile error in this file.
WidgetKind::ColourWheel
| WidgetKind::CropOverlay
| WidgetKind::GradientHandle
| WidgetKind::BrushMask
| WidgetKind::WhitePoint => None,
});
if let Some(row) = row {
rows.push(row);
continue;
// `choose` walks the operation's preference list and hands back
// the first entry this frontend implements (ARCH §4.3a). A kind
// it does not implement falls through to sliders — the designed
// behaviour, not a gap, since every parameter is an individually
// addressable scalar.
if let Some(widget) = presentation.choose(supported) {
// **Yielded to the canvas, and this is what replaced naming
// framing.**
//
// This loop used to open with `if op.id == framing::ID { continue }`
// and a paragraph explaining that a crop is dragged on the
// photograph rather than typed into four boxes. All of that is
// true and none of it was this file's to know: it is a fact
// about the operation, and it now arrives as one. Any stage
// preferring an on-canvas widget is skipped here on the same
// terms, with nothing named.
//
// Skipped rather than rendered as an affordance row, because
// the affordance is `GeometryPanel` — a bespoke control for a
// known stage, which is a thing the interface is entitled to
// build (ARCH §4.3a draws the line at the *generated* panel
// naming stages, not at the interface having hand-made
// widgets).
if widget.is_on_canvas() {
continue;
}
// The `match` is exhaustive on purpose. Adding a `WidgetKind`
// to the core stops this compiling until someone has decided,
// here, whether the panel draws it.
let row = match widget {
WidgetKind::ToneCurve => curve_row(op_index, group_head, op, presentation),
// Canvas-hosted kinds returned above; the rest are not
// implemented and reached sliders via `choose`.
WidgetKind::ColourWheel
| WidgetKind::CropOverlay
| WidgetKind::GradientHandle
| WidgetKind::BrushMask
| WidgetKind::WhitePoint => None,
};
if let Some(row) = row {
rows.push(row);
continue;
}
}
}
@@ -1029,52 +1044,74 @@ mod tests {
#[test]
fn framing_is_not_generated_as_sliders() {
// The geometry panel presents crop, rotation, flips and straightening
// as the gestures they are. If the generic path emitted them too, the
// `GeometryPanel` presents crop, rotation, flips and straightening as
// the gestures they are. If the generic path emitted them too the
// sidebar would carry both — including four "Crop Left/Top/Width/
// Height" sliders that no one can compose a photograph with.
// Height" sliders no one can compose a photograph with.
let graph = EditGraph::default_chain();
let caps = graph.capabilities();
assert!(
caps.iter().any(|c| c.id == dr_pipeline::framing::ID),
"the chain must still expose framing — the panel reads it"
);
// Asserted through the row count rather than by inspecting labels: a
// leaked framing group would add its eight parameters as eight rows,
// and the difference is exactly what `rows_of` must not contain.
let framing_params = caps
let framing = caps
.iter()
.find(|c| c.id == dr_pipeline::framing::ID)
.map(|c| c.params.len())
.expect("framing is in the chain");
assert!(framing_params > 0);
.position(|c| c.id == dr_pipeline::framing::ID)
.expect("the chain must still expose framing — the panel reads it");
assert!(!caps[framing].params.is_empty());
let generated = rows_of(&caps).len();
let with_framing = rows_of_unfiltered(&caps).len();
assert_eq!(
with_framing - generated,
framing_params,
// Checked against the real generator, and by *routing* rather than by
// counting: a row carries the capability index it writes back to, so
// "no row belongs to framing" is the property directly, and it cannot
// be satisfied accidentally by two miscounts cancelling out.
let rows = rows_from(&caps);
assert!(
rows.iter().all(|r| r.op_index as usize != framing),
"framing parameters leaked into the generated panel"
);
// Every other operation still arrives, so the skip is specific rather
// than the panel having quietly stopped generating.
assert!(rows.len() > caps.len() - 1);
}
/// `rows_of` without the framing skip — the shape the panel would have if
/// framing were generated, which is what the test above measures against.
fn rows_of_unfiltered(caps: &[OpCapability]) -> Vec<(usize, usize)> {
let mut rows = Vec::new();
for op in caps {
let head = rows.len();
let collapses = op
.presentation
.as_ref()
.is_some_and(|p| p.params.len() == op.params.len());
let len = if collapses { 1 } else { op.params.len() };
for _ in 0..len {
rows.push((head, len));
}
}
rows
#[test]
fn a_stage_is_yielded_to_the_canvas_by_what_it_declares_not_by_its_name() {
// The property that replaced `if op.id == framing::ID`. An invented
// stage preferring an on-canvas widget must be skipped on exactly the
// same terms — if this needs a name added anywhere to pass, the
// special case has grown back.
use dr_pipeline::{LocalizedKey, ParamCapability, WidgetDemand};
let param = |id: &'static str| ParamCapability {
id: ParamId(id),
label: LocalizedKey("param.invented"),
kind: ParamKind::Scalar {
min: 0.0,
max: 1.0,
scale: dr_pipeline::Scale::Linear,
unit: Unit::None,
precision: 2,
},
default: 0.0,
value: 0.0,
facet: None,
};
let on_canvas = OpCapability {
id: OpId("invented_mask"),
label: LocalizedKey("op.invented_mask"),
active: false,
presentation: Some(Presentation {
// Prefers a gradient handle; this frontend has none, so it
// falls back to the next entry, which the canvas does host.
widgets: &[WidgetKind::GradientHandle, WidgetKind::CropOverlay],
demand: WidgetDemand {
two_dimensional: true,
precise_pointing: false,
},
params: &[ParamId("a"), ParamId("b")],
}),
params: vec![param("a"), param("b")],
};
assert!(rows_from(&[on_canvas]).is_empty());
}
#[test]
@@ -1281,32 +1318,27 @@ mod tests {
],
};
assert!(!draws(WidgetKind::ColourWheel), "precondition");
assert!(!supported(WidgetKind::ColourWheel), "precondition");
let rows = rows_from(&[wheel]);
assert_eq!(rows.len(), 2, "both parameters must remain reachable");
assert!(rows.iter().all(|r| r.kind == "scalar"));
}
/// Each generated row's `(group_head, group_len)`.
///
/// Taken from the real generator rather than re-derived. This used to be a
/// hand-written simulation of `rows_from` — it walked the capabilities and
/// reproduced the grouping rules, including a copy of the framing skip —
/// which meant the tests below asserted against a second implementation
/// that had to be kept in step with the first by hand. It was not: giving
/// framing a presentation changed the real panel and the simulation
/// disagreed, which is how a passing test suite would have hidden the
/// change entirely.
fn rows_of(caps: &[OpCapability]) -> Vec<(usize, usize)> {
let mut rows = Vec::new();
for op in caps {
// Framing is presented by `GeometryPanel`, not generated — mirror
// the skip, or these tests assert against a panel that is not the
// one the interface builds.
if op.id == dr_pipeline::framing::ID {
continue;
}
let head = rows.len();
let collapses = op
.presentation
.as_ref()
.is_some_and(|p| p.params.len() == op.params.len());
let len = if collapses { 1 } else { op.params.len() };
for _ in 0..len {
rows.push((head, len));
}
}
rows
rows_from(caps)
.iter()
.map(|r| (r.group_head as usize, r.group_len as usize))
.collect()
}
#[test]
@@ -1579,7 +1611,7 @@ mod tests {
.expect("the curve declares a widget");
// Asked the way the panel asks it: the first preference this frontend
// implements, not a fixed single kind.
assert_eq!(presentation.choose(draws), Some(WidgetKind::ToneCurve));
assert_eq!(presentation.choose(supported), Some(WidgetKind::ToneCurve));
// Every parameter is owned by the widget, so none is left over to be
// rendered as a stray slider.
assert_eq!(presentation.params.len(), curve_cap.params.len());