dtourolle/DarkRoom
1
0
Fork 0
Code Issues 39 Pull Requests Actions Packages Projects Releases 20 Wiki Activity

Keep an edit under a name, not just on the clipboard

Browse Source 
FR-DEV-6 asks for three things — named presets, copy/paste between
images, and batch-apply to a selection. The last two have been here for
a while; this is the first.

The format is the sidecar's, deliberately. A preset *is* the non-default
half of a version, so the lines are the same lines keyed the same way,
which makes the two files diffable against each other and lets someone
debugging an edit paste a block from one into the other. One file rather
than one per preset: a preset per file makes the name a path, and every
name then has to survive a filesystem — a `/` becomes a directory, a
name differing only in case collides on one platform and not another,
and renaming becomes two operations that can half-fail. As a key in a
document it is none of those.

Unknown *parameters* needed no machinery. `Preset` already holds
whatever keys it is given and resolves them against the descriptors only
at apply time, so one written by a newer build survives by being stored.
Only lines that are not `op.param = float` at all are preserved
verbatim, which is the sidecar's version-skew promise made here too.

Applying is the paste path with a different source, so a preset reaches
a selection through the sidecar read-modify-write that was already
there: no graph, no decode, no GPU, forty files or one.

Two smaller decisions worth the record. A library that fails to parse is
held empty in memory and *not* written back over — settings regenerate
themselves and this is work, so a parse failure must not be the moment
it is destroyed. And every save persists immediately and rolls the
in-memory copy back if the write fails, so the sheet never lists a
preset the file does not have.

The grid's "Presets" button is gated on the selection alone, unlike the
"Paste to 40" beside it. That button needs a clipboard armed this
session; the preset list is whatever was saved last month, and hiding it
behind an unrelated action is what makes a feature only its author knows
about.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
dtourolle
2026-08-29 20:07:14 +02:00
co-authored by Claude Opus 5
parent 5133e53bc8
commit 5a8327824f
10 changed files with 1502 additions and 47 deletions
Download Patch File Download Diff File Expand all files Collapse all files
ui/dr-ui/src/presets.rs
+421 -6
View File
@@ -35,10 +35,11 @@ use std::cell::RefCell;
use std::path::{Path, PathBuf};
use std::rc::Rc;
use dr_pipeline::{Preset, Scope, Sidecar};
use dr_pipeline::{NameError, Preset, PresetLibrary, Scope, Sidecar};
use slint::ComponentHandle;
use crate::develop::DevelopSession;
use crate::preset_store::PresetStore;
use crate::{library, library_ui, settings_ui, AppWindow, ParamRow};
/// Where the develop view's current edit is stored.
@@ -121,11 +122,21 @@ impl Clipboard {
let Some(preset) = self.preset.borrow().clone() else {
return String::new();
};
match preset.op_count(scope) {
0 => "Neutral".to_string(),
1 => "1 adjustment".to_string(),
n => format!("{n} adjustments"),
}
describe(&preset, scope)
}
}
/// A short description of a preset's contents, for a label.
///
/// Free rather than a method on [`Clipboard`] because the named-preset sheet
/// describes what *saving* would capture, and a second phrasing of the same
/// count is a second thing to keep in step — the sheet saying "3 settings"
/// beside a panel saying "3 adjustments" would read as two different numbers.
pub fn describe(preset: &Preset, scope: Scope) -> String {
match preset.op_count(scope) {
0 => "Neutral".to_string(),
1 => "1 adjustment".to_string(),
n => format!("{n} adjustments"),
}
}
@@ -528,6 +539,308 @@ pub fn wire(
}
}
// ---------------------------------------------------------------------------
// Named presets (FR-DEV-6)
// ---------------------------------------------------------------------------
/// TRACES: FR-DEV-6
/// The saved preset library, and the file it lives in.
///
/// # Why the library is held in memory as well as on disk
///
/// Every change writes the whole file (see [`PresetStore`]), so the in-memory
/// copy is what the sheet is drawn from and what the next edit is applied to.
/// Reading the file back after each change would be the same bytes and one
/// more chance for a failed read to empty a list the user is looking at.
///
/// # A failed save is reported, not swallowed
///
/// The clipboard cannot fail — it is memory. This can: a full disk, a config
/// directory that is not writable. Losing a preset the user just named, with
/// the sheet cheerfully listing it, would be discovered at the worst possible
/// moment, so the write's result reaches the window.
pub struct NamedPresets {
store: PresetStore,
library: RefCell<PresetLibrary>,
}
impl NamedPresets {
/// Load the library from its usual place.
pub fn open() -> Rc<Self> {
Self::at(PresetStore::open())
}
/// Load from an explicit store — for tests, and for a non-default location.
#[cfg(test)]
pub fn open_at(path: PathBuf) -> Rc<Self> {
Self::at(PresetStore::open_at(path))
}
fn at(store: PresetStore) -> Rc<Self> {
let library = RefCell::new(store.load());
Rc::new(Self { store, library })
}
/// The stored names, in the order they are written.
pub fn names(&self) -> Vec<String> {
self.library.borrow().names().map(String::from).collect()
}
/// The preset stored under `name`.
pub fn get(&self, name: &str) -> Option<Preset> {
self.library.borrow().get(name).cloned()
}
/// Store `preset` under `name` and persist.
///
/// The in-memory library is updated first and rolled back if the write
/// fails, so what the sheet lists is always what is on disk. The
/// alternative — writing first — would mean holding a preset the file does
/// not have on every failure path.
fn insert(&self, name: &str, preset: Preset) -> Result<(), SaveError> {
let previous = {
let mut library = self.library.borrow_mut();
let existing = library.get(name.trim()).cloned();
library.insert(name, preset).map_err(SaveError::Name)?;
existing
};
self.persist(|library| match previous {
Some(p) => {
let _ = library.insert(name, p);
}
None => {
library.remove(name.trim());
}
})
}
/// Save the library, undoing the in-memory change if the write fails.
fn persist(&self, rollback: impl FnOnce(&mut PresetLibrary)) -> Result<(), SaveError> {
let result = self.store.save(&self.library.borrow());
match result {
Ok(()) => Ok(()),
Err(e) => {
rollback(&mut self.library.borrow_mut());
// Named, the way the settings page names its file: "could not
// save" without saying where leaves the user nothing to check
// and nothing to fix.
Err(SaveError::Write(format!(
"{}: {e}",
self.store.path().display()
)))
}
}
}
}
/// Why a preset could not be saved.
enum SaveError {
/// The name itself was refused.
Name(NameError),
/// The library could not be written.
Write(String),
}
impl SaveError {
/// What to put in front of the user.
///
/// The prose lives here rather than in `dr-pipeline`, which depends on
/// nothing and has no business holding user-facing strings.
fn message(&self) -> String {
match self {
Self::Name(NameError::Empty) => "Give the preset a name.".to_string(),
Self::Name(NameError::Unrepresentable) => {
"A preset name cannot contain brackets or line breaks.".to_string()
}
Self::Write(e) => format!("Could not save presets: {e}"),
}
}
}
/// Push the stored names onto the window.
pub fn render_named(window: &AppWindow, named: &Rc<NamedPresets>) {
let names: Vec<slint::SharedString> = named.names().into_iter().map(Into::into).collect();
window.set_preset_names(slint::ModelRc::new(slint::VecModel::from(names)));
}
/// Wire saving, applying, renaming and deleting named presets.
///
/// Takes the same [`Develop`] bundle the clipboard wiring does, and for the
/// same reason: applying a preset to the open image changes the graph, so it
/// has to rebuild the panel, redraw the canvas and know where to save.
pub fn wire_named(
window: &AppWindow,
named: Rc<NamedPresets>,
develop: Develop,
settings: Rc<settings_ui::SettingsController>,
library: Rc<library_ui::LibraryController>,
collections: Rc<crate::collections_ui::CollectionsController>,
) {
let Develop {
session,
rows,
redraw,
open,
} = develop;
render_named(window, &named);
// --- save the open edit under a name ---------------------------------
{
let weak = window.as_weak();
let named = named.clone();
let session = session.clone();
window.on_save_preset(move |name| {
let Some(w) = weak.upgrade() else { return };
let Some(preset) = session.borrow().as_ref().map(|s| s.copy_settings()) else {
w.set_preset_name_error("Open a photograph first.".into());
return;
};
// Captured at full scope, exactly as a copy is: the scope is a
// decision about applying, and a preset that had already discarded
// the crop could never grow it back (see `Preset::capture`).
match named.insert(&name, preset) {
Ok(()) => {
w.set_preset_name_error(Default::default());
render_named(&w, &named);
}
Err(e) => w.set_preset_name_error(e.message().into()),
}
});
}
// A refusal the user has started correcting is stale, and a message that
// outlives its cause is one the user learns to ignore.
{
let weak = window.as_weak();
window.on_preset_name_edited(move |_| {
if let Some(w) = weak.upgrade() {
w.set_preset_name_error(Default::default());
}
});
}
// --- what saving would capture ----------------------------------------
{
let weak = window.as_weak();
let session = session.clone();
window.on_presets_opened(move || {
let Some(w) = weak.upgrade() else { return };
// At the scope a save would use, which is full: a preset keeps
// the framing it was captured with and drops it at apply time.
let summary = session
.borrow()
.as_ref()
.map(|s| describe(&s.copy_settings(), Scope::Everything))
.unwrap_or_default();
w.set_preset_capture_summary(summary.into());
});
}
// --- apply ------------------------------------------------------------
//
// One callback for both targets. Which one is meant is not a guess: the
// sheet was opened from a view that set `preset-apply-count`, and the
// label the user just read said "Applies to 12 selected photographs" or
// said nothing. Deciding here from the same number keeps the promise.
{
let weak = window.as_weak();
let named = named.clone();
let settings = settings.clone();
let library = library.clone();
let collections = collections.clone();
let session = session.clone();
let rows = rows.clone();
let redraw = redraw.clone();
let open = open.clone();
window.on_apply_preset(move |name| {
let Some(w) = weak.upgrade() else { return };
let Some(preset) = named.get(&name) else {
// Another window may have deleted it since this list was drawn.
render_named(&w, &named);
return;
};
let scope = scope_for(&settings.snapshot());
if w.get_preset_apply_count() > 0 {
library_ui::paste_settings_to_selection(
&w,
&library,
&collections.selected(),
&preset,
scope,
);
} else {
{
let mut slot = session.borrow_mut();
let Some(s) = slot.as_mut() else { return };
s.apply_settings(&preset, scope);
}
// The same three steps a paste takes, for the same reasons:
// many controls moved without any of them being touched, and
// a deliberate discrete action is saved immediately.
crate::sync_rows(&w, &rows, &session);
redraw(&w);
save_open_edit(&w, &open.borrow(), &session, &library);
}
w.set_presets_open(false);
});
}
// --- rename -----------------------------------------------------------
{
let weak = window.as_weak();
let named = named.clone();
window.on_rename_preset(move |from, to| {
let Some(w) = weak.upgrade() else { return };
let renamed = {
let mut library = named.library.borrow_mut();
library.rename(&from, &to)
};
match renamed {
Ok(_) => {
// Nothing to roll back to on a failed write beyond the
// name it had, which is what this restores.
let from = from.to_string();
let to = to.to_string();
if let Err(e) = named.persist(move |library| {
let _ = library.rename(&to, &from);
}) {
w.set_preset_name_error(e.message().into());
}
}
Err(e) => w.set_preset_name_error(SaveError::Name(e).message().into()),
}
render_named(&w, &named);
});
}
// --- delete -----------------------------------------------------------
{
let weak = window.as_weak();
let named = named.clone();
window.on_delete_preset(move |name| {
let Some(w) = weak.upgrade() else { return };
let removed = {
let mut library = named.library.borrow_mut();
let previous = library.get(&name).cloned();
library.remove(&name);
previous
};
if let Some(previous) = removed {
let name = name.to_string();
if let Err(e) = named.persist(move |library| {
let _ = library.insert(&name, previous);
}) {
w.set_preset_name_error(e.message().into());
}
}
render_named(&w, &named);
});
}
}
/// Seconds since the epoch, or zero if the clock is before it.
///
/// Zero rather than a panic: a wrong timestamp costs a tie-break in the merge,
@@ -804,4 +1117,106 @@ mod tests {
assert!(clipboard.is_armed());
assert_eq!(clipboard.describe(Scope::Adjustments), "Neutral");
}
// -----------------------------------------------------------------------
// Named presets
// -----------------------------------------------------------------------
fn named(name: &str) -> (Rc<NamedPresets>, PathBuf) {
let dir = tempdir(name);
(NamedPresets::open_at(dir.join("presets.drpl")), dir)
}
#[test]
fn a_saved_preset_is_on_disk_before_the_call_returns() {
// Not on the way out, and not on a timer: a preset the user named and
// then lost to a crash is the one failure this feature cannot have.
let (presets, dir) = named("saved-immediately");
presets
.insert("Warm", Preset::capture(&edited()))
.ok()
.expect("saved");
let reloaded = NamedPresets::open_at(dir.join("presets.drpl"));
assert_eq!(reloaded.names(), vec!["Warm".to_string()]);
}
#[test]
fn a_saved_preset_carries_the_edit_it_captured() {
let (presets, _dir) = named("carries-the-edit");
presets.insert("Warm", Preset::capture(&edited())).ok();
let preset = presets.get("Warm").expect("stored");
let mut target = EditGraph::default_chain();
preset.apply(&mut target, Scope::Adjustments);
assert_eq!(
target.param(
dr_pipeline::ops::exposure::ID,
dr_pipeline::ops::exposure::EXPOSURE
),
Some(1.5)
);
}
#[test]
fn a_name_that_cannot_be_stored_is_refused_rather_than_mangled() {
let (presets, _dir) = named("refused-name");
assert!(presets.insert("", Preset::default()).is_err());
assert!(presets.insert("bracket]", Preset::default()).is_err());
assert!(presets.names().is_empty());
}
#[test]
fn a_write_that_fails_leaves_the_list_showing_what_is_on_disk() {
// The rollback. A sheet listing a preset the file does not have is a
// loss the user discovers later, at the moment they reach for it.
let dir = tempdir("failed-write");
// A *file* where the store wants a directory, so `create_dir_all`
// fails and the save cannot succeed.
let blocked = dir.join("blocked");
std::fs::write(&blocked, b"not a directory").unwrap();
let presets = NamedPresets::open_at(blocked.join("presets.drpl"));
assert!(presets.insert("Warm", Preset::default()).is_err());
assert!(
presets.names().is_empty(),
"the failed save left a preset behind"
);
}
#[test]
fn a_failed_overwrite_puts_the_original_back() {
// The other half of the rollback, and the one that loses work if it is
// wrong: overwriting is destructive, so a failed overwrite has to
// restore what was there rather than leave the name holding the new
// value the file never received.
use std::os::unix::fs::PermissionsExt;
let dir = tempdir("failed-overwrite");
let path = dir.join("presets.drpl");
let presets = NamedPresets::open_at(path.clone());
presets.insert("Warm", Preset::capture(&edited())).ok();
let original = presets.get("Warm").expect("stored");
// The directory exists, so `create_dir_all` still succeeds and it is
// the write of the temporary file that fails — which is the path a
// full disk takes.
let mut perms = std::fs::metadata(&dir).unwrap().permissions();
perms.set_mode(0o500);
std::fs::set_permissions(&dir, perms.clone()).unwrap();
let failed = presets.insert("Warm", Preset::default()).is_err();
// Restore the permissions before asserting, so a failure here does not
// leave an undeletable directory behind for the next run.
perms.set_mode(0o700);
std::fs::set_permissions(&dir, perms).unwrap();
assert!(failed, "the write should have failed");
assert_eq!(
presets.get("Warm"),
Some(original),
"the failed overwrite kept the new value"
);
}
}