Albums: one-pass children map, before-itself is a no-op, more order tests

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>

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<AlbumId>) -> 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<Option<AlbumId>, Vec<&Album>> {
+        let mut map: std::collections::HashMap<Option<AlbumId>, 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<AlbumId>) -> 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)]

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<AlbumId> = map.get(&parent).map(|v| v.iter().map(|a| a.id).collect()).unwrap_or_default();
+        let direct: Vec<AlbumId> = 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() {

crates/engine/src/cmd/library.rs

@@ -893,13 +893,17 @@ pub fn specs() -> Vec<CommandSpec> {
             "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<AlbumId> = 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<CommandSpec> {
             "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")?;

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]