diff --git a/crates/catalog/src/lib.rs b/crates/catalog/src/lib.rs index 8a19b6e22..884ec6583 100644 --- a/crates/catalog/src/lib.rs +++ b/crates/catalog/src/lib.rs @@ -292,9 +292,19 @@ impl Catalog { /// without one after, by name), else by name. pub fn album_children(&self, parent: Option) -> Vec<&Album> { let mut kids: Vec<&Album> = self.albums.values().filter(|a| a.parent == parent).collect(); - kids.sort_by_cached_key(|a| (!a.folder, a.order.is_none(), a.order.unwrap_or(0), a.name.to_lowercase(), a.id)); + kids.sort_by_cached_key(|a| album_order_key(a)); kids } + /// [`Self::album_children`] of every folder (and of the top level, under `None`) in one pass: + /// what a tree view that draws all of them wants each frame. + pub fn album_children_by_parent(&self) -> std::collections::HashMap, Vec<&Album>> { + let mut map: std::collections::HashMap, Vec<&Album>> = std::collections::HashMap::new(); + for a in self.albums.values() { + map.entry(a.parent).or_default().push(a); + } + map.values_mut().for_each(|kids| kids.sort_by_cached_key(|a| album_order_key(a))); + map + } /// Whether anything inside `parent` has a place of its own (the folder is ordered by hand). pub fn album_children_are_ordered(&self, parent: Option) -> bool { self.albums.values().any(|a| a.parent == parent && a.order.is_some()) @@ -698,6 +708,12 @@ impl Catalog { } } +/// How siblings are listed: folders first, then the ones with a place of their own by it, then by +/// name; the id settles any tie. +fn album_order_key(a: &Album) -> (bool, bool, u32, String, AlbumId) { + (!a.folder, a.order.is_none(), a.order.unwrap_or(0), a.name.to_lowercase(), a.id) +} + #[cfg(test)] mod tests; #[cfg(test)] diff --git a/crates/catalog/src/tests_album_order.rs b/crates/catalog/src/tests_album_order.rs index dfd463f9c..a6fb597b8 100644 --- a/crates/catalog/src/tests_album_order.rs +++ b/crates/catalog/src/tests_album_order.rs @@ -57,6 +57,26 @@ fn ordered_children_follow_their_order() { assert_eq!(names(&c, Some(f)), ["Z", "Y"]); } +/// The one-pass map lists every folder's children exactly as `album_children` does. +#[test] +fn the_children_map_matches_the_children_of_each_folder() { + let mut c = Catalog::new(); + let f = add(&mut c, "F", None, true); + let g = add(&mut c, "G", Some(f), true); + for (name, parent) in [("b", None), ("a", None), ("y", Some(f)), ("x", Some(f)), ("q", Some(g))] { + add(&mut c, name, parent, false); + } + let x = c.albums().find(|a| a.name == "x").unwrap().id; + place(&mut c, x, Some(7)); + let map = c.album_children_by_parent(); + for parent in [None, Some(f), Some(g)] { + let from_map: Vec = map.get(&parent).map(|v| v.iter().map(|a| a.id).collect()).unwrap_or_default(); + let direct: Vec = c.album_children(parent).iter().map(|a| a.id).collect(); + assert_eq!(from_map, direct, "{parent:?}"); + } + assert!(map.get(&Some(AlbumId(999))).is_none()); +} + /// A place is one undo step, and `None` puts the album back among the alphabetical ones. #[test] fn a_place_can_be_set_cleared_and_undone() { diff --git a/crates/engine/src/cmd/library.rs b/crates/engine/src/cmd/library.rs index 11f665898..d900eacff 100644 --- a/crates/engine/src/cmd/library.rs +++ b/crates/engine/src/cmd/library.rs @@ -893,13 +893,17 @@ pub fn specs() -> Vec { "Reorder Album", [], None, - "{id, parent?: folderId|null, before?: albumId|null} — places the album (or folder) in `parent` (default: where it is; null: the top level) before the sibling `before` of the same kind, or last of its kind; the folder is then ordered by hand. One undo step. → {changed}", + "{id, parent?: folderId|null, before?: albumId|null} — places the album (or folder) in `parent` (default: where it is; null: the top level) before the sibling `before` of the same kind, or last of its kind; the folder is then ordered by hand. One undo step. → {changed}: how many edits it took (0: already there)", always, |s, p| { let id = album_param(p, "id", "album.reorder")?; let me = s.catalog.album(id).ok_or_else(|| bad("album.reorder", "no such album"))?; let parent = if p.get("parent").is_some() { opt_album_param(p, "parent", "album.reorder")? } else { me.parent }; let before = opt_album_param(p, "before", "album.reorder")?; + if before == Some(id) { + // before itself: where it is + return Ok(json!({"changed": 0})); + } let mut order: Vec = s.catalog.album_children(parent).into_iter().filter(|a| a.id != id).map(|a| a.id).collect(); let at = match before { Some(b) => { @@ -939,7 +943,7 @@ pub fn specs() -> Vec { "Sort Albums by Name", [], None, - "{parent?: folderId|null} — drops the hand order of the albums inside the folder (default: the top level), so they are listed by name → {changed}", + "{parent?: folderId|null} — drops the hand order of the albums inside the folder (default: the top level), so they are listed by name → {changed}: how many albums lost their place", always, |s, p| { let parent = opt_album_param(p, "parent", "album.sort")?; diff --git a/crates/engine/src/tests_album_order.rs b/crates/engine/src/tests_album_order.rs index 324c0db5c..cff524cbe 100644 --- a/crates/engine/src/tests_album_order.rs +++ b/crates/engine/src/tests_album_order.rs @@ -37,8 +37,8 @@ fn an_album_is_placed_before_a_sibling_or_last() { assert_eq!(names(&s, None), ["A", "B", "C"]); reorder(&mut s, a, json!({})).unwrap(); assert_eq!(names(&s, None), ["B", "C", "A"], "no `before`: last"); - reorder(&mut s, b, json!({"before": b})).expect_err("not before itself"); - let _ = (a, b); + assert_eq!(reorder(&mut s, b, json!({"before": b})).unwrap()["changed"], 0, "before itself is where it is"); + assert_eq!(names(&s, None), ["B", "C", "A"]); } /// Folders stay ahead of albums: a folder is placed among folders, an album among albums. @@ -74,6 +74,33 @@ fn placing_across_folders_moves_it_and_undoes_in_one_step() { assert_eq!(s.catalog.album(AlbumId(loose)).unwrap().order, None); } +/// Undo and redo of placing across folders, of sorting, and of moving a folder that has children. +#[test] +fn placing_and_sorting_undo_and_redo() { + let mut s = Session::new(); + let top = make(&mut s, "Top", None, true); + let child = make(&mut s, "Child", Some(top), false); + let (a, b) = (make(&mut s, "A", None, false), make(&mut s, "B", None, false)); + reorder(&mut s, b, json!({"before": a})).unwrap(); + // a folder with children, placed inside another folder + let other = make(&mut s, "Other", None, true); + reorder(&mut s, top, json!({"parent": other})).unwrap(); + assert_eq!(names(&s, Some(other)), ["Top"]); + assert_eq!(names(&s, Some(top)), ["Child"], "its children came along"); + s.execute("edit.undo", &json!({})).unwrap(); + assert_eq!(s.catalog.album(AlbumId(top)).unwrap().parent, None); + s.execute("edit.redo", &json!({})).unwrap(); + assert_eq!(s.catalog.album(AlbumId(top)).unwrap().parent, Some(AlbumId(other))); + // sort, undo, redo + s.execute("album.sort", &json!({})).unwrap(); + assert_eq!(names(&s, None), ["Other", "A", "B"]); + s.execute("edit.undo", &json!({})).unwrap(); + assert_eq!(names(&s, None), ["Other", "B", "A"], "the hand order is back"); + s.execute("edit.redo", &json!({})).unwrap(); + assert_eq!(names(&s, None), ["Other", "A", "B"]); + assert_eq!(s.catalog.album(AlbumId(child)).unwrap().parent, Some(AlbumId(top))); +} + /// Bad requests change nothing: a `before` that is not in the target folder, a target that is not /// a folder or lies inside the album, an unknown album. #[test]