Merge PR #2825: LR-LIB-ALBUM: album commands skip photos the library lacks and record nothing when they add nothing

crates/engine/src/tests.rs: both test modules. docs: union.
This commit is contained in:
aiongg
2026-10-31 02:08:18 +00:00
8 changed files with 309 additions and 75 deletions
+14
View File
@@ -193,6 +193,20 @@ pub(crate) fn names_photos(p: &Value) -> bool {
["ids", "id"].iter().any(|k| p.get(k).is_some_and(|v| !v.is_null()))
}
/// The call names its target photos in `ids`, whatever else it carries in `id` (`album.addPhotos`'s `id` is the album).
pub(crate) fn names_photo_ids(p: &Value) -> bool {
p.get("ids").is_some_and(|v| !v.is_null())
}
/// For a command that opts into `explicit_targets` but whose `id` isn't a photo: [`Session::execute`] skipped the
/// selection check because the call carries an `id`, so the command makes it itself unless `ids` names the photos.
pub(crate) fn enabled_unless_ids(s: &Session, p: &Value, id: &str, enabled: Enabled) -> Result<()> {
if names_photo_ids(p) {
return Ok(());
}
enabled(s).map_err(|why| EngineError::Disabled(id.to_string(), why))
}
/// The photos `ids` names: `None` when it is missing or null (the command then falls back to the
/// selection, or its own default), else every entry as a photo id. An `ids` that isn't a list of
/// whole numbers is an error, never `None`: falling back would act on the selection instead of
+15 -3
View File
@@ -35,10 +35,22 @@ pub(super) fn strs(p: &Value, key: &str) -> Vec<String> {
/// The targets an album can hold and how many were browsed (Local) photos, which album views
/// never show (issue #684): those are skipped, and reported, instead of becoming invisible members.
/// An id the catalog doesn't have (named outright, or kept selected after an undo took its photo
/// away) is dropped before anything changes, and isn't counted: `skipped` stays the Local count
/// the toasts name.
pub(super) fn album_targets(s: &Session, targets: Vec<PhotoId>) -> (Vec<PhotoId>, usize) {
let before = targets.len();
let kept: Vec<PhotoId> = targets.into_iter().filter(|id| s.catalog.photo(*id).is_none_or(|p| !p.local)).collect();
let skipped = before - kept.len();
let mut skipped = 0;
let kept: Vec<PhotoId> = targets
.into_iter()
.filter(|id| match s.catalog.photo(*id) {
None => false,
Some(p) if p.local => {
skipped += 1;
false
}
Some(_) => true,
})
.collect();
(kept, skipped)
}
+99 -68
View File
@@ -1027,76 +1027,107 @@ pub fn specs() -> Vec<CommandSpec> {
Ok(json!({"changed": changed}))
}
),
cmd!(
"album.addPhotos",
"Add to Album",
["Photo"],
None,
"{id: albumId, ids?: [photoIds]} (default: selection) → {added, skipped}; Local photos are skipped (albums hold library photos), only Local ones is an error",
has_library_selection,
|s, p| {
let id = album_param(p, "id", "album.addPhotos")?;
let targets = crate::cmd::photo_ids(p, "album.addPhotos")?.unwrap_or_else(|| s.selected());
let al = s.catalog.album(id).ok_or_else(|| bad("album.addPhotos", "no such album"))?;
if al.is_smart() || al.folder {
return Err(bad("album.addPhotos", "smart albums and folders can't hold photos"));
}
let (targets, skipped) = album_targets_or_err(s, "album.addPhotos", targets)?;
let mut photos = al.photos.clone();
let before = photos.len();
// looked up, not searched for: adding thousands to a big album stays linear
let mut held: std::collections::HashSet<PhotoId> = photos.iter().copied().collect();
for t in targets {
if held.insert(t) {
photos.push(t);
CommandSpec {
explicit_targets: true,
..cmd!(
"album.addPhotos",
"Add to Album",
["Photo"],
None,
"{id: albumId, ids?: [photoIds]} (default: selection) → {added, skipped}; Local photos are skipped (albums hold library photos), only Local ones is an error; ids the library doesn't have are dropped; nothing new to add changes nothing (no undo step)",
has_library_selection,
|s, p| {
crate::cmd::enabled_unless_ids(s, p, "album.addPhotos", has_library_selection)?;
let id = album_param(p, "id", "album.addPhotos")?;
let targets = crate::cmd::photo_ids(p, "album.addPhotos")?.unwrap_or_else(|| s.selected());
let al = s.catalog.album(id).ok_or_else(|| bad("album.addPhotos", "no such album"))?;
if al.is_smart() || al.folder {
return Err(bad("album.addPhotos", "smart albums and folders can't hold photos"));
}
}
let added = photos.len() - before;
let cover = al.cover.or(photos.first().copied());
s.commit("Add to Album", Op::Batch { ops: vec![Op::SetAlbumPhotos { id, photos }, Op::SetAlbumCover { id, cover }] })?;
Ok(json!({"added": added, "skipped": skipped}))
}
),
cmd!(
"album.toggleTarget",
"Add to Target Album",
[],
None,
"{ids?} — B: adds the photos to the target album (the Quick Collection unless one is set), or removes them when they're all in it → {album, added, skipped}; Local photos are skipped",
has_library_selection,
|s, p| {
let targets =
album_targets_or_err(s, "album.toggleTarget", crate::cmd::photo_ids(p, "album.toggleTarget")?.unwrap_or_else(|| s.selected()))?;
let (targets, skipped) = targets;
let target = s.target_album.filter(|a| s.catalog.album(*a).is_some_and(|al| !al.is_smart() && !al.folder));
let id = match target.or_else(|| s.catalog.quick_collection()) {
Some(id) => id,
None => {
let id = s.catalog.alloc_album_id();
let album = Album { quick: true, ..Album::new(id, "Quick Collection") };
s.commit("New Quick Collection", Op::AddAlbum { album })?;
id
let (targets, skipped) = album_targets_or_err(s, "album.addPhotos", targets)?;
let mut photos = al.photos.clone();
let before = photos.len();
// looked up, not searched for: adding thousands to a big album stays linear
let mut held: std::collections::HashSet<PhotoId> = photos.iter().copied().collect();
for t in targets {
if held.insert(t) {
photos.push(t);
}
}
};
let al = s.catalog.album(id).ok_or_else(|| bad("album.toggleTarget", "no target album"))?;
// looked up, not searched for: thousands toggled in a big album stay linear
let mut held: std::collections::HashSet<PhotoId> = al.photos.iter().copied().collect();
let all_in = !targets.is_empty() && targets.iter().all(|t| held.contains(t));
let photos: Vec<PhotoId> = if all_in {
let targets: std::collections::HashSet<PhotoId> = targets.iter().copied().collect();
al.photos.iter().copied().filter(|x| !targets.contains(x)).collect()
} else {
al.photos.iter().copied().chain(targets.iter().copied().filter(|t| held.insert(*t))).collect()
};
let name = al.name.clone();
let cover = al.cover.filter(|c| photos.contains(c)).or(photos.first().copied());
s.commit(
if all_in { "Remove from Target Album" } else { "Add to Target Album" },
Op::Batch { ops: vec![Op::SetAlbumPhotos { id, photos }, Op::SetAlbumCover { id, cover }] },
)?;
Ok(json!({"album": id.0, "name": name, "added": !all_in, "count": s.catalog.album_count(id), "skipped": skipped}))
}
),
let added = photos.len() - before;
// all of them were there already (or none was left to add): the album is unchanged,
// so no undo step, and redo stays
if added == 0 {
return Ok(json!({"added": 0, "skipped": skipped}));
}
let cover = al.cover.or(photos.first().copied());
s.commit("Add to Album", Op::Batch { ops: vec![Op::SetAlbumPhotos { id, photos }, Op::SetAlbumCover { id, cover }] })?;
Ok(json!({"added": added, "skipped": skipped}))
}
)
},
CommandSpec {
explicit_targets: true,
..cmd!(
"album.toggleTarget",
"Add to Target Album",
[],
None,
"{ids?} — B: adds the photos to the target album (the Quick Collection unless one is set), or removes them when they're all in it → {album, added, toggled, skipped}; Local photos are skipped, ids the library doesn't have are dropped (none left: nothing changes and `added` is null)",
has_library_selection,
|s, p| {
crate::cmd::enabled_unless_ids(s, p, "album.toggleTarget", has_library_selection)?;
let targets = album_targets_or_err(
s,
"album.toggleTarget",
crate::cmd::photo_ids(p, "album.toggleTarget")?.unwrap_or_else(|| s.selected()),
)?;
let (targets, skipped) = targets;
// nothing left to toggle (every id named is gone): no album is made or changed
if targets.is_empty() {
let existing = s
.target_album
.filter(|a| s.catalog.album(*a).is_some_and(|al| !al.is_smart() && !al.folder))
.or_else(|| s.catalog.quick_collection());
let al = existing.and_then(|a| s.catalog.album(a));
return Ok(
json!({"album": al.map(|a| a.id.0), "name": al.map(|a| a.name.clone()), "added": null, "toggled": 0, "count": al.map_or(0, |a| s.catalog.album_count(a.id)), "skipped": skipped}),
);
}
let target = s.target_album.filter(|a| s.catalog.album(*a).is_some_and(|al| !al.is_smart() && !al.folder));
let id = match target.or_else(|| s.catalog.quick_collection()) {
Some(id) => id,
None => {
let id = s.catalog.alloc_album_id();
let album = Album { quick: true, ..Album::new(id, "Quick Collection") };
s.commit("New Quick Collection", Op::AddAlbum { album })?;
id
}
};
let al = s.catalog.album(id).ok_or_else(|| bad("album.toggleTarget", "no target album"))?;
// looked up, not searched for: thousands toggled in a big album stay linear
let mut held: std::collections::HashSet<PhotoId> = al.photos.iter().copied().collect();
let all_in = targets.iter().all(|t| held.contains(t));
let photos: Vec<PhotoId> = if all_in {
let targets: std::collections::HashSet<PhotoId> = targets.iter().copied().collect();
al.photos.iter().copied().filter(|x| !targets.contains(x)).collect()
} else {
al.photos.iter().copied().chain(targets.iter().copied().filter(|t| held.insert(*t))).collect()
};
let name = al.name.clone();
// photos that went in or came out: what a toast can say (not the ones that were in already)
let toggled = al.photos.len().abs_diff(photos.len());
let cover = al.cover.filter(|c| photos.contains(c)).or(photos.first().copied());
s.commit(
if all_in { "Remove from Target Album" } else { "Add to Target Album" },
Op::Batch { ops: vec![Op::SetAlbumPhotos { id, photos }, Op::SetAlbumCover { id, cover }] },
)?;
Ok(
json!({"album": id.0, "name": name, "added": !all_in, "toggled": toggled, "count": s.catalog.album_count(id), "skipped": skipped}),
)
}
)
},
cmd!("album.setTarget", "Set as Target Album", [], None, "{id: albumId | null} (null = the Quick Collection)", always, |s, p| {
s.target_album = match p.get("id").and_then(Value::as_u64) {
Some(a) => {
+1
View File
@@ -3,6 +3,7 @@ use serde_json::{Value, json};
use crate::{Selection, Session, command_specs};
mod album_loops;
mod album_missing;
mod album_order;
mod album_remove;
mod albums_local;
+167
View File
@@ -0,0 +1,167 @@
//! Album commands only take photos the catalog has, and a call that changes nothing records
//! nothing.
//!
//! Scenarios:
//! - Given ids the catalog doesn't have (named outright, or still selected after an undo took the
//! photos away), when they are added to an album, toggled into the target album, or used for a
//! new album, then they reach no album.
//! - Given nothing is left to add, or every photo is in the album already, then there is no undo
//! step and redo is left alone.
//! - Given a normal add or toggle, then one step is recorded and one undo restores the album.
use lightcraft_catalog::{AlbumId, PhotoId};
use serde_json::json;
use crate::{Selection, Session};
const GONE: PhotoId = PhotoId(987_654);
fn album(s: &mut Session, name: &str) -> AlbumId {
AlbumId(s.execute("album.create", &json!({"name": name})).unwrap()["id"].as_u64().unwrap())
}
/// A demo session, an album, and a redo entry waiting (a rating set, then undone).
fn with_redo() -> (Session, AlbumId, PhotoId) {
let mut s = Session::with_demo();
let photo = s.catalog.photos().next().unwrap().id;
let a = album(&mut s, "Trip");
s.execute("photo.rate", &json!({"ids": [photo.0], "rating": 4})).unwrap();
s.execute("edit.undo", &json!({})).unwrap();
assert_eq!(s.redo.len(), 1);
(s, a, photo)
}
#[test]
fn photos_the_catalog_lacks_reach_no_album() {
let (mut s, a, photo) = with_redo();
let (undo, redo) = (s.undo.len(), s.redo.len());
let r = s.execute("album.addPhotos", &json!({"id": a.0, "ids": [GONE.0]})).unwrap();
assert_eq!((r["added"].as_u64(), r["skipped"].as_u64()), (Some(0), Some(0)), "{r}");
assert!(s.catalog.album(a).unwrap().photos.is_empty());
assert_eq!((s.undo.len(), s.redo.len()), (undo, redo), "no step, redo kept");
// a mix: the one that is there goes in, the other doesn't
let r = s.execute("album.addPhotos", &json!({"id": a.0, "ids": [GONE.0, photo.0]})).unwrap();
assert_eq!(r["added"], 1, "{r}");
assert_eq!(s.catalog.album(a).unwrap().photos, vec![photo]);
assert_eq!(s.undo.len(), undo + 1);
assert_eq!(s.redo.len(), 0, "a real change clears redo");
// toggling: nothing is made or changed for ids that are gone
let r = s.execute("album.toggleTarget", &json!({"ids": [GONE.0]})).unwrap();
assert!(r["added"].is_null() && r["toggled"] == 0, "{r}");
assert!(s.catalog.quick_collection().is_none(), "no Quick Collection for nothing");
assert_eq!(s.undo.len(), undo + 1);
// a new album from a selection that is gone starts empty
s.selection = Selection::single(GONE);
let r = s.execute("album.create", &json!({"name": "New", "addSelected": true})).unwrap();
let new = s.catalog.album(AlbumId(r["id"].as_u64().unwrap())).unwrap();
assert!(new.photos.is_empty() && new.cover.is_none(), "{r}");
}
/// Leaves one redo entry waiting: a rating set, then undone.
fn leave_redo(s: &mut Session, photo: PhotoId) {
s.execute("photo.rate", &json!({"ids": [photo.0], "rating": 4})).unwrap();
s.execute("edit.undo", &json!({})).unwrap();
assert_eq!(s.redo.len(), 1);
}
#[test]
fn toggling_gone_photos_leaves_the_target_album_and_redo_alone() {
let mut s = Session::with_demo();
let photo = s.catalog.photos().next().unwrap().id;
let a = album(&mut s, "Trip");
s.execute("album.setTarget", &json!({"id": a.0})).unwrap();
s.execute("album.toggleTarget", &json!({"ids": [photo.0]})).unwrap();
leave_redo(&mut s, photo);
let undo = s.undo.len();
let r = s.execute("album.toggleTarget", &json!({"ids": [GONE.0]})).unwrap();
assert_eq!(r["album"].as_u64(), Some(a.0), "{r}");
assert!(r["added"].is_null() && r["toggled"] == 0, "{r}");
assert_eq!(s.catalog.album(a).unwrap().photos, vec![photo]);
assert_eq!((s.undo.len(), s.redo.len()), (undo, 1));
}
#[test]
fn adding_photos_that_are_all_there_already_records_nothing() {
let mut s = Session::with_demo();
let photo = s.catalog.photos().next().unwrap().id;
let a = album(&mut s, "Trip");
s.execute("album.addPhotos", &json!({"id": a.0, "ids": [photo.0]})).unwrap();
leave_redo(&mut s, photo);
let undo = s.undo.len();
let r = s.execute("album.addPhotos", &json!({"id": a.0, "ids": [photo.0]})).unwrap();
assert_eq!(r["added"], 0, "{r}");
assert_eq!((s.undo.len(), s.redo.len()), (undo, 1), "no step, redo kept");
}
#[test]
fn a_normal_add_or_toggle_is_one_step_and_one_undo_restores_it() {
let (mut s, a, photo) = with_redo();
let undo = s.undo.len();
s.execute("album.addPhotos", &json!({"id": a.0, "ids": [photo.0]})).unwrap();
assert_eq!(s.undo.len(), undo + 1);
s.execute("edit.undo", &json!({})).unwrap();
assert!(s.catalog.album(a).unwrap().photos.is_empty());
s.execute("album.setTarget", &json!({"id": a.0})).unwrap();
let undo = s.undo.len();
let r = s.execute("album.toggleTarget", &json!({"ids": [GONE.0, photo.0]})).unwrap();
assert_eq!((r["added"].as_bool(), r["toggled"].as_u64(), s.undo.len()), (Some(true), Some(1), undo + 1), "{r}");
s.execute("edit.undo", &json!({})).unwrap();
assert!(s.catalog.album(a).unwrap().photos.is_empty());
s.execute("edit.redo", &json!({})).unwrap();
// toggled off: also one step, one undo puts the photo back
let undo = s.undo.len();
let r = s.execute("album.toggleTarget", &json!({"ids": [photo.0]})).unwrap();
assert_eq!((r["added"].as_bool(), r["toggled"].as_u64(), s.undo.len()), (Some(false), Some(1), undo + 1), "{r}");
assert!(s.catalog.album(a).unwrap().photos.is_empty());
s.execute("edit.undo", &json!({})).unwrap();
assert_eq!(s.catalog.album(a).unwrap().photos, vec![photo]);
}
#[test]
fn a_toggle_counts_only_the_photos_it_added() {
let mut s = Session::with_demo();
let ids: Vec<PhotoId> = s.catalog.photos().take(3).map(|p| p.id).collect();
let a = album(&mut s, "Trip");
s.execute("album.setTarget", &json!({"id": a.0})).unwrap();
s.execute("album.addPhotos", &json!({"id": a.0, "ids": [ids[0].0, ids[1].0]})).unwrap();
let all: Vec<u64> = ids.iter().map(|p| p.0).collect();
let r = s.execute("album.toggleTarget", &json!({"ids": all})).unwrap();
assert_eq!((r["added"].as_bool(), r["toggled"].as_u64()), (Some(true), Some(1)), "two were in already: {r}");
assert_eq!(s.catalog.album(a).unwrap().photos.len(), 3);
}
#[test]
fn photos_named_outright_reach_the_album_commands_with_nothing_selected() {
let mut s = Session::with_demo();
let photo = s.catalog.photos().next().unwrap().id;
let a = album(&mut s, "Trip");
s.execute("album.setTarget", &json!({"id": a.0})).unwrap();
s.selection = Selection::default();
// ids the library lacks: the no-op result, not a refusal
let r = s.execute("album.addPhotos", &json!({"id": a.0, "ids": [GONE.0]})).unwrap();
assert_eq!((r["added"].as_u64(), r["skipped"].as_u64()), (Some(0), Some(0)), "{r}");
let r = s.execute("album.toggleTarget", &json!({"ids": [GONE.0]})).unwrap();
assert!(r["added"].is_null(), "{r}");
// ids the library has: they go in
let r = s.execute("album.addPhotos", &json!({"id": a.0, "ids": [photo.0]})).unwrap();
assert_eq!(r["added"], 1, "{r}");
let r = s.execute("album.toggleTarget", &json!({"ids": [photo.0]})).unwrap();
assert_eq!((r["added"].as_bool(), r["toggled"].as_u64()), (Some(false), Some(1)), "{r}");
}
#[test]
fn the_album_an_add_names_is_not_a_photo_that_skips_the_selection_check() {
let mut s = Session::with_demo();
let a = album(&mut s, "Trip");
s.selection = Selection::default();
// no `ids`: the selection is what is added, and it is empty
assert!(matches!(s.execute("album.addPhotos", &json!({"id": a.0})), Err(crate::EngineError::Disabled(..))));
assert!(matches!(s.execute("album.addPhotos", &json!({"id": a.0, "ids": null})), Err(crate::EngineError::Disabled(..))));
assert!(matches!(s.execute("album.toggleTarget", &json!({})), Err(crate::EngineError::Disabled(..))));
}
+3 -2
View File
@@ -727,8 +727,9 @@ fn brush_key(app: &mut LightcraftApp, ctx: &egui::Context) -> bool {
if !library_grid(app) {
return false;
}
if let Ok(r) = app.run("album.toggleTarget", json!({})) {
let n = app.session.selected().len();
// `added` is null when nothing was left to toggle (the selected photos are gone): nothing to say
if let Some(r) = app.run("album.toggleTarget", json!({})).ok().filter(|r| r["added"].is_boolean()) {
let n = r["toggled"].as_u64().unwrap_or(0);
let what = crate::i18n::tr_format!("{n} photo{}", if n == 1 { "" } else { "s" }, n = n);
let name = r["name"].as_str().unwrap_or("Quick Collection").to_string();
app.toast(
File diff suppressed because one or more lines are too long
+7
View File
@@ -45,6 +45,13 @@
the sidebar or on any other row or thumbnail that has one. Before, the menu opened on the row the photos were over.
The menu of a text field (Cut, Copy, Paste) is not held back.
### Adding photos that are already in an album no longer costs an undo step
- Add to Album and dragging photos to an album now leave the album, the undo history and Redo alone when every
photo was in the album already. Before, they still recorded an undo step, so Redo was lost and Undo had a step
that changed nothing. Photos the library no longer has (a selection kept after an Undo took them away) are also
left out of a new album made from the selection, out of Add to Album, and out of the target album (B), which does
nothing and says nothing when none of them is left, and counts only the photos it changed.
## 2026-10-23
### Videos in the grid are no longer sent to the image decoders