Fix merging for albums

src/core/collection/album.rs

@@ -1,7 +1,7 @@
 use std::mem;
 
 use crate::core::collection::{
-    merge::{Merge, MergeSorted},
+    merge::{Merge, MergeSorted, WithId},
     track::Track,
 };
 
@@ -14,6 +14,14 @@ pub struct Album {
     pub tracks: Vec<Track>,
 }
 
+impl WithId for Album {
+    type Id = AlbumId;
+
+    fn id(&self) -> &Self::Id {
+        &self.id
+    }
+}
+
 /// The album identifier.
 #[derive(Clone, Debug, PartialEq, PartialOrd, Ord, Eq, Hash)]
 pub struct AlbumId {
@@ -29,6 +37,16 @@ pub struct AlbumDate {
     pub day: u8,
 }
 
+impl Default for AlbumDate {
+    fn default() -> Self {
+        AlbumDate {
+            year: 0,
+            month: AlbumMonth::None,
+            day: 0,
+        }
+    }
+}
+
 /// The album's sequence to determine order when two or more albums have the same release date.
 #[derive(Clone, Copy, Debug, PartialEq, PartialOrd, Ord, Eq, Hash)]
 pub struct AlbumSeq(pub u8);
@@ -92,6 +110,7 @@ impl Ord for Album {
 impl Merge for Album {
     fn merge_in_place(&mut self, other: Self) {
         assert_eq!(self.id, other.id);
+        self.seq = std::cmp::max(self.seq, other.seq);
         let tracks = mem::take(&mut self.tracks);
         self.tracks = MergeSorted::new(tracks.into_iter(), other.tracks.into_iter()).collect();
     }

src/core/collection/artist.rs

@@ -9,7 +9,7 @@ use uuid::Uuid;
 
 use crate::core::collection::{
     album::Album,
-    merge::{Merge, MergeSorted},
+    merge::{Merge, MergeCollections, WithId},
     Error,
 };
 
@@ -23,6 +23,14 @@ pub struct Artist {
     pub albums: Vec<Album>,
 }
 
+impl WithId for Artist {
+    type Id = ArtistId;
+
+    fn id(&self) -> &Self::Id {
+        &self.id
+    }
+}
+
 /// The artist identifier.
 #[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)]
 pub struct ArtistId {
@@ -121,11 +129,13 @@ impl Ord for Artist {
 impl Merge for Artist {
     fn merge_in_place(&mut self, other: Self) {
         assert_eq!(self.id, other.id);
+
         self.sort = self.sort.take().or(other.sort);
         self.musicbrainz = self.musicbrainz.take().or(other.musicbrainz);
         self.properties.merge_in_place(other.properties);
+
         let albums = mem::take(&mut self.albums);
-        self.albums = MergeSorted::new(albums.into_iter(), other.albums.into_iter()).collect();
+        self.albums = MergeCollections::merge_iter(albums, other.albums);
     }
 }
 

src/core/collection/merge.rs

@@ -1,4 +1,4 @@
-use std::{cmp::Ordering, collections::HashMap, hash::Hash, iter::Peekable};
+use std::{cmp::Ordering, collections::HashMap, hash::Hash, iter::Peekable, marker::PhantomData};
 
 /// A trait for merging two objects. The merge is asymmetric with the left argument considered to be
 /// the primary whose properties are to be kept in case of collisions.
@@ -79,3 +79,43 @@ where
         }
     }
 }
+
+pub trait WithId {
+    type Id;
+
+    fn id(&self) -> &Self::Id;
+}
+
+pub struct MergeCollections<ID, T> {
+    _id: PhantomData<ID>,
+    _t: PhantomData<T>,
+}
+
+impl<ID, T> MergeCollections<ID, T>
+where
+    ID: Eq + Hash + Clone,
+    T: WithId<Id = ID> + Merge + Ord,
+{
+    pub fn merge_iter<IT: IntoIterator<Item = T>>(primary: IT, secondary: IT) -> Vec<T> {
+        let primary = primary
+            .into_iter()
+            .map(|item| (item.id().clone(), item))
+            .collect();
+        Self::merge(primary, secondary)
+    }
+
+    pub fn merge<IT: IntoIterator<Item = T>>(mut primary: HashMap<ID, T>, secondary: IT) -> Vec<T> {
+        for secondary_item in secondary {
+            if let Some(ref mut primary_item) = primary.get_mut(secondary_item.id()) {
+                primary_item.merge_in_place(secondary_item);
+            } else {
+                primary.insert(secondary_item.id().clone(), secondary_item);
+            }
+        }
+
+        let mut collection: Vec<T> = primary.into_values().collect();
+        collection.sort_unstable();
+
+        collection
+    }
+}

src/core/collection/mod.rs

@@ -5,7 +5,7 @@ pub mod artist;
 pub mod track;
 
 mod merge;
-pub use merge::Merge;
+pub use merge::MergeCollections;
 
 use std::fmt::{self, Display};
 

src/core/database/json/mod.rs

@@ -72,11 +72,7 @@ mod tests {
     use mockall::predicate;
 
     use crate::core::{
-        collection::{
-            album::{AlbumDate, AlbumMonth},
-            artist::Artist,
-            Collection,
-        },
+        collection::{album::AlbumDate, artist::Artist, Collection},
         testmod::FULL_COLLECTION,
     };
 
@@ -87,11 +83,7 @@ mod tests {
         let mut expected = FULL_COLLECTION.to_owned();
         for artist in expected.iter_mut() {
             for album in artist.albums.iter_mut() {
-                album.date = AlbumDate {
-                    year: 0,
-                    month: AlbumMonth::None,
-                    day: 0,
-                };
+                album.date = AlbumDate::default();
                 album.tracks.clear();
             }
         }

src/core/database/json/testmod.rs

@@ -10,8 +10,8 @@ pub static DATABASE_JSON: &str = "{\
     \"Qobuz\":[\"https://www.qobuz.com/nl-nl/interpreter/artist-a/download-streaming-albums\"]\
     },\
     \"albums\":[\
-    {\"title\":\"album_title a.a\",\"seq\":0},\
-    {\"title\":\"album_title a.b\",\"seq\":0}\
+    {\"title\":\"album_title a.a\",\"seq\":1},\
+    {\"title\":\"album_title a.b\",\"seq\":1}\
     ]\
     },\
     {\
@@ -27,10 +27,10 @@ pub static DATABASE_JSON: &str = "{\
     \"Qobuz\":[\"https://www.qobuz.com/nl-nl/interpreter/artist-b/download-streaming-albums\"]\
     },\
     \"albums\":[\
-    {\"title\":\"album_title b.a\",\"seq\":0},\
-    {\"title\":\"album_title b.b\",\"seq\":0},\
-    {\"title\":\"album_title b.c\",\"seq\":0},\
-    {\"title\":\"album_title b.d\",\"seq\":0}\
+    {\"title\":\"album_title b.a\",\"seq\":1},\
+    {\"title\":\"album_title b.b\",\"seq\":3},\
+    {\"title\":\"album_title b.c\",\"seq\":2},\
+    {\"title\":\"album_title b.d\",\"seq\":4}\
     ]\
     },\
     {\

src/core/database/serde/deserialize.rs

@@ -4,14 +4,17 @@ use serde::Deserialize;
 
 use crate::core::{
     collection::{
-        album::{Album, AlbumDate, AlbumId, AlbumMonth, AlbumSeq},
+        album::{Album, AlbumDate, AlbumId, AlbumSeq},
         artist::{Artist, ArtistId, MusicBrainz},
         Collection,
     },
-    database::{serde::Database, LoadError},
+    database::LoadError,
 };
 
-pub type DeserializeDatabase = Database<DeserializeArtist>;
+#[derive(Debug, Deserialize)]
+pub enum DeserializeDatabase {
+    V20240302(Vec<DeserializeArtist>),
+}
 
 #[derive(Debug, Deserialize)]
 pub struct DeserializeArtist {
@@ -33,7 +36,7 @@ impl TryFrom<DeserializeDatabase> for Collection {
 
     fn try_from(database: DeserializeDatabase) -> Result<Self, Self::Error> {
         match database {
-            Database::V20240302(collection) | Database::V20240210(collection) => collection
+            DeserializeDatabase::V20240302(collection) => collection
                 .into_iter()
                 .map(|artist| artist.try_into())
                 .collect(),
@@ -59,11 +62,7 @@ impl From<DeserializeAlbum> for Album {
     fn from(album: DeserializeAlbum) -> Self {
         Album {
             id: AlbumId { title: album.title },
-            date: AlbumDate {
-                year: 0,
-                month: AlbumMonth::None,
-                day: 0,
-            },
+            date: AlbumDate::default(),
             seq: AlbumSeq(album.seq),
             tracks: vec![],
         }

src/core/database/serde/mod.rs

@@ -2,11 +2,3 @@
 
 pub mod deserialize;
 pub mod serialize;
-
-use serde::{Deserialize, Serialize};
-
-#[derive(Debug, Serialize, Deserialize)]
-pub enum Database<ARTIST> {
-    V20240302(Vec<ARTIST>),
-    V20240210(Vec<ARTIST>),
-}

src/core/database/serde/serialize.rs

@@ -2,12 +2,12 @@ use std::collections::BTreeMap;
 
 use serde::Serialize;
 
-use crate::core::{
-    collection::{album::Album, artist::Artist, Collection},
-    database::serde::Database,
-};
+use crate::core::collection::{album::Album, artist::Artist, Collection};
 
-pub type SerializeDatabase<'a> = Database<SerializeArtist<'a>>;
+#[derive(Debug, Serialize)]
+pub enum SerializeDatabase<'a> {
+    V20240302(Vec<SerializeArtist<'a>>),
+}
 
 #[derive(Debug, Serialize)]
 pub struct SerializeArtist<'a> {
@@ -26,7 +26,7 @@ pub struct SerializeAlbum<'a> {
 
 impl<'a> From<&'a Collection> for SerializeDatabase<'a> {
     fn from(collection: &'a Collection) -> Self {
-        Database::V20240302(collection.iter().map(|artist| artist.into()).collect())
+        SerializeDatabase::V20240302(collection.iter().map(Into::into).collect())
     }
 }
 

src/core/musichoard/musichoard.rs

@@ -5,7 +5,7 @@ use crate::core::{
         album::{Album, AlbumDate, AlbumId, AlbumSeq},
         artist::{Artist, ArtistId},
         track::{Track, TrackId, TrackNum, TrackQuality},
-        Collection, Merge,
+        Collection, MergeCollections,
     },
     database::IDatabase,
     library::{ILibrary, Item, Query},
@@ -73,19 +73,7 @@ impl<LIB, DB> MusicHoard<LIB, DB> {
     }
 
     fn merge_collections(&self) -> Collection {
-        let mut primary = self.library_cache.clone();
-        for secondary_artist in self.database_cache.iter().cloned() {
-            if let Some(ref mut primary_artist) = primary.get_mut(&secondary_artist.id) {
-                primary_artist.merge_in_place(secondary_artist);
-            } else {
-                primary.insert(secondary_artist.id.clone(), secondary_artist);
-            }
-        }
-
-        let mut collection: Collection = primary.into_values().collect();
-        Self::sort_artists(&mut collection);
-
-        collection
+        MergeCollections::merge(self.library_cache.clone(), self.database_cache.clone())
     }
 
     fn items_to_artists(items: Vec<Item>) -> Result<HashMap<ArtistId, Artist>, Error> {

src/tests.rs

@@ -457,8 +457,7 @@ macro_rules! full_collection {
         artist_a.musicbrainz = Some(
             MusicBrainz::new(
                 "https://musicbrainz.org/artist/00000000-0000-0000-0000-000000000000",
-            )
-                .unwrap(),
+            ).unwrap(),
         );
 
         artist_a.properties = HashMap::from([
@@ -472,6 +471,9 @@ macro_rules! full_collection {
             ]),
         ]);
 
+        artist_a.albums[0].seq = AlbumSeq(1);
+        artist_a.albums[1].seq = AlbumSeq(1);
+
         let artist_b = iter.next().unwrap();
         assert_eq!(artist_b.id.name, "Album_Artist ‘B’");
 
@@ -494,6 +496,11 @@ macro_rules! full_collection {
             ]),
         ]);
 
+        artist_b.albums[0].seq = AlbumSeq(1);
+        artist_b.albums[1].seq = AlbumSeq(3);
+        artist_b.albums[2].seq = AlbumSeq(2);
+        artist_b.albums[3].seq = AlbumSeq(4);
+
         let artist_c = iter.next().unwrap();
         assert_eq!(artist_c.id.name, "The Album_Artist ‘C’");
 

tests/database/json.rs

@@ -4,7 +4,7 @@ use once_cell::sync::Lazy;
 use tempfile::NamedTempFile;
 
 use musichoard::{
-    collection::{artist::Artist, Collection},
+    collection::{album::AlbumDate, artist::Artist, Collection},
     database::{
         json::{backend::JsonDatabaseFileBackend, JsonDatabase},
         IDatabase,
@@ -19,7 +19,10 @@ pub static DATABASE_TEST_FILE: Lazy<PathBuf> =
 fn expected() -> Collection {
     let mut expected = COLLECTION.to_owned();
     for artist in expected.iter_mut() {
-        artist.albums.clear();
+        for album in artist.albums.iter_mut() {
+            album.date = AlbumDate::default();
+            album.tracks.clear();
+        }
     }
     expected
 }

tests/files/database/database.json

@@ -1 +1 @@
-{"V20240210":[{"name":"Аркона","sort":"Arkona","musicbrainz":"https://musicbrainz.org/artist/baad262d-55ef-427a-83c7-f7530964f212","properties":{"Bandcamp":["https://arkonamoscow.bandcamp.com/"],"MusicButler":["https://www.musicbutler.io/artist-page/283448581"],"Qobuz":["https://www.qobuz.com/nl-nl/interpreter/arkona/download-streaming-albums"]}},{"name":"Eluveitie","sort":null,"musicbrainz":"https://musicbrainz.org/artist/8000598a-5edb-401c-8e6d-36b167feaf38","properties":{"MusicButler":["https://www.musicbutler.io/artist-page/269358403"],"Qobuz":["https://www.qobuz.com/nl-nl/interpreter/eluveitie/download-streaming-albums"]}},{"name":"Frontside","sort":null,"musicbrainz":"https://musicbrainz.org/artist/3a901353-fccd-4afd-ad01-9c03f451b490","properties":{"MusicButler":["https://www.musicbutler.io/artist-page/826588800"],"Qobuz":["https://www.qobuz.com/nl-nl/interpreter/frontside/download-streaming-albums"]}},{"name":"Heaven’s Basement","sort":"Heaven’s Basement","musicbrainz":"https://musicbrainz.org/artist/c2c4d56a-d599-4a18-bd2f-ae644e2198cc","properties":{"MusicButler":["https://www.musicbutler.io/artist-page/291158685"],"Qobuz":["https://www.qobuz.com/nl-nl/interpreter/heaven-s-basement/download-streaming-albums"]}},{"name":"Metallica","sort":null,"musicbrainz":"https://musicbrainz.org/artist/65f4f0c5-ef9e-490c-aee3-909e7ae6b2ab","properties":{"MusicButler":["https://www.musicbutler.io/artist-page/3996865"],"Qobuz":["https://www.qobuz.com/nl-nl/interpreter/metallica/download-streaming-albums"]}}]}
+{"V20240302":[{"name":"Аркона","sort":"Arkona","musicbrainz":"https://musicbrainz.org/artist/baad262d-55ef-427a-83c7-f7530964f212","properties":{"Bandcamp":["https://arkonamoscow.bandcamp.com/"],"MusicButler":["https://www.musicbutler.io/artist-page/283448581"],"Qobuz":["https://www.qobuz.com/nl-nl/interpreter/arkona/download-streaming-albums"]},"albums":[{"title":"Slovo","seq":0}]},{"name":"Eluveitie","sort":null,"musicbrainz":"https://musicbrainz.org/artist/8000598a-5edb-401c-8e6d-36b167feaf38","properties":{"MusicButler":["https://www.musicbutler.io/artist-page/269358403"],"Qobuz":["https://www.qobuz.com/nl-nl/interpreter/eluveitie/download-streaming-albums"]},"albums":[{"title":"Vên [re‐recorded]","seq":0},{"title":"Slania","seq":0}]},{"name":"Frontside","sort":null,"musicbrainz":"https://musicbrainz.org/artist/3a901353-fccd-4afd-ad01-9c03f451b490","properties":{"MusicButler":["https://www.musicbutler.io/artist-page/826588800"],"Qobuz":["https://www.qobuz.com/nl-nl/interpreter/frontside/download-streaming-albums"]},"albums":[{"title":"…nasze jest królestwo, potęga i chwała na wieki…","seq":0}]},{"name":"Heaven’s Basement","sort":"Heaven’s Basement","musicbrainz":"https://musicbrainz.org/artist/c2c4d56a-d599-4a18-bd2f-ae644e2198cc","properties":{"MusicButler":["https://www.musicbutler.io/artist-page/291158685"],"Qobuz":["https://www.qobuz.com/nl-nl/interpreter/heaven-s-basement/download-streaming-albums"]},"albums":[{"title":"Paper Plague","seq":0},{"title":"Unbreakable","seq":0}]},{"name":"Metallica","sort":null,"musicbrainz":"https://musicbrainz.org/artist/65f4f0c5-ef9e-490c-aee3-909e7ae6b2ab","properties":{"MusicButler":["https://www.musicbutler.io/artist-page/3996865"],"Qobuz":["https://www.qobuz.com/nl-nl/interpreter/metallica/download-streaming-albums"]},"albums":[{"title":"Ride the Lightning","seq":0},{"title":"S&M","seq":0}]}]}