Name the frame's category for the decision, not the maths
`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) <noreply@anthropic.com>
This commit is contained in:
@@ -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: |
|
||||
|
||||
@@ -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,
|
||||
];
|
||||
}
|
||||
|
||||
@@ -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,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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,
|
||||
})
|
||||
|
||||
@@ -78,7 +78,7 @@ static DESCRIPTOR: LazyLock<Arc<OpDescriptor>> = 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![
|
||||
|
||||
@@ -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]
|
||||
|
||||
@@ -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"
|
||||
);
|
||||
|
||||
|
||||
@@ -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) |
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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",
|
||||
|
||||
@@ -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),
|
||||
);
|
||||
}
|
||||
|
||||
|
||||
@@ -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 <bool> enabled: true;
|
||||
in property <float> angle: 0.0;
|
||||
in property <float> 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(); }
|
||||
|
||||
@@ -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 <bool> 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;
|
||||
|
||||
@@ -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 {
|
||||
|
||||
Reference in New Issue
Block a user