From 69b12e327f6ffa4936585052f9f4beb81a758c39 Mon Sep 17 00:00:00 2001 From: Duncan Tourolle Date: Sun, 16 Aug 2026 23:29:23 +0200 Subject: [PATCH] Let framing say it wants the canvas, instead of the panel knowing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- core/dr-pipeline/src/framing.rs | 51 ++++++- core/dr-pipeline/src/graph.rs | 10 +- ui/dr-ui/src/develop.rs | 240 ++++++++++++++++++-------------- 3 files changed, 192 insertions(+), 109 deletions(-) diff --git a/core/dr-pipeline/src/framing.rs b/core/dr-pipeline/src/framing.rs index 3760ff3..33863c1 100644 --- a/core/dr-pipeline/src/framing.rs +++ b/core/dr-pipeline/src/framing.rs @@ -41,7 +41,10 @@ use std::f32::consts::PI; use std::fmt::Write as _; -use crate::descriptor::{LocalizedKey, OpDescriptor, OpId, ParamDescriptor, ParamId, Scale, Unit}; +use crate::descriptor::{ + LocalizedKey, OpDescriptor, OpId, ParamDescriptor, ParamId, Presentation, Scale, Unit, + WidgetDemand, WidgetKind, +}; use crate::operation::Affects; pub const ID: OpId = OpId("framing"); @@ -62,6 +65,14 @@ pub const CROP_H: ParamId = ParamId("crop_h"); /// the edits actually are. pub const MAX_STRAIGHTEN: f32 = 45.0; +/// The parameters the framing widget owns — every one of them. +/// +/// In the order the widget expects: the rect first, then the angle it is +/// straightened by, then the exact reorientations. +static FRAMING_PARAMS: [ParamId; 8] = [ + CROP_X, CROP_Y, CROP_W, CROP_H, ANGLE, ROTATION, FLIP_H, FLIP_V, +]; + static DESCRIPTOR: OpDescriptor = OpDescriptor { id: ID, label: LocalizedKey("op.framing"), @@ -242,6 +253,44 @@ impl Framing { &DESCRIPTOR } + /// TRACES: FR-DEV-3a | FR-DEV-3b | FR-UI-7 + /// How framing would like to be presented. + /// + /// **This is what stops a frontend having to name this stage.** Rendered + /// generically these eight parameters are eight bad controls: four crop + /// edges the photographer would have to type coordinates into, a "rotate" + /// slider running 0..3, and two switches. Every one of them is a worse + /// control than the gesture it stands for — a crop is dragged on the + /// photograph and a quarter turn is a button. + /// + /// Before this existed, the frontend knew that by *checking the operation + /// id* and skipping it, which is precisely the naming ARCH §4.3a forbids: + /// a second frontend would have had to learn the same special case, and + /// nothing in the capability output said why. Now the preference is + /// declared, the demand says what the widget needs, and a frontend that + /// cannot meet it falls back to the eight sliders — tedious, but complete, + /// which is the guarantee the whole hint mechanism rests on. + /// + /// The widget owns **all eight** parameters rather than only the rect: a + /// frontend that takes this on is taking on 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. + pub fn presentation(&self) -> Option { + Some(Presentation { + widgets: &[WidgetKind::CropOverlay], + demand: WidgetDemand { + // A crop rect is dragged by its corners; nothing about that + // reduces to one axis at a time. + two_dimensional: true, + // Deliberately false. The handles are large and a crop is + // forgiving — FR-UI-7 has the interaction regions grow to the + // modality, so a thumb is as workable as a mouse. + precise_pointing: false, + }, + params: &FRAMING_PARAMS, + }) + } + /// What this stage affects, for invalidation scoping (FR-DEV-3d). pub fn affects(&self) -> Affects { Affects::Geometry diff --git a/core/dr-pipeline/src/graph.rs b/core/dr-pipeline/src/graph.rs index 1ecca06..89f0ca9 100644 --- a/core/dr-pipeline/src/graph.rs +++ b/core/dr-pipeline/src/graph.rs @@ -186,10 +186,12 @@ impl EditGraph { facet: p.facet, }) .collect(), - // Framing is not an `Operation`, so it has no `presentation` to - // ask for. A crop overlay is a viewport interaction rather than a - // panel widget, which is a different mechanism again. - presentation: None, + // Framing is not an `Operation`, but it has the same thing to say + // about how it wants drawing: a crop is dragged on the photograph. + // Declaring it here is what lets the frontend skip generating + // sliders for framing *without naming framing* — see + // `Framing::presentation`. + presentation: self.framing.presentation(), }; ops.chain(std::iter::once(framing)).collect() diff --git a/ui/dr-ui/src/develop.rs b/ui/dr-ui/src/develop.rs index 9074529..9311945 100644 --- a/ui/dr-ui/src/develop.rs +++ b/ui/dr-ui/src/develop.rs @@ -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 { 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 { 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());