diff --git a/core/dr-pipeline/tests/mask_sidecar.rs b/core/dr-pipeline/tests/mask_sidecar.rs index d3af680..fcc02cd 100644 --- a/core/dr-pipeline/tests/mask_sidecar.rs +++ b/core/dr-pipeline/tests/mask_sidecar.rs @@ -383,3 +383,56 @@ fn an_untouched_layer_is_left_alone() { assert!(ours.merge(&theirs, Some(&base)).is_empty()); assert_eq!(ours.masks.len(), 2, "our new layer is not a conflict"); } + +/// Print a real sidecar carrying a subject mask, for eyeballing the format. +/// +/// Ignored because it asserts nothing — it exists so the on-disk shape can be +/// looked at without reverse-engineering it from the writer. +/// +/// ```sh +/// cargo test -p dr-pipeline --test mask_sidecar show_a_sidecar -- --ignored --nocapture +/// ``` +#[test] +#[ignore = "prints the format rather than checking it"] +fn show_a_sidecar() { + let mut graph = EditGraph::default_chain(); + graph.set_param(dr_pipeline::OpId("exposure"), ParamId("exposure"), 0.35); + + let mut subject = MaskLayer::new( + "m1", + MaskSource::Subject { + signature: 0x9f2c_41aa, + index: 0, + class: "person".into(), + score: 0.94, + }, + ); + subject.name = "person".into(); + subject.invert = true; + subject.feather = 0.02; + subject.falloff = dr_pipeline::mask::Falloff::Gaussian; + subject.morphology = dr_pipeline::mask::Morphology::Dilate; + subject.morph_radius = 0.012; + subject.set_param("saturation", ParamId("saturation"), -100.0); + subject.set_param("exposure", ParamId("exposure"), -0.4); + graph.masks_mut().push(subject); + + let mut grad = MaskLayer::new( + "m2", + MaskSource::Linear { + centre: (0.5, 0.25), + angle: 1.5708, + width: 0.4, + }, + ); + grad.set_param("exposure", ParamId("exposure"), -0.6); + graph.masks_mut().push(grad); + + let mut sidecar = Sidecar::new(); + let mut v = Version::from_graph("default", "Default", &graph); + v.is_default = true; + v.rating = 4; + sidecar.put(v); + + println!("\n{}", sidecar.to_text()); +} diff --git a/ui/dr-ui/src/develop.rs b/ui/dr-ui/src/develop.rs index f55b958..39ada07 100644 --- a/ui/dr-ui/src/develop.rs +++ b/ui/dr-ui/src/develop.rs @@ -86,6 +86,12 @@ pub struct DevelopSession { active_mask: Option, /// Whether to draw the false-coloured region overlay. show_overlay: bool, + /// Which attribute the panel is filtered to, or all of them. + /// + /// `None` is "show everything" and is what a frontend that ignores + /// attributes leaves it at — the tabs are the interface's idea, not the + /// core's, and nothing breaks without them (ARCH §4.3a). + active_tab: Option, } impl DevelopSession { @@ -150,6 +156,7 @@ impl DevelopSession { subject_key: 0, active_mask: None, show_overlay: false, + active_tab: None, } } @@ -158,16 +165,68 @@ impl DevelopSession { /// Built entirely from the capability list. The `kind` string chooses the /// widget; nothing switches on a parameter's identity. pub fn rows(&self) -> Vec { - match self.active_layer() { - // A selected layer takes over the panel. The rows are built from - // the layer's own capability list, so every control the global - // chain offers is offered here too — including ones added later, - // which need no work to become local. - Some(layer) => rows_from(&layer.capabilities()), - None => rows_from(&self.graph.capabilities()), + let caps = self.scoped_capabilities(); + match self.active_tab { + Some(attribute) => rows_filtered(&caps, |op| op.attributes.contains(&attribute)), + None => rows_from(&caps), } } + /// The capability list the panel is currently describing. + /// + /// A selected mask layer takes over the panel, so every control the global + /// chain offers is offered on a layer too — including operations added + /// later, which need no work to become local. + fn scoped_capabilities(&self) -> Vec { + match self.active_layer() { + Some(layer) => layer.capabilities(), + None => self.graph.capabilities(), + } + } + + /// The attributes worth offering as tabs, in declaration order. + /// + /// **Derived from the chain, never listed here.** The groups are whatever + /// the operations say they are about, so a new operation joins the right + /// tab by declaring its nature and this file goes on naming none of them + /// (FR-DEV-3a). An attribute nothing carries is left out rather than + /// offered as a tab that opens onto nothing. + /// + /// Geometry is excluded: its one operation prefers an on-canvas widget and + /// is skipped by the row builder, so a Geometry tab would be empty of rows + /// while `GeometryPanel` holds the real controls. + pub fn tabs(&self) -> Vec<(dr_pipeline::Attribute, String)> { + use dr_pipeline::Attribute; + let caps = self.scoped_capabilities(); + Attribute::ALL + .into_iter() + .filter(|a| *a != Attribute::Geometry) + .filter(|a| { + caps.iter() + .any(|c| c.attributes.contains(a) && !rows_filtered(&caps, |o| o.attributes.contains(a)).is_empty()) + }) + .map(|a| (a, crate::labels::resolve(a.label().0))) + .collect() + } + + /// Which tab is selected, as an index into [`Self::tabs`]. `-1` is "all". + pub fn active_tab(&self) -> i32 { + let Some(active) = self.active_tab else { + return -1; + }; + self.tabs() + .iter() + .position(|(a, _)| *a == active) + .map_or(-1, |i| i as i32) + } + + /// Select a tab by its index in [`Self::tabs`], or `-1` for all. + pub fn set_active_tab(&mut self, index: i32) { + self.active_tab = usize::try_from(index) + .ok() + .and_then(|i| self.tabs().get(i).map(|(a, _)| *a)); + } + fn active_layer(&self) -> Option<&MaskLayer> { let id = self.active_mask.as_ref()?; self.graph.masks().get(id) @@ -254,8 +313,28 @@ pub(crate) fn supported(widget: WidgetKind) -> bool { /// heard of appearing in a generated panel — and it cannot be asserted at all /// if generating a row requires a device. pub(crate) fn rows_from(caps: &[OpCapability]) -> Vec { + rows_filtered(caps, |_| true) +} + +/// The panel model for the capabilities `keep` accepts. +/// +/// **`op_index` counts over every capability, not over the kept ones.** It is +/// how a row routes back to the core, so filtering must not renumber it — a +/// row that survived a filter has to still name the operation it came from. +/// `group_head` is the opposite: a position within the *emitted* rows, because +/// the panel walks back to it through the model it was given. +/// +/// Getting that backwards is how a slider ends up driving a different +/// operation, which is the kind of fault that looks like a rendering bug. +pub(crate) fn rows_filtered( + caps: &[OpCapability], + keep: impl Fn(&OpCapability) -> bool, +) -> Vec { let mut rows = Vec::new(); for (op_index, op) in caps.iter().enumerate() { + if !keep(op) { + continue; + } // Where this operation's rows begin. The panel groups by walking // back to it, so it has to be taken before any row is pushed. let group_head = rows.len(); @@ -533,10 +612,7 @@ impl DevelopSession { /// here is curve-shaped; it walks whatever parameters the operation /// declares. pub fn reset_op(&mut self, op_index: i32) { - let caps = match self.active_layer() { - Some(layer) => layer.capabilities(), - None => self.graph.capabilities(), - }; + let caps = self.scoped_capabilities(); let Some(cap) = usize::try_from(op_index).ok().and_then(|i| caps.get(i)) else { return; }; @@ -1182,10 +1258,10 @@ impl DevelopSession { // last described — the indices the interface is holding are positions // in *that* list, and reading the global chain while a layer is // selected would map a slider onto a different operation. - let caps = match self.active_layer() { - Some(layer) => layer.capabilities(), - None => self.graph.capabilities(), - }; + // The *unfiltered* scoped list, because `op_index` counts over every + // capability — see `rows_filtered`. Indexing a filtered list here is + // how a slider would drive the wrong operation once a tab is chosen. + let caps = self.scoped_capabilities(); let op = caps.get(usize::try_from(op_index).ok()?)?; let param = op.params.get(usize::try_from(param_index).ok()?)?; Some((op.id, param.id)) @@ -3055,6 +3131,110 @@ mod tests { } } } + + // ---------------------------------------------------------------------- + // Group tabs (ARCH §4.3a, FR-DEV-3a) + // ---------------------------------------------------------------------- + + fn tabbed_session(ctx: &GpuContext) -> DevelopSession { + let rgba: Vec = (0..16 * 16).flat_map(|_| [128, 128, 128, 255]).collect(); + DevelopSession::open_rgb(ctx, &rgba, 16, 16, dr_types::Orientation::NORMAL) + .expect("session") + } + + #[test] + fn the_tabs_come_from_the_chain_not_from_a_list() { + let Some(ctx) = headless() else { return }; + let session = tabbed_session(&ctx); + + let names: Vec = session.tabs().into_iter().map(|(_, n)| n).collect(); + assert!(names.contains(&"Light".to_string()), "got {names:?}"); + assert!(names.contains(&"Colour".to_string()), "got {names:?}"); + // Nothing offers a group with no rows behind it. + for (attribute, name) in session.tabs() { + let mut probe = tabbed_session(&ctx); + let index = probe + .tabs() + .iter() + .position(|(a, _)| *a == attribute) + .expect("just listed"); + probe.set_active_tab(index as i32); + assert!(!probe.rows().is_empty(), "tab {name} opens onto nothing"); + } + } + + #[test] + fn choosing_a_tab_narrows_the_panel() { + let Some(ctx) = headless() else { return }; + let mut session = tabbed_session(&ctx); + + let all = session.rows().len(); + session.set_active_tab(0); + let narrowed = session.rows().len(); + + assert!(narrowed > 0, "a tab must show something"); + assert!(narrowed < all, "and less than everything: {narrowed} of {all}"); + } + + /// The trap: `op_index` counts over *every* capability, so a row that + /// survived a filter must still route to the operation it came from. If it + /// renumbered, a slider would drive a different operation once a tab was + /// chosen. + #[test] + fn a_filtered_row_still_drives_its_own_operation() { + let Some(ctx) = headless() else { return }; + let mut session = tabbed_session(&ctx); + + // Find a colour row while unfiltered, and remember where it points. + let colour = session + .tabs() + .iter() + .position(|(_, n)| n == "Colour") + .expect("the chain has colour operations"); + session.set_active_tab(colour as i32); + + let row = session.rows().into_iter().next().expect("a row"); + let (op, param) = (row.op_index, row.param_index); + + session.set_param(op, param, 0.5); + let after = session + .rows() + .into_iter() + .find(|r| r.op_index == op && r.param_index == param) + .expect("the row survived"); + + assert!( + (after.value - 0.5).abs() < 1e-5, + "the value landed on the row that asked for it, not another" + ); + } + + #[test] + fn all_is_reachable_again() { + let Some(ctx) = headless() else { return }; + let mut session = tabbed_session(&ctx); + let all = session.rows().len(); + + session.set_active_tab(0); + assert!(session.rows().len() < all); + + session.set_active_tab(-1); + assert_eq!(session.rows().len(), all, "-1 means everything"); + assert_eq!(session.active_tab(), -1); + } + + /// An out-of-range index is navigation nonsense, not an edit; it must not + /// leave the panel showing nothing. + #[test] + fn a_nonsense_tab_falls_back_to_everything() { + let Some(ctx) = headless() else { return }; + let mut session = tabbed_session(&ctx); + let all = session.rows().len(); + + session.set_active_tab(99); + assert_eq!(session.rows().len(), all); + } + } /// `dr_pipeline`'s morphology, as `dr_segment` names it. diff --git a/ui/dr-ui/src/labels.rs b/ui/dr-ui/src/labels.rs index 9a65b2f..4d2f242 100644 --- a/ui/dr-ui/src/labels.rs +++ b/ui/dr-ui/src/labels.rs @@ -13,6 +13,15 @@ pub fn resolve(key: &str) -> String { match key { // Operations + // The attribute names. Short on purpose: these are read as a strip + // of tabs, where a long word crowds out the next one. + "attr.tone" => "Light".into(), + "attr.colour" => "Colour".into(), + "attr.detail" => "Detail".into(), + "attr.optics" => "Optics".into(), + "attr.geometry" => "Geometry".into(), + "attr.effect" => "Effects".into(), + "op.white_balance" => "White Balance".into(), "op.exposure" => "Exposure".into(), "op.highlights_shadows" => "Highlights & Shadows".into(), diff --git a/ui/dr-ui/src/lib.rs b/ui/dr-ui/src/lib.rs index c41036b..c7d45ae 100644 --- a/ui/dr-ui/src/lib.rs +++ b/ui/dr-ui/src/lib.rs @@ -519,6 +519,20 @@ pub(crate) fn sync_rows( // and flips of an image that no longer has them. sync_framing(window, session); + // TRACES: FR-DEV-3a + // The groups, derived from what the operations say they are about. This + // side resolves the labels; nothing here or in `adjust.slint` names an + // operation or a group. + let (tabs, active_tab) = match session.borrow().as_ref() { + Some(s) => ( + s.tabs().into_iter().map(|(_, name)| name.into()).collect(), + s.active_tab(), + ), + None => (Vec::::new(), -1), + }; + window.set_adjust_tabs(slint::ModelRc::new(slint::VecModel::from(tabs))); + window.set_adjust_active_tab(active_tab); + let current = match session.borrow().as_ref() { Some(s) => s.rows(), None => Vec::new(), @@ -1740,6 +1754,21 @@ pub fn run(paths: Vec) -> Result<()> { // The local-adjustment panel. Wired as a block rather than inline because // it is a dozen callbacks that all say the same three things, and they // read better beside each other than scattered through this function. + { + // Choosing a group re-filters the panel and nothing else — no edit, + // no render. It is navigation. + let weak = window.as_weak(); + let session = session.clone(); + let rows = rows.clone(); + window.on_adjust_tab_picked(move |index| { + let Some(w) = weak.upgrade() else { return }; + if let Some(s) = session.borrow_mut().as_mut() { + s.set_active_tab(index); + } + sync_rows(&w, &rows, &session); + }); + } + masks_ui::wire(&window, &session, &rows, &redraw, gpu.clone()); { diff --git a/ui/dr-ui/ui/adjust.slint b/ui/dr-ui/ui/adjust.slint index ace5453..b1cbeb5 100644 --- a/ui/dr-ui/ui/adjust.slint +++ b/ui/dr-ui/ui/adjust.slint @@ -630,6 +630,14 @@ export component TransferPanel inherits VerticalLayout { export component AdjustPanel inherits Rectangle { in property <[ParamRow]> rows; + /// The groups worth offering, derived from what the operations say they + /// are about. **This file names none of them** — the strings arrive + /// already resolved and the panel only has to draw them, which is what + /// keeps a new operation a zero-edit change here (FR-DEV-3a). + in property <[string]> tabs; + /// Index into `tabs`, or -1 for "everything". + in property active-tab: -1; + callback tab-picked(int); in property enabled: true; /// The tone curve's sampled shape, evaluated in Rust by the same spline /// the shader runs so the drawn line cannot disagree with the applied one. @@ -677,6 +685,50 @@ export component AdjustPanel inherits Rectangle { if !root.enabled: Caption { text: "No image"; } + // The group strip. Only when there is more than one group to choose + // between — a single tab is a control with one option, which is + // decoration. + if root.enabled && root.tabs.length > 1: HorizontalLayout { + spacing: Theme.gap-sm; + alignment: start; + + all := TouchArea { + width: 34px; + height: Theme.touch-target; + mouse-cursor: pointer; + clicked => { root.tab-picked(-1); } + Label { + text: "All"; + emphasised: root.active-tab == -1 || all.has-hover; + vertical-alignment: center; + } + } + + for tab[i] in root.tabs: tab-area := TouchArea { + width: label.preferred-width + 8px; + height: Theme.touch-target; + mouse-cursor: pointer; + clicked => { root.tab-picked(i); } + + label := Label { + text: tab; + emphasised: root.active-tab == i || tab-area.has-hover; + vertical-alignment: center; + } + + // The selected group is underlined rather than filled: the + // accent means *modified* everywhere else in this interface, + // and spending it on "which tab" would blunt the one signal + // the panel has (ui-refinement.md, principle 2). + Rectangle { + y: parent.height - 2px; + height: 2px; + width: parent.width; + background: root.active-tab == i ? Theme.ink : transparent; + } + } + } + // No Flickable here any more. The histogram, the capture metadata and // the geometry controls sat *above* this one and could not be scrolled // away, so on a 280px column in portrait they ate the height the diff --git a/ui/dr-ui/ui/app.slint b/ui/dr-ui/ui/app.slint index 9caba48..7cdbe0a 100644 --- a/ui/dr-ui/ui/app.slint +++ b/ui/dr-ui/ui/app.slint @@ -673,6 +673,9 @@ export component AppWindow inherits Window { // do. This window knows the *shape* of a control, never which operations // exist (FR-DEV-3a). in property <[ParamRow]> adjust-rows; + in property <[string]> adjust-tabs; + in property adjust-active-tab: -1; + callback adjust-tab-picked(int); in property adjust-enabled: false; // The tone curve's sampled shape, evaluated by the core so the drawn // line and the applied one cannot disagree. @@ -1960,6 +1963,9 @@ in property panel-visible: true; adjust := AdjustPanel { rows: root.adjust-rows; + tabs: root.adjust-tabs; + active-tab: root.adjust-active-tab; + tab-picked(i) => { root.adjust-tab-picked(i); } enabled: root.adjust-enabled; curve-samples: root.curve-samples; param-changed(op, param, value) => {