From 9b2ee0d0ebf331bafadf7ad61887022ae67f7166 Mon Sep 17 00:00:00 2001 From: Duncan Tourolle Date: Sun, 16 Aug 2026 16:14:13 +0200 Subject: [PATCH] Show the colour mixer as three runs of twelve, each row a colour MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The mixer was thirty-six sliders reading "Hue / Sat / Lum" twelve times over with nothing saying which band any row belonged to. The identity was there all along — the descriptor declares param.mixer.orange.sat and BANDS carries orange at 30° — and was discarded on the way out: labels.rs had no mixer entries, so every key fell through to a derived label that yields the bare channel name. A parameter can now say which aspect it adjusts and which subject it adjusts it on, with the subject's hue where the subject is a colour (descriptor::Facet). That is data about what the operation does, not a layout: the mixer genuinely weights pixels around 30°. What to draw from 30°, and in what order to stack the runs, stay in dr-ui (ARCH §4.3a) — develop.rs brings rows sharing an aspect together and marks the first of each, and adjust.slint names the run once and draws a swatch, a track and a readout on one line. Grouped by channel rather than by band because an edit is almost never "everything about orange"; it is the saturation of the greens, made by comparing one channel across neighbouring bands. Twelve band sections put those twelve rows in twelve different places. The swatch is the label, which is what makes twelve rows fit where four did. The band name is not lost: it is the row's accessible label, so the control is not colour-only, and labels.rs is where the mapping is written down — including chartreuse as "Yellow-Green" and spring as "Blue-Green", since nobody hunting foliage scans a list for "Spring". Co-Authored-By: Claude Opus 5 --- core/dr-pipeline/src/descriptor.rs | 86 ++++++++++- core/dr-pipeline/src/graph.rs | 9 +- core/dr-pipeline/src/lib.rs | 4 +- core/dr-pipeline/src/ops/colour_mixer.rs | 109 +++++++++++--- docs/ui-refinement.md | 62 ++++++-- ui/dr-ui/src/develop.rs | 177 ++++++++++++++++++++++- ui/dr-ui/src/labels.rs | 32 ++++ ui/dr-ui/style.yaml | 9 ++ ui/dr-ui/ui/adjust.slint | 170 +++++++++++++++++++++- ui/dr-ui/ui/widgets.slint | 39 +++++ 10 files changed, 653 insertions(+), 44 deletions(-) diff --git a/core/dr-pipeline/src/descriptor.rs b/core/dr-pipeline/src/descriptor.rs index 1888bfd..4d1cd23 100644 --- a/core/dr-pipeline/src/descriptor.rs +++ b/core/dr-pipeline/src/descriptor.rs @@ -113,6 +113,39 @@ pub struct Presentation { pub params: &'static [ParamId], } +/// TRACES: FR-DEV-3a +/// A parameter's place in an operation whose parameters form a grid. +/// +/// Most operations are a short list of unrelated controls. A few are the +/// *same* control applied to a series of subjects: the colour mixer is twelve +/// hue bands times hue, saturation and luminance, and rendered as a flat list +/// of thirty-six it says none of that — the panel showed "Hue / Sat / Lum" +/// twelve times over with nothing naming the band. +/// +/// So a parameter may say which **aspect** it adjusts and which **subject** it +/// adjusts it on. A UI is free to ignore both and render a flat list; nothing +/// becomes unreachable, it merely reads as thirty-six anonymous sliders again. +/// +/// **Why this is not the core deciding presentation** (ARCH §4.3a). Which +/// band a parameter belongs to, and that its centre is at 30°, are facts about +/// what the operation *does* — the mixer genuinely weights pixels around 30°, +/// and that number is the one it weights around. What colour to draw from it, +/// at what saturation, whether to draw anything at all, and in what order to +/// stack the runs are all presentation, and stay in `dr-ui`. The line: a hue +/// in degrees is data; a hex colour in a descriptor would be the core choosing +/// appearance, and is forbidden. +#[derive(Debug, Clone, Copy, PartialEq)] +pub struct Facet { + /// What this parameter adjusts. Parameters sharing an aspect are one + /// control applied to different subjects. + pub aspect: LocalizedKey, + /// What it adjusts it on. + pub subject: LocalizedKey, + /// Where the subject sits on the hue wheel, in degrees, where the subject + /// is a colour. `None` for one that is not. + pub subject_hue: Option, +} + /// One parameter of an operation. #[derive(Debug, Clone, PartialEq)] pub struct ParamDescriptor { @@ -120,6 +153,10 @@ pub struct ParamDescriptor { pub label: LocalizedKey, pub kind: ParamKind, pub default: f32, + /// Where this parameter sits among its siblings, for an operation whose + /// parameters form a grid. `None` — the usual case — is a parameter that + /// stands on its own. + pub facet: Option, } impl ParamDescriptor { @@ -136,6 +173,7 @@ impl ParamDescriptor { precision: 2, }, default: 0.0, + facet: None, } } @@ -153,6 +191,7 @@ impl ParamDescriptor { precision: 0, }, default: 0.0, + facet: None, } } @@ -163,6 +202,7 @@ impl ParamDescriptor { label: LocalizedKey(label), kind: ParamKind::Bool, default: 0.0, + facet: None, } } @@ -184,15 +224,19 @@ impl ParamDescriptor { precision: 4, }, default, + facet: None, } } /// A general scalar with an explicit range and default. // // Eight arguments, and a builder would be the usual answer — but this has - // to be `const` so descriptors can be `static`, and `const` functions - // cannot use a builder's method chain. The two common shapes have their - // own constructors above; this is the escape hatch for the rest. + // to be `const` so descriptors can be `static`, which rules out the + // `&mut self` builder the pattern usually takes. (A `self`-by-value step + // *is* const-callable — `faceted` below is one — but eight of them would + // be eight methods to say what one call already says.) The two common + // shapes have their own constructors above; this is the escape hatch for + // the rest. #[allow(clippy::too_many_arguments)] pub const fn scalar( id: &'static str, @@ -215,9 +259,23 @@ impl ParamDescriptor { precision, }, default, + facet: None, } } + /// The same parameter, placed in its operation's grid. + /// + /// A method rather than a sixth constructor, because a facet is orthogonal + /// to the shape of the value: a faceted parameter is still an amount, or + /// still a scalar in stops, and pairing every constructor with a faceted + /// twin would double the list above to say one thing. Taking `self` by + /// value is what keeps it usable in the `static` descriptors — a `&mut + /// self` builder is what cannot be `const`. + pub const fn faceted(mut self, facet: Facet) -> Self { + self.facet = Some(facet); + self + } + /// Clamp a value into this parameter's declared range. /// /// Applied before the value reaches a shader: a slider dragged past its @@ -280,6 +338,28 @@ mod tests { assert_eq!(P.clamp(f32::INFINITY), P.default); } + #[test] + fn a_parameter_stands_alone_unless_it_says_otherwise() { + // The default has to be "no grid": every operation but the mixer is a + // short list of unrelated controls, and one that accidentally claimed + // a facet would have its panel section split under a heading it never + // asked for. + assert!(P.facet.is_none()); + assert!(ParamDescriptor::switch("s", "s").facet.is_none()); + + let faceted = P.faceted(Facet { + aspect: LocalizedKey("param.channel.sat"), + subject: LocalizedKey("band.orange"), + subject_hue: Some(30.0), + }); + // Placing a parameter in a grid must not change what the parameter + // *is* — the value it carries, its range and its default are the same + // either way. + assert_eq!(faceted.kind, P.kind); + assert_eq!(faceted.default, P.default); + assert_eq!(faceted.facet.unwrap().subject_hue, Some(30.0)); + } + #[test] fn amount_controls_are_neutral_at_zero() { // Double-tap-to-reset and "is this op doing anything" both depend on diff --git a/core/dr-pipeline/src/graph.rs b/core/dr-pipeline/src/graph.rs index 62c082a..f122a7e 100644 --- a/core/dr-pipeline/src/graph.rs +++ b/core/dr-pipeline/src/graph.rs @@ -7,7 +7,9 @@ //! Order is data, not code: operations run in the sequence this holds them, //! so reordering the pipeline needs no code change. -use crate::descriptor::{LocalizedKey, OpDescriptor, OpId, ParamId, ParamKind, Presentation}; +use crate::descriptor::{ + Facet, LocalizedKey, OpDescriptor, OpId, ParamId, ParamKind, Presentation, +}; use crate::framing::{CropRect, Framing}; use crate::operation::{compose_with_framing, ComposedShader, Operation}; use crate::ops; @@ -50,6 +52,9 @@ pub struct ParamCapability { pub default: f32, /// The current setting, so the control opens where the edit actually is. pub value: f32, + /// Where this parameter sits among its siblings, when the operation's + /// parameters form a grid rather than a list. `None` for the usual case. + pub facet: Option, } impl ParamCapability { @@ -165,6 +170,7 @@ impl EditGraph { kind: p.kind.clone(), default: p.default, value: op.param(p.id), + facet: p.facet, }) .collect(), presentation: op.presentation(), @@ -187,6 +193,7 @@ impl EditGraph { kind: p.kind.clone(), default: p.default, value: self.framing.param(p.id), + facet: p.facet, }) .collect(), // Framing is not an `Operation`, so it has no `presentation` to diff --git a/core/dr-pipeline/src/lib.rs b/core/dr-pipeline/src/lib.rs index 3d3968a..1256ff5 100644 --- a/core/dr-pipeline/src/lib.rs +++ b/core/dr-pipeline/src/lib.rs @@ -40,8 +40,8 @@ pub mod ops; pub mod sidecar; pub use descriptor::{ - LocalizedKey, OpDescriptor, OpId, ParamDescriptor, ParamId, ParamKind, Presentation, Scale, - Unit, WidgetKind, + Facet, LocalizedKey, OpDescriptor, OpId, ParamDescriptor, ParamId, ParamKind, Presentation, + Scale, Unit, WidgetKind, }; pub use framing::{CropRect, Framing}; pub use graph::{EditGraph, OpCapability, ParamCapability}; diff --git a/core/dr-pipeline/src/ops/colour_mixer.rs b/core/dr-pipeline/src/ops/colour_mixer.rs index baefacb..254e581 100644 --- a/core/dr-pipeline/src/ops/colour_mixer.rs +++ b/core/dr-pipeline/src/ops/colour_mixer.rs @@ -21,7 +21,7 @@ //! setting every band's saturation to +100 gives the same result as setting //! the global saturation to +100 rather than something far stronger. -use crate::descriptor::{LocalizedKey, OpDescriptor, OpId, ParamDescriptor, ParamId}; +use crate::descriptor::{Facet, LocalizedKey, OpDescriptor, OpId, ParamDescriptor, ParamId}; use crate::operation::{Helper, Operation, Uniform}; use crate::ops::helpers; @@ -114,43 +114,72 @@ impl Channel { // Parameter descriptors, one per band per channel. Written out rather than // generated because `ParamDescriptor` must be `const` to live in a `static`, // and a const loop cannot build a slice. The macro keeps it honest. +// +// **Every one of them is faceted**, and that is what makes the operation +// legible in a panel. Thirty-six parameters presented as a flat list are +// thirty-six sliders reading "Hue / Sat / Lum" twelve times with nothing +// saying which band any row belongs to — the identity is right here in the +// descriptor and used to be discarded on the way out. The facet carries it: +// the channel as the aspect, the band as the subject, and the band's centre +// hue so a frontend can identify the row by the colour it edits rather than +// by a word. What a frontend *does* with 30° is its own business (ARCH +// §4.3a); this only says the parameter acts on the band centred there. macro_rules! band_params { - ($($key:literal),* $(,)?) => { + ($(($key:literal, $hue:literal)),* $(,)?) => { &[ $( ParamDescriptor::amount( concat!($key, "_hue"), concat!("param.mixer.", $key, ".hue"), - ), + ) + .faceted(Facet { + aspect: LocalizedKey("param.channel.hue"), + subject: LocalizedKey(concat!("band.", $key)), + subject_hue: Some($hue), + }), ParamDescriptor::amount( concat!($key, "_sat"), concat!("param.mixer.", $key, ".sat"), - ), + ) + .faceted(Facet { + aspect: LocalizedKey("param.channel.sat"), + subject: LocalizedKey(concat!("band.", $key)), + subject_hue: Some($hue), + }), ParamDescriptor::amount( concat!($key, "_lum"), concat!("param.mixer.", $key, ".lum"), - ), + ) + .faceted(Facet { + aspect: LocalizedKey("param.channel.lum"), + subject: LocalizedKey(concat!("band.", $key)), + subject_hue: Some($hue), + }), )* ] }; } +// A fourth table parallel to `BANDS` and the three uniform-name tables, for +// the same reason as those: `concat!` needs literals, so the keys and hues +// cannot be read out of `BANDS` here. `facets_match_their_bands` below is +// what keeps them from drifting. static DESCRIPTOR: OpDescriptor = OpDescriptor { id: ID, label: LocalizedKey("op.colour_mixer"), params: band_params![ - "red", - "orange", - "yellow", - "chartreuse", - "green", - "spring", - "cyan", - "azure", - "blue", - "violet", - "magenta", - "rose", + ("red", 0.0), + ("orange", 30.0), + ("yellow", 60.0), + ("chartreuse", 90.0), + ("green", 120.0), + ("spring", 150.0), + ("cyan", 180.0), + ("azure", 210.0), + ("blue", 240.0), + ("violet", 270.0), + ("magenta", 300.0), + ("rose", 330.0), ], }; @@ -501,6 +530,52 @@ mod tests { } } + #[test] + fn facets_match_their_bands() { + // The macro's `(key, hue)` list is a fourth table parallel to `BANDS`, + // and a hue mistyped there would put a row's swatch on a colour the + // band does not act on — a control that lies about what it edits, + // which is worse than one with no swatch at all. + for p in DESCRIPTOR.params { + let facet = p.facet.expect("every mixer parameter is faceted"); + let (band_key, _) = p.id.0.rsplit_once('_').expect("id is band_channel"); + let band = BANDS + .iter() + .find(|b| b.key == band_key) + .expect("the id names a band"); + + assert_eq!( + facet.subject.0, + format!("band.{band_key}"), + "{} is subject to the wrong band", + p.id + ); + assert_eq!( + facet.subject_hue, + Some(band.hue), + "{} claims a hue its band does not have", + p.id + ); + } + } + + #[test] + fn each_channel_is_one_aspect_across_every_band() { + // What lets a panel name the run once instead of twelve times: the + // twelve hue parameters must agree they are the same control. Were + // the aspect keyed per band, grouping by it would produce thirty-six + // groups of one and nothing would have been gained. + let mut per_aspect = std::collections::BTreeMap::new(); + for p in DESCRIPTOR.params { + let facet = p.facet.expect("faceted"); + *per_aspect.entry(facet.aspect.0).or_insert(0) += 1; + } + assert_eq!(per_aspect.len(), Channel::ALL.len()); + for (aspect, count) in per_aspect { + assert_eq!(count, BANDS.len(), "{aspect} does not cover every band"); + } + } + #[test] fn a_fresh_mixer_is_inactive() { assert!(!ColourMixer::new().is_active()); diff --git a/docs/ui-refinement.md b/docs/ui-refinement.md index 79bd452..08aa5de 100644 --- a/docs/ui-refinement.md +++ b/docs/ui-refinement.md @@ -170,7 +170,7 @@ thirty-six anonymous sliders reading `Hue 0 / Sat 16 / Lum 0` twelve times over, with nothing saying which band any row belongs to. Three separate defects meet here. -### M1 — Band identity is lost (a bug, not a style issue) +### M1 — Band identity is lost (a bug, not a style issue) — **done** `labels.rs` has no `param.mixer.*` entries, so all thirty-six keys fall through to a default that yields the bare channel name. The core is not at @@ -181,11 +181,30 @@ discarded at resolution. **Deliverable.** Resolve mixer keys to their band. A row reads `Orange · Sat`, or `Sat` under a band heading — M3 decides which. -### M2 — Bands carry their centre hue as capability data +**Landed** as a `band.*` catalogue resolving the *subject* of a faceted +parameter (M2), rather than as entries for the thirty-six `param.mixer.*` +keys. Three bands are catalogued to something other than their key: chartreuse +reads "Yellow-Green" and spring "Blue-Green", because a photographer looking +for foliage does not scan a list for "Spring". + +### M2 — Bands carry their centre hue as capability data — **done** **Deliverable.** `ParamDescriptor` gains an optional band hue in degrees, set by `band_params!` from `BANDS`. The UI converts degrees to a swatch. +**Landed** as `descriptor::Facet` — a little wider than "a band hue", and the +width is what M3 turned out to need. A parameter may say which **aspect** it +adjusts (the channel) and which **subject** it adjusts it on (the band), with +the subject's hue attached where the subject is a colour. `ParamDescriptor` is +otherwise unchanged and `faceted()` is a const builder step, so the thirty-six +descriptors stay `static` and every other operation says nothing at all. + +The hue reaches the screen as `Swatch` in `widgets.slint`, which owns the +saturation and brightness. Those two are *not* in `style.yaml`: it holds +colours and lengths, and a third section for two floats used in one component +buys less than it costs. `Theme.swatch` — the square's size — is a length and +does live there. + **Why this is not a §4.3a violation.** A band's centre hue is a *fact about the operation* — the mixer genuinely acts on the 30° band, and that number is what it acts on. The core says "this parameter belongs to the band centred at @@ -202,15 +221,33 @@ exactly as an image is data. That is categorically different from an accent decorating a heading, which is what the palette rule forbids. Keep them small and let them identify, never dominate. -### M3 — Structure: twelve collapsible bands +### M3 — Structure: three runs of twelve, not twelve of three — **done** -**Deliverable.** The mixer renders as twelve `Section`s — one per band, titled -by band name, carrying its swatch — each holding Hue, Sat and Lum. Collapsed -by default; the modified dot (Workstream C) shows which bands hold an edit -without expanding them. +~~The mixer renders as twelve `Section`s — one per band, titled by band name, +carrying its swatch — each holding Hue, Sat and Lum. Collapsed by default.~~ -This needs no new `WidgetKind`: it is Workstream C's sections applied to a -grouping the frontend derives from parameter ids. Depends on C. +**Superseded, twice over.** The lids came off the develop column entirely +(Workstream C's sections are now plain headings), so "collapsed by default" +had nothing left to mean. And the grouping was the wrong way round: an edit is +almost never "everything about orange", it is "the saturation of the greens", +made by comparing one channel across neighbouring bands. Twelve band sections +put the twelve rows you want to compare in twelve different places. + +**Landed.** Three runs — Hue, Saturation, Luminance — of twelve rows each, +under the operation's own heading. `develop.rs` stacks the rows by aspect +(`presentation_order`) and marks the first of each run; `adjust.slint` names +the run once and draws the rest. Each row is a swatch, a track and a readout on +one line: the swatch *is* the label, which is what makes twelve rows fit where +four did, and the band name lives on as the row's accessible label so the +control is not colour-only. + +The mixer's declaration order is untouched — it declares band by band, which +is the order the shader wants. Rearranging it for the panel would have been +the core laying out a screen (§4.3a); doing it in `develop.rs` is the same +frontend-side derivation that decides there are groups at all. + +Nothing here is mixer-specific: any operation whose parameters carry facets +groups this way, and one that carries none is untouched. ### M4 — A single-parameter operation should not cost a heading @@ -222,9 +259,10 @@ parameters number one wants to *be* a named row, not a group containing one. row labelled by the operation. Derived frontend-side from the parameter count — the core says nothing about it, per §4.3a. -**Done when.** Every mixer row says which band it edits; bands collapse with -modified state visible while collapsed; Vibrance and Saturation are one row -each; and no hex colour appears in any descriptor. +**Done when.** Every mixer row says which band it edits; the rows for one +channel read as a run rather than as twelve unrelated sliders; Vibrance and +Saturation are one row each; and no hex colour appears in any descriptor. +**All four are met.** --- diff --git a/ui/dr-ui/src/develop.rs b/ui/dr-ui/src/develop.rs index 37b24d1..4dc281a 100644 --- a/ui/dr-ui/src/develop.rs +++ b/ui/dr-ui/src/develop.rs @@ -130,7 +130,13 @@ impl DevelopSession { let group_modified = op.params.iter().any(|p| p.value != p.default); let group_len = op.params.len() as i32; - for (param_index, p) in op.params.iter().enumerate() { + // The aspect the previous row belonged to, so a run can be told + // from its continuation. Reset per operation: two operations that + // happened to facet on the same key are still two groups. + let mut previous_aspect: Option<&str> = None; + + for param_index in presentation_order(&op.params) { + let p = &op.params[param_index]; let (kind, min, max, precision, unit) = match &p.kind { ParamKind::Scalar { min, @@ -148,11 +154,29 @@ impl DevelopSession { ParamKind::Bool => ("bool", 0.0, 1.0, 0, ""), }; + // A faceted parameter is named by its *subject* — the band — + // because its aspect is already written above the run it sits + // in. Unfaceted parameters keep their own label, which is + // every operation but the mixer. + let param_label = match &p.facet { + Some(f) => labels::resolve(f.subject.0), + None => labels::resolve(p.label.0), + }; + let aspect = p.facet.as_ref().map(|f| f.aspect.0); + let starts_facet = aspect.is_some() && aspect != previous_aspect; + previous_aspect = aspect; + rows.push(ParamRow { op_index: op_index as i32, param_index: param_index as i32, op_label: labels::resolve(op.label.0).into(), - param_label: labels::resolve(p.label.0).into(), + param_label: param_label.into(), + facet_label: aspect.map(labels::resolve).unwrap_or_default().into(), + starts_facet, + // -1 rather than an `Option`, which a Slint struct cannot + // carry: 0° is red, so no value in range can stand for + // "no swatch". + swatch_hue: p.facet.as_ref().and_then(|f| f.subject_hue).unwrap_or(-1.0), group_head: group_head as i32, group_len, group_modified, @@ -216,6 +240,11 @@ impl DevelopSession { param_index: base as i32, op_label: labels::resolve(op.label.0).into(), param_label: String::new().into(), + // A widget spanning a whole operation is not a row in anyone's + // grid, so it heads no run and carries no swatch. + facet_label: String::new().into(), + starts_facet: false, + swatch_hue: -1.0, group_head: group_head as i32, // One widget standing for every parameter of the operation, so // the group it heads is itself and nothing else. @@ -724,6 +753,47 @@ fn fit(sw: u32, sh: u32, max_w: u32, max_h: u32) -> (u32, u32) { ) } +/// The order an operation's parameters are shown in. +/// +/// Declaration order, unless the operation facets them — in which case +/// parameters sharing an aspect are brought together, so the panel names +/// each run once instead of repeating "Hue / Saturation / Luminance" +/// twelve times over. The colour mixer declares band by band, which is the +/// order the shader wants; a photographer works channel by channel. +/// +/// **This is presentation, and so it lives here** (ARCH §4.3a). The core +/// says which aspect a parameter belongs to; deciding that an aspect is +/// worth stacking rows by is the panel's composition to make, exactly as +/// grouping by operation is. Routing is unaffected — `param_index` stays +/// the position in the capability list however the rows are stacked. +/// +/// A stable sort by the aspect's first appearance, so an operation with no +/// facets comes back untouched, and one that mixes plain parameters with +/// faceted ones keeps the plain ones first and in order. +fn presentation_order(params: &[dr_pipeline::ParamCapability]) -> Vec { + let mut aspects: Vec<&str> = Vec::new(); + let rank: Vec = params + .iter() + .map(|p| match &p.facet { + None => 0, + Some(f) => { + let at = aspects.iter().position(|a| *a == f.aspect.0); + // First appearance defines the run's place, so the panel's + // sections come out in the order the operation introduced + // them rather than alphabetically. + 1 + at.unwrap_or_else(|| { + aspects.push(f.aspect.0); + aspects.len() - 1 + }) + } + }) + .collect(); + + let mut order: Vec = (0..params.len()).collect(); + order.sort_by_key(|i| rank[*i]); + order +} + /// Suffix shown after a value. Comes from the descriptor's declared unit, so /// this function needs no knowledge of which parameter it is formatting. fn unit_suffix(unit: Unit) -> &'static str { @@ -1151,6 +1221,109 @@ mod tests { assert!(rotate_crop(CropRect::default(), -3).is_full()); } + #[test] + fn an_operation_without_facets_keeps_its_declared_order() { + // Every operation but the mixer. Reordering one of these would move + // Highlights below Shadows for no reason anybody could see in the + // code, so the stable sort has to be a no-op when nothing is faceted. + let graph = EditGraph::default_chain(); + for cap in graph.capabilities() { + if cap.params.iter().any(|p| p.facet.is_some()) { + continue; + } + let order = presentation_order(&cap.params); + assert_eq!( + order, + (0..cap.params.len()).collect::>(), + "{} was reordered", + cap.id + ); + } + } + + #[test] + fn faceted_parameters_are_stacked_one_run_per_aspect() { + // The panel names a run once and then draws its rows. That only works + // if a run is *contiguous*: the mixer declares band by band — red hue, + // red sat, red lum, orange hue — so shown in declaration order every + // single row would begin a new run, and the panel would draw + // thirty-six headings over thirty-six sliders. + let graph = EditGraph::default_chain(); + let cap = graph + .capabilities() + .into_iter() + .find(|c| c.params.iter().any(|p| p.facet.is_some())) + .expect("the chain has a faceted operation"); + + let mut seen: Vec<&str> = Vec::new(); + let mut previous: Option<&str> = None; + for i in presentation_order(&cap.params) { + let aspect = cap.params[i] + .facet + .as_ref() + .expect("this operation facets every parameter") + .aspect + .0; + if previous != Some(aspect) { + assert!( + !seen.contains(&aspect), + "{aspect} is split into two runs — a heading would be \ + drawn over each half" + ); + seen.push(aspect); + previous = Some(aspect); + } + } + assert!(seen.len() > 1, "the fixture must have several aspects"); + } + + #[test] + fn reordering_rows_does_not_move_where_a_change_is_routed() { + // The rows are stacked for reading; `param_index` still addresses the + // capability list. Were the two confused, dragging a band's Hue would + // silently write to whichever parameter happened to sit at that + // position — an edit landing on the wrong control, which reads as the + // renderer being broken rather than the panel. + let graph = EditGraph::default_chain(); + let cap = graph + .capabilities() + .into_iter() + .find(|c| c.params.iter().any(|p| p.facet.is_some())) + .expect("the chain has a faceted operation"); + + let mut order = presentation_order(&cap.params); + order.sort_unstable(); + assert_eq!( + order, + (0..cap.params.len()).collect::>(), + "the order must be a permutation: every parameter reachable from \ + exactly one row, and every row addressing a parameter that exists" + ); + } + + #[test] + fn every_faceted_parameter_resolves_to_a_band_name() { + // The bug this closes: `labels.rs` had no `param.mixer.*` entries, so + // all thirty-six keys fell through to a derived label that yields the + // bare channel name — twelve rows reading "Hue" with nothing saying + // which band. A row identified only by a swatch depends on this + // resolving, since the name is what a screen reader speaks and what + // anyone who cannot separate two squares by eye has to go on. + let graph = EditGraph::default_chain(); + for cap in graph.capabilities() { + for p in &cap.params { + let Some(facet) = &p.facet else { continue }; + let subject = labels::resolve(facet.subject.0); + let aspect = labels::resolve(facet.aspect.0); + assert!(!subject.is_empty(), "{} has no subject name", p.id); + assert!(!aspect.is_empty(), "{} has no aspect name", p.id); + // Not the channel name repeated: that is exactly the failure + // the catalogue entries were added to fix. + assert_ne!(subject, aspect, "{} is named after its channel", p.id); + } + } + } + #[test] fn unit_suffixes_come_from_the_descriptor() { assert_eq!(unit_suffix(Unit::Stops), " EV"); diff --git a/ui/dr-ui/src/labels.rs b/ui/dr-ui/src/labels.rs index f73a1c8..9a65b2f 100644 --- a/ui/dr-ui/src/labels.rs +++ b/ui/dr-ui/src/labels.rs @@ -20,6 +20,7 @@ pub fn resolve(key: &str) -> String { "op.brilliance" => "Brilliance".into(), "op.vibrance" => "Vibrance".into(), "op.saturation" => "Saturation".into(), + "op.colour_mixer" => "Colour Mixer".into(), "op.framing" => "Crop & Rotate".into(), // Parameters @@ -34,6 +35,37 @@ pub fn resolve(key: &str) -> String { "param.vibrance" => "Vibrance".into(), "param.saturation" => "Saturation".into(), + // What a faceted parameter adjusts — the mixer's three channels. + // + // Spelled out rather than left to `derive`, which would give "Sat" and + // "Lum" from the keys. These name a run of twelve rows apiece, and an + // abbreviation at the head of a section is a word the reader has to + // expand every time they scan past it. + "param.channel.hue" => "Hue".into(), + "param.channel.sat" => "Saturation".into(), + "param.channel.lum" => "Luminance".into(), + + // The hue bands, which a faceted row is *subject* to. + // + // Catalogued even where `derive` would produce the same word, because + // three of them are not the word the key spells: "spring" is spring + // green, and reading "Spring" beside "Green" in a list of colours says + // nothing. These are also the only place a swatch's meaning is written + // down in words, which is what a photographer who cannot separate the + // squares by eye has to go on. + "band.red" => "Red".into(), + "band.orange" => "Orange".into(), + "band.yellow" => "Yellow".into(), + "band.chartreuse" => "Yellow-Green".into(), + "band.green" => "Green".into(), + "band.spring" => "Blue-Green".into(), + "band.cyan" => "Cyan".into(), + "band.azure" => "Azure".into(), + "band.blue" => "Blue".into(), + "band.violet" => "Violet".into(), + "band.magenta" => "Magenta".into(), + "band.rose" => "Rose".into(), + // Framing. "Straighten" rather than "Angle" because that is the task // the control performs; the number it reports is still degrees. "param.angle" => "Straighten".into(), diff --git a/ui/dr-ui/style.yaml b/ui/dr-ui/style.yaml index cc029d0..e52db69 100644 --- a/ui/dr-ui/style.yaml +++ b/ui/dr-ui/style.yaml @@ -161,3 +161,12 @@ lengths: doc: | Floor on button width, so a one-word label is still a comfortable target and a row of buttons has an even rhythm. + + swatch: + value: 12 + note: | + The colour square identifying which hue band a row edits. Small on + purpose: a swatch is data, not chrome — the one sanctioned exception to + an achromatic palette — and it earns that exception by identifying the + row without competing with the photograph beside it. Big enough to name + a hue at a glance, and no bigger. diff --git a/ui/dr-ui/ui/adjust.slint b/ui/dr-ui/ui/adjust.slint index a113eb3..ae1e6be 100644 --- a/ui/dr-ui/ui/adjust.slint +++ b/ui/dr-ui/ui/adjust.slint @@ -8,7 +8,7 @@ // (FR-DEV-3c). import { Theme } from "theme.slint"; -import { PanelHeading, Label, Value, Caption, Button, IconButton } from "widgets.slint"; +import { PanelHeading, Label, Value, Caption, Button, IconButton, Swatch } from "widgets.slint"; // One parameter, flattened for Slint's model system. // @@ -45,6 +45,28 @@ export struct ParamRow { // operation can request through its presentation. kind: string, // "scalar" | "bool" | "curve" + // A run of rows inside a group, for an operation whose parameters form a + // grid rather than a list. + // + // The colour mixer is twelve hue bands times three channels, and as a flat + // list it read as "Hue / Saturation / Luminance" twelve times over with + // nothing saying which band any row belonged to. Rust stacks the rows so + // each channel's twelve are together and marks the first of each run; this + // file names the run and lets the swatch identify the row. + // + // `facet-label` is the run's name and is empty on an ordinary parameter, + // which is every operation but the mixer. + facet-label: string, + starts-facet: bool, + + // Where this row's subject sits on the hue wheel, in degrees, or -1 for a + // row whose subject is not a colour. + // + // A sentinel because a Slint struct cannot carry an optional, and -1 + // rather than any in-range value because 0° is red — a real band, and the + // first one. + swatch-hue: float, + value: float, default-value: float, minimum: float, @@ -288,6 +310,22 @@ component SliderTrack inherits Rectangle { } } +// A parameter's value, at the precision its descriptor declares. +// +// A global rather than the same ternary written into every control that shows +// one. There are two such controls now and the rule belongs to neither of +// them: precision comes from the descriptor, so a control in stops reads 1.25 +// while one in whole units reads 25, and a copy that fell behind would show +// the same parameter two ways in the same panel. `SliderTrack`'s preamble is +// the longer version of this argument. +global Readout { + public pure function of(data: ParamRow) -> string { + return data.precision == 0 + ? Math.round(data.value) + data.unit + : (Math.round(data.value * 100) / 100) + data.unit; + } +} + // One generated parameter: a label, a readout, and the track above. component ParamSlider inherits Rectangle { in property data; @@ -310,11 +348,7 @@ component ParamSlider inherits Rectangle { Rectangle { horizontal-stretch: 1; } Value { - // Precision comes from the descriptor, so a control in stops - // reads 1.25 while one in whole units reads 25. - text: root.data.precision == 0 - ? Math.round(root.data.value) + root.data.unit - : (Math.round(root.data.value * 100) / 100) + root.data.unit; + text: Readout.of(root.data); // The one readout in the panel that has moved off its default // is what the eye is hunting for, and `modified` is the only // thing left to say it with once hue is gone. @@ -337,6 +371,101 @@ component ParamSlider inherits Rectangle { } } +// The name of a run of rows inside a group: the mixer's Hue, Saturation and +// Luminance. +// +// A `Label` rather than a third `PanelHeading`. Two levels of +// caps-with-tracking already stack above it — the panel's own `ADJUST` and the +// operation's name — and a third in the same treatment would read as their +// peer instead of as something *inside* the operation. Sentence case at the +// same size says "a part of the group above" without another size or colour. +component FacetHeading inherits Rectangle { + in property title; + + height: Theme.control-height; + + HorizontalLayout { + padding-left: Theme.gap-sm; + padding-top: Theme.gap-sm; + + Label { text: root.title; } + } +} + +// One faceted parameter: a swatch, a track and a readout, on a single line. +// +// **The swatch is the label.** Twelve of these sit under a heading that +// already names the channel, so the only thing a row has left to say is which +// band it edits — and a 12px square says it in a fraction of the width the +// word would take. That is what makes twelve rows fit where four did: the +// mixer is thirty-six controls, and at `ParamSlider`'s two-line 46px it was +// most of a screen of scrolling with the band name absent from every row of it +// anyway. +// +// **The name is not thrown away, it moves.** It is the row's accessible label, +// so a screen reader says "Orange" where the eye reads the colour, and the +// catalogue in `labels.rs` is where the mapping is written down for anyone who +// cannot separate two squares by eye. A row identified by colour *alone* +// would be a control some photographers could not use, which is why the +// spoken name is part of the design and not an afterthought. +component SwatchSlider inherits Rectangle { + in property data; + callback changed(float); + callback reset(); + /// Forwarded from the track, for the panel's Flickable. + callback drag-changed(bool); + + // The track's own height plus a hairline of air. Denser than a + // `ParamSlider` because the label line it would need is gone, not because + // the touch target shrank — `SliderTrack` still owns a full-width hit area + // and the gestures behind it (FR-UI-3). + height: Theme.touch-target / 2 + 4px; + + accessible-role: slider; + accessible-label: root.data.param-label; + accessible-value: Readout.of(root.data); + accessible-value-minimum: root.data.minimum; + accessible-value-maximum: root.data.maximum; + + HorizontalLayout { + padding-left: Theme.gap-sm; + spacing: Theme.gap-sm; + + Swatch { + hue: root.data.swatch-hue; + // Centred against the track rather than the row, which a layout + // would do for a stretching child and cannot do for a fixed one. + y: (parent.height - self.height) / 2; + } + + SliderTrack { + horizontal-stretch: 1; + + value: root.data.value; + default-value: root.data.default-value; + minimum: root.data.minimum; + maximum: root.data.maximum; + + changed(v) => { root.changed(v); } + reset => { root.reset(); } + engaged-changed(on) => { root.drag-changed(on); } + } + + Value { + text: Readout.of(root.data); + modified: root.data.value != root.data.default-value; + placeholder: root.data.value == root.data.default-value; + compact: true; + // Fixed and right-aligned: a readout sized to its own text would + // pull the track's end left and right as the number changed, and + // twelve tracks that each ended somewhere different would be + // impossible to compare down the column. + width: 30px; + horizontal-alignment: right; + } + } +} + // A slider the interface names itself, rather than one generated from a row. // // The straighten angle is reached through the session's own accessor, not @@ -836,7 +965,34 @@ export component AdjustPanel inherits Rectangle { spacing: 0px; - if entry.kind == "scalar": ParamSlider { + // A group whose parameters form a grid names each + // run once. Rust has already stacked the rows so a + // run is contiguous and marked its first row, for + // the same reason `group-head` exists: this model + // is flat and a `for` cannot nest inside a + // boundary discovered at runtime. + if entry.starts-facet: FacetHeading { + title: entry.facet-label; + } + + // A row whose subject is a colour is identified by + // that colour; every other scalar keeps its name. + // The two differ only in what stands in for the + // label — the track, the gestures and the routing + // are the same underneath. + if entry.kind == "scalar" && entry.swatch-hue >= 0: SwatchSlider { + data: entry; + drag-changed(on) => { root.slider-dragging = on; } + changed(v) => { + root.param-changed( + entry.op-index, entry.param-index, v); + } + reset => { + root.param-reset(entry.op-index, entry.param-index); + } + } + + if entry.kind == "scalar" && entry.swatch-hue < 0: ParamSlider { data: entry; drag-changed(on) => { root.slider-dragging = on; } changed(v) => { diff --git a/ui/dr-ui/ui/widgets.slint b/ui/dr-ui/ui/widgets.slint index b98f58d..c1a0448 100644 --- a/ui/dr-ui/ui/widgets.slint +++ b/ui/dr-ui/ui/widgets.slint @@ -455,6 +455,45 @@ export component Value inherits Text { vertical-alignment: center; } +// The colour a row edits, as a small square: the label for a control whose +// subject is a hue rather than a word. +// +// **The one place hue enters the chrome, and it is not an exception to the +// palette rule so much as outside it.** The rule forbids colour used as +// decoration — an accent on a heading, a tinted border — because a saturated +// patch beside the photograph shifts how the photograph reads. This is the +// same kind of thing as the image itself: data. A row that edits the 30° band +// has to say *which* band, and no achromatic treatment can say "orange". +// +// **The core supplies degrees; everything else is decided here.** The +// operation knows its band is centred at 30° because that is the number it +// weights pixels around (ARCH §4.3a). Saturation and brightness are this +// file's to choose, and they are chosen well short of full: a fully saturated +// row of twelve squares is a paintbox sitting next to a print. Held back far +// enough to read as instrument markings, and no further, since a swatch that +// cannot be told from its neighbour has stopped identifying anything. +export component Swatch inherits Rectangle { + /// Where the subject sits on the hue wheel, in degrees. + in property hue; + /// Muted from a full-strength hue, so twelve of these read as a scale + /// rather than as a palette. + in property saturation: 0.55; + /// Bright enough to separate from the surface it sits on, which is dark; + /// a swatch at full value would be the brightest thing in the panel and + /// outrank the modified marker. + in property brightness: 0.85; + + width: Theme.swatch; + height: Theme.swatch; + horizontal-stretch: 0; + border-radius: Theme.radius-sm; + background: hsv(root.hue, root.saturation, root.brightness); + // A hairline, so a dark swatch (a deep blue at this brightness) still has + // an edge against the surface and reads as a square rather than a smudge. + border-width: 1px; + border-color: Theme.rule; +} + // Supporting text: a hint under a field, a count beside a title, an empty // state's second line. The faintest ink, because it is there for the reader // who stopped to look and should not catch the eye of the one who did not.