From 8e7b1350bf49397b8495c03456876532bd2b0bc2 Mon Sep 17 00:00:00 2001 From: Duncan Tourolle Date: Sat, 5 Sep 2026 14:34:03 +0200 Subject: [PATCH] Name the frame's category for the decision, not the maths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Attribute::Geometry` becomes `Attribute::Compose`, and `film_sim` moves from `[tone, colour]` to `[effect]`. Two categories were doing the wrong job. "Geometry" describes what crop, straighten and the quarter turns do to coordinates — but it describes lens distortion correction exactly as well, and that is not a compositional choice at all. Naming the attribute for the photographer's decision is what separates it from `Optics`: one is what the lens did, the other is what they chose. The maths the two have in common is not the thing worth filing them under. A film stock declared both `tone` and `colour`, so "Kodachrome" appeared in the Light group beside exposure and again in Colour beside white balance — two places, neither of which is where anyone looks for it. It is neither: `Effect` is defined in this same file as "applied rather than corrected — a look, not a fix", which is what a stock is. That it moves tone and colour is true of every look, and is not what the attribute is for. `from_name` still accepts "geometry" on the way in. That string is persisted in `develop.copy_attributes`, and an entry it fails to parse is not an error — `presets::scope_for` logs it and drops it — so without the alias an existing settings file would have quietly narrowed what a paste carries. `name` writes the current spelling, so the file migrates itself the first time it is saved. Co-Authored-By: Claude Opus 5 (1M context) --- core/dr-pipeline/ops/film_sim.yaml | 8 +++++++- core/dr-pipeline/src/declared/decl.rs | 6 +++--- core/dr-pipeline/src/declared/mod.rs | 2 +- core/dr-pipeline/src/descriptor.rs | 27 +++++++++++++++++++++------ core/dr-pipeline/src/framing.rs | 2 +- core/dr-pipeline/src/preset.rs | 10 +++++----- core/dr-pipeline/tests/attributes.rs | 2 +- docs/traceability.md | 2 +- ui/dr-ui/src/develop.rs | 14 +++++++------- ui/dr-ui/src/labels.rs | 2 +- ui/dr-ui/src/presets.rs | 4 ++-- ui/dr-ui/ui/adjust.slint | 4 ++-- ui/dr-ui/ui/app.slint | 4 ++-- ui/dr-ui/ui/history.slint | 2 +- 14 files changed, 55 insertions(+), 34 deletions(-) diff --git a/core/dr-pipeline/ops/film_sim.yaml b/core/dr-pipeline/ops/film_sim.yaml index ce43cfd..f753f08 100644 --- a/core/dr-pipeline/ops/film_sim.yaml +++ b/core/dr-pipeline/ops/film_sim.yaml @@ -1,6 +1,12 @@ id: film_sim order: 25 -attributes: [tone, colour] +# Effect, not tone-and-colour. It declared both, which put "Kodachrome" in +# the Light group beside exposure and again in Colour beside white balance — +# two places, neither of which is where anyone looks for it, and both of which +# it crowded. A stock is `Effect`'s own definition: applied rather than +# corrected, a look and not a fix. That it moves tone and colour is true of +# every look and is not what the attribute is for. +attributes: [effect] rust: FilmSim why_rust: | diff --git a/core/dr-pipeline/src/declared/decl.rs b/core/dr-pipeline/src/declared/decl.rs index adb1039..7c327f8 100644 --- a/core/dr-pipeline/src/declared/decl.rs +++ b/core/dr-pipeline/src/declared/decl.rs @@ -120,7 +120,7 @@ pub enum Attr { Colour, Detail, Optics, - Geometry, + Compose, Effect, } @@ -132,7 +132,7 @@ impl Attr { Attr::Colour => "colour", Attr::Detail => "detail", Attr::Optics => "optics", - Attr::Geometry => "geometry", + Attr::Compose => "compose", Attr::Effect => "effect", } } @@ -144,7 +144,7 @@ impl Attr { Attr::Colour, Attr::Detail, Attr::Optics, - Attr::Geometry, + Attr::Compose, Attr::Effect, ]; } diff --git a/core/dr-pipeline/src/declared/mod.rs b/core/dr-pipeline/src/declared/mod.rs index f243b92..f7a9a72 100644 --- a/core/dr-pipeline/src/declared/mod.rs +++ b/core/dr-pipeline/src/declared/mod.rs @@ -350,7 +350,7 @@ fn attribute(a: decl::Attr) -> Attribute { decl::Attr::Colour => Attribute::Colour, decl::Attr::Detail => Attribute::Detail, decl::Attr::Optics => Attribute::Optics, - decl::Attr::Geometry => Attribute::Geometry, + decl::Attr::Compose => Attribute::Compose, decl::Attr::Effect => Attribute::Effect, } } diff --git a/core/dr-pipeline/src/descriptor.rs b/core/dr-pipeline/src/descriptor.rs index 0ce9848..af97c80 100644 --- a/core/dr-pipeline/src/descriptor.rs +++ b/core/dr-pipeline/src/descriptor.rs @@ -580,8 +580,15 @@ pub enum Attribute { /// Corrections for the lens that took the photograph: distortion, /// chromatic aberration, vignetting. Optics, - /// The shape of the frame: crop, straighten, rotation, flips. - Geometry, + /// How the frame is composed: crop, straighten, rotation, flips. + /// + /// Named for the decision rather than for the maths. The lens corrections + /// are geometry too — distortion moves pixels exactly as a straighten + /// does — and lumping the two together would file a correction the + /// photographer never asked for beside a choice that is the whole reason + /// they opened the photograph. [`Self::Optics`] is what the lens did; + /// this is what they decided. + Compose, /// Applied rather than corrected — a look, not a fix. Effect, } @@ -598,7 +605,7 @@ impl Attribute { Attribute::Colour, Attribute::Detail, Attribute::Optics, - Attribute::Geometry, + Attribute::Compose, Attribute::Effect, ]; @@ -613,7 +620,7 @@ impl Attribute { Self::Colour => "attr.colour", Self::Detail => "attr.detail", Self::Optics => "attr.optics", - Self::Geometry => "attr.geometry", + Self::Compose => "attr.compose", Self::Effect => "attr.effect", }) } @@ -630,7 +637,7 @@ impl Attribute { Self::Colour => "colour", Self::Detail => "detail", Self::Optics => "optics", - Self::Geometry => "geometry", + Self::Compose => "compose", Self::Effect => "effect", } } @@ -642,7 +649,15 @@ impl Attribute { "colour" => Self::Colour, "detail" => Self::Detail, "optics" => Self::Optics, - "geometry" => Self::Geometry, + "compose" => Self::Compose, + // The name this attribute was persisted under before it was + // called Compose. Settings written by an older build carry it in + // `develop.copy_attributes`, and `from_name` returning `None` + // there does not fail loudly — `presets::scope_for` logs and drops + // the entry, silently narrowing what a paste carries. Accepted on + // the way in only; `name` writes the current spelling, so a + // settings file rewrites itself the first time it is saved. + "geometry" => Self::Compose, "effect" => Self::Effect, _ => return None, }) diff --git a/core/dr-pipeline/src/framing.rs b/core/dr-pipeline/src/framing.rs index 0758dcd..a8737c4 100644 --- a/core/dr-pipeline/src/framing.rs +++ b/core/dr-pipeline/src/framing.rs @@ -78,7 +78,7 @@ static DESCRIPTOR: LazyLock> = LazyLock::new(|| { Arc::new(OpDescriptor { // The shape of the frame, and the only operation that changes the // output's dimensions. - attributes: vec![Attribute::Geometry], + attributes: vec![Attribute::Compose], id: ID, label: LocalizedKey("op.framing"), params: vec![ diff --git a/core/dr-pipeline/src/preset.rs b/core/dr-pipeline/src/preset.rs index 55c9a40..2b78e28 100644 --- a/core/dr-pipeline/src/preset.rs +++ b/core/dr-pipeline/src/preset.rs @@ -59,7 +59,7 @@ use crate::graph::EditGraph; /// /// It also dissolves a special case. Framing used to be excluded by an /// explicit test against one operation's id; it is now excluded because -/// [`Attribute::Geometry`] is not in the default set, and the argument below +/// [`Attribute::Compose`] is not in the default set, and the argument below /// is a statement about a kind of edit rather than about a particular node. /// /// # Why geometry is out by default @@ -116,7 +116,7 @@ impl Scope { /// its shape. The default. pub fn adjustments() -> Self { Self { - bits: Self::all_bits() & !Self::bit(Attribute::Geometry), + bits: Self::all_bits() & !Self::bit(Attribute::Compose), carries_unclassified: true, } } @@ -278,7 +278,7 @@ impl Preset { pub fn touches_framing(&self) -> bool { self.params .keys() - .any(|(op, _)| attributes_of(op).is_some_and(|a| a.contains(&Attribute::Geometry))) + .any(|(op, _)| attributes_of(op).is_some_and(|a| a.contains(&Attribute::Compose))) } /// How many operations this preset touches, in scope. @@ -1254,8 +1254,8 @@ mod tests { .into_iter() .filter(|a| !default_scope.has(*a)) .collect(); - assert_eq!(excluded, vec![Attribute::Geometry]); - assert!(Scope::everything().has(Attribute::Geometry)); + assert_eq!(excluded, vec![Attribute::Compose]); + assert!(Scope::everything().has(Attribute::Compose)); } #[test] diff --git a/core/dr-pipeline/tests/attributes.rs b/core/dr-pipeline/tests/attributes.rs index 05e1072..e3796e5 100644 --- a/core/dr-pipeline/tests/attributes.rs +++ b/core/dr-pipeline/tests/attributes.rs @@ -46,7 +46,7 @@ fn the_groups_are_derivable_from_the_chain() { ); assert!(present.contains(&Attribute::Colour)); assert!( - present.contains(&Attribute::Geometry), + present.contains(&Attribute::Compose), "framing is in the capability list and is geometry" ); diff --git a/docs/traceability.md b/docs/traceability.md index de84bf8..da76c7e 100644 --- a/docs/traceability.md +++ b/docs/traceability.md @@ -112,7 +112,7 @@ _None._ | FR-PLAT-AND-6 | [`apps/darkroom-android/src/lib.rs:388`](../apps/darkroom-android/src/lib.rs#L388) | | FR-PLAT-LIN-1 | [`core/dr-types/src/settings.rs:1`](../core/dr-types/src/settings.rs#L1), [`platform/dr-plat/src/storage.rs:344`](../platform/dr-plat/src/storage.rs#L344), [`ui/dr-ui/src/lib.rs:999`](../ui/dr-ui/src/lib.rs#L999), [`ui/dr-ui/src/preset_store.rs:1`](../ui/dr-ui/src/preset_store.rs#L1), [`ui/dr-ui/src/settings_store.rs:1`](../ui/dr-ui/src/settings_store.rs#L1) | | FR-PLAT-LIN-2 | [`platform/dr-plat/src/display.rs:1`](../platform/dr-plat/src/display.rs#L1), [`platform/dr-plat/src/display/wayland.rs:1`](../platform/dr-plat/src/display/wayland.rs#L1), [`platform/dr-plat/src/display/x11.rs:1`](../platform/dr-plat/src/display/x11.rs#L1) | -| FR-PLG-2 | [`core/dr-pipeline/src/declared/decl.rs:1`](../core/dr-pipeline/src/declared/decl.rs#L1), [`core/dr-pipeline/src/declared/expr.rs:152`](../core/dr-pipeline/src/declared/expr.rs#L152), [`core/dr-pipeline/src/declared/expr.rs:1`](../core/dr-pipeline/src/declared/expr.rs#L1), [`core/dr-pipeline/src/declared/mod.rs:1`](../core/dr-pipeline/src/declared/mod.rs#L1), [`core/dr-pipeline/src/declared/mod.rs:82`](../core/dr-pipeline/src/declared/mod.rs#L82), [`core/dr-pipeline/src/descriptor.rs:15`](../core/dr-pipeline/src/descriptor.rs#L15), [`core/dr-pipeline/src/descriptor.rs:652`](../core/dr-pipeline/src/descriptor.rs#L652), [`core/dr-pipeline/src/operation.rs:232`](../core/dr-pipeline/src/operation.rs#L232), [`core/dr-pipeline/tests/declared_parity.rs:1`](../core/dr-pipeline/tests/declared_parity.rs#L1), [`core/dr-pipeline/tests/declared_parity.rs:240`](../core/dr-pipeline/tests/declared_parity.rs#L240), [`core/dr-pipeline/tests/declared_parity.rs:305`](../core/dr-pipeline/tests/declared_parity.rs#L305), [`core/dr-pipeline/tests/declared_parity.rs:358`](../core/dr-pipeline/tests/declared_parity.rs#L358), [`core/dr-pipeline/tests/declared_parity.rs:416`](../core/dr-pipeline/tests/declared_parity.rs#L416) | +| FR-PLG-2 | [`core/dr-pipeline/src/declared/decl.rs:1`](../core/dr-pipeline/src/declared/decl.rs#L1), [`core/dr-pipeline/src/declared/expr.rs:152`](../core/dr-pipeline/src/declared/expr.rs#L152), [`core/dr-pipeline/src/declared/expr.rs:1`](../core/dr-pipeline/src/declared/expr.rs#L1), [`core/dr-pipeline/src/declared/mod.rs:1`](../core/dr-pipeline/src/declared/mod.rs#L1), [`core/dr-pipeline/src/declared/mod.rs:82`](../core/dr-pipeline/src/declared/mod.rs#L82), [`core/dr-pipeline/src/descriptor.rs:15`](../core/dr-pipeline/src/descriptor.rs#L15), [`core/dr-pipeline/src/descriptor.rs:667`](../core/dr-pipeline/src/descriptor.rs#L667), [`core/dr-pipeline/src/operation.rs:232`](../core/dr-pipeline/src/operation.rs#L232), [`core/dr-pipeline/tests/declared_parity.rs:1`](../core/dr-pipeline/tests/declared_parity.rs#L1), [`core/dr-pipeline/tests/declared_parity.rs:240`](../core/dr-pipeline/tests/declared_parity.rs#L240), [`core/dr-pipeline/tests/declared_parity.rs:305`](../core/dr-pipeline/tests/declared_parity.rs#L305), [`core/dr-pipeline/tests/declared_parity.rs:358`](../core/dr-pipeline/tests/declared_parity.rs#L358), [`core/dr-pipeline/tests/declared_parity.rs:416`](../core/dr-pipeline/tests/declared_parity.rs#L416) | | FR-PLG-2d | [`core/dr-pipeline/src/declared/decl.rs:112`](../core/dr-pipeline/src/declared/decl.rs#L112), [`core/dr-pipeline/src/declared/decl.rs:152`](../core/dr-pipeline/src/declared/decl.rs#L152), [`core/dr-pipeline/src/declared/decl.rs:1`](../core/dr-pipeline/src/declared/decl.rs#L1), [`core/dr-pipeline/src/declared/decl.rs:420`](../core/dr-pipeline/src/declared/decl.rs#L420), [`core/dr-pipeline/src/declared/decl.rs:67`](../core/dr-pipeline/src/declared/decl.rs#L67), [`core/dr-pipeline/src/declared/mod.rs:1`](../core/dr-pipeline/src/declared/mod.rs#L1), [`core/dr-pipeline/src/declared/mod.rs:384`](../core/dr-pipeline/src/declared/mod.rs#L384), [`core/dr-pipeline/src/declared/mod.rs:403`](../core/dr-pipeline/src/declared/mod.rs#L403) | | FR-PLG-8 | [`core/dr-pipeline/src/sidecar.rs:1807`](../core/dr-pipeline/src/sidecar.rs#L1807), [`core/dr-pipeline/src/sidecar.rs:1840`](../core/dr-pipeline/src/sidecar.rs#L1840) | | FR-RAW-1 | [`core/dr-decode/src/lib.rs:243`](../core/dr-decode/src/lib.rs#L243), [`core/dr-types/src/lib.rs:132`](../core/dr-types/src/lib.rs#L132), [`core/dr-types/src/lib.rs:203`](../core/dr-types/src/lib.rs#L203) | diff --git a/ui/dr-ui/src/develop.rs b/ui/dr-ui/src/develop.rs index c3652c6..a73dbb0 100644 --- a/ui/dr-ui/src/develop.rs +++ b/ui/dr-ui/src/develop.rs @@ -1021,15 +1021,15 @@ impl DevelopSession { /// (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. + /// Compose is excluded: its one operation prefers an on-canvas widget and + /// is skipped by the row builder, so a Compose tab would be empty of rows + /// while `ComposePanel` 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| *a != Attribute::Compose) .filter(|a| { caps.iter().any(|c| { c.attributes.contains(a) @@ -1171,7 +1171,7 @@ pub(crate) fn supported(widget: WidgetKind) -> bool { // Drawn in the panel. WidgetKind::ToneCurve => true, // Hosted on the canvas: the overlay is drawn over the photograph and - // the panel contributes `GeometryPanel`, the affordance that turns it + // the panel contributes `ComposePanel`, the affordance that turns it // on. WidgetKind::CropOverlay => true, // Not implemented. Listed rather than caught by a wildcard so the next @@ -1254,7 +1254,7 @@ pub(crate) fn rows_filtered( // terms, with nothing named. // // Skipped rather than rendered as an affordance row, because - // the affordance is `GeometryPanel` — a bespoke control for a + // the affordance is `ComposePanel` — a bespoke control for a // known stage, which is a thing the interface is entitled to // build (ARCH §4.3a draws the line at the *generated* panel // naming stages, not at the interface having hand-made @@ -5748,7 +5748,7 @@ mod tests { #[test] fn framing_is_not_generated_as_sliders() { - // `GeometryPanel` presents crop, rotation, flips and straightening as + // `ComposePanel` presents crop, rotation, flips and straightening as // the gestures they are. If the generic path emitted them too the // sidebar would carry both — including four "Crop Left/Top/Width/ // Height" sliders no one can compose a photograph with. diff --git a/ui/dr-ui/src/labels.rs b/ui/dr-ui/src/labels.rs index 49929e8..28954b2 100644 --- a/ui/dr-ui/src/labels.rs +++ b/ui/dr-ui/src/labels.rs @@ -127,7 +127,7 @@ fn catalogued(key: &str) -> Option<&'static str> { "attr.colour" => "Colour", "attr.detail" => "Detail", "attr.optics" => "Optics", - "attr.geometry" => "Geometry", + "attr.compose" => "Compose", "attr.effect" => "Effects", "op.white_balance" => "White Balance", diff --git a/ui/dr-ui/src/presets.rs b/ui/dr-ui/src/presets.rs index 05d3c80..7e995a1 100644 --- a/ui/dr-ui/src/presets.rs +++ b/ui/dr-ui/src/presets.rs @@ -210,7 +210,7 @@ fn label_for(attribute: dr_pipeline::Attribute) -> &'static str { Attribute::Colour => "Colour", Attribute::Detail => "Detail", Attribute::Optics => "Optics", - Attribute::Geometry => "Geometry", + Attribute::Compose => "Compose", Attribute::Effect => "Effect", } } @@ -548,7 +548,7 @@ pub fn render( // holding no crop loses nothing to the setting, and saying so anyway would // train the user to ignore the line. window.set_settings_framing_withheld( - clipboard.holds_framing() && !scope.has(dr_pipeline::Attribute::Geometry), + clipboard.holds_framing() && !scope.has(dr_pipeline::Attribute::Compose), ); } diff --git a/ui/dr-ui/ui/adjust.slint b/ui/dr-ui/ui/adjust.slint index ae2a438..417aa4c 100644 --- a/ui/dr-ui/ui/adjust.slint +++ b/ui/dr-ui/ui/adjust.slint @@ -535,7 +535,7 @@ component ParamControl inherits Rectangle { // This does not weaken ARCH §4.3: nothing here reads a parameter *value* out // of a descriptor or routes by index. It calls named session actions, which is // what a bespoke widget for a known stage is entitled to do. -export component GeometryPanel inherits Rectangle { +export component ComposePanel inherits Rectangle { in property enabled: true; in property angle: 0.0; in property max-straighten: 45.0; @@ -586,7 +586,7 @@ export component GeometryPanel inherits Rectangle { // naming the group, flagging that it holds an edit, and offering the // reset — without the hiding. GroupHeading { - title: "GEOMETRY"; + title: "COMPOSE"; modified: root.modified; has-reset: root.modified; reset => { root.reset(); } diff --git a/ui/dr-ui/ui/app.slint b/ui/dr-ui/ui/app.slint index 8997a70..658a3d0 100644 --- a/ui/dr-ui/ui/app.slint +++ b/ui/dr-ui/ui/app.slint @@ -1,5 +1,5 @@ import { Theme } from "theme.slint"; -import { AdjustPanel, GeometryPanel, GroupStrip, ParamRow, TransferPanel, ViewMode } from "adjust.slint"; +import { AdjustPanel, ComposePanel, GroupStrip, ParamRow, TransferPanel, ViewMode } from "adjust.slint"; import { CategoryRow, GradientHandle, GradientHandles, HandleRole, MaskPanel, MaskRow, SubjectRow } from "masks.slint"; import { SpotHandle, SpotHandles, SpotPanel, SpotRole } from "spots.slint"; import { CropOverlay } from "crop.slint"; @@ -2471,7 +2471,7 @@ in property panel-visible: true; // made rather than how it is applied: the frame is decided // by eye first and the pipeline runs it last (see // `dr_pipeline::framing` on the coordinate order). - if !root.local-mode && !root.repairing: GeometryPanel { + if !root.local-mode && !root.repairing: ComposePanel { enabled: root.adjust-enabled; angle: root.straighten; max-straighten: root.max-straighten; diff --git a/ui/dr-ui/ui/history.slint b/ui/dr-ui/ui/history.slint index 9acad69..9e5f27c 100644 --- a/ui/dr-ui/ui/history.slint +++ b/ui/dr-ui/ui/history.slint @@ -121,7 +121,7 @@ export component HistoryPanel inherits Rectangle { spacing: Theme.gap-sm; alignment: start; - // A heading, not a collapsible. `GeometryPanel` carries the argument + // A heading, not a collapsible. `ComposePanel` carries the argument // and it holds here: the list is last in the column, so what a lid // would save is scrolling past nothing. HorizontalLayout {