Let a paste carry some kinds of edit and not others
FR-DEV-6 asks for presets "covering a subset of the edit graph". What landed with the named presets covered two subsets: everything, and everything but the crop. "Match the colour but not the sharpening" had no way to be said. `Scope` is now a set of `Attribute` — the same six kinds every operation already declares and the develop panel already builds its tabs from. The photographer ticking "tone and colour" is naming the groups they navigate by, and neither this module nor the interface has to name an operation to do it (FR-DEV-3c). The pleasing part is what left. Framing used to be excluded by an explicit test against one operation's id; it is now excluded because Geometry is not in the default set. The special case dissolved into the general rule, and the argument for it — a crop is a decision about *this* photograph, and carrying it across forty destroys forty compositions — is now a statement about a kind of edit rather than about a node. All thirty-three existing preset tests pass unchanged, which is the evidence that the generalisation kept its promises. One decision that is a field rather than a rule, because the two cases genuinely differ. An operation this build cannot classify — from a newer version, arriving over sync — travels under "everything" and "everything but the crop", because those are claims about the whole edit and an unrecognised operation is part of it (FR-NC-8). It does not travel under a hand-picked set, because that is a claim about kinds, and an unknown kind is not one of the kinds that were ticked. The settings page's "Copy crop and rotation" checkbox is gone, replaced by the same chips the preset sheet draws. It asked the right first question — geometry is the kind whose accidental travel destroys work — but it was the only question a boolean could ask. The field stays in `Settings`, read exactly once to seed the new set, so anyone who had ticked it keeps their behaviour. The chips are deliberately not in the develop column. Six of them there would set the width of the whole sidebar, which is the bug `ChipGrid`'s comment records at length. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
Claude Opus 5
@@ -2051,6 +2051,13 @@ pub fn run(paths: Vec<PathBuf>) -> Result<()> {
|
||||
presets::save_open_edit(&w, &open_image.borrow(), &session, &library);
|
||||
}));
|
||||
|
||||
// TRACES: FR-DEV-6
|
||||
// The scope chips, shared by the preset sheet and the settings page.
|
||||
// Wired once and rendered once: both surfaces draw one set, because
|
||||
// "what does a paste carry" is one question about how this
|
||||
// photographer works rather than one per place it is asked.
|
||||
presets::wire_scope(&window, settings.clone(), clipboard.clone());
|
||||
presets::render_scope(&window, &settings);
|
||||
presets::render(&window, &clipboard, &settings);
|
||||
}
|
||||
|
||||
|
||||
+178
-25
@@ -40,7 +40,7 @@ use slint::ComponentHandle;
|
||||
|
||||
use crate::develop::DevelopSession;
|
||||
use crate::preset_store::PresetStore;
|
||||
use crate::{library, library_ui, settings_ui, AppWindow, ParamRow};
|
||||
use crate::{library, library_ui, settings_ui, AppWindow, ParamRow, ScopeKind};
|
||||
|
||||
/// Where the develop view's current edit is stored.
|
||||
///
|
||||
@@ -142,16 +142,125 @@ pub fn describe(preset: &Preset, scope: Scope) -> String {
|
||||
|
||||
/// The scope a paste should use, from the user's preference.
|
||||
///
|
||||
/// One function rather than the boolean read at each call site, so "the
|
||||
/// setting is off by default and means framing stays behind" is stated once.
|
||||
/// One function rather than the setting read at each call site, so what the
|
||||
/// stored names mean — including what their absence means — is stated once.
|
||||
///
|
||||
/// # The migration lives here
|
||||
///
|
||||
/// `copy_attributes` is `None` on a device that has only ever run a build with
|
||||
/// the old boolean, and the answer is derived from that boolean exactly once:
|
||||
/// a photographer who had asked for the crop to travel keeps getting the crop.
|
||||
/// Writing the derived set back is the *caller's* business — see
|
||||
/// [`settings_ui`] — because a read has no business having a side effect on
|
||||
/// the file it read from.
|
||||
///
|
||||
/// A name this build does not recognise is dropped rather than refused, the
|
||||
/// same tolerance the sidecar shows an unknown operation: a config written by
|
||||
/// a newer build must not stop an older one from pasting.
|
||||
pub fn scope_for(settings: &dr_types::Settings) -> Scope {
|
||||
if settings.develop.copy_includes_framing {
|
||||
Scope::Everything
|
||||
} else {
|
||||
Scope::Adjustments
|
||||
let Some(names) = settings.develop.copy_attributes.as_ref() else {
|
||||
return if settings.develop.copy_includes_framing {
|
||||
Scope::everything()
|
||||
} else {
|
||||
Scope::adjustments()
|
||||
};
|
||||
};
|
||||
Scope::of(names.iter().filter_map(|n| {
|
||||
let attribute = dr_pipeline::Attribute::from_name(n);
|
||||
if attribute.is_none() {
|
||||
log::debug!("preset scope: ignoring unknown attribute {n:?}");
|
||||
}
|
||||
attribute
|
||||
}))
|
||||
}
|
||||
|
||||
/// The names to store for a scope, for the setting [`scope_for`] reads back.
|
||||
pub fn names_for(scope: Scope) -> Vec<String> {
|
||||
scope.attributes().map(|a| a.name().to_string()).collect()
|
||||
}
|
||||
|
||||
/// The scope chips, in the order the pipeline declares its attributes.
|
||||
///
|
||||
/// Declaration order is roughly the order a photographer works in, which is
|
||||
/// the order [`Attribute::ALL`] is written for. Nothing here chooses a
|
||||
/// sequence of its own, and nothing here names an operation (FR-DEV-3c) — the
|
||||
/// six kinds come from the pipeline and the labels from their descriptors.
|
||||
pub fn scope_kinds(scope: Scope) -> Vec<ScopeKind> {
|
||||
dr_pipeline::Attribute::ALL
|
||||
.into_iter()
|
||||
.map(|attribute| ScopeKind {
|
||||
name: attribute.name().into(),
|
||||
label: label_for(attribute).into(),
|
||||
on: scope.has(attribute),
|
||||
})
|
||||
.collect()
|
||||
}
|
||||
|
||||
/// The word for an attribute.
|
||||
///
|
||||
/// Written here rather than taken from `Attribute::label`, which returns a
|
||||
/// localisation key (`attr.tone`) for a catalogue that does not exist yet.
|
||||
/// When it does, this becomes a lookup and the strings leave this file — which
|
||||
/// is why they are gathered in one function rather than spread through the
|
||||
/// panel.
|
||||
fn label_for(attribute: dr_pipeline::Attribute) -> &'static str {
|
||||
use dr_pipeline::Attribute;
|
||||
match attribute {
|
||||
Attribute::Tone => "Tone",
|
||||
Attribute::Colour => "Colour",
|
||||
Attribute::Detail => "Detail",
|
||||
Attribute::Optics => "Optics",
|
||||
Attribute::Geometry => "Geometry",
|
||||
Attribute::Effect => "Effect",
|
||||
}
|
||||
}
|
||||
|
||||
/// Push the current scope onto the window, wherever it is drawn.
|
||||
pub fn render_scope(window: &AppWindow, settings: &Rc<settings_ui::SettingsController>) {
|
||||
let scope = scope_for(&settings.snapshot());
|
||||
let kinds: Vec<ScopeKind> = scope_kinds(scope);
|
||||
window.set_copy_scope_kinds(slint::ModelRc::new(slint::VecModel::from(kinds)));
|
||||
window.set_copy_scope_empty(scope.is_empty());
|
||||
}
|
||||
|
||||
/// Wire the scope chips, which every surface that draws them shares.
|
||||
pub fn wire_scope(
|
||||
window: &AppWindow,
|
||||
settings: Rc<settings_ui::SettingsController>,
|
||||
clipboard: Rc<Clipboard>,
|
||||
) {
|
||||
let weak = window.as_weak();
|
||||
window.on_copy_scope_toggled(move |name| {
|
||||
let Some(w) = weak.upgrade() else { return };
|
||||
let Some(attribute) = dr_pipeline::Attribute::from_name(&name) else {
|
||||
return;
|
||||
};
|
||||
|
||||
// Read, flip, write the whole set. The stored value is a list of names
|
||||
// rather than a bitfield precisely so that this is the only place that
|
||||
// has to know how the two representations line up.
|
||||
let scope = scope_for(&settings.snapshot());
|
||||
let flipped: Vec<dr_pipeline::Attribute> = dr_pipeline::Attribute::ALL
|
||||
.into_iter()
|
||||
.filter(|a| {
|
||||
if *a == attribute {
|
||||
!scope.has(*a)
|
||||
} else {
|
||||
scope.has(*a)
|
||||
}
|
||||
})
|
||||
.collect();
|
||||
|
||||
settings.edit(|s| s.develop.copy_attributes = Some(names_for(Scope::of(flipped))));
|
||||
|
||||
render_scope(&w, &settings);
|
||||
// The clipboard's summary counts operations *in scope*, so it changes
|
||||
// whenever this does — and a summary describing a scope the buttons
|
||||
// are no longer using is worse than no summary.
|
||||
render(&w, &clipboard, &settings);
|
||||
});
|
||||
}
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Local sidecars
|
||||
// ---------------------------------------------------------------------------
|
||||
@@ -312,7 +421,7 @@ pub fn save_open_edit(
|
||||
// `Everything`: this is the image's *own* edit being written back,
|
||||
// not a paste onto someone else's. Excluding framing here would
|
||||
// make a crop the one adjustment that never survived a restart.
|
||||
if let Err(e) = save_local(path, &preset, Scope::Everything, Some(&masks)) {
|
||||
if let Err(e) = save_local(path, &preset, Scope::everything(), Some(&masks)) {
|
||||
log::warn!("saving {}: {e}", path.display());
|
||||
}
|
||||
}
|
||||
@@ -323,7 +432,7 @@ pub fn save_open_edit(
|
||||
version_uuid: version_uuid.clone(),
|
||||
amendment: library::Amendment::Settings {
|
||||
preset,
|
||||
scope: Scope::Everything,
|
||||
scope: Scope::everything(),
|
||||
// The image's own edit, so the whole of it: `Everything`
|
||||
// already says framing travels, and the local adjustments
|
||||
// have to travel for the same reason. Without this the
|
||||
@@ -431,7 +540,9 @@ pub fn render(
|
||||
// Only worth saying when it is actually true of *this* copy: a clipboard
|
||||
// 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 == Scope::Adjustments);
|
||||
window.set_settings_framing_withheld(
|
||||
clipboard.holds_framing() && !scope.has(dr_pipeline::Attribute::Geometry),
|
||||
);
|
||||
}
|
||||
|
||||
/// The develop view's shared state, as this module needs it.
|
||||
@@ -731,7 +842,7 @@ pub fn wire_named(
|
||||
let summary = session
|
||||
.borrow()
|
||||
.as_ref()
|
||||
.map(|s| describe(&s.copy_settings(), Scope::Everything))
|
||||
.map(|s| describe(&s.copy_settings(), Scope::everything()))
|
||||
.unwrap_or_default();
|
||||
w.set_preset_capture_summary(summary.into());
|
||||
});
|
||||
@@ -908,7 +1019,7 @@ mod tests {
|
||||
save_local(
|
||||
&image,
|
||||
&Preset::capture(&graph),
|
||||
Scope::Everything,
|
||||
Scope::everything(),
|
||||
Some(graph.masks()),
|
||||
)
|
||||
.unwrap();
|
||||
@@ -952,7 +1063,13 @@ mod tests {
|
||||
let dir = tempdir("round-trip");
|
||||
let image = dir.join("a.CR2");
|
||||
|
||||
save_local(&image, &Preset::capture(&edited()), Scope::Everything, None).unwrap();
|
||||
save_local(
|
||||
&image,
|
||||
&Preset::capture(&edited()),
|
||||
Scope::everything(),
|
||||
None,
|
||||
)
|
||||
.unwrap();
|
||||
|
||||
let sidecar = load_local(&image).expect("a sidecar was written");
|
||||
let mut restored = EditGraph::default_chain();
|
||||
@@ -979,8 +1096,20 @@ mod tests {
|
||||
let dir = tempdir("one-version");
|
||||
let image = dir.join("a.CR2");
|
||||
|
||||
save_local(&image, &Preset::capture(&edited()), Scope::Everything, None).unwrap();
|
||||
save_local(&image, &Preset::capture(&edited()), Scope::Everything, None).unwrap();
|
||||
save_local(
|
||||
&image,
|
||||
&Preset::capture(&edited()),
|
||||
Scope::everything(),
|
||||
None,
|
||||
)
|
||||
.unwrap();
|
||||
save_local(
|
||||
&image,
|
||||
&Preset::capture(&edited()),
|
||||
Scope::everything(),
|
||||
None,
|
||||
)
|
||||
.unwrap();
|
||||
|
||||
assert_eq!(load_local(&image).expect("a sidecar").versions.len(), 1);
|
||||
}
|
||||
@@ -992,13 +1121,25 @@ mod tests {
|
||||
let dir = tempdir("revision");
|
||||
let image = dir.join("a.CR2");
|
||||
|
||||
save_local(&image, &Preset::capture(&edited()), Scope::Everything, None).unwrap();
|
||||
save_local(
|
||||
&image,
|
||||
&Preset::capture(&edited()),
|
||||
Scope::everything(),
|
||||
None,
|
||||
)
|
||||
.unwrap();
|
||||
let first = load_local(&image)
|
||||
.unwrap()
|
||||
.default_version()
|
||||
.unwrap()
|
||||
.revision;
|
||||
save_local(&image, &Preset::capture(&edited()), Scope::Everything, None).unwrap();
|
||||
save_local(
|
||||
&image,
|
||||
&Preset::capture(&edited()),
|
||||
Scope::everything(),
|
||||
None,
|
||||
)
|
||||
.unwrap();
|
||||
let second = load_local(&image)
|
||||
.unwrap()
|
||||
.default_version()
|
||||
@@ -1026,7 +1167,13 @@ mod tests {
|
||||
});
|
||||
std::fs::write(local_sidecar_path(&image), sidecar.to_text()).unwrap();
|
||||
|
||||
save_local(&image, &Preset::capture(&edited()), Scope::Everything, None).unwrap();
|
||||
save_local(
|
||||
&image,
|
||||
&Preset::capture(&edited()),
|
||||
Scope::everything(),
|
||||
None,
|
||||
)
|
||||
.unwrap();
|
||||
|
||||
let back = load_local(&image).unwrap();
|
||||
let v = back.default_version().unwrap();
|
||||
@@ -1043,7 +1190,13 @@ mod tests {
|
||||
let image = dir.join("a.CR2");
|
||||
std::fs::write(local_sidecar_path(&image), "drsc 99\n").unwrap();
|
||||
|
||||
assert!(save_local(&image, &Preset::capture(&edited()), Scope::Everything, None).is_err());
|
||||
assert!(save_local(
|
||||
&image,
|
||||
&Preset::capture(&edited()),
|
||||
Scope::everything(),
|
||||
None
|
||||
)
|
||||
.is_err());
|
||||
assert_eq!(
|
||||
std::fs::read_to_string(local_sidecar_path(&image)).unwrap(),
|
||||
"drsc 99\n",
|
||||
@@ -1057,7 +1210,7 @@ mod tests {
|
||||
save_local(
|
||||
&dir.join("a.CR2"),
|
||||
&Preset::capture(&edited()),
|
||||
Scope::Everything,
|
||||
Scope::everything(),
|
||||
None,
|
||||
)
|
||||
.unwrap();
|
||||
@@ -1083,12 +1236,12 @@ mod tests {
|
||||
let mut settings = dr_types::Settings::default();
|
||||
assert_eq!(
|
||||
scope_for(&settings),
|
||||
Scope::Adjustments,
|
||||
Scope::adjustments(),
|
||||
"a fresh install must not carry crops between photographs"
|
||||
);
|
||||
|
||||
settings.develop.copy_includes_framing = true;
|
||||
assert_eq!(scope_for(&settings), Scope::Everything);
|
||||
assert_eq!(scope_for(&settings), Scope::everything());
|
||||
}
|
||||
|
||||
#[test]
|
||||
@@ -1103,7 +1256,7 @@ mod tests {
|
||||
clipboard.put(Preset::capture(&g));
|
||||
|
||||
// Three parameters, two operations.
|
||||
assert_eq!(clipboard.describe(Scope::Adjustments), "2 adjustments");
|
||||
assert_eq!(clipboard.describe(Scope::adjustments()), "2 adjustments");
|
||||
}
|
||||
|
||||
#[test]
|
||||
@@ -1115,7 +1268,7 @@ mod tests {
|
||||
clipboard.put(Preset::capture(&EditGraph::default_chain()));
|
||||
|
||||
assert!(clipboard.is_armed());
|
||||
assert_eq!(clipboard.describe(Scope::Adjustments), "Neutral");
|
||||
assert_eq!(clipboard.describe(Scope::adjustments()), "Neutral");
|
||||
}
|
||||
|
||||
// -----------------------------------------------------------------------
|
||||
@@ -1148,7 +1301,7 @@ mod tests {
|
||||
|
||||
let preset = presets.get("Warm").expect("stored");
|
||||
let mut target = EditGraph::default_chain();
|
||||
preset.apply(&mut target, Scope::Adjustments);
|
||||
preset.apply(&mut target, Scope::adjustments());
|
||||
assert_eq!(
|
||||
target.param(
|
||||
dr_pipeline::ops::exposure::ID,
|
||||
|
||||
@@ -111,7 +111,12 @@ impl SettingsController {
|
||||
/// accidentally write back a stale copy of the fields it was not editing —
|
||||
/// with a save on every keystroke, two controls holding their own snapshots
|
||||
/// would overwrite each other.
|
||||
fn edit(&self, f: impl FnOnce(&mut Settings)) {
|
||||
///
|
||||
/// `pub(crate)` rather than private because the settings page is no longer
|
||||
/// the only surface that writes a preference: the scope chips are drawn
|
||||
/// here *and* in the preset sheet, and both change one stored set. The
|
||||
/// closure is what makes that safe, which is why the shape stays.
|
||||
pub(crate) fn edit(&self, f: impl FnOnce(&mut Settings)) {
|
||||
{
|
||||
let mut settings = self.settings.borrow_mut();
|
||||
f(&mut settings);
|
||||
@@ -145,7 +150,6 @@ pub fn render(window: &AppWindow, controller: &SettingsController) {
|
||||
window.set_settings_thumbnail_budget(budget::label(s.cache.thumbnail_budget_bytes).into());
|
||||
window.set_settings_thumbnail_unlimited(s.cache.thumbnail_budget_bytes.is_none());
|
||||
window.set_settings_keep_opened(s.cache.keep_opened_originals);
|
||||
window.set_settings_copy_includes_framing(s.develop.copy_includes_framing);
|
||||
window.set_settings_cache_usage(controller.usage_label.borrow().clone().into());
|
||||
|
||||
// --- library -------------------------------------------------------
|
||||
@@ -627,20 +631,6 @@ pub fn wire<F, G>(
|
||||
});
|
||||
}
|
||||
|
||||
// TRACES: FR-DEV-6
|
||||
// Whether a copied edit carries the crop. Off by default — see
|
||||
// `DevelopSettings::copy_includes_framing` for why that is the safe
|
||||
// direction rather than merely the conservative one.
|
||||
{
|
||||
let weak = window.as_weak();
|
||||
let ctl = controller.clone();
|
||||
window.on_settings_copy_includes_framing_toggled(move |on| {
|
||||
let Some(w) = weak.upgrade() else { return };
|
||||
ctl.edit(|s| s.develop.copy_includes_framing = on);
|
||||
render(&w, &ctl);
|
||||
});
|
||||
}
|
||||
|
||||
{
|
||||
let weak = window.as_weak();
|
||||
let ctl = controller.clone();
|
||||
|
||||
Reference in New Issue
Block a user