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.