diff --git a/core/dr-pipeline/src/descriptor.rs b/core/dr-pipeline/src/descriptor.rs index 0241d56..8640cb1 100644 --- a/core/dr-pipeline/src/descriptor.rs +++ b/core/dr-pipeline/src/descriptor.rs @@ -421,6 +421,27 @@ impl ParamDescriptor { } } + /// A toggle that is **on** when nothing has been chosen. + /// + /// The reset contract is unchanged — a default is still the neutral value + /// and a sidecar still stores only departures from it. What differs is + /// which state is neutral, and that is a fact about the setting rather + /// than about switches: a correction the file itself asked for is on + /// unless the photographer says otherwise, so "off" is the edit and + /// storing it is right. A switch written the other way round would have to + /// be labelled for its negation — "Ignore the lens profile" — and every + /// photograph that simply wanted correcting would carry a stored + /// parameter saying so. + pub const fn switch_on(id: &'static str, label: &'static str) -> Self { + Self { + id: ParamId(id), + label: LocalizedKey(label), + kind: ParamKind::Bool, + default: 1.0, + facet: None, + } + } + /// One of a fixed list of alternatives, defaulting to the first. /// /// The first rather than a caller-chosen index, so the reset contract diff --git a/core/dr-pipeline/src/graph.rs b/core/dr-pipeline/src/graph.rs index 7964dce..2343224 100644 --- a/core/dr-pipeline/src/graph.rs +++ b/core/dr-pipeline/src/graph.rs @@ -148,6 +148,21 @@ pub struct EditGraph { /// derived from the file's EXIF and a database, exactly as the film's /// baked tables are derived from a stock's id. lens_profile: Option, + /// Whether the profile above is being used. + /// + /// **The one part of automatic lens correction that is an edit.** The + /// coefficients are a measurement of the lens and belong to the file; + /// whether to accept them is the photographer's, and it is the answer a + /// tick box in the panel gives. So this rides with the parameters rather + /// than with the profile: it is published as + /// [`crate::lens::profile_switch`], captured by [`Preset`], stored in the + /// sidecar and replayed by the undo stack, all by the one road every other + /// setting travels (FR-DEV-3c). + /// + /// True on a fresh graph, because a profile that was found is a + /// correction the photograph asked for — see + /// [`crate::descriptor::ParamDescriptor::switch_on`]. + lens_profile_applied: bool, } /// TRACES: FR-DEV-3f @@ -203,6 +218,7 @@ impl EditGraph { Box::new(crate::ops::Aberration::new()), ], lens_profile: None, + lens_profile_applied: true, } } @@ -309,6 +325,51 @@ impl EditGraph { /// [`crate::Operation::set_lens_profile`]. pub fn set_lens_profile(&mut self, profile: Option) { self.lens_profile = profile; + self.fan_out_lens_profile(); + } + + /// The profile this photograph was matched to, if any. + /// + /// Reports what was *found*, not what is in effect: a profile the + /// photographer has switched off is still the profile for this lens, and + /// the switch is what says whether it is being used. A caller wanting the + /// coefficients that are actually in the shader asks + /// [`Self::lens_profile_applied`] as well — which is what the interface's + /// lens line does, because "corrected" and "correction available, off" are + /// two different things to tell a photographer. + pub fn lens_profile(&self) -> Option<&LensProfile> { + self.lens_profile.as_ref() + } + + /// TRACES: FR-DEV-3 + /// Whether the matched profile is being applied. + pub fn lens_profile_applied(&self) -> bool { + self.lens_profile_applied + } + + /// TRACES: FR-DEV-3 + /// Use the matched profile, or decline it. + /// + /// Reached through `set_param` by the panel, like every other setting; + /// this is the named door for a caller that has a `bool` rather than a + /// parameter id. + /// + /// Setting this with no profile matched is meaningful and harmless: a + /// preset carrying the switch may land on a photograph whose lens the + /// database has never heard of, and remembering the answer costs nothing. + pub fn set_lens_profile_applied(&mut self, applied: bool) { + self.lens_profile_applied = applied; + self.fan_out_lens_profile(); + } + + /// Hand every correction the coefficients it should be using. + /// + /// `None` where the switch is off, which is the same call opening an + /// unrecognised lens makes — the corrections cannot tell the difference + /// between "no profile" and "not this one, thank you", and have no reason + /// to. + fn fan_out_lens_profile(&mut self) { + let profile = self.lens_profile.filter(|_| self.lens_profile_applied); for warp in &mut self.warps { warp.set_profile(profile.as_ref()); } @@ -317,11 +378,6 @@ impl EditGraph { } } - /// The profile currently applied, if any. - pub fn lens_profile(&self) -> Option<&LensProfile> { - self.lens_profile.as_ref() - } - /// Descriptors for the coordinate-domain lens corrections, in order. /// /// The counterpart to [`Self::descriptors`] and split from it for the same @@ -433,7 +489,46 @@ impl EditGraph { attributes: desc.attributes.clone(), }; - warps.chain(ops).chain(std::iter::once(framing)).collect() + // The lens profile switch, ahead of the corrections it drives — and + // **only when a profile was matched**. A tick box on a photograph + // whose lens the database has never heard of would be a control that + // does nothing, which is the failure `dr_lens` names: an automatic + // correction is allowed to be unavailable and is not allowed to look + // available and be inert. Where there is no profile the interface says + // so in words instead. + let switch = self.lens_profile.map(|_| { + let desc = crate::lens::profile_switch::descriptor(); + OpCapability { + id: desc.id, + label: desc.label, + // A profile that is on is doing something to this photograph, + // which is what the panel's modified marker is for. Off is the + // departure from default, and reads as one either way. + active: self.lens_profile_applied, + params: desc + .params + .iter() + .map(|p| ParamCapability { + id: p.id, + label: p.label, + kind: p.kind.clone(), + default: p.default, + value: if self.lens_profile_applied { 1.0 } else { 0.0 }, + facet: p.facet, + }) + .collect(), + // An ordinary switch. Nothing about it wants the canvas. + presentation: None, + attributes: desc.attributes.clone(), + } + }); + + switch + .into_iter() + .chain(warps) + .chain(ops) + .chain(std::iter::once(framing)) + .collect() } /// TRACES: FR-DEV-3a @@ -517,6 +612,11 @@ impl EditGraph { // rather than restored — the same reason the film's tables travel // as an id and not as numbers. lens_profile: _, + // Whether that profile is *used* is an edit, and it is in the + // state below: it reaches `Preset::capture` through + // `capabilities`, with the operations and the warps and for the + // same reason (FR-DEV-3c). + lens_profile_applied: _, masks, film, spots, @@ -582,6 +682,18 @@ impl EditGraph { } pub fn set_param(&mut self, op: OpId, param: ParamId, value: f32) { + if op == crate::lens::profile_switch::ID { + if param != crate::lens::profile_switch::APPLY { + log::warn!("unknown parameter {param} on {op}; ignoring"); + return; + } + // Accepted whether or not a profile was matched, for the reason + // `set_lens_profile_applied` gives: a preset may carry the answer + // to a photograph that has nothing to apply it to. + self.set_lens_profile_applied(value != 0.0); + return; + } + if op == crate::framing::ID { // Bound rather than chained: `descriptor()` hands back an owned // `Arc` now, so a `param()` borrowed straight out of the call @@ -627,6 +739,10 @@ impl EditGraph { /// Read a parameter back. pub fn param(&self, op: OpId, param: ParamId) -> Option { + if op == crate::lens::profile_switch::ID { + return (param == crate::lens::profile_switch::APPLY) + .then_some(if self.lens_profile_applied { 1.0 } else { 0.0 }); + } if op == crate::framing::ID { return self .framing @@ -665,6 +781,10 @@ impl EditGraph { // stock, and turning a name into tables needs the profile database, // which this crate deliberately does not link (ARCH §6.5a). self.set_film(None); + // The *matched profile* stays — it is the file's, not the edit's, and + // a reset does not change which lens took the photograph. What returns + // to default is the answer to whether to use it, which is on. + self.set_lens_profile_applied(true); } /// Set the crop rectangle. Clamped to keep it inside the frame. @@ -1114,6 +1234,171 @@ mod tests { assert!(g.lens_profile().is_none()); } + /// A measured profile, for the switch's tests. Coefficients are one real + /// wide-angle's, rounded — what matters is that each of the three + /// corrections gets something to do. + fn measured() -> crate::lens::LensProfile { + use crate::lens::{LensProfile, Tca}; + use crate::ops::{distortion, vignetting}; + + LensProfile { + distortion: Some(distortion::PtLens { + a: 0.0, + b: -0.012, + c: 0.0, + }), + tca: Some(Tca { + red_scale: 1.000_32, + blue_scale: 0.999_93, + }), + vignetting: Some(vignetting::Pa { + k1: -0.42, + k2: 0.05, + k3: 0.0, + }), + } + } + + /// TRACES: FR-DEV-3 + /// The switch takes the whole profile out of the shader, and puts it back. + /// + /// The control the panel draws is this call, and what it has to mean is + /// "develop this photograph as if the database had never heard of the + /// lens" — all three corrections, not the geometry alone. + #[test] + fn declining_the_profile_removes_every_correction_it_was_driving() { + use crate::lens::profile_switch; + + let mut g = EditGraph::default_chain(); + g.set_lens_profile(Some(measured())); + assert!(g.compose().source.contains("---- warp: distortion ----")); + + g.set_param(profile_switch::ID, profile_switch::APPLY, 0.0); + let declined = g.compose().source; + assert!(!declined.contains("---- warp: "), "{declined}"); + assert!(!declined.contains("---- vignetting ----")); + + // The profile itself is untouched. It is a fact about the file, and + // the interface still has to be able to say which lens this was. + assert!( + g.lens_profile().is_some(), + "declining a profile must not forget it, or the switch could \ + never be turned back on" + ); + + g.set_param(profile_switch::ID, profile_switch::APPLY, 1.0); + assert!(g.compose().source.contains("---- warp: distortion ----")); + } + + /// TRACES: FR-DEV-3 + /// The manual trims keep working with the profile declined. + /// + /// The two are independent by construction — each correction composes the + /// profile with its own slider — and that is what makes the switch safe to + /// offer: turning it off is "correct this by hand", not "stop correcting". + #[test] + fn declining_the_profile_leaves_the_manual_corrections_alone() { + use crate::lens::profile_switch; + use crate::ops::distortion; + + let mut g = EditGraph::default_chain(); + g.set_lens_profile(Some(measured())); + g.set_param(distortion::ID, distortion::AMOUNT, 40.0); + g.set_param(profile_switch::ID, profile_switch::APPLY, 0.0); + + assert_eq!(g.param(distortion::ID, distortion::AMOUNT), Some(40.0)); + assert!( + g.compose().source.contains("---- warp: distortion ----"), + "the slider still bends the frame with the profile switched off" + ); + } + + /// TRACES: FR-DEV-3 + /// The switch is only offered where there is a profile to switch. + /// + /// `dr_lens`'s rule, as a property of the capability list: a tick box on a + /// photograph whose lens the database has never heard of would be a + /// control that looks available and does nothing, which is the failure the + /// automatic correction is supposed to avoid rather than an instance of + /// it. + #[test] + fn the_switch_is_absent_until_a_profile_is_matched() { + use crate::lens::profile_switch; + + let mut g = EditGraph::default_chain(); + assert!( + !g.capabilities().iter().any(|c| c.id == profile_switch::ID), + "an unmatched lens must not grow a tick box" + ); + + g.set_lens_profile(Some(measured())); + let cap = g + .capabilities() + .into_iter() + .find(|c| c.id == profile_switch::ID) + .expect("a matched profile is offered as a control"); + assert_eq!(cap.params.len(), 1); + assert_eq!(cap.params[0].kind, ParamKind::Bool); + // On, and on is the default — so a photograph nobody has touched + // carries nothing in its sidecar and is still corrected. + assert_eq!(cap.params[0].value, 1.0); + assert_eq!(cap.params[0].default, 1.0); + assert!(!cap.params[0].is_modified()); + + // Ahead of the corrections it drives, so the panel reads top down: + // the profile, then what to add to it by hand. + let ids: Vec<&str> = g.capabilities().iter().map(|c| c.id.0).collect(); + assert_eq!(ids.first(), Some(&profile_switch::ID.0)); + } + + /// TRACES: FR-DEV-3 | FR-DEV-5 + /// Declining the profile is an edit, so it travels like one. + /// + /// The reason it is a parameter at all: capture and apply are the road the + /// sidecar, the clipboard and the undo stack all take, and a `bool` on the + /// side would have had to be added to each of them by hand. + #[test] + fn the_switch_survives_a_state_round_trip() { + use crate::lens::profile_switch; + + let mut g = EditGraph::default_chain(); + g.set_lens_profile(Some(measured())); + g.set_param(profile_switch::ID, profile_switch::APPLY, 0.0); + + let state = g.state(); + + // Reopened: the profile is looked up again from the file, and the + // stored edit says what to do with it. + let mut reopened = EditGraph::default_chain(); + reopened.set_lens_profile(Some(measured())); + assert_eq!(reopened.set_state(&state), FilmRebake::NotNeeded); + + assert_eq!( + reopened.param(profile_switch::ID, profile_switch::APPLY), + Some(0.0), + "a declined profile came back applied, so reopening the \ + photograph would silently correct it again" + ); + assert!(!reopened.compose().source.contains("---- warp: ")); + } + + /// TRACES: FR-DEV-3 + /// A reset accepts the profile again, and keeps it. + #[test] + fn resetting_returns_to_the_measured_profile() { + use crate::lens::profile_switch; + + let mut g = EditGraph::default_chain(); + g.set_lens_profile(Some(measured())); + g.set_param(profile_switch::ID, profile_switch::APPLY, 0.0); + + g.reset(); + + assert!(g.lens_profile().is_some(), "the file still names a lens"); + assert!(g.lens_profile_applied()); + assert!(g.compose().source.contains("---- warp: distortion ----")); + } + /// A warp is an edit, so `reset` has to reach it. It did not until the /// loop was added: a reset that left the lens corrections standing would /// mean "back to the file as it is" quietly did not mean that. diff --git a/core/dr-pipeline/src/lens.rs b/core/dr-pipeline/src/lens.rs index 0c468e7..a1bead2 100644 --- a/core/dr-pipeline/src/lens.rs +++ b/core/dr-pipeline/src/lens.rs @@ -59,9 +59,9 @@ //! wearing the same lens, which defeats the point of a lens profile. use std::fmt::Write as _; -use std::sync::Arc; +use std::sync::{Arc, LazyLock}; -use crate::descriptor::{OpDescriptor, ParamId}; +use crate::descriptor::{Attribute, LocalizedKey, OpDescriptor, OpId, ParamDescriptor, ParamId}; use crate::operation::{Helper, Uniform}; /// Lateral chromatic aberration, as a per-channel radial scale. @@ -90,6 +90,64 @@ pub struct LensProfile { pub vignetting: Option, } +/// TRACES: FR-DEV-3 +/// The lens profile itself, as one switch a photographer can reach. +/// +/// **Not an operation and not a warp** — it corrects nothing on its own. It +/// is the answer to "use the measured profile for this lens, or not", and the +/// three corrections that read the coefficients ([`crate::ops::Distortion`], +/// [`crate::ops::Aberration`] and the vignetting node) are where the +/// correction actually happens. Framing is in the capability list on the same +/// terms: something a photographer sets which is not an `Operation`. +/// +/// # Why this exists at all +/// +/// The profile arrives from the file's EXIF and a database, and applying it +/// silently was the whole of the interface for it. That reads as the feature +/// being absent: the corrections in the panel are manual sliders, the +/// automatic part is a line of grey text under the camera, and nothing +/// anywhere says "this is on and you may turn it off". `dr_lens`'s honesty +/// rule — an automatic correction the user cannot see is worse than one they +/// can see is unavailable — asks for a control here, not just a caption. +/// +/// # Why it is a parameter rather than a flag on the session +/// +/// Everything a photographer sets travels by one road: the capability list +/// feeds the generated panel, [`crate::Preset`] captures it, the sidecar +/// stores it and the undo stack replays it (FR-DEV-3c). A `bool` on the +/// session would have needed its own place in each of those four, and would +/// have been forgotten in at least one — which is exactly how the mask stack +/// came to be missing from the history. As a parameter it is in all of them +/// with nothing registered. +pub mod profile_switch { + use super::*; + + pub const ID: OpId = OpId("lens_profile"); + pub const APPLY: ParamId = ParamId("apply"); + + /// On by default: the profile is a measurement of the lens that took the + /// photograph, so applying it is the neutral state and declining it is + /// the edit. See [`ParamDescriptor::switch_on`]. + pub(crate) static DESCRIPTOR: LazyLock> = LazyLock::new(|| { + Arc::new(OpDescriptor { + id: ID, + label: LocalizedKey("op.lens_profile"), + params: vec![ParamDescriptor::switch_on( + "apply", + "param.lens_profile.apply", + )], + // Optics, so it sits with the corrections it drives rather than in + // a group of its own. + attributes: vec![Attribute::Optics], + }) + }); + + /// This switch's description, for a caller building a control from it. + pub fn descriptor() -> Arc { + DESCRIPTOR.clone() + } +} + /// A coordinate-domain operation, applied before the source is sampled. /// /// Object-safe for the same reason [`crate::operation::Operation`] is: the diff --git a/core/dr-pipeline/src/preset.rs b/core/dr-pipeline/src/preset.rs index 2b78e28..7a73d91 100644 --- a/core/dr-pipeline/src/preset.rs +++ b/core/dr-pipeline/src/preset.rs @@ -198,7 +198,16 @@ impl Scope { fn attributes_of(op: &str) -> Option<&'static [Attribute]> { static TABLE: std::sync::LazyLock>> = std::sync::LazyLock::new(|| { - EditGraph::default_chain() + let mut chain = EditGraph::default_chain(); + // With a profile in place, because one capability only exists once + // a photograph has brought a lens profile with it — the switch + // that accepts or declines it. Table entries are what + // [`Scope::covers`] classifies by, and an id missing from here + // falls through to `carries_unclassified`: the switch would then + // travel with a scope that named only tone, which is precisely + // what a scope is for refusing. + chain.set_lens_profile(Some(crate::lens::LensProfile::default())); + chain .capabilities() .into_iter() .map(|cap| (cap.id.0.to_string(), cap.attributes)) @@ -739,6 +748,35 @@ mod tests { assert!(Preset::capture(&EditGraph::default_chain()).is_empty()); } + /// TRACES: FR-DEV-3 | FR-DEV-3c + /// The lens profile switch is classified, so a scope can refuse it. + /// + /// It is the one capability that exists only once a photograph has brought + /// a lens profile with it, and the attribute table is built from a chain + /// — so it was missing from that table before the chain used to build it + /// was given a profile. An unclassified id falls through to + /// `carries_unclassified`, and a copy of somebody's tone edit would have + /// arrived carrying "do not correct this lens". + #[test] + fn declining_a_lens_profile_is_an_optical_edit() { + use crate::lens::profile_switch; + + let mut g = EditGraph::default_chain(); + g.set_lens_profile(Some(crate::lens::LensProfile::default())); + g.set_param(profile_switch::ID, profile_switch::APPLY, 0.0); + + let preset = Preset::capture(&g); + assert!( + preset + .params() + .contains_key(&(profile_switch::ID.0.into(), profile_switch::APPLY.0.into())), + "declining the profile is an edit and must be copyable" + ); + + assert!(Scope::of([Attribute::Optics]).covers(profile_switch::ID.0)); + assert!(!Scope::of([Attribute::Tone]).covers(profile_switch::ID.0)); + } + #[test] fn a_copy_captures_framing_even_though_the_default_scope_drops_it() { // Capture is deliberately unfiltered: the decision about framing is diff --git a/docs/requirements.md b/docs/requirements.md index 6b730a7..1719c64 100644 --- a/docs/requirements.md +++ b/docs/requirements.md @@ -276,7 +276,13 @@ once, at the final export or display stage. - Vibrance and saturation - Texture / clarity - Sharpening and noise reduction (luminance and chroma) -- Lens corrections: distortion, chromatic aberration, vignetting +- Lens corrections: distortion, chromatic aberration, vignetting — driven by the lens profile the + file's own EXIF matches in the bundled Lensfun database where there is one, and by hand where + there is not. The profile is a control rather than a silent step: a photograph whose lens is + recognised carries one switch that accepts or declines the measurement, and the manual sliders + trim whatever it leaves. A photograph whose lens is unrecognised is told so in words and offered + no switch, because a correction that looks available and does nothing is worse than one that is + visibly unavailable - Crop, straighten, rotate, flip - Local adjustments: linear gradient, radial gradient, and brush masks diff --git a/ui/dr-ui/src/develop.rs b/ui/dr-ui/src/develop.rs index a06dbbb..f55b369 100644 --- a/ui/dr-ui/src/develop.rs +++ b/ui/dr-ui/src/develop.rs @@ -3978,10 +3978,14 @@ impl DevelopSession { // would send someone hunting for a profile that was never missing. return "Lens not recorded".into(); }; - if self.lens_profile_found { - format!("{lens} · corrected") - } else { - format!("{lens} · no profile") + match (self.lens_profile_found, self.graph.lens_profile_applied()) { + (true, true) => format!("{lens} · corrected"), + // A profile exists and is switched off, which is neither of the + // other two answers: the photographer turned it off, and a line + // reading "no profile" would send them looking for one that is + // sitting right there in the panel with its box unticked. + (true, false) => format!("{lens} · profile off"), + (false, _) => format!("{lens} · no profile"), } } @@ -5545,6 +5549,41 @@ mod tests { ); } + /// TRACES: FR-DEV-3 + /// A profile switched off is a third answer, and has to read as one. + /// + /// "No profile" sends a photographer looking for a lens the database does + /// not have. If the profile is sitting in the panel with its box unticked, + /// that is a different sentence, and the line has to say which. + /// + /// Depends on the bundled database holding a common lens, exactly as + /// `dr_lens`'s own tests do — this is the only path that sets + /// `lens_profile_found`, and standing a profile up by hand would test the + /// formatting while skipping the lookup it is reporting on. + #[test] + fn the_lens_line_separates_a_declined_profile_from_a_missing_one() { + let Some(ctx) = headless() else { return }; + let rgba: Vec = (0..8 * 8).flat_map(|_| [128u8, 128, 128, 255]).collect(); + let mut session = + DevelopSession::open_rgb(&ctx, &rgba, 8, 8, dr_types::Orientation::NORMAL) + .expect("session"); + + const LENS: &str = "Canon EF 16-35mm f/2.8L USM"; + session.set_source_metadata(dr_decode::Metadata { + lens: Some(LENS.into()), + focal_length: Some(20.0), + aperture: Some(2.8), + ..Default::default() + }); + assert_eq!(session.lens_summary(), format!("{LENS} · corrected")); + + session.graph.set_lens_profile_applied(false); + assert_eq!(session.lens_summary(), format!("{LENS} · profile off")); + + session.graph.set_lens_profile_applied(true); + assert_eq!(session.lens_summary(), format!("{LENS} · corrected")); + } + /// Opening a second photograph must not correct it for the first one's lens. /// /// The clearing case, and the reason `apply_lens_profile` runs on every @@ -6873,6 +6912,57 @@ mod tests { /// A widget hint only collapses an operation to one row when it is /// *honoured*; `rows` falls back to sliders otherwise, and mirroring that /// here is what keeps the test honest when a hint stops applying. + /// TRACES: FR-DEV-3a + /// A declared switch reaches the panel as a switch. + /// + /// `ParamKind::Bool` was in the core's closed enum, was mapped to the row + /// kind `"bool"` here, and had no control behind it in `adjust.slint` — + /// which meant a parameter declaring itself a switch was flattened into a + /// row that drew nothing at all. Nothing shipped had one until the lens + /// profile did, so the gap cost nothing and was invisible. + /// + /// This asserts the Rust half; the Slint half is the `if kind == "bool"` + /// branch in the control registry, which this cannot reach. + #[test] + fn a_switch_becomes_a_switch_row() { + use dr_pipeline::{LocalizedKey, ParamCapability}; + + let switched = OpCapability { + id: OpId("invented_switch"), + label: LocalizedKey("op.invented_switch"), + active: true, + presentation: None, + params: vec![ParamCapability { + id: ParamId("engaged"), + label: LocalizedKey("param.invented_switch.engaged"), + kind: ParamKind::Bool, + // On by default, which is the shape a correction the file + // itself asked for takes: off is the edit. + default: 1.0, + value: 0.0, + facet: None, + }], + attributes: vec![dr_pipeline::Attribute::Optics], + }; + + let rows = rows_from(&[switched]); + assert_eq!(rows.len(), 1); + assert_eq!(rows[0].kind, "bool"); + assert_eq!(rows[0].value, 0.0); + assert_eq!(rows[0].default_value, 1.0); + // A lone parameter is titled by its operation, so the box carries the + // name the withheld heading would have. + assert_eq!( + rows[0].param_label, + labels::resolve("op.invented_switch").as_str() + ); + assert!( + rows[0].group_modified, + "a switch turned off differs from its default like any other \ + parameter, and the group's marker has to say so" + ); + } + /// TRACES: FR-DEV-3c /// An operation this file has never heard of, appearing in the panel. /// diff --git a/ui/dr-ui/src/labels.rs b/ui/dr-ui/src/labels.rs index 3d567fd..9505e3a 100644 --- a/ui/dr-ui/src/labels.rs +++ b/ui/dr-ui/src/labels.rs @@ -175,8 +175,17 @@ fn catalogued(key: &str) -> Option<&'static str> { // it sit sliders called "Red" and "Blue", which only make sense once // the heading has said what is being separated. "op.aberration" => "Chromatic Aberration", + // The switch that accepts or declines the measured profile for the + // lens the file names. "Lens Profile" rather than "Apply Lens Profile": + // it is a tick box, and a checkbox labelled with a verb reads as a + // button that does something once rather than a state that is on. + "op.lens_profile" => "Lens Profile", // Parameters + // Named for what it does rather than what it is, since a lone + // parameter is titled by its operation and this one never reaches the + // panel under its own name — see `rows_filtered`. + "param.lens_profile.apply" => "Apply", "param.temperature" => "Temperature", "param.tint" => "Tint", "param.exposure" => "Exposure", diff --git a/ui/dr-ui/ui/adjust.slint b/ui/dr-ui/ui/adjust.slint index a29aaee..734cc19 100644 --- a/ui/dr-ui/ui/adjust.slint +++ b/ui/dr-ui/ui/adjust.slint @@ -9,7 +9,7 @@ import { Theme } from "theme.slint"; import { PanelHeading, Label, Value, Caption, Button, IconButton, Swatch } from "widgets.slint"; -import { SliderTrack, ControlRow, CurveEditor, Segmented, ChoiceChip, ChipGrid } from "controls.slint"; +import { SliderTrack, ControlRow, CurveEditor, Segmented, ChoiceChip, ChipGrid, Check } from "controls.slint"; // One parameter, flattened for Slint's model system. // @@ -529,6 +529,27 @@ component ParamControl inherits Rectangle { } } + // A setting that is either on or off. The value is 1 or 0, so a tick + // is an ordinary parameter change like a slider's — the row carries a + // `float` either way and the core clamps it back to a boolean. + // + // No reset affordance, and none is missing: a switch has two states + // and its default is one of them, so returning to the default is a + // click on the box. The `modified` marker still comes from the group, + // which is where the panel says something has been changed. + if root.data.kind == "bool": Check { + label: root.data.param-label; + checked: root.data.value != 0; + // The graph owns the value — an undo step and a pasted preset both + // move it without anyone touching this box — so the row draws what + // the model says and never decides for itself. + controlled: true; + toggled(on) => { + root.param-changed( + root.data.op-index, root.data.param-index, on ? 1 : 0); + } + } + // A fixed list of alternatives. The value *is* the index, so picking // one is an ordinary parameter change and needs no separate route. // diff --git a/ui/dr-ui/ui/controls.slint b/ui/dr-ui/ui/controls.slint index 7182b81..11517d7 100644 --- a/ui/dr-ui/ui/controls.slint +++ b/ui/dr-ui/ui/controls.slint @@ -432,9 +432,35 @@ export component Check inherits Rectangle { /// switches makes the user guess at what each one costs. in property hint; in-out property checked; + /// Whether the tick is decided here or by whoever supplied `checked`. + /// + /// A settings page and a generated panel want opposite things, and the + /// difference is not cosmetic. A page owns its switches: ticking one is + /// the whole event, and the box flipping under the finger is the feedback. + /// A panel row is a *view of a model* — the value lives in the edit graph, + /// the same graph an undo step or a pasted preset can move — so a box that + /// set its own state would answer the click with a binding replaced by a + /// literal, and the next time the value changed from anywhere else the + /// tick would stay where the finger left it. + /// + /// Controlled, the click is reported and nothing else happens; the tick + /// follows `checked`, which is what it was always drawing. + in property controlled: false; callback toggled(bool); + // One toggle, called from the pointer and from the accessibility action + // alike, so the two cannot drift — the keyboard route had already been + // written twice. + function toggle() { + if (root.controlled) { + root.toggled(!root.checked); + } else { + root.checked = !root.checked; + root.toggled(root.checked); + } + } + // The hint becomes the description rather than part of the name. It exists // to say what a setting *costs* — "location is stripped", "upscaling is // off" — which is the second thing a reader wants and never the first, and @@ -445,10 +471,7 @@ export component Check inherits Rectangle { accessible-description: root.hint; accessible-checkable: true; accessible-checked: root.checked; - accessible-action-default => { - root.checked = !root.checked; - root.toggled(root.checked); - } + accessible-action-default => { root.toggle(); } height: max(row.preferred-height, Theme.control-height); @@ -458,10 +481,7 @@ export component Check inherits Rectangle { height: max(parent.height, Theme.touch-target); y: (parent.height - self.height) / 2; mouse-cursor: pointer; - clicked => { - root.checked = !root.checked; - root.toggled(root.checked); - } + clicked => { root.toggle(); } } row := HorizontalLayout {