Offer the lens profile as a tick box, since applying it silently reads as absent
The develop panel's Optics group is three manual sliders: distortion, chromatic aberration and lens vignetting. The automatic correction was already there — the file's EXIF lens is matched against the bundled Lensfun database on open and the coefficients are fanned out to all three — but nothing in the interface said so except a line of grey text under the camera reading "· corrected", and there was no way to decline it. From the outside that is indistinguishable from the feature not existing, which is how it was read. `dr-lens` states the rule this breaks: an automatic correction that silently does nothing is worse than one the user can see is unavailable. The caption satisfied the letter of it and not the point — a photographer looking for "apply the lens profile" found three sliders and no switch. So the profile is now a control. It is a capability rather than a flag on the session, because everything a photographer sets travels one road: the capability list feeds the generated panel, `Preset` captures it, the sidecar stores it and the undo stack replays it. A bool on the side would have needed adding to each of those four by hand and would have been forgotten in at least one — which is exactly how the mask stack came to be missing from the history. It is on by default, which is what `switch_on` is for: the coefficients are a measurement of the lens that took the photograph, so accepting them is neutral and declining them is the edit. The sidecar therefore stores nothing for the ordinary case and the correction still happens. The switch appears only where a profile was matched. 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 rule above names rather than an instance of following it — those photographs are told "· no profile" in words instead, and one whose box is unticked now says "· profile off", which is a third fact and not either of the other two. Two things had to be built underneath. `ParamKind::Bool` was in the core's closed enum and mapped to a row kind here, and had no control behind it in `adjust.slint`: a parameter declaring itself a switch was flattened into a row that drew nothing at all. Nothing shipped had one until now, so the gap cost nothing and was invisible. And `Check` self-toggled, which is right for a settings page that owns its value and wrong for a panel row that is a view of the edit graph — the click would have answered by replacing the binding with a literal, and the next undo or pasted preset would have moved the value with the tick left where the finger put it. It now takes `controlled`, and the generated row uses it. The manual sliders are unchanged and still trim whatever the profile leaves, so switching it off is "correct this by hand" rather than "stop correcting".
This commit is contained in:
+94
-4
@@ -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<u8> = (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.
|
||||
///
|
||||
|
||||
@@ -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",
|
||||
|
||||
@@ -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.
|
||||
//
|
||||
|
||||
@@ -432,9 +432,35 @@ export component Check inherits Rectangle {
|
||||
/// switches makes the user guess at what each one costs.
|
||||
in property <string> hint;
|
||||
in-out property <bool> 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 <bool> 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 {
|
||||
|
||||
Reference in New Issue
Block a user