From 16dc2917f0dee34e6ef2d396ca0413475d9f74c9 Mon Sep 17 00:00:00 2001 From: Wojciech Kozlowski Date: Sun, 12 Jan 2025 21:38:30 +0100 Subject: [PATCH 01/12] Update default database path --- src/main.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main.rs b/src/main.rs index 655c47b..927863c 100644 --- a/src/main.rs +++ b/src/main.rs @@ -66,7 +66,7 @@ struct DbOpt { #[structopt( long = "database", help = "Database file path", - default_value = "database.json" + default_value = "database.db" )] database_file_path: PathBuf, -- 2.47.1 From a8cd1d341d6c95a867f248bb71a314f9beb4d8e2 Mon Sep 17 00:00:00 2001 From: Wojciech Kozlowski Date: Sun, 12 Jan 2025 22:04:47 +0100 Subject: [PATCH 02/12] Remove pre_commit --- src/core/musichoard/builder.rs | 4 ---- src/core/musichoard/database.rs | 36 ++++++++++++++++++--------------- src/core/musichoard/library.rs | 4 ++-- src/core/musichoard/mod.rs | 1 - 4 files changed, 22 insertions(+), 23 deletions(-) diff --git a/src/core/musichoard/builder.rs b/src/core/musichoard/builder.rs index 46c9f4c..e828718 100644 --- a/src/core/musichoard/builder.rs +++ b/src/core/musichoard/builder.rs @@ -65,7 +65,6 @@ impl MusicHoard { filter: CollectionFilter::default(), filtered: vec![], collection: vec![], - pre_commit: vec![], database: NoDatabase, library: NoLibrary, library_cache: vec![], @@ -87,7 +86,6 @@ impl MusicHoard { filter: CollectionFilter::default(), filtered: vec![], collection: vec![], - pre_commit: vec![], database: NoDatabase, library, library_cache: vec![], @@ -109,7 +107,6 @@ impl MusicHoard { filter: CollectionFilter::default(), filtered: vec![], collection: vec![], - pre_commit: vec![], database, library: NoLibrary, library_cache: vec![], @@ -131,7 +128,6 @@ impl MusicHoard { filter: CollectionFilter::default(), filtered: vec![], collection: vec![], - pre_commit: vec![], database, library, library_cache: vec![], diff --git a/src/core/musichoard/database.rs b/src/core/musichoard/database.rs index ae9e58e..51f8ad5 100644 --- a/src/core/musichoard/database.rs +++ b/src/core/musichoard/database.rs @@ -122,8 +122,6 @@ impl IMusicHoardDatabase for MusicHoard IMusicHoardDatabasePrivate for MusicHoard { fn commit(&mut self) -> Result<(), Error> { - self.collection = self.pre_commit.clone(); self.filtered = self.filter_collection(); Ok(()) } @@ -373,14 +370,11 @@ impl IMusicHoardDatabasePrivate for MusicHoard { impl IMusicHoardDatabasePrivate for MusicHoard { fn commit(&mut self) -> Result<(), Error> { - if self.collection != self.pre_commit { - if let Err(err) = self.database.save(&self.pre_commit) { - self.pre_commit = self.collection.clone(); - return Err(err.into()); - } - self.collection = self.pre_commit.clone(); - self.filtered = self.filter_collection(); + if let Err(err) = self.database.save(&self.collection) { + self.reload_database()?; + return Err(err.into()); } + self.filtered = self.filter_collection(); Ok(()) } } @@ -390,7 +384,7 @@ impl MusicHoard { where FnColl: FnOnce(&mut Collection), { - fn_coll(&mut self.pre_commit); + fn_coll(&mut self.collection); self.commit() } @@ -404,7 +398,7 @@ impl MusicHoard { FnArtist: FnOnce(&mut Artist), FnColl: FnOnce(&mut Collection), { - let artist = Self::get_artist_mut_or_err(&mut self.pre_commit, artist_id)?; + let artist = Self::get_artist_mut_or_err(&mut self.collection, artist_id)?; fn_artist(artist); self.update_collection(fn_coll) } @@ -431,7 +425,7 @@ impl MusicHoard { FnAlbum: FnOnce(&mut Album), FnArtist: FnOnce(&mut Artist), { - let artist = Self::get_artist_mut_or_err(&mut self.pre_commit, artist_id)?; + let artist = Self::get_artist_mut_or_err(&mut self.collection, artist_id)?; let album = Self::get_album_mut_or_err(artist, album_id)?; fn_album(album); fn_artist(artist); @@ -493,7 +487,7 @@ mod tests { .returning(|| Ok(FULL_COLLECTION.to_owned())); database .expect_save() - .times(1) + .times(3) .in_sequence(&mut seq) .with(predicate::eq(with_artist.clone())) .returning(|_| Ok(())); @@ -789,7 +783,7 @@ mod tests { .returning(|| Ok(FULL_COLLECTION.to_owned())); database .expect_save() - .times(1) + .times(3) .in_sequence(&mut seq) .with(predicate::eq(with_album.clone())) .returning(|_| Ok(())); @@ -1018,11 +1012,21 @@ mod tests { let database_result = Err(database::SaveError::IoError(String::from("I/O error"))); - database.expect_load().return_once(|| Ok(vec![])); + let mut seq = Sequence::new(); + database + .expect_load() + .times(1) + .in_sequence(&mut seq) + .return_once(|| Ok(vec![])); database .expect_save() .times(1) .return_once(|_: &Collection| database_result); + database + .expect_load() + .times(1) + .in_sequence(&mut seq) + .return_once(|| Ok(vec![])); let mut music_hoard = MusicHoard::database(database); music_hoard.reload_database().unwrap(); diff --git a/src/core/musichoard/library.rs b/src/core/musichoard/library.rs index d0deb30..29a5ba0 100644 --- a/src/core/musichoard/library.rs +++ b/src/core/musichoard/library.rs @@ -23,7 +23,7 @@ pub trait IMusicHoardLibrary { impl IMusicHoardLibrary for MusicHoard { fn rescan_library(&mut self) -> Result<(), Error> { - self.pre_commit = self.rescan_library_inner(vec![])?; + self.collection = self.rescan_library_inner(vec![])?; self.commit() } } @@ -33,7 +33,7 @@ impl IMusicHoardLibrary for MusicHoard { filter: CollectionFilter, filtered: Collection, collection: Collection, - pre_commit: Collection, database: Database, library: Library, library_cache: Collection, -- 2.47.1 From c2973680e816d7389afc7ea0d5f5d61134fa6c9d Mon Sep 17 00:00:00 2001 From: Wojciech Kozlowski Date: Sun, 12 Jan 2025 22:40:59 +0100 Subject: [PATCH 03/12] Remove unused methods --- src/core/musichoard/base.rs | 5 - src/core/musichoard/database.rs | 409 +------------------------------- 2 files changed, 13 insertions(+), 401 deletions(-) diff --git a/src/core/musichoard/base.rs b/src/core/musichoard/base.rs index e8daf36..b1775f2 100644 --- a/src/core/musichoard/base.rs +++ b/src/core/musichoard/base.rs @@ -37,7 +37,6 @@ pub trait IMusicHoardBasePrivate { fn filter_collection(&self) -> Collection; fn filter_artist(&self, artist: &Artist) -> Option; - fn get_artist<'a>(collection: &'a Collection, artist_id: &ArtistId) -> Option<&'a Artist>; fn get_artist_mut<'a>( collection: &'a mut Collection, artist_id: &ArtistId, @@ -105,10 +104,6 @@ impl IMusicHoardBasePrivate for MusicHoard Some(Artist { meta, albums }) } - fn get_artist<'a>(collection: &'a Collection, artist_id: &ArtistId) -> Option<&'a Artist> { - collection.iter().find(|a| &a.meta.id == artist_id) - } - fn get_artist_mut<'a>( collection: &'a mut Collection, artist_id: &ArtistId, diff --git a/src/core/musichoard/database.rs b/src/core/musichoard/database.rs index 51f8ad5..24065f8 100644 --- a/src/core/musichoard/database.rs +++ b/src/core/musichoard/database.rs @@ -8,7 +8,7 @@ use crate::{ }, core::{ collection::{ - album::{Album, AlbumId, AlbumSeq}, + album::{Album, AlbumId}, artist::{Artist, ArtistId}, Collection, }, @@ -20,9 +20,6 @@ use crate::{ pub trait IMusicHoardDatabase { fn reload_database(&mut self) -> Result<(), Error>; - fn add_artist>(&mut self, artist_id: IntoId) -> Result<(), Error>; - fn remove_artist>(&mut self, artist_id: Id) -> Result<(), Error>; - fn set_artist_mb_ref>( &mut self, artist_id: Id, @@ -30,13 +27,6 @@ pub trait IMusicHoardDatabase { ) -> Result<(), Error>; fn clear_artist_mb_ref>(&mut self, artist_id: Id) -> Result<(), Error>; - fn set_artist_sort, S: Into>( - &mut self, - artist_id: Id, - artist_sort: S, - ) -> Result<(), Error>; - fn clear_artist_sort>(&mut self, artist_id: Id) -> Result<(), Error>; - fn merge_artist_info>( &mut self, artist_id: Id, @@ -44,30 +34,6 @@ pub trait IMusicHoardDatabase { ) -> Result<(), Error>; fn clear_artist_info>(&mut self, artist_id: Id) -> Result<(), Error>; - fn add_to_artist_property, S: AsRef + Into>( - &mut self, - artist_id: Id, - property: S, - values: Vec, - ) -> Result<(), Error>; - fn remove_from_artist_property, S: AsRef>( - &mut self, - artist_id: Id, - property: S, - values: Vec, - ) -> Result<(), Error>; - fn set_artist_property, S: AsRef + Into>( - &mut self, - artist_id: Id, - property: S, - values: Vec, - ) -> Result<(), Error>; - fn clear_artist_property, S: AsRef>( - &mut self, - artist_id: Id, - property: S, - ) -> Result<(), Error>; - fn add_album>( &mut self, artist_id: ArtistIdRef, @@ -90,17 +56,7 @@ pub trait IMusicHoardDatabase { artist_id: ArtistIdRef, album_id: AlbumIdRef, ) -> Result<(), Error>; - fn set_album_seq, AlbumIdRef: AsRef>( - &mut self, - artist_id: ArtistIdRef, - album_id: AlbumIdRef, - seq: u8, - ) -> Result<(), Error>; - fn clear_album_seq, AlbumIdRef: AsRef>( - &mut self, - artist_id: ArtistIdRef, - album_id: AlbumIdRef, - ) -> Result<(), Error>; + fn merge_album_info, AlbumIdRef: AsRef>( &mut self, artist_id: ArtistIdRef, @@ -125,28 +81,6 @@ impl IMusicHoardDatabase for MusicHoard>(&mut self, artist_id: IntoId) -> Result<(), Error> { - let artist_id: ArtistId = artist_id.into(); - - self.update_collection(|collection| { - if Self::get_artist(collection, &artist_id).is_none() { - collection.push(Artist::new(artist_id)); - Self::sort_artists(collection); - } - }) - } - - fn remove_artist>(&mut self, artist_id: Id) -> Result<(), Error> { - self.update_collection(|collection| { - let index_opt = collection - .iter() - .position(|a| &a.meta.id == artist_id.as_ref()); - if let Some(index) = index_opt { - collection.remove(index); - } - }) - } - fn set_artist_mb_ref>( &mut self, artist_id: Id, @@ -167,26 +101,6 @@ impl IMusicHoardDatabase for MusicHoard, S: Into>( - &mut self, - artist_id: Id, - artist_sort: S, - ) -> Result<(), Error> { - self.update_artist_and( - artist_id.as_ref(), - |artist| artist.meta.set_sort_key(artist_sort), - |collection| Self::sort_artists(collection), - ) - } - - fn clear_artist_sort>(&mut self, artist_id: Id) -> Result<(), Error> { - self.update_artist_and( - artist_id.as_ref(), - |artist| artist.meta.clear_sort_key(), - |collection| Self::sort_artists(collection), - ) - } - fn merge_artist_info>( &mut self, artist_id: Id, @@ -204,49 +118,6 @@ impl IMusicHoardDatabase for MusicHoard, S: AsRef + Into>( - &mut self, - artist_id: Id, - property: S, - values: Vec, - ) -> Result<(), Error> { - self.update_artist(artist_id.as_ref(), |artist| { - artist.meta.info.add_to_property(property, values) - }) - } - - fn remove_from_artist_property, S: AsRef>( - &mut self, - artist_id: Id, - property: S, - values: Vec, - ) -> Result<(), Error> { - self.update_artist(artist_id.as_ref(), |artist| { - artist.meta.info.remove_from_property(property, values) - }) - } - - fn set_artist_property, S: AsRef + Into>( - &mut self, - artist_id: Id, - property: S, - values: Vec, - ) -> Result<(), Error> { - self.update_artist(artist_id.as_ref(), |artist| { - artist.meta.info.set_property(property, values) - }) - } - - fn clear_artist_property, S: AsRef>( - &mut self, - artist_id: Id, - property: S, - ) -> Result<(), Error> { - self.update_artist(artist_id.as_ref(), |artist| { - artist.meta.info.clear_property(property) - }) - } - fn add_album>( &mut self, artist_id: ArtistIdRef, @@ -307,33 +178,6 @@ impl IMusicHoardDatabase for MusicHoard, AlbumIdRef: AsRef>( - &mut self, - artist_id: ArtistIdRef, - album_id: AlbumIdRef, - seq: u8, - ) -> Result<(), Error> { - self.update_album_and( - artist_id.as_ref(), - album_id.as_ref(), - |album| album.meta.set_seq(AlbumSeq(seq)), - |artist| artist.albums.sort_unstable(), - ) - } - - fn clear_album_seq, AlbumIdRef: AsRef>( - &mut self, - artist_id: ArtistIdRef, - album_id: AlbumIdRef, - ) -> Result<(), Error> { - self.update_album_and( - artist_id.as_ref(), - album_id.as_ref(), - |album| album.meta.clear_seq(), - |artist| artist.albums.sort_unstable(), - ) - } - fn merge_album_info, AlbumIdRef: AsRef>( &mut self, artist_id: Id, @@ -457,7 +301,7 @@ mod tests { core::{ collection::artist::ArtistId, interface::database::{self, MockIDatabase}, - musichoard::{base::IMusicHoardBase, NoLibrary}, + musichoard::base::IMusicHoardBase, testmod::FULL_COLLECTION, }, }; @@ -465,106 +309,6 @@ mod tests { use super::*; static MBID: &str = "d368baa8-21ca-4759-9731-0b2753071ad8"; - static MUSICBUTLER: &str = "https://www.musicbutler.io/artist-page/483340948"; - static MUSICBUTLER_2: &str = "https://www.musicbutler.io/artist-page/658903042/"; - - #[test] - fn artist_new_delete() { - let artist_id = ArtistId::new("an artist"); - let artist_id_2 = ArtistId::new("another artist"); - - let collection = FULL_COLLECTION.to_owned(); - let mut with_artist = collection.clone(); - with_artist.push(Artist::new(artist_id.clone())); - - let mut database = MockIDatabase::new(); - let mut seq = Sequence::new(); - database - .expect_load() - .times(1) - .times(1) - .in_sequence(&mut seq) - .returning(|| Ok(FULL_COLLECTION.to_owned())); - database - .expect_save() - .times(3) - .in_sequence(&mut seq) - .with(predicate::eq(with_artist.clone())) - .returning(|_| Ok(())); - database - .expect_save() - .times(1) - .in_sequence(&mut seq) - .with(predicate::eq(collection.clone())) - .returning(|_| Ok(())); - - let mut music_hoard = MusicHoard::database(database); - music_hoard.reload_database().unwrap(); - assert_eq!(music_hoard.collection, collection); - - assert!(music_hoard.add_artist(artist_id.clone()).is_ok()); - assert_eq!(music_hoard.collection, with_artist); - - assert!(music_hoard.add_artist(artist_id.clone()).is_ok()); - assert_eq!(music_hoard.collection, with_artist); - - assert!(music_hoard.remove_artist(&artist_id_2).is_ok()); - assert_eq!(music_hoard.collection, with_artist); - - assert!(music_hoard.remove_artist(&artist_id).is_ok()); - assert_eq!(music_hoard.collection, collection); - } - - #[test] - fn artist_sort_set_clear() { - let mut database = MockIDatabase::new(); - database.expect_save().times(4).returning(|_| Ok(())); - - type MH = MusicHoard; - let mut music_hoard: MH = MusicHoard::database(database); - - let artist_1_id = ArtistId::new("the artist"); - let artist_1_sort = String::from("artist, the"); - - // Must be after "artist, the", but before "the artist" - let artist_2_id = ArtistId::new("b-artist"); - - assert!(artist_1_sort < artist_2_id.name); - assert!(artist_2_id < artist_1_id); - - assert!(music_hoard.add_artist(artist_1_id.clone()).is_ok()); - assert!(music_hoard.add_artist(artist_2_id.clone()).is_ok()); - - let artist_1: &Artist = MH::get_artist(&music_hoard.collection, &artist_1_id).unwrap(); - let artist_2: &Artist = MH::get_artist(&music_hoard.collection, &artist_2_id).unwrap(); - - assert!(artist_2 < artist_1); - - assert_eq!(artist_1, &music_hoard.collection[1]); - assert_eq!(artist_2, &music_hoard.collection[0]); - - music_hoard - .set_artist_sort(artist_1_id.as_ref(), artist_1_sort.clone()) - .unwrap(); - - let artist_1: &Artist = MH::get_artist(&music_hoard.collection, &artist_1_id).unwrap(); - let artist_2: &Artist = MH::get_artist(&music_hoard.collection, &artist_2_id).unwrap(); - - assert!(artist_1 < artist_2); - - assert_eq!(artist_1, &music_hoard.collection[0]); - assert_eq!(artist_2, &music_hoard.collection[1]); - - music_hoard.clear_artist_sort(artist_1_id.as_ref()).unwrap(); - - let artist_1: &Artist = MH::get_artist(&music_hoard.collection, &artist_1_id).unwrap(); - let artist_2: &Artist = MH::get_artist(&music_hoard.collection, &artist_2_id).unwrap(); - - assert!(artist_2 < artist_1); - - assert_eq!(artist_1, &music_hoard.collection[1]); - assert_eq!(artist_2, &music_hoard.collection[0]); - } #[test] fn collection_error() { @@ -584,13 +328,14 @@ mod tests { #[test] fn set_clear_artist_mb_ref() { let mut database = MockIDatabase::new(); - database.expect_save().times(3).returning(|_| Ok(())); + database.expect_save().times(2).returning(|_| Ok(())); let mut artist_id = ArtistId::new("an artist"); let artist_id_2 = ArtistId::new("another artist"); let mut music_hoard = MusicHoard::database(database); - assert!(music_hoard.add_artist(artist_id.clone()).is_ok()); + music_hoard.collection.push(Artist::new(artist_id.clone())); + music_hoard.collection.sort_unstable(); let mut expected = ArtistMbRef::None; assert_eq!(music_hoard.collection[0].meta.id.mb_ref, expected); @@ -629,13 +374,14 @@ mod tests { #[test] fn set_clear_artist_info() { let mut database = MockIDatabase::new(); - database.expect_save().times(3).returning(|_| Ok(())); + database.expect_save().times(2).returning(|_| Ok(())); let artist_id = ArtistId::new("an artist"); let artist_id_2 = ArtistId::new("another artist"); let mut music_hoard = MusicHoard::database(database); - assert!(music_hoard.add_artist(artist_id.clone()).is_ok()); + music_hoard.collection.push(Artist::new(artist_id.clone())); + music_hoard.collection.sort_unstable(); let mut expected = ArtistInfo::default(); assert_eq!(music_hoard.collection[0].meta.info, expected); @@ -669,97 +415,6 @@ mod tests { assert_eq!(music_hoard.collection[0].meta.info, expected); } - #[test] - fn add_to_remove_from_property() { - let mut database = MockIDatabase::new(); - database.expect_save().times(3).returning(|_| Ok(())); - - let artist_id = ArtistId::new("an artist"); - let artist_id_2 = ArtistId::new("another artist"); - let mut music_hoard = MusicHoard::database(database); - - assert!(music_hoard.add_artist(artist_id.clone()).is_ok()); - - let mut expected: Vec = vec![]; - assert!(music_hoard.collection[0].meta.info.properties.is_empty()); - - // Adding URLs to an artist not in the collection is an error. - assert!(music_hoard - .add_to_artist_property(&artist_id_2, "MusicButler", vec![MUSICBUTLER]) - .is_err()); - assert!(music_hoard.collection[0].meta.info.properties.is_empty()); - - // Adding mutliple URLs without clashes. - assert!(music_hoard - .add_to_artist_property(&artist_id, "MusicButler", vec![MUSICBUTLER, MUSICBUTLER_2]) - .is_ok()); - expected.push(MUSICBUTLER.to_owned()); - expected.push(MUSICBUTLER_2.to_owned()); - let info = &music_hoard.collection[0].meta.info; - assert_eq!(info.properties.get("MusicButler"), Some(&expected)); - - // Removing URLs from an artist not in the collection is an error. - assert!(music_hoard - .remove_from_artist_property(&artist_id_2, "MusicButler", vec![MUSICBUTLER]) - .is_err()); - let info = &music_hoard.collection[0].meta.info; - assert_eq!(info.properties.get("MusicButler"), Some(&expected)); - - // Removing multiple URLs without clashes. - assert!(music_hoard - .remove_from_artist_property( - &artist_id, - "MusicButler", - vec![MUSICBUTLER, MUSICBUTLER_2] - ) - .is_ok()); - expected.clear(); - assert!(music_hoard.collection[0].meta.info.properties.is_empty()); - } - - #[test] - fn set_clear_property() { - let mut database = MockIDatabase::new(); - database.expect_save().times(3).returning(|_| Ok(())); - - let artist_id = ArtistId::new("an artist"); - let artist_id_2 = ArtistId::new("another artist"); - let mut music_hoard = MusicHoard::database(database); - - assert!(music_hoard.add_artist(artist_id.clone()).is_ok()); - - let mut expected: Vec = vec![]; - assert!(music_hoard.collection[0].meta.info.properties.is_empty()); - - // Seting URL on an artist not in the collection is an error. - assert!(music_hoard - .set_artist_property(&artist_id_2, "MusicButler", vec![MUSICBUTLER]) - .is_err()); - assert!(music_hoard.collection[0].meta.info.properties.is_empty()); - - // Set URLs. - assert!(music_hoard - .set_artist_property(&artist_id, "MusicButler", vec![MUSICBUTLER, MUSICBUTLER_2]) - .is_ok()); - expected.clear(); - expected.push(MUSICBUTLER.to_owned()); - expected.push(MUSICBUTLER_2.to_owned()); - let info = &music_hoard.collection[0].meta.info; - assert_eq!(info.properties.get("MusicButler"), Some(&expected)); - - // Clearing URLs on an artist that does not exist is an error. - assert!(music_hoard - .clear_artist_property(&artist_id_2, "MusicButler") - .is_err()); - - // Clear URLs. - assert!(music_hoard - .clear_artist_property(&artist_id, "MusicButler") - .is_ok()); - expected.clear(); - assert!(music_hoard.collection[0].meta.info.properties.is_empty()); - } - #[test] fn album_new_delete() { let album_id = AlbumId::new("an album"); @@ -876,47 +531,6 @@ mod tests { assert_eq!(album.meta.id.mb_ref, AlbumMbRef::None); } - #[test] - fn set_clear_album_seq() { - let mut database = MockIDatabase::new(); - - let artist_id = ArtistId::new("an artist"); - let album_id = AlbumId::new("an album"); - let album_id_2 = AlbumId::new("another album"); - - let mut database_result = vec![Artist::new(artist_id.clone())]; - database_result[0].albums.push(Album::new(album_id.clone())); - - database - .expect_load() - .times(1) - .return_once(|| Ok(database_result)); - database.expect_save().times(2).returning(|_| Ok(())); - - let mut music_hoard = MusicHoard::database(database); - music_hoard.reload_database().unwrap(); - assert_eq!(music_hoard.collection[0].albums[0].meta.seq, AlbumSeq(0)); - - // Seting seq on an album not belonging to the artist is an error. - assert!(music_hoard - .set_album_seq(&artist_id, &album_id_2, 6) - .is_err()); - assert_eq!(music_hoard.collection[0].albums[0].meta.seq, AlbumSeq(0)); - - // Set seq. - assert!(music_hoard.set_album_seq(&artist_id, &album_id, 6).is_ok()); - assert_eq!(music_hoard.collection[0].albums[0].meta.seq, AlbumSeq(6)); - - // Clearing seq on an album that does not exist is an error. - assert!(music_hoard - .clear_album_seq(&artist_id, &album_id_2) - .is_err()); - - // Clear seq. - assert!(music_hoard.clear_album_seq(&artist_id, &album_id).is_ok()); - assert_eq!(music_hoard.collection[0].albums[0].meta.seq, AlbumSeq(0)); - } - #[test] fn set_clear_album_info() { let mut database = MockIDatabase::new(); @@ -1031,8 +645,11 @@ mod tests { let mut music_hoard = MusicHoard::database(database); music_hoard.reload_database().unwrap(); + let artist_id = ArtistId::new("an artist"); + music_hoard.collection.push(Artist::new(artist_id.clone())); + let actual_err = music_hoard - .add_artist(ArtistId::new("an artist")) + .add_album(artist_id, AlbumMeta::new("an album")) .unwrap_err(); let expected_err = Error::DatabaseError( database::SaveError::IoError(String::from("I/O error")).to_string(), -- 2.47.1 From 6a80c059098c322b32a4a6f174a80177ef12355a Mon Sep 17 00:00:00 2001 From: Wojciech Kozlowski Date: Sun, 12 Jan 2025 22:56:26 +0100 Subject: [PATCH 04/12] Correctly sort --- src/core/musichoard/base.rs | 5 ---- src/core/musichoard/database.rs | 52 +++++++++++++++------------------ 2 files changed, 23 insertions(+), 34 deletions(-) diff --git a/src/core/musichoard/base.rs b/src/core/musichoard/base.rs index b1775f2..edd63f1 100644 --- a/src/core/musichoard/base.rs +++ b/src/core/musichoard/base.rs @@ -30,7 +30,6 @@ impl IMusicHoardBase for MusicHoard { } pub trait IMusicHoardBasePrivate { - fn sort_artists(collection: &mut [Artist]); fn sort_albums_and_tracks<'a, C: Iterator>(collection: C); fn merge_collections>(&self, database: It) -> Collection; @@ -55,10 +54,6 @@ pub trait IMusicHoardBasePrivate { } impl IMusicHoardBasePrivate for MusicHoard { - fn sort_artists(collection: &mut [Artist]) { - collection.sort_unstable(); - } - fn sort_albums_and_tracks<'a, COL: Iterator>(collection: COL) { for artist in collection { artist.albums.sort_unstable(); diff --git a/src/core/musichoard/database.rs b/src/core/musichoard/database.rs index 24065f8..26a0b71 100644 --- a/src/core/musichoard/database.rs +++ b/src/core/musichoard/database.rs @@ -86,19 +86,11 @@ impl IMusicHoardDatabase for MusicHoard Result<(), Error> { - self.update_artist_and( - artist_id.as_ref(), - |artist| artist.meta.set_mb_ref(mb_ref), - |collection| Self::sort_artists(collection), - ) + self.update_artist(artist_id.as_ref(), |artist| artist.meta.set_mb_ref(mb_ref)) } fn clear_artist_mb_ref>(&mut self, artist_id: Id) -> Result<(), Error> { - self.update_artist_and( - artist_id.as_ref(), - |artist| artist.meta.clear_mb_ref(), - |collection| Self::sort_artists(collection), - ) + self.update_artist(artist_id.as_ref(), |artist| artist.meta.clear_mb_ref()) } fn merge_artist_info>( @@ -157,12 +149,9 @@ impl IMusicHoardDatabase for MusicHoard Result<(), Error> { - self.update_album_and( - artist_id.as_ref(), - album_id.as_ref(), - |album| album.meta.set_mb_ref(mb_ref), - |artist| artist.albums.sort_unstable(), - ) + self.update_album(artist_id.as_ref(), album_id.as_ref(), |album| { + album.meta.set_mb_ref(mb_ref) + }) } fn clear_album_mb_ref, AlbumIdRef: AsRef>( @@ -170,12 +159,9 @@ impl IMusicHoardDatabase for MusicHoard Result<(), Error> { - self.update_album_and( - artist_id.as_ref(), - album_id.as_ref(), - |album| album.meta.clear_mb_ref(), - |artist| artist.albums.sort_unstable(), - ) + self.update_album(artist_id.as_ref(), album_id.as_ref(), |album| { + album.meta.clear_mb_ref() + }) } fn merge_album_info, AlbumIdRef: AsRef>( @@ -184,10 +170,15 @@ impl IMusicHoardDatabase for MusicHoard Result<(), Error> { - self.update_album(artist_id.as_ref(), album_id.as_ref(), |album| { - mem::swap(&mut album.meta.info, &mut info); - album.meta.info.merge_in_place(info); - }) + self.update_album_and( + artist_id.as_ref(), + album_id.as_ref(), + |album| { + mem::swap(&mut album.meta.info, &mut info); + album.meta.info.merge_in_place(info); + }, + |artist| artist.albums.sort_unstable(), + ) } fn clear_album_info, AlbumIdRef: AsRef>( @@ -195,9 +186,12 @@ impl IMusicHoardDatabase for MusicHoard Result<(), Error> { - self.update_album(artist_id.as_ref(), album_id.as_ref(), |album| { - album.meta.info = AlbumInfo::default() - }) + self.update_album_and( + artist_id.as_ref(), + album_id.as_ref(), + |album| album.meta.info = AlbumInfo::default(), + |artist| artist.albums.sort_unstable(), + ) } } -- 2.47.1 From 64f3e2d8b73b500f7999615f671c7c71a9de26af Mon Sep 17 00:00:00 2001 From: Wojciech Kozlowski Date: Mon, 13 Jan 2025 19:22:38 +0100 Subject: [PATCH 05/12] Convenience method --- src/core/musichoard/database.rs | 36 +++++++++++++++++++-------------- 1 file changed, 21 insertions(+), 15 deletions(-) diff --git a/src/core/musichoard/database.rs b/src/core/musichoard/database.rs index 26a0b71..d73b817 100644 --- a/src/core/musichoard/database.rs +++ b/src/core/musichoard/database.rs @@ -170,15 +170,10 @@ impl IMusicHoardDatabase for MusicHoard Result<(), Error> { - self.update_album_and( - artist_id.as_ref(), - album_id.as_ref(), - |album| { - mem::swap(&mut album.meta.info, &mut info); - album.meta.info.merge_in_place(info); - }, - |artist| artist.albums.sort_unstable(), - ) + self.update_album_and_sort(artist_id.as_ref(), album_id.as_ref(), |album| { + mem::swap(&mut album.meta.info, &mut info); + album.meta.info.merge_in_place(info); + }) } fn clear_album_info, AlbumIdRef: AsRef>( @@ -186,12 +181,9 @@ impl IMusicHoardDatabase for MusicHoard Result<(), Error> { - self.update_album_and( - artist_id.as_ref(), - album_id.as_ref(), - |album| album.meta.info = AlbumInfo::default(), - |artist| artist.albums.sort_unstable(), - ) + self.update_album_and_sort(artist_id.as_ref(), album_id.as_ref(), |album| { + album.meta.info = AlbumInfo::default() + }) } } @@ -270,6 +262,20 @@ impl MusicHoard { self.update_collection(|_| {}) } + fn update_album_and_sort( + &mut self, + artist_id: &ArtistId, + album_id: &AlbumId, + fn_album: FnAlbum, + ) -> Result<(), Error> + where + FnAlbum: FnOnce(&mut Album), + { + self.update_album_and(artist_id, album_id, fn_album, |artist| { + artist.albums.sort_unstable() + }) + } + fn update_album( &mut self, artist_id: &ArtistId, -- 2.47.1 From aea710eb323fd12dbd0ad0b3dc9cc0ed78f1e483 Mon Sep 17 00:00:00 2001 From: Wojciech Kozlowski Date: Tue, 14 Jan 2025 21:49:05 +0100 Subject: [PATCH 06/12] Update tables without dropping --- src/core/interface/database/mod.rs | 13 +++++++++++++ src/external/database/sql/backend.rs | 28 ++++++++++++++++++++++----- src/external/database/sql/mod.rs | 23 ++++++++++++++++++---- tests/database/sql.rs | 27 +++++++++++++++++++++++++- tests/files/database/database.db | Bin 20480 -> 28672 bytes tests/lib.rs | 2 +- 6 files changed, 82 insertions(+), 11 deletions(-) diff --git a/src/core/interface/database/mod.rs b/src/core/interface/database/mod.rs index 27293d0..cc300f3 100644 --- a/src/core/interface/database/mod.rs +++ b/src/core/interface/database/mod.rs @@ -10,6 +10,9 @@ use crate::core::collection::{self, Collection}; /// Trait for interacting with the database. #[cfg_attr(test, automock)] pub trait IDatabase { + /// Reset all content. + fn reset(&mut self) -> Result<(), SaveError>; + /// Load collection from the database. fn load(&mut self) -> Result; @@ -21,6 +24,10 @@ pub trait IDatabase { pub struct NullDatabase; impl IDatabase for NullDatabase { + fn reset(&mut self) -> Result<(), SaveError> { + Ok(()) + } + fn load(&mut self) -> Result { Ok(vec![]) } @@ -98,6 +105,12 @@ mod tests { use super::*; + #[test] + fn null_database_reset() { + let mut database = NullDatabase; + assert!(database.reset().is_ok()); + } + #[test] fn null_database_load() { let mut database = NullDatabase; diff --git a/src/external/database/sql/backend.rs b/src/external/database/sql/backend.rs index 3ad96db..3a87781 100644 --- a/src/external/database/sql/backend.rs +++ b/src/external/database/sql/backend.rs @@ -108,7 +108,8 @@ impl ISqlTransactionBackend for SqlTransactionSqliteBackend<'_> { name TEXT NOT NULL, mbid JSON NOT NULL DEFAULT '\"None\"', sort TEXT NULL, - properties JSON NOT NULL DEFAULT '{}' + properties JSON NOT NULL DEFAULT '{}', + UNIQUE(name, mbid) )", ); Self::execute(&mut stmt, ()) @@ -131,7 +132,8 @@ impl ISqlTransactionBackend for SqlTransactionSqliteBackend<'_> { day INT NULL, seq INT NOT NULL, primary_type JSON NOT NULL DEFAULT 'null', - secondary_types JSON NOT NULL DEFAULT '[]' + secondary_types JSON NOT NULL DEFAULT '[]', + UNIQUE(title, lib_id, mbid) )", ); Self::execute(&mut stmt, ()) @@ -145,7 +147,11 @@ impl ISqlTransactionBackend for SqlTransactionSqliteBackend<'_> { fn insert_database_version(&self, version: &str) -> Result<(), Error> { let mut stmt = self.prepare_cached( "INSERT INTO database_metadata (name, value) - VALUES (?1, ?2)", + VALUES (?1, ?2) + ON CONFLICT(name) DO UPDATE SET value = ?2 + WHERE EXISTS ( + SELECT 1 EXCEPT SELECT 1 WHERE value = ?2 + )", ); Self::execute(&mut stmt, ("version", version)) } @@ -163,7 +169,11 @@ impl ISqlTransactionBackend for SqlTransactionSqliteBackend<'_> { fn insert_artist(&self, artist: &SerializeArtist<'_>) -> Result<(), Error> { let mut stmt = self.prepare_cached( "INSERT INTO artists (name, mbid, sort, properties) - VALUES (?1, ?2, ?3, ?4)", + VALUES (?1, ?2, ?3, ?4) + ON CONFLICT(name, mbid) DO UPDATE SET sort = ?3, properties = ?4 + WHERE EXISTS ( + SELECT 1 EXCEPT SELECT 1 WHERE sort = ?3 AND properties = ?4 + )", ); Self::execute( &mut stmt, @@ -198,7 +208,15 @@ impl ISqlTransactionBackend for SqlTransactionSqliteBackend<'_> { let mut stmt = self.prepare_cached( "INSERT INTO albums (title, lib_id, mbid, artist_name, year, month, day, seq, primary_type, secondary_types) - VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9, ?10)", + VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9, ?10) + ON CONFLICT(title, lib_id, mbid) DO UPDATE SET + artist_name = ?4, year = ?5, month = ?6, day = ?7, seq = ?8, primary_type = ?9, + secondary_types = ?10 + WHERE EXISTS ( + SELECT 1 EXCEPT SELECT 1 WHERE + artist_name = ?4 AND year = ?5 AND month = ?6 AND day = ?7 AND seq = ?8 AND + primary_type = ?9 AND secondary_types = ?10 + )", ); Self::execute( &mut stmt, diff --git a/src/external/database/sql/mod.rs b/src/external/database/sql/mod.rs index 78a3305..711ee86 100644 --- a/src/external/database/sql/mod.rs +++ b/src/external/database/sql/mod.rs @@ -152,6 +152,15 @@ impl ISqlDatabaseBackend<'conn>> SqlDatabase { } impl ISqlDatabaseBackend<'conn>> IDatabase for SqlDatabase { + fn reset(&mut self) -> Result<(),SaveError> { + let tx = self.backend.transaction()?; + + Self::drop_tables(&tx)?; + Self::create_tables(&tx)?; + + Ok(tx.commit()?) + } + fn load(&mut self) -> Result { let tx = self.backend.transaction()?; @@ -178,7 +187,6 @@ impl ISqlDatabaseBackend<'conn>> IDatabase for SqlDatabase let database: SerializeDatabase = collection.into(); let tx = self.backend.transaction()?; - Self::drop_tables(&tx)?; Self::create_tables(&tx)?; match database { @@ -300,13 +308,22 @@ mod tests { SqlDatabase::new(backend).unwrap() } + #[test] + fn reset() { + let mut tx = MockISqlTransactionBackend::new(); + let mut seq = Sequence::new(); + expect_drop!(tx, seq); + expect_create!(tx, seq); + then0!(tx, seq, expect_commit); + assert!(database(VecDeque::from([tx])).reset().is_ok()); + } + #[test] fn save() { let write_data = FULL_COLLECTION.to_owned(); let mut tx = MockISqlTransactionBackend::new(); let mut seq = Sequence::new(); - expect_drop!(tx, seq); expect_create!(tx, seq); then1!(tx, seq, expect_insert_database_version).with(predicate::eq(V20250103)); for artist in write_data.iter() { @@ -396,7 +413,6 @@ mod tests { fn save_backend_exec_error() { let mut tx = MockISqlTransactionBackend::new(); let mut seq = Sequence::new(); - expect_drop!(tx, seq); expect_create!(tx, seq); then!(tx, seq, expect_insert_database_version) .with(predicate::eq(V20250103)) @@ -426,7 +442,6 @@ mod tests { let mut tx = MockISqlTransactionBackend::new(); let mut seq = Sequence::new(); - expect_drop!(tx, seq); expect_create!(tx, seq); then1!(tx, seq, expect_insert_database_version).with(predicate::eq(V20250103)); then!(tx, seq, expect_insert_artist) diff --git a/tests/database/sql.rs b/tests/database/sql.rs index ea7461a..017167b 100644 --- a/tests/database/sql.rs +++ b/tests/database/sql.rs @@ -9,7 +9,7 @@ use musichoard::{ }; use tempfile::NamedTempFile; -use crate::testlib::COLLECTION; +use crate::{copy_file_into_temp, testlib::COLLECTION}; pub static DATABASE_TEST_FILE: Lazy = Lazy::new(|| fs::canonicalize("./tests/files/database/database.db").unwrap()); @@ -24,6 +24,16 @@ fn expected() -> Collection { expected } +#[test] +fn reset() { + let file = NamedTempFile::new().unwrap(); + + let backend = SqlDatabaseSqliteBackend::new(file.path()).unwrap(); + let mut database = SqlDatabase::new(backend).unwrap(); + + database.reset().unwrap(); +} + #[test] fn save() { let file = NamedTempFile::new().unwrap(); @@ -61,3 +71,18 @@ fn reverse() { let expected = expected(); assert_eq!(read_data, expected); } + +#[test] +fn reverse_with_reset() { + let file = copy_file_into_temp(&*DATABASE_TEST_FILE); + + let backend = SqlDatabaseSqliteBackend::new(file.path()).unwrap(); + let mut database = SqlDatabase::new(backend).unwrap(); + + let expected: Vec = database.load().unwrap(); + database.reset().unwrap(); + database.save(&expected).unwrap(); + let read_data: Vec = database.load().unwrap(); + + assert_eq!(read_data, expected); +} diff --git a/tests/files/database/database.db b/tests/files/database/database.db index d61193509f0fd6070ba013057c4233f9e4f96025..75559ccf868cca8eb0425869f86a4e620282c79a 100644 GIT binary patch delta 636 zcmZozz}WDBae}lU3j+fKI}pPF|3n>QNfrjZssdjA9}H~VaSVL___y+I;XBVA$J@TK z@fnv)iyAAtxTGXwhk8k3Qch}OPEu)ZF@$8>tjTkMaq>kT;rdWN&%jVujgrigoKzi! zoXn*7%oH7k+@#DDO)dpwpsCHqCT=gu$dH+rl3Gz*n3GwO8lPBNk`Ly`Lkx~LgwX6w z63p!4($b7A>=4U~N-~Q}iW4Cei*t~xV~DFlh@;bF1HSzrcLYG)k(ZbYbOtu-by?WN z9i?$v4>1{NKZL=$`5&L9!Q@LehLdO6t4ubxH<}!1XRd5vY+_;vo z4E#;}E_{FZ_VFKJVB!%PxfZCT{A87IeDTd=XZvls)J7B)_7ED%vE zTMIU32vgk7f{n?ZMKzmq@)tYnMGgWIAfq|>_A~IS^WEax&p(U*2|o+}4t{T-(aZT1 zrC5cPB^iU&e5;kh5_3vZm8{HamHhJaQk6I~Sq*_wljZ%Cwb+$dm4WQgyriPk#B5Lm z!j+pqm9r_cs)CfS@{?s}^<%9DvETc7sIthiN&?xzIf;3hiEv#;P+iR0tfnB96Z}Wae}lU69WSSD-go~=R_T2Q6>hxssdjA9}Fx!ybOH*__y+L@bGRH6qv~^ z(_F#AE-op_*zR4Dn3R*6n3GhRTMQvtH&=0AV3g!iKn9wP#mwyD($b90wv+eq-sMM= s-TaTw(qM9rz0u?;c7~HP>{TY8u{Gm1vM@F=u`snTREpjF-=9$c04MM&d;kCd diff --git a/tests/lib.rs b/tests/lib.rs index 551af37..6cceef8 100644 --- a/tests/lib.rs +++ b/tests/lib.rs @@ -19,7 +19,7 @@ use tempfile::NamedTempFile; use crate::testlib::COLLECTION; -fn copy_file_into_temp>(path: P) -> NamedTempFile { +pub fn copy_file_into_temp>(path: P) -> NamedTempFile { let temp = NamedTempFile::new().unwrap(); fs::copy(path.into(), temp.path()).unwrap(); temp -- 2.47.1 From 8a9030bf610dade67c2ca955b924ed73777e594e Mon Sep 17 00:00:00 2001 From: Wojciech Kozlowski Date: Wed, 15 Jan 2025 21:42:28 +0100 Subject: [PATCH 07/12] Tests --- README.md | 9 +++++- tests/database/sql.rs | 13 ++------- .../database/{database.db => complete.db} | Bin tests/files/database/partial.db | Bin 0 -> 28672 bytes tests/lib.rs | 27 ++++++++++++++++-- 5 files changed, 35 insertions(+), 14 deletions(-) rename tests/files/database/{database.db => complete.db} (100%) create mode 100644 tests/files/database/partial.db diff --git a/README.md b/README.md index 15d5d1a..8de3320 100644 --- a/README.md +++ b/README.md @@ -8,7 +8,7 @@ This feature requires the `sqlite` library. -Either install system libraries: with +Either install system libraries with: On Fedora: ``` sh @@ -17,6 +17,13 @@ sudo dnf install sqlite-devel Or use a bundled version by enabling the `database-sqlite-bundled` feature. +To run the tests you will also need `sqldiff`. + +On Fedora: +``` sh +sudo dnf install sqlite-tools +``` + #### musicbrainz-api This feature requires the `openssl` system library. diff --git a/tests/database/sql.rs b/tests/database/sql.rs index 017167b..7690082 100644 --- a/tests/database/sql.rs +++ b/tests/database/sql.rs @@ -1,7 +1,3 @@ -use std::{fs, path::PathBuf}; - -use once_cell::sync::Lazy; - use musichoard::{ collection::{artist::Artist, Collection}, external::database::sql::{backend::SqlDatabaseSqliteBackend, SqlDatabase}, @@ -9,10 +5,7 @@ use musichoard::{ }; use tempfile::NamedTempFile; -use crate::{copy_file_into_temp, testlib::COLLECTION}; - -pub static DATABASE_TEST_FILE: Lazy = - Lazy::new(|| fs::canonicalize("./tests/files/database/database.db").unwrap()); +use crate::{copy_file_into_temp, testlib::COLLECTION, COMPLETE_DB_TEST_FILE}; fn expected() -> Collection { let mut expected = COLLECTION.to_owned(); @@ -47,7 +40,7 @@ fn save() { #[test] fn load() { - let backend = SqlDatabaseSqliteBackend::new(&*DATABASE_TEST_FILE).unwrap(); + let backend = SqlDatabaseSqliteBackend::new(&*COMPLETE_DB_TEST_FILE).unwrap(); let mut database = SqlDatabase::new(backend).unwrap(); let read_data: Vec = database.load().unwrap(); @@ -74,7 +67,7 @@ fn reverse() { #[test] fn reverse_with_reset() { - let file = copy_file_into_temp(&*DATABASE_TEST_FILE); + let file = copy_file_into_temp(&*COMPLETE_DB_TEST_FILE); let backend = SqlDatabaseSqliteBackend::new(file.path()).unwrap(); let mut database = SqlDatabase::new(backend).unwrap(); diff --git a/tests/files/database/database.db b/tests/files/database/complete.db similarity index 100% rename from tests/files/database/database.db rename to tests/files/database/complete.db diff --git a/tests/files/database/partial.db b/tests/files/database/partial.db new file mode 100644 index 0000000000000000000000000000000000000000..aae6c9a14671b08fe303fd0d820766698a147f13 GIT binary patch literal 28672 zcmeI4Pi)&%9LMc6PP%2WDFaHVV3w&r$Eh=kKkW!G=*qUX6I&I+i~Z7?ICf^c zEu*Tm79=<#Bo16TAi)UdAv;BM$t6&2+va#6>ojXp}BEnA8noc2YSnSr_(q! zTD~XA{Yu3W&rf00e*l5C8%|;NMH&QJcItd_e5T0Hj89A!`Jjxy{~*6}^9;|#{7mtg8Gh<%@|~TW9NB%9 zX;jLF&ObT*^i+5m|7h{C@!824{W#;#uJ#R@r|;d*5Y)o<$c@Bbh|)DSGrZZ_;OI)8BW77>d3iL_O3K=Cq&kwX|@C8V$HVnO>4K4`CO@;@$A&; zbF;w`^_IwA+;;&L5($p2$kL1Y`Jgmhnhza5mj3%3WTSrCmjRf00e*l5C8%|00;m9An@-eaG35JI-vT>Cgl@g$)P* z0U!VbfB+Bx0zd!=0D*lFSbK(v9jQ_4<7Y@3v1u9_S`nu0Iu=d}d8L}GW;8{_)dCiC zg^VU5jAgNqDc~`L^OY>B2&*f?nMK#oCKgHRzayNS6BazL;hs#VmzS4E>wZPn#?=8&X{6+K%e!RK>1oRtcyru80owjfE0npb;^e4!bN=!$;ox9@dV z0&AE0nw?vG%pt#Aa19-YLCUC*kz_>{tD2^hAgj8F^o%4Hw2WNMDN=>>w;$xb`=+{i zt8Si+^f+kLY+g}SHIpITyLKrO1XnZ{m#{$+^uyrD1sSEFB2mG5Ma*R+O;mAS7v+3K z%8ykss>evLStU}YNM5KZ>fOT!zleRXx?qm64DA8))D zMp;2f&*rnbs3^E9=CWf*RAp^Utd1#irjXB7vr@LFE<{evwoo_!MuZ7u=^Cmxwm!Cj zUEOvydwI0dx}f<8`=ok%O=ML$ms1s0B0cW^|DR#6QQSKBDEk|Gjk`fb*xQre?BjI@ zmjVGG00e*l5C8%|00;m9AOHk_z~4up(4ROwoH`OAzw#cscly|w72$&at8(FFel=JO zB9x}$Ukc;?vJ}FcFlj7dp)?UsNSmeu=ZU_-dkdJKH0BpPi)cY%OUka6Zfv$xD#jAS z!zsCW`+-@jLUg2>zW_#PoNF2XDIB_^$wwO5Z;0KqWbv#ejDiG zQvBP{gg}>RiB6E!MvrcWVlUZS@3U1U2<;AX$EkkyI>n8!-?7)Zceo$9DEA3hCT+k5 z1b_e#00KY&2mk>f00e*l5C8%|;GaU^9)_ej(Crc&`cE({ld+-pvq|g5E6#^c`b}R72lN Rq>0?c5NY7Rw7EkR_yafj!7u;- literal 0 HcmV?d00001 diff --git a/tests/lib.rs b/tests/lib.rs index 6cceef8..5900803 100644 --- a/tests/lib.rs +++ b/tests/lib.rs @@ -6,7 +6,7 @@ mod library; mod testlib; -use std::{fs, path::PathBuf}; +use std::{ffi::OsStr, fs, path::PathBuf, process::Command}; use musichoard::{ external::{ @@ -15,16 +15,33 @@ use musichoard::{ }, IMusicHoardBase, IMusicHoardDatabase, IMusicHoardLibrary, MusicHoard, }; +use once_cell::sync::Lazy; use tempfile::NamedTempFile; use crate::testlib::COLLECTION; +pub static PARTIAL_DB_TEST_FILE: Lazy = + Lazy::new(|| fs::canonicalize("./tests/files/database/partial.db").unwrap()); +pub static COMPLETE_DB_TEST_FILE: Lazy = + Lazy::new(|| fs::canonicalize("./tests/files/database/complete.db").unwrap()); + pub fn copy_file_into_temp>(path: P) -> NamedTempFile { let temp = NamedTempFile::new().unwrap(); fs::copy(path.into(), temp.path()).unwrap(); temp } +pub fn sqldiff(left: &OsStr, right: &OsStr) { + let output = Command::new("sqldiff") + .arg(left) + .arg(right) + .output() + .unwrap(); + assert!(output.status.success()); + let stdout = String::from_utf8_lossy(&output.stdout); + assert_eq!(stdout, ""); +} + #[test] fn merge_library_then_database() { // Acquired the lock on the beets config file. We need to own the underlying object so later we @@ -37,7 +54,7 @@ fn merge_library_then_database() { .config(Some(&*library::beets::BEETS_TEST_CONFIG_PATH)); let library = BeetsLibrary::new(executor); - let file = copy_file_into_temp(&*database::sql::DATABASE_TEST_FILE); + let file = copy_file_into_temp(&*PARTIAL_DB_TEST_FILE); let backend = SqlDatabaseSqliteBackend::new(file.path()).unwrap(); let database = SqlDatabase::new(backend).unwrap(); @@ -47,6 +64,8 @@ fn merge_library_then_database() { music_hoard.reload_database().unwrap(); assert_eq!(music_hoard.get_collection(), &*COLLECTION); + + sqldiff(&*COMPLETE_DB_TEST_FILE.as_os_str(), file.path().as_os_str()); } #[test] @@ -61,7 +80,7 @@ fn merge_database_then_library() { .config(Some(&*library::beets::BEETS_TEST_CONFIG_PATH)); let library = BeetsLibrary::new(executor); - let file = copy_file_into_temp(&*database::sql::DATABASE_TEST_FILE); + let file = copy_file_into_temp(&*PARTIAL_DB_TEST_FILE); let backend = SqlDatabaseSqliteBackend::new(file.path()).unwrap(); let database = SqlDatabase::new(backend).unwrap(); @@ -71,4 +90,6 @@ fn merge_database_then_library() { music_hoard.rescan_library().unwrap(); assert_eq!(music_hoard.get_collection(), &*COLLECTION); + + sqldiff(&*COMPLETE_DB_TEST_FILE.as_os_str(), file.path().as_os_str()); } -- 2.47.1 From 7934d7eccf0ea7b2597c23239a8f02519087b9c8 Mon Sep 17 00:00:00 2001 From: Wojciech Kozlowski Date: Sat, 18 Jan 2025 09:45:28 +0100 Subject: [PATCH 08/12] Correct artist id-info split --- src/core/collection/artist.rs | 62 +-------------------- src/core/musichoard/base.rs | 4 +- src/core/musichoard/library.rs | 8 +-- src/external/database/serde/deserialize.rs | 2 +- src/external/database/serde/serialize.rs | 2 +- src/external/database/sql/mod.rs | 2 +- src/testmod/full.rs | 8 +-- src/testmod/library.rs | 8 +-- src/tui/app/machine/search_state.rs | 2 +- src/tui/lib/external/musicbrainz/api/mod.rs | 2 +- tests/lib.rs | 4 +- tests/testlib.rs | 10 ++-- 12 files changed, 29 insertions(+), 85 deletions(-) diff --git a/src/core/collection/artist.rs b/src/core/collection/artist.rs index d67ca94..a605c10 100644 --- a/src/core/collection/artist.rs +++ b/src/core/collection/artist.rs @@ -22,13 +22,13 @@ pub struct Artist { #[derive(Clone, Debug, PartialEq, Eq)] pub struct ArtistMeta { pub id: ArtistId, - pub sort: Option, pub info: ArtistInfo, } /// Artist non-identifier metadata. #[derive(Clone, Debug, Default, PartialEq, Eq)] pub struct ArtistInfo { + pub sort: Option, pub properties: HashMap>, } @@ -152,7 +152,6 @@ impl ArtistMeta { pub fn new>(id: Id) -> Self { ArtistMeta { id: id.into(), - sort: None, info: ArtistInfo::default(), } } @@ -166,15 +165,7 @@ impl ArtistMeta { } pub fn get_sort_key(&self) -> (&str,) { - (self.sort.as_ref().unwrap_or(&self.id.name),) - } - - pub fn set_sort_key>(&mut self, sort: S) { - self.sort = Some(sort.into()); - } - - pub fn clear_sort_key(&mut self) { - self.sort.take(); + (self.info.sort.as_ref().unwrap_or(&self.id.name),) } } @@ -240,13 +231,13 @@ impl Merge for ArtistMeta { fn merge_in_place(&mut self, other: Self) { assert!(self.id.compatible(&other.id)); self.id.mb_ref = self.id.mb_ref.take().or(other.id.mb_ref); - self.sort = self.sort.take().or(other.sort); self.info.merge_in_place(other.info); } } impl Merge for ArtistInfo { fn merge_in_place(&mut self, other: Self) { + self.sort = self.sort.take().or(other.sort); self.properties.merge_in_place(other.properties); } } @@ -318,53 +309,6 @@ mod tests { static MUSICBUTLER: &str = "https://www.musicbutler.io/artist-page/483340948"; static MUSICBUTLER_2: &str = "https://www.musicbutler.io/artist-page/658903042/"; - #[test] - fn artist_sort_set_clear() { - let artist_id = ArtistId::new("an artist"); - let sort_id_1 = String::from("sort id 1"); - let sort_id_2 = String::from("sort id 2"); - - let mut artist = Artist::new(&artist_id.name); - - assert_eq!(artist.meta.id, artist_id); - assert_eq!(artist.meta.sort, None); - assert_eq!(artist.meta.get_sort_key(), (artist_id.name.as_str(),)); - assert!(artist.meta < ArtistMeta::new(sort_id_1.clone())); - assert!(artist.meta < ArtistMeta::new(sort_id_2.clone())); - assert!(artist < Artist::new(sort_id_1.clone())); - assert!(artist < Artist::new(sort_id_2.clone())); - - artist.meta.set_sort_key(sort_id_1.clone()); - - assert_eq!(artist.meta.id, artist_id); - assert_eq!(artist.meta.sort.as_ref(), Some(&sort_id_1)); - assert_eq!(artist.meta.get_sort_key(), (sort_id_1.as_str(),)); - assert!(artist.meta > ArtistMeta::new(artist_id.clone())); - assert!(artist.meta < ArtistMeta::new(sort_id_2.clone())); - assert!(artist > Artist::new(artist_id.clone())); - assert!(artist < Artist::new(sort_id_2.clone())); - - artist.meta.set_sort_key(sort_id_2.clone()); - - assert_eq!(artist.meta.id, artist_id); - assert_eq!(artist.meta.sort.as_ref(), Some(&sort_id_2)); - assert_eq!(artist.meta.get_sort_key(), (sort_id_2.as_str(),)); - assert!(artist.meta > ArtistMeta::new(artist_id.clone())); - assert!(artist.meta > ArtistMeta::new(sort_id_1.clone())); - assert!(artist > Artist::new(artist_id.clone())); - assert!(artist > Artist::new(sort_id_1.clone())); - - artist.meta.clear_sort_key(); - - assert_eq!(artist.meta.id, artist_id); - assert_eq!(artist.meta.sort, None); - assert_eq!(artist.meta.get_sort_key(), (artist_id.name.as_str(),)); - assert!(artist.meta < ArtistMeta::new(sort_id_1.clone())); - assert!(artist.meta < ArtistMeta::new(sort_id_2.clone())); - assert!(artist < Artist::new(sort_id_1.clone())); - assert!(artist < Artist::new(sort_id_2.clone())); - } - #[test] fn set_clear_musicbrainz_url() { let mut artist = Artist::new(ArtistId::new("an artist")); diff --git a/src/core/musichoard/base.rs b/src/core/musichoard/base.rs index edd63f1..f7d684e 100644 --- a/src/core/musichoard/base.rs +++ b/src/core/musichoard/base.rs @@ -216,13 +216,13 @@ mod tests { assert!(right.first().unwrap() > left.first().unwrap()); let artist_sort = Some(String::from("Album_Artist 0")); - right[0].meta.sort = artist_sort.clone(); + right[0].meta.info.sort = artist_sort.clone(); assert!(right.first().unwrap() < left.first().unwrap()); // The result of the merge should be the same list of artists, but with the last artist now // in first place. let mut expected = left.to_owned(); - expected.last_mut().as_mut().unwrap().meta.sort = artist_sort.clone(); + expected.last_mut().as_mut().unwrap().meta.info.sort = artist_sort.clone(); expected.rotate_right(1); let mut mh = MusicHoard { diff --git a/src/core/musichoard/library.rs b/src/core/musichoard/library.rs index 29a5ba0..e1e7f1d 100644 --- a/src/core/musichoard/library.rs +++ b/src/core/musichoard/library.rs @@ -92,17 +92,17 @@ impl MusicHoard { .or_insert_with(|| Artist::new(artist_id)), }; - if artist.meta.sort.is_some() { - if artist_sort.is_some() && (artist.meta.sort != artist_sort) { + if artist.meta.info.sort.is_some() { + if artist_sort.is_some() && (artist.meta.info.sort != artist_sort) { return Err(Error::CollectionError(format!( "multiple album_artist_sort found for artist '{}': '{}' != '{}'", artist.meta.id, - artist.meta.sort.as_ref().unwrap(), + artist.meta.info.sort.as_ref().unwrap(), artist_sort.as_ref().unwrap() ))); } } else if artist_sort.is_some() { - artist.meta.sort = artist_sort; + artist.meta.info.sort = artist_sort; } // Do a linear search as few artists have more than a handful of albums. Search from the diff --git a/src/external/database/serde/deserialize.rs b/src/external/database/serde/deserialize.rs index 0f4eb09..5d5a302 100644 --- a/src/external/database/serde/deserialize.rs +++ b/src/external/database/serde/deserialize.rs @@ -122,8 +122,8 @@ impl From for Artist { name: artist.name, mb_ref: artist.mb_ref.into(), }, - sort: artist.sort, info: ArtistInfo { + sort: artist.sort, properties: artist.properties, }, }, diff --git a/src/external/database/serde/serialize.rs b/src/external/database/serde/serialize.rs index dbb4145..8f99b4b 100644 --- a/src/external/database/serde/serialize.rs +++ b/src/external/database/serde/serialize.rs @@ -76,7 +76,7 @@ impl<'a> From<&'a Artist> for SerializeArtist<'a> { SerializeArtist { name: &artist.meta.id.name, mb_ref: (&artist.meta.id.mb_ref).into(), - sort: &artist.meta.sort, + sort: &artist.meta.info.sort, properties: &artist.meta.info.properties, albums: artist.albums.iter().map(Into::into).collect(), } diff --git a/src/external/database/sql/mod.rs b/src/external/database/sql/mod.rs index 711ee86..eecb94d 100644 --- a/src/external/database/sql/mod.rs +++ b/src/external/database/sql/mod.rs @@ -152,7 +152,7 @@ impl ISqlDatabaseBackend<'conn>> SqlDatabase { } impl ISqlDatabaseBackend<'conn>> IDatabase for SqlDatabase { - fn reset(&mut self) -> Result<(),SaveError> { + fn reset(&mut self) -> Result<(), SaveError> { let tx = self.backend.transaction()?; Self::drop_tables(&tx)?; diff --git a/src/testmod/full.rs b/src/testmod/full.rs index 4e5c1d2..d16c111 100644 --- a/src/testmod/full.rs +++ b/src/testmod/full.rs @@ -9,8 +9,8 @@ macro_rules! full_collection { "https://musicbrainz.org/artist/00000000-0000-0000-0000-000000000000" ).unwrap()), }, - sort: None, info: ArtistInfo { + sort: None, properties: HashMap::from([ (String::from("MusicButler"), vec![ String::from("https://www.musicbutler.io/artist-page/000000000"), @@ -139,8 +139,8 @@ macro_rules! full_collection { "https://musicbrainz.org/artist/11111111-1111-1111-1111-111111111111" ).unwrap()), }, - sort: None, info: ArtistInfo { + sort: None, properties: HashMap::from([ (String::from("MusicButler"), vec![ String::from("https://www.musicbutler.io/artist-page/111111111"), @@ -338,8 +338,8 @@ macro_rules! full_collection { name: "The Album_Artist ‘C’".to_string(), mb_ref: ArtistMbRef::CannotHaveMbid, }, - sort: Some("Album_Artist ‘C’, The".to_string()), info: ArtistInfo { + sort: Some("Album_Artist ‘C’, The".to_string()), properties: HashMap::new(), }, }, @@ -436,8 +436,8 @@ macro_rules! full_collection { name: "Album_Artist ‘D’".to_string(), mb_ref: ArtistMbRef::None, }, - sort: None, info: ArtistInfo { + sort: None, properties: HashMap::new(), }, }, diff --git a/src/testmod/library.rs b/src/testmod/library.rs index 90a1a69..f8cc7db 100644 --- a/src/testmod/library.rs +++ b/src/testmod/library.rs @@ -8,8 +8,8 @@ macro_rules! library_collection { name: "Album_Artist ‘A’".to_string(), mb_ref: ArtistMbRef::None, }, - sort: None, info: ArtistInfo { + sort: None, properties: HashMap::new(), }, }, @@ -119,8 +119,8 @@ macro_rules! library_collection { name: "Album_Artist ‘B’".to_string(), mb_ref: ArtistMbRef::None, }, - sort: None, info: ArtistInfo { + sort: None, properties: HashMap::new(), }, }, @@ -289,8 +289,8 @@ macro_rules! library_collection { name: "The Album_Artist ‘C’".to_string(), mb_ref: ArtistMbRef::None, }, - sort: Some("Album_Artist ‘C’, The".to_string()), info: ArtistInfo { + sort: Some("Album_Artist ‘C’, The".to_string()), properties: HashMap::new(), }, }, @@ -381,8 +381,8 @@ macro_rules! library_collection { name: "Album_Artist ‘D’".to_string(), mb_ref: ArtistMbRef::None, }, - sort: None, info: ArtistInfo { + sort: None, properties: HashMap::new(), }, }, diff --git a/src/tui/app/machine/search_state.rs b/src/tui/app/machine/search_state.rs index 1e4dc0d..a7f5701 100644 --- a/src/tui/app/machine/search_state.rs +++ b/src/tui/app/machine/search_state.rs @@ -162,7 +162,7 @@ impl IAppInteractSearchPrivate for AppMachine { let name = string::normalize_string_with(&probe.meta.id.name, !case_sens, !char_sens); let mut result = name.string.starts_with(search); - if let Some(ref probe_sort) = probe.meta.sort { + if let Some(ref probe_sort) = probe.meta.info.sort { if !result { let name = string::normalize_string_with(probe_sort, !case_sens, !char_sens); result = name.string.starts_with(search); diff --git a/src/tui/lib/external/musicbrainz/api/mod.rs b/src/tui/lib/external/musicbrainz/api/mod.rs index d648b8d..1ec116d 100644 --- a/src/tui/lib/external/musicbrainz/api/mod.rs +++ b/src/tui/lib/external/musicbrainz/api/mod.rs @@ -119,8 +119,8 @@ fn from_mb_artist_meta(meta: MbArtistMeta) -> (ArtistMeta, Option) { name: meta.name, mb_ref: ArtistMbRef::Some(meta.id.into()), }, - sort, info: ArtistInfo { + sort, properties: HashMap::new(), }, }, diff --git a/tests/lib.rs b/tests/lib.rs index 5900803..1abeee7 100644 --- a/tests/lib.rs +++ b/tests/lib.rs @@ -65,7 +65,7 @@ fn merge_library_then_database() { assert_eq!(music_hoard.get_collection(), &*COLLECTION); - sqldiff(&*COMPLETE_DB_TEST_FILE.as_os_str(), file.path().as_os_str()); + sqldiff(COMPLETE_DB_TEST_FILE.as_os_str(), file.path().as_os_str()); } #[test] @@ -91,5 +91,5 @@ fn merge_database_then_library() { assert_eq!(music_hoard.get_collection(), &*COLLECTION); - sqldiff(&*COMPLETE_DB_TEST_FILE.as_os_str(), file.path().as_os_str()); + sqldiff(COMPLETE_DB_TEST_FILE.as_os_str(), file.path().as_os_str()); } diff --git a/tests/testlib.rs b/tests/testlib.rs index 119042a..36a0035 100644 --- a/tests/testlib.rs +++ b/tests/testlib.rs @@ -22,8 +22,8 @@ pub static COLLECTION: Lazy> = Lazy::new(|| -> Collection { "https://musicbrainz.org/artist/baad262d-55ef-427a-83c7-f7530964f212" ).unwrap()), }, - sort: Some(String::from("Arkona")), info: ArtistInfo { + sort: Some(String::from("Arkona")), properties: HashMap::from([ (String::from("MusicButler"), vec![ String::from("https://www.musicbutler.io/artist-page/283448581"), @@ -217,8 +217,8 @@ pub static COLLECTION: Lazy> = Lazy::new(|| -> Collection { "https://musicbrainz.org/artist/8000598a-5edb-401c-8e6d-36b167feaf38" ).unwrap()), }, - sort: None, info: ArtistInfo { + sort: None, properties: HashMap::from([ (String::from("MusicButler"), vec![ String::from("https://www.musicbutler.io/artist-page/269358403"), @@ -472,8 +472,8 @@ pub static COLLECTION: Lazy> = Lazy::new(|| -> Collection { "https://musicbrainz.org/artist/3a901353-fccd-4afd-ad01-9c03f451b490" ).unwrap()), }, - sort: None, info: ArtistInfo { + sort: None, properties: HashMap::from([ (String::from("MusicButler"), vec![ String::from("https://www.musicbutler.io/artist-page/826588800"), @@ -631,8 +631,8 @@ pub static COLLECTION: Lazy> = Lazy::new(|| -> Collection { "https://musicbrainz.org/artist/c2c4d56a-d599-4a18-bd2f-ae644e2198cc" ).unwrap()), }, - sort: Some(String::from("Heaven’s Basement")), info: ArtistInfo { + sort: Some(String::from("Heaven’s Basement")), properties: HashMap::from([ (String::from("MusicButler"), vec![ String::from("https://www.musicbutler.io/artist-page/291158685"), @@ -770,8 +770,8 @@ pub static COLLECTION: Lazy> = Lazy::new(|| -> Collection { "https://musicbrainz.org/artist/65f4f0c5-ef9e-490c-aee3-909e7ae6b2ab" ).unwrap()), }, - sort: None, info: ArtistInfo { + sort: None, properties: HashMap::from([ (String::from("MusicButler"), vec![ String::from("https://www.musicbutler.io/artist-page/3996865"), -- 2.47.1 From fdd6d131468eaa485c613b59cf0d2862e064d72f Mon Sep 17 00:00:00 2001 From: Wojciech Kozlowski Date: Sat, 18 Jan 2025 10:14:18 +0100 Subject: [PATCH 09/12] Correct album id-info split --- src/core/collection/album.rs | 82 +++++++++++------- src/core/musichoard/database.rs | 7 +- src/core/testmod.rs | 4 +- src/external/database/serde/deserialize.rs | 4 +- src/external/database/serde/serialize.rs | 4 +- src/testmod/full.rs | 96 ++++++++------------- src/testmod/library.rs | 40 +++------ src/tui/app/machine/match_state.rs | 12 ++- src/tui/lib/external/musicbrainz/api/mod.rs | 6 +- src/tui/testmod.rs | 4 +- src/tui/ui/browse_state.rs | 2 +- src/tui/ui/display.rs | 2 +- src/tui/ui/mod.rs | 12 ++- tests/testlib.rs | 70 ++++++--------- 14 files changed, 152 insertions(+), 193 deletions(-) diff --git a/src/core/collection/album.rs b/src/core/collection/album.rs index 9e5c7e4..7945187 100644 --- a/src/core/collection/album.rs +++ b/src/core/collection/album.rs @@ -19,14 +19,14 @@ pub struct Album { #[derive(Clone, Debug, PartialEq, Eq)] pub struct AlbumMeta { pub id: AlbumId, - pub date: AlbumDate, - pub seq: AlbumSeq, pub info: AlbumInfo, } /// Album non-identifier metadata. #[derive(Clone, Debug, Default, PartialEq, Eq)] pub struct AlbumInfo { + pub date: AlbumDate, + pub seq: AlbumSeq, pub primary_type: Option, pub secondary_types: Vec, } @@ -93,6 +93,12 @@ impl From<(u32, u8, u8)> for AlbumDate { #[derive(Clone, Copy, Debug, Default, PartialEq, PartialOrd, Ord, Eq, Hash)] pub struct AlbumSeq(pub u8); +impl From for AlbumSeq { + fn from(value: u8) -> Self { + AlbumSeq(value) + } +} + /// Based on [MusicBrainz types](https://musicbrainz.org/doc/Release_Group/Type). #[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord)] pub enum AlbumPrimaryType { @@ -181,7 +187,7 @@ impl Album { } pub fn with_date>(mut self, date: Date) -> Self { - self.meta.date = date.into(); + self.meta.info.date = date.into(); self } @@ -214,14 +220,12 @@ impl AlbumMeta { pub fn new>(id: Id) -> Self { AlbumMeta { id: id.into(), - date: AlbumDate::default(), - seq: AlbumSeq::default(), info: AlbumInfo::default(), } } pub fn with_date>(mut self, date: Date) -> Self { - self.date = date.into(); + self.info.date = date.into(); self } @@ -232,8 +236,8 @@ impl AlbumMeta { pub fn get_sort_key(&self) -> (&AlbumDate, &AlbumSeq, &str, &Option) { ( - &self.date, - &self.seq, + &self.info.date, + &self.info.seq, &self.id.title, &self.info.primary_type, ) @@ -247,6 +251,36 @@ impl AlbumMeta { self.id.clear_mb_ref(); } + pub fn set_seq(&mut self, seq: AlbumSeq) { + self.info.set_seq(seq); + } + + pub fn clear_seq(&mut self) { + self.info.clear_seq(); + } +} + +impl AlbumInfo { + pub fn with_date>(mut self, date: Date) -> Self { + self.date = date.into(); + self + } + + pub fn with_seq>(mut self, seq: Seq) -> Self { + self.seq = seq.into(); + self + } + + pub fn with_primary_type(mut self, primary_type: AlbumPrimaryType) -> Self { + self.primary_type = Some(primary_type); + self + } + + pub fn with_secondary_types(mut self, secondary_types: Vec) -> Self { + self.secondary_types = secondary_types; + self + } + pub fn set_seq(&mut self, seq: AlbumSeq) { self.seq = seq; } @@ -256,18 +290,6 @@ impl AlbumMeta { } } -impl AlbumInfo { - pub fn new( - primary_type: Option, - secondary_types: Vec, - ) -> Self { - AlbumInfo { - primary_type, - secondary_types, - } - } -} - impl PartialOrd for AlbumMeta { fn partial_cmp(&self, other: &Self) -> Option { Some(self.cmp(other)) @@ -284,16 +306,16 @@ impl Merge for AlbumMeta { fn merge_in_place(&mut self, other: Self) { assert!(self.id.compatible(&other.id)); self.id.mb_ref = self.id.mb_ref.take().or(other.id.mb_ref); - if self.date.year.is_none() && other.date.year.is_some() { - self.date = other.date; - } - self.seq = std::cmp::max(self.seq, other.seq); self.info.merge_in_place(other.info); } } impl Merge for AlbumInfo { fn merge_in_place(&mut self, other: Self) { + if self.date.year.is_none() && other.date.year.is_some() { + self.date = other.date; + } + self.seq = std::cmp::max(self.seq, other.seq); self.primary_type = self.primary_type.take().or(other.primary_type); if self.secondary_types.is_empty() { self.secondary_types = other.secondary_types; @@ -385,21 +407,21 @@ mod tests { fn set_clear_seq() { let mut album = Album::new("An album"); - assert_eq!(album.meta.seq, AlbumSeq(0)); + assert_eq!(album.meta.info.seq, AlbumSeq(0)); // Setting a seq on an album. album.meta.set_seq(AlbumSeq(6)); - assert_eq!(album.meta.seq, AlbumSeq(6)); + assert_eq!(album.meta.info.seq, AlbumSeq(6)); album.meta.set_seq(AlbumSeq(6)); - assert_eq!(album.meta.seq, AlbumSeq(6)); + assert_eq!(album.meta.info.seq, AlbumSeq(6)); album.meta.set_seq(AlbumSeq(8)); - assert_eq!(album.meta.seq, AlbumSeq(8)); + assert_eq!(album.meta.info.seq, AlbumSeq(8)); // Clearing seq. album.meta.clear_seq(); - assert_eq!(album.meta.seq, AlbumSeq(0)); + assert_eq!(album.meta.info.seq, AlbumSeq(0)); } #[test] @@ -439,7 +461,7 @@ mod tests { #[test] fn merge_album_dates() { - let meta = AlbumMeta::new(AlbumId::new("An album")); + let meta = AlbumInfo::default(); // No merge if years are different. let left = meta.clone().with_date((2000, 1, 6)); diff --git a/src/core/musichoard/database.rs b/src/core/musichoard/database.rs index d73b817..3928724 100644 --- a/src/core/musichoard/database.rs +++ b/src/core/musichoard/database.rs @@ -554,10 +554,9 @@ mod tests { assert_eq!(meta.info.primary_type, None); assert_eq!(meta.info.secondary_types, Vec::new()); - let info = AlbumInfo::new( - Some(AlbumPrimaryType::Album), - vec![AlbumSecondaryType::Live], - ); + let info = AlbumInfo::default() + .with_primary_type(AlbumPrimaryType::Album) + .with_secondary_types(vec![AlbumSecondaryType::Live]); // Seting info on an album not belonging to the artist is an error. assert!(music_hoard diff --git a/src/core/testmod.rs b/src/core/testmod.rs index a812f67..0605975 100644 --- a/src/core/testmod.rs +++ b/src/core/testmod.rs @@ -2,9 +2,7 @@ use once_cell::sync::Lazy; use std::collections::HashMap; use crate::core::collection::{ - album::{ - Album, AlbumId, AlbumInfo, AlbumLibId, AlbumMbRef, AlbumMeta, AlbumPrimaryType, AlbumSeq, - }, + album::{Album, AlbumId, AlbumInfo, AlbumLibId, AlbumMbRef, AlbumMeta, AlbumPrimaryType}, artist::{Artist, ArtistId, ArtistInfo, ArtistMbRef, ArtistMeta}, musicbrainz::{MbAlbumRef, MbArtistRef}, track::{Track, TrackFormat, TrackId, TrackNum, TrackQuality}, diff --git a/src/external/database/serde/deserialize.rs b/src/external/database/serde/deserialize.rs index 5d5a302..81a1872 100644 --- a/src/external/database/serde/deserialize.rs +++ b/src/external/database/serde/deserialize.rs @@ -141,9 +141,9 @@ impl From for Album { lib_id: album.lib_id.into(), mb_ref: album.mb_ref.into(), }, - date: album.date.into(), - seq: AlbumSeq(album.seq), info: AlbumInfo { + date: album.date.into(), + seq: AlbumSeq(album.seq), primary_type: album.primary_type.map(Into::into), secondary_types: album.secondary_types.into_iter().map(Into::into).collect(), }, diff --git a/src/external/database/serde/serialize.rs b/src/external/database/serde/serialize.rs index 8f99b4b..3213c25 100644 --- a/src/external/database/serde/serialize.rs +++ b/src/external/database/serde/serialize.rs @@ -89,8 +89,8 @@ impl<'a> From<&'a Album> for SerializeAlbum<'a> { title: &album.meta.id.title, lib_id: album.meta.id.lib_id.into(), mb_ref: (&album.meta.id.mb_ref).into(), - date: album.meta.date.into(), - seq: album.meta.seq.0, + date: album.meta.info.date.into(), + seq: album.meta.info.seq.0, primary_type: album.meta.info.primary_type.map(Into::into), secondary_types: album .meta diff --git a/src/testmod/full.rs b/src/testmod/full.rs index d16c111..bc5f79b 100644 --- a/src/testmod/full.rs +++ b/src/testmod/full.rs @@ -33,12 +33,10 @@ macro_rules! full_collection { "https://musicbrainz.org/release-group/00000000-0000-0000-0000-000000000000" ).unwrap()), }, - date: 1998.into(), - seq: AlbumSeq(1), - info: AlbumInfo { - primary_type: Some(AlbumPrimaryType::Album), - secondary_types: vec![], - }, + info: AlbumInfo::default() + .with_date(1998) + .with_seq(1) + .with_primary_type(AlbumPrimaryType::Album), }, tracks: vec![ Track { @@ -97,12 +95,10 @@ macro_rules! full_collection { lib_id: AlbumLibId::Value(2), mb_ref: AlbumMbRef::None, }, - date: (2015, 4).into(), - seq: AlbumSeq(1), - info: AlbumInfo { - primary_type: Some(AlbumPrimaryType::Album), - secondary_types: vec![], - }, + info: AlbumInfo::default() + .with_date((2015, 4)) + .with_seq(1) + .with_primary_type(AlbumPrimaryType::Album), }, tracks: vec![ Track { @@ -165,12 +161,10 @@ macro_rules! full_collection { lib_id: AlbumLibId::Value(3), mb_ref: AlbumMbRef::None, }, - date: (2003, 6, 6).into(), - seq: AlbumSeq(1), - info: AlbumInfo { - primary_type: Some(AlbumPrimaryType::Album), - secondary_types: vec![], - }, + info: AlbumInfo::default() + .with_date((2003, 6, 6)) + .with_seq(1) + .with_primary_type(AlbumPrimaryType::Album), }, tracks: vec![ Track { @@ -209,12 +203,10 @@ macro_rules! full_collection { "https://musicbrainz.org/release-group/11111111-1111-1111-1111-111111111111" ).unwrap()), }, - date: 2008.into(), - seq: AlbumSeq(3), - info: AlbumInfo { - primary_type: Some(AlbumPrimaryType::Album), - secondary_types: vec![], - }, + info: AlbumInfo::default() + .with_date(2008) + .with_seq(3) + .with_primary_type(AlbumPrimaryType::Album), }, tracks: vec![ Track { @@ -253,12 +245,10 @@ macro_rules! full_collection { "https://musicbrainz.org/release-group/11111111-1111-1111-1111-111111111112" ).unwrap()), }, - date: 2009.into(), - seq: AlbumSeq(2), - info: AlbumInfo { - primary_type: Some(AlbumPrimaryType::Album), - secondary_types: vec![], - }, + info: AlbumInfo::default() + .with_date(2009) + .with_seq(2) + .with_primary_type(AlbumPrimaryType::Album), }, tracks: vec![ Track { @@ -295,12 +285,10 @@ macro_rules! full_collection { lib_id: AlbumLibId::Value(6), mb_ref: AlbumMbRef::None, }, - date: 2015.into(), - seq: AlbumSeq(4), - info: AlbumInfo { - primary_type: Some(AlbumPrimaryType::Album), - secondary_types: vec![], - }, + info: AlbumInfo::default() + .with_date(2015) + .with_seq(4) + .with_primary_type(AlbumPrimaryType::Album), }, tracks: vec![ Track { @@ -351,12 +339,9 @@ macro_rules! full_collection { lib_id: AlbumLibId::Value(7), mb_ref: AlbumMbRef::None, }, - date: 1985.into(), - seq: AlbumSeq(0), - info: AlbumInfo { - primary_type: Some(AlbumPrimaryType::Album), - secondary_types: vec![], - }, + info: AlbumInfo::default() + .with_date(1985) + .with_primary_type(AlbumPrimaryType::Album), }, tracks: vec![ Track { @@ -393,12 +378,9 @@ macro_rules! full_collection { lib_id: AlbumLibId::Value(8), mb_ref: AlbumMbRef::None, }, - date: 2018.into(), - seq: AlbumSeq(0), - info: AlbumInfo { - primary_type: Some(AlbumPrimaryType::Album), - secondary_types: vec![], - }, + info: AlbumInfo::default() + .with_date(2018) + .with_primary_type(AlbumPrimaryType::Album), }, tracks: vec![ Track { @@ -449,12 +431,9 @@ macro_rules! full_collection { lib_id: AlbumLibId::Value(9), mb_ref: AlbumMbRef::None, }, - date: 1995.into(), - seq: AlbumSeq(0), - info: AlbumInfo { - primary_type: Some(AlbumPrimaryType::Album), - secondary_types: vec![], - }, + info: AlbumInfo::default() + .with_date(1995) + .with_primary_type(AlbumPrimaryType::Album), }, tracks: vec![ Track { @@ -491,12 +470,9 @@ macro_rules! full_collection { lib_id: AlbumLibId::Value(10), mb_ref: AlbumMbRef::None, }, - date: 2028.into(), - seq: AlbumSeq(0), - info: AlbumInfo { - primary_type: Some(AlbumPrimaryType::Album), - secondary_types: vec![], - }, + info: AlbumInfo::default() + .with_date(2028) + .with_primary_type(AlbumPrimaryType::Album), }, tracks: vec![ Track { diff --git a/src/testmod/library.rs b/src/testmod/library.rs index f8cc7db..c5d0cfb 100644 --- a/src/testmod/library.rs +++ b/src/testmod/library.rs @@ -21,9 +21,7 @@ macro_rules! library_collection { lib_id: AlbumLibId::Value(1), mb_ref: AlbumMbRef::None, }, - date: 1998.into(), - seq: AlbumSeq(0), - info: AlbumInfo::default(), + info: AlbumInfo::default().with_date(1998), }, tracks: vec![ Track { @@ -82,9 +80,7 @@ macro_rules! library_collection { lib_id: AlbumLibId::Value(2), mb_ref: AlbumMbRef::None, }, - date: (2015, 4).into(), - seq: AlbumSeq(0), - info: AlbumInfo::default(), + info: AlbumInfo::default().with_date((2015, 4)), }, tracks: vec![ Track { @@ -132,9 +128,7 @@ macro_rules! library_collection { lib_id: AlbumLibId::Value(3), mb_ref: AlbumMbRef::None, }, - date: (2003, 6, 6).into(), - seq: AlbumSeq(0), - info: AlbumInfo::default(), + info: AlbumInfo::default().with_date((2003, 6, 6)), }, tracks: vec![ Track { @@ -171,9 +165,7 @@ macro_rules! library_collection { lib_id: AlbumLibId::Value(4), mb_ref: AlbumMbRef::None, }, - date: 2008.into(), - seq: AlbumSeq(0), - info: AlbumInfo::default(), + info: AlbumInfo::default().with_date(2008), }, tracks: vec![ Track { @@ -210,9 +202,7 @@ macro_rules! library_collection { lib_id: AlbumLibId::Value(5), mb_ref: AlbumMbRef::None, }, - date: 2009.into(), - seq: AlbumSeq(0), - info: AlbumInfo::default(), + info: AlbumInfo::default().with_date(2009), }, tracks: vec![ Track { @@ -249,9 +239,7 @@ macro_rules! library_collection { lib_id: AlbumLibId::Value(6), mb_ref: AlbumMbRef::None, }, - date: 2015.into(), - seq: AlbumSeq(0), - info: AlbumInfo::default(), + info: AlbumInfo::default().with_date(2015), }, tracks: vec![ Track { @@ -302,9 +290,7 @@ macro_rules! library_collection { lib_id: AlbumLibId::Value(7), mb_ref: AlbumMbRef::None, }, - date: 1985.into(), - seq: AlbumSeq(0), - info: AlbumInfo::default(), + info: AlbumInfo::default().with_date(1985), }, tracks: vec![ Track { @@ -341,9 +327,7 @@ macro_rules! library_collection { lib_id: AlbumLibId::Value(8), mb_ref: AlbumMbRef::None, }, - date: 2018.into(), - seq: AlbumSeq(0), - info: AlbumInfo::default(), + info: AlbumInfo::default().with_date(2018), }, tracks: vec![ Track { @@ -394,9 +378,7 @@ macro_rules! library_collection { lib_id: AlbumLibId::Value(9), mb_ref: AlbumMbRef::None, }, - date: 1995.into(), - seq: AlbumSeq(0), - info: AlbumInfo::default(), + info: AlbumInfo::default().with_date(1995), }, tracks: vec![ Track { @@ -433,9 +415,7 @@ macro_rules! library_collection { lib_id: AlbumLibId::Value(10), mb_ref: AlbumMbRef::None, }, - date: 2028.into(), - seq: AlbumSeq(0), - info: AlbumInfo::default(), + info: AlbumInfo::default().with_date(2028), }, tracks: vec![ Track { diff --git a/src/tui/app/machine/match_state.rs b/src/tui/app/machine/match_state.rs index 7abbef1..b6f4f2d 100644 --- a/src/tui/app/machine/match_state.rs +++ b/src/tui/app/machine/match_state.rs @@ -393,10 +393,14 @@ mod tests { fn album_meta(id: AlbumId) -> AlbumMeta { AlbumMeta::new(id) .with_date(AlbumDate::new(Some(1990), Some(5), None)) - .with_info(AlbumInfo::new( - Some(AlbumPrimaryType::Album), - vec![AlbumSecondaryType::Live, AlbumSecondaryType::Compilation], - )) + .with_info( + AlbumInfo::default() + .with_primary_type(AlbumPrimaryType::Album) + .with_secondary_types(vec![ + AlbumSecondaryType::Live, + AlbumSecondaryType::Compilation, + ]), + ) } fn album_match() -> EntityMatches { diff --git a/src/tui/lib/external/musicbrainz/api/mod.rs b/src/tui/lib/external/musicbrainz/api/mod.rs index 1ec116d..49d5c69 100644 --- a/src/tui/lib/external/musicbrainz/api/mod.rs +++ b/src/tui/lib/external/musicbrainz/api/mod.rs @@ -75,7 +75,7 @@ impl IMusicBrainz for MusicBrainz { ) -> Result>, Error> { // Some release groups may have a promotional early release messing up the search. Searching // with just the year should be enough anyway. - let date = AlbumDate::new(album.date.year, None, None); + let date = AlbumDate::new(album.info.date.year, None, None); let query = SearchReleaseGroupRequest::new() .arid(arid) @@ -135,9 +135,9 @@ fn from_mb_release_group_meta(meta: MbReleaseGroupMeta) -> AlbumMeta { lib_id: AlbumLibId::None, mb_ref: AlbumMbRef::Some(meta.id.into()), }, - date: meta.first_release_date, - seq: AlbumSeq::default(), info: AlbumInfo { + date: meta.first_release_date, + seq: AlbumSeq::default(), primary_type: meta.primary_type, secondary_types: meta.secondary_types.unwrap_or_default(), }, diff --git a/src/tui/testmod.rs b/src/tui/testmod.rs index 6346109..6c71186 100644 --- a/src/tui/testmod.rs +++ b/src/tui/testmod.rs @@ -1,9 +1,7 @@ use std::collections::HashMap; use musichoard::collection::{ - album::{ - Album, AlbumId, AlbumInfo, AlbumLibId, AlbumMbRef, AlbumMeta, AlbumPrimaryType, AlbumSeq, - }, + album::{Album, AlbumId, AlbumInfo, AlbumLibId, AlbumMbRef, AlbumMeta, AlbumPrimaryType}, artist::{Artist, ArtistId, ArtistInfo, ArtistMbRef, ArtistMeta}, musicbrainz::{MbAlbumRef, MbArtistRef}, track::{Track, TrackFormat, TrackId, TrackNum, TrackQuality}, diff --git a/src/tui/ui/browse_state.rs b/src/tui/ui/browse_state.rs index a99e195..b53e5b1 100644 --- a/src/tui/ui/browse_state.rs +++ b/src/tui/ui/browse_state.rs @@ -166,7 +166,7 @@ impl<'a, 'b> AlbumState<'a, 'b> { Ownership: {}", album.map(|a| a.meta.id.title.as_str()).unwrap_or(""), album - .map(|a| UiDisplay::display_date(&a.meta.date, &a.meta.seq)) + .map(|a| UiDisplay::display_date(&a.meta.info.date, &a.meta.info.seq)) .unwrap_or_default(), album .map(|a| UiDisplay::display_album_type( diff --git a/src/tui/ui/display.rs b/src/tui/ui/display.rs index 1c91792..efdb03d 100644 --- a/src/tui/ui/display.rs +++ b/src/tui/ui/display.rs @@ -171,7 +171,7 @@ impl UiDisplay { fn display_option_album(album: &AlbumMeta, _disambiguation: &Option) -> String { format!( "{:010} | {} [{}]", - UiDisplay::display_album_date(&album.date), + UiDisplay::display_album_date(&album.info.date), album.id.title, UiDisplay::display_album_type(&album.info.primary_type, &album.info.secondary_types), ) diff --git a/src/tui/ui/mod.rs b/src/tui/ui/mod.rs index c3e3ee3..b281816 100644 --- a/src/tui/ui/mod.rs +++ b/src/tui/ui/mod.rs @@ -360,10 +360,14 @@ mod tests { fn album_meta(id: AlbumId) -> AlbumMeta { AlbumMeta::new(id) .with_date(AlbumDate::new(Some(1990), Some(5), None)) - .with_info(AlbumInfo::new( - Some(AlbumPrimaryType::Album), - vec![AlbumSecondaryType::Live, AlbumSecondaryType::Compilation], - )) + .with_info( + AlbumInfo::default() + .with_primary_type(AlbumPrimaryType::Album) + .with_secondary_types(vec![ + AlbumSecondaryType::Live, + AlbumSecondaryType::Compilation, + ]), + ) } fn album_matches() -> EntityMatches { diff --git a/tests/testlib.rs b/tests/testlib.rs index 36a0035..a244c31 100644 --- a/tests/testlib.rs +++ b/tests/testlib.rs @@ -4,7 +4,7 @@ use std::collections::HashMap; use musichoard::collection::{ album::{ Album, AlbumId, AlbumInfo, AlbumLibId, AlbumMbRef, AlbumMeta, AlbumPrimaryType, - AlbumSecondaryType, AlbumSeq, + AlbumSecondaryType, }, artist::{Artist, ArtistId, ArtistInfo, ArtistMbRef, ArtistMeta}, musicbrainz::MbArtistRef, @@ -44,12 +44,9 @@ pub static COLLECTION: Lazy> = Lazy::new(|| -> Collection { lib_id: AlbumLibId::Value(7), mb_ref: AlbumMbRef::None, }, - date: 2011.into(), - seq: AlbumSeq(0), - info: AlbumInfo { - primary_type: Some(AlbumPrimaryType::Album), - secondary_types: vec![], - }, + info: AlbumInfo::default() + .with_date(2011) + .with_primary_type(AlbumPrimaryType::Album), }, tracks: vec![ Track { @@ -237,12 +234,9 @@ pub static COLLECTION: Lazy> = Lazy::new(|| -> Collection { lib_id: AlbumLibId::Value(1), mb_ref: AlbumMbRef::None, }, - date: 2004.into(), - seq: AlbumSeq(0), - info: AlbumInfo { - primary_type: Some(AlbumPrimaryType::Ep), - secondary_types: vec![], - }, + info: AlbumInfo::default() + .with_date(2004) + .with_primary_type(AlbumPrimaryType::Ep), }, tracks: vec![ Track { @@ -320,12 +314,9 @@ pub static COLLECTION: Lazy> = Lazy::new(|| -> Collection { lib_id: AlbumLibId::Value(2), mb_ref: AlbumMbRef::None, }, - date: 2008.into(), - seq: AlbumSeq(0), - info: AlbumInfo { - primary_type: Some(AlbumPrimaryType::Album), - secondary_types: vec![], - }, + info: AlbumInfo::default() + .with_date(2008) + .with_primary_type(AlbumPrimaryType::Album), }, tracks: vec![ Track { @@ -491,12 +482,9 @@ pub static COLLECTION: Lazy> = Lazy::new(|| -> Collection { lib_id: AlbumLibId::Value(3), mb_ref: AlbumMbRef::None, }, - date: 2001.into(), - seq: AlbumSeq(0), - info: AlbumInfo { - primary_type: Some(AlbumPrimaryType::Album), - secondary_types: vec![], - }, + info: AlbumInfo::default() + .with_date(2001) + .with_primary_type(AlbumPrimaryType::Album), }, tracks: vec![ Track { @@ -650,9 +638,7 @@ pub static COLLECTION: Lazy> = Lazy::new(|| -> Collection { lib_id: AlbumLibId::Singleton, mb_ref: AlbumMbRef::None, }, - date: 2011.into(), - seq: AlbumSeq(0), - info: AlbumInfo::default(), + info: AlbumInfo::default().with_date(2011), }, tracks: vec![ Track { @@ -674,12 +660,9 @@ pub static COLLECTION: Lazy> = Lazy::new(|| -> Collection { lib_id: AlbumLibId::Value(4), mb_ref: AlbumMbRef::None, }, - date: 2011.into(), - seq: AlbumSeq(0), - info: AlbumInfo { - primary_type: Some(AlbumPrimaryType::Album), - secondary_types: vec![], - }, + info: AlbumInfo::default() + .with_date(2011) + .with_primary_type(AlbumPrimaryType::Album), }, tracks: vec![ Track { @@ -790,12 +773,9 @@ pub static COLLECTION: Lazy> = Lazy::new(|| -> Collection { lib_id: AlbumLibId::Value(5), mb_ref: AlbumMbRef::None, }, - date: 1984.into(), - seq: AlbumSeq(0), - info: AlbumInfo { - primary_type: Some(AlbumPrimaryType::Album), - secondary_types: vec![], - }, + info: AlbumInfo::default() + .with_date(1984) + .with_primary_type(AlbumPrimaryType::Album), }, tracks: vec![ Track { @@ -895,12 +875,10 @@ pub static COLLECTION: Lazy> = Lazy::new(|| -> Collection { lib_id: AlbumLibId::Value(6), mb_ref: AlbumMbRef::None, }, - date: 1999.into(), - seq: AlbumSeq(0), - info: AlbumInfo { - primary_type: Some(AlbumPrimaryType::Album), - secondary_types: vec![AlbumSecondaryType::Live], - }, + info: AlbumInfo::default() + .with_date(1999) + .with_primary_type(AlbumPrimaryType::Album) + .with_secondary_types(vec![AlbumSecondaryType::Live]), }, tracks: vec![ Track { -- 2.47.1 From 3901c161bd83f17b05b8894be61a16d4f41f8805 Mon Sep 17 00:00:00 2001 From: Wojciech Kozlowski Date: Sat, 18 Jan 2025 11:30:41 +0100 Subject: [PATCH 10/12] Move artist mbref back to info --- src/core/collection/artist.rs | 85 ++++++++++--------- src/core/collection/musicbrainz.rs | 6 ++ src/core/musichoard/database.rs | 73 ++-------------- src/core/musichoard/library.rs | 3 +- src/external/database/serde/deserialize.rs | 2 +- src/external/database/serde/serialize.rs | 2 +- src/testmod/full.rs | 16 ++-- src/testmod/library.rs | 8 +- src/tui/app/machine/fetch_state.rs | 8 +- src/tui/app/machine/match_state.rs | 49 +++-------- src/tui/lib/external/musicbrainz/api/mod.rs | 2 +- .../lib/external/musicbrainz/daemon/mod.rs | 6 +- src/tui/lib/mod.rs | 15 +--- src/tui/ui/info_state.rs | 2 +- tests/testlib.rs | 30 +++---- 15 files changed, 108 insertions(+), 199 deletions(-) diff --git a/src/core/collection/artist.rs b/src/core/collection/artist.rs index a605c10..76c8222 100644 --- a/src/core/collection/artist.rs +++ b/src/core/collection/artist.rs @@ -29,6 +29,7 @@ pub struct ArtistMeta { #[derive(Clone, Debug, Default, PartialEq, Eq)] pub struct ArtistInfo { pub sort: Option, + pub mb_ref: ArtistMbRef, pub properties: HashMap>, } @@ -36,7 +37,6 @@ pub struct ArtistInfo { #[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)] pub struct ArtistId { pub name: String, - pub mb_ref: ArtistMbRef, } /// Unique database identifier. Use MBID for this purpose. @@ -156,12 +156,27 @@ impl ArtistMeta { } } + // TODO: move to info once name moves there too. + pub fn compatible(&self, other: &ArtistMeta) -> bool { + let names_compatible = + string::normalize_string(&self.id.name) == string::normalize_string(&other.id.name); + let mb_ref_compatible = self.info.mb_ref.is_none() + || other.info.mb_ref.is_none() + || (self.info.mb_ref == other.info.mb_ref); + names_compatible && mb_ref_compatible + } + + pub fn with_mb_ref(mut self, mb_ref: ArtistMbRef) -> Self { + self.info.set_mb_ref(mb_ref); + self + } + pub fn set_mb_ref(&mut self, mb_ref: ArtistMbRef) { - self.id.set_mb_ref(mb_ref); + self.info.set_mb_ref(mb_ref); } pub fn clear_mb_ref(&mut self) { - self.id.clear_mb_ref(); + self.info.clear_mb_ref(); } pub fn get_sort_key(&self) -> (&str,) { @@ -194,6 +209,19 @@ impl ArtistInfo { } } + pub fn with_mb_ref(mut self, mb_ref: ArtistMbRef) -> Self { + self.set_mb_ref(mb_ref); + self + } + + pub fn set_mb_ref(&mut self, mb_ref: ArtistMbRef) { + self.mb_ref = mb_ref; + } + + pub fn clear_mb_ref(&mut self) { + self.mb_ref.take(); + } + pub fn remove_from_property>(&mut self, property: S, values: Vec) { if let Some(container) = self.properties.get_mut(property.as_ref()) { container.retain(|val| !values.iter().any(|x| x.as_ref() == val)); @@ -229,8 +257,7 @@ impl Ord for ArtistMeta { impl Merge for ArtistMeta { fn merge_in_place(&mut self, other: Self) { - assert!(self.id.compatible(&other.id)); - self.id.mb_ref = self.id.mb_ref.take().or(other.id.mb_ref); + assert!(self.compatible(&other)); self.info.merge_in_place(other.info); } } @@ -238,6 +265,7 @@ impl Merge for ArtistMeta { impl Merge for ArtistInfo { fn merge_in_place(&mut self, other: Self) { self.sort = self.sort.take().or(other.sort); + self.mb_ref = self.mb_ref.take().or(other.mb_ref); self.properties.merge_in_place(other.properties); } } @@ -256,31 +284,7 @@ impl AsRef for ArtistId { impl ArtistId { pub fn new>(name: S) -> ArtistId { - ArtistId { - name: name.into(), - mb_ref: ArtistMbRef::None, - } - } - - pub fn with_mb_ref(mut self, mb_ref: ArtistMbRef) -> Self { - self.mb_ref = mb_ref; - self - } - - pub fn set_mb_ref(&mut self, mb_ref: ArtistMbRef) { - self.mb_ref = mb_ref; - } - - pub fn clear_mb_ref(&mut self) { - self.mb_ref.take(); - } - - pub fn compatible(&self, other: &ArtistId) -> bool { - let names_compatible = - string::normalize_string(&self.name) == string::normalize_string(&other.name); - let mb_ref_compatible = - self.mb_ref.is_none() || other.mb_ref.is_none() || (self.mb_ref == other.mb_ref); - names_compatible && mb_ref_compatible + ArtistId { name: name.into() } } } @@ -314,30 +318,30 @@ mod tests { let mut artist = Artist::new(ArtistId::new("an artist")); let mut expected: MbRefOption = MbRefOption::None; - assert_eq!(artist.meta.id.mb_ref, expected); + assert_eq!(artist.meta.info.mb_ref, expected); // Setting a URL on an artist. - artist.meta.id.set_mb_ref(MbRefOption::Some( + artist.meta.info.set_mb_ref(MbRefOption::Some( MbArtistRef::from_url_str(MUSICBRAINZ).unwrap(), )); expected.replace(MbArtistRef::from_url_str(MUSICBRAINZ).unwrap()); - assert_eq!(artist.meta.id.mb_ref, expected); + assert_eq!(artist.meta.info.mb_ref, expected); - artist.meta.id.set_mb_ref(MbRefOption::Some( + artist.meta.info.set_mb_ref(MbRefOption::Some( MbArtistRef::from_url_str(MUSICBRAINZ).unwrap(), )); - assert_eq!(artist.meta.id.mb_ref, expected); + assert_eq!(artist.meta.info.mb_ref, expected); - artist.meta.id.set_mb_ref(MbRefOption::Some( + artist.meta.info.set_mb_ref(MbRefOption::Some( MbArtistRef::from_url_str(MUSICBRAINZ_2).unwrap(), )); expected.replace(MbArtistRef::from_url_str(MUSICBRAINZ_2).unwrap()); - assert_eq!(artist.meta.id.mb_ref, expected); + assert_eq!(artist.meta.info.mb_ref, expected); // Clearing URLs. - artist.meta.id.clear_mb_ref(); + artist.meta.info.clear_mb_ref(); expected.take(); - assert_eq!(artist.meta.id.mb_ref, expected); + assert_eq!(artist.meta.info.mb_ref, expected); } #[test] @@ -453,7 +457,7 @@ mod tests { let left = FULL_COLLECTION[0].to_owned(); let mut right = FULL_COLLECTION[1].to_owned(); right.meta.id = left.meta.id.clone(); - right.meta.id.mb_ref = MbRefOption::None; + right.meta.info.mb_ref = MbRefOption::None; right.meta.info.properties = HashMap::new(); let mut expected = left.clone(); @@ -490,6 +494,7 @@ mod tests { let mut left = FULL_COLLECTION[0].to_owned(); let mut right = FULL_COLLECTION[1].to_owned(); right.meta.id = left.meta.id.clone(); + right.meta.info.mb_ref = left.meta.info.mb_ref.clone(); // The right collection needs more albums than we modify to make sure some do not overlap. assert!(right.albums.len() > 2); diff --git a/src/core/collection/musicbrainz.rs b/src/core/collection/musicbrainz.rs index 092144f..49f9f1b 100644 --- a/src/core/collection/musicbrainz.rs +++ b/src/core/collection/musicbrainz.rs @@ -45,6 +45,12 @@ pub enum MbRefOption { None, } +impl Default for MbRefOption { + fn default() -> Self { + MbRefOption::None + } +} + impl MbRefOption { pub fn is_some(&self) -> bool { matches!(self, MbRefOption::Some(_)) diff --git a/src/core/musichoard/database.rs b/src/core/musichoard/database.rs index 3928724..aac0310 100644 --- a/src/core/musichoard/database.rs +++ b/src/core/musichoard/database.rs @@ -3,7 +3,7 @@ use std::mem; use crate::{ collection::{ album::{AlbumInfo, AlbumMbRef, AlbumMeta}, - artist::{ArtistInfo, ArtistMbRef}, + artist::ArtistInfo, merge::Merge, }, core::{ @@ -20,13 +20,6 @@ use crate::{ pub trait IMusicHoardDatabase { fn reload_database(&mut self) -> Result<(), Error>; - fn set_artist_mb_ref>( - &mut self, - artist_id: Id, - mb_ref: ArtistMbRef, - ) -> Result<(), Error>; - fn clear_artist_mb_ref>(&mut self, artist_id: Id) -> Result<(), Error>; - fn merge_artist_info>( &mut self, artist_id: Id, @@ -81,18 +74,6 @@ impl IMusicHoardDatabase for MusicHoard>( - &mut self, - artist_id: Id, - mb_ref: ArtistMbRef, - ) -> Result<(), Error> { - self.update_artist(artist_id.as_ref(), |artist| artist.meta.set_mb_ref(mb_ref)) - } - - fn clear_artist_mb_ref>(&mut self, artist_id: Id) -> Result<(), Error> { - self.update_artist(artist_id.as_ref(), |artist| artist.meta.clear_mb_ref()) - } - fn merge_artist_info>( &mut self, artist_id: Id, @@ -296,6 +277,7 @@ mod tests { use crate::{ collection::{ album::{AlbumPrimaryType, AlbumSecondaryType}, + artist::ArtistMbRef, musicbrainz::MbArtistRef, }, core::{ @@ -325,52 +307,6 @@ mod tests { assert_eq!(actual_err.to_string(), expected_err.to_string()); } - #[test] - fn set_clear_artist_mb_ref() { - let mut database = MockIDatabase::new(); - database.expect_save().times(2).returning(|_| Ok(())); - - let mut artist_id = ArtistId::new("an artist"); - let artist_id_2 = ArtistId::new("another artist"); - let mut music_hoard = MusicHoard::database(database); - - music_hoard.collection.push(Artist::new(artist_id.clone())); - music_hoard.collection.sort_unstable(); - - let mut expected = ArtistMbRef::None; - assert_eq!(music_hoard.collection[0].meta.id.mb_ref, expected); - - let mb_ref = ArtistMbRef::Some(MbArtistRef::from_uuid_str(MBID).unwrap()); - - // Setting a mb_ref on an artist not in the collection is an error. - assert!(music_hoard - .set_artist_mb_ref(&artist_id_2, mb_ref.clone()) - .is_err()); - assert_eq!(music_hoard.collection[0].meta.id.mb_ref, expected); - - // Setting a mb_ref on an artist. - assert!(music_hoard - .set_artist_mb_ref(&artist_id, mb_ref.clone()) - .is_ok()); - expected.replace(MbArtistRef::from_uuid_str(MBID).unwrap()); - assert_eq!(music_hoard.collection[0].meta.id.mb_ref, expected); - - // Clearing mb_ref on an artist that does not exist is an error. - assert!(music_hoard.clear_artist_mb_ref(&artist_id_2).is_err()); - assert_eq!(music_hoard.collection[0].meta.id.mb_ref, expected); - - // Clearing mb_ref from an artist without the mb_ref set is an error. Effectively the album - // does not exist. - assert!(music_hoard.clear_artist_mb_ref(&artist_id).is_err()); - assert_eq!(music_hoard.collection[0].meta.id.mb_ref, expected); - - // Clearing mb_ref. - artist_id.set_mb_ref(mb_ref); - assert!(music_hoard.clear_artist_mb_ref(&artist_id).is_ok()); - expected.take(); - assert_eq!(music_hoard.collection[0].meta.id.mb_ref, expected); - } - #[test] fn set_clear_artist_info() { let mut database = MockIDatabase::new(); @@ -378,6 +314,7 @@ mod tests { let artist_id = ArtistId::new("an artist"); let artist_id_2 = ArtistId::new("another artist"); + let mb_ref = ArtistMbRef::Some(MbArtistRef::from_uuid_str(MBID).unwrap()); let mut music_hoard = MusicHoard::database(database); music_hoard.collection.push(Artist::new(artist_id.clone())); @@ -386,7 +323,7 @@ mod tests { let mut expected = ArtistInfo::default(); assert_eq!(music_hoard.collection[0].meta.info, expected); - let mut info = ArtistInfo::default(); + let mut info = ArtistInfo::default().with_mb_ref(mb_ref.clone()); info.add_to_property("property", vec!["value-1", "value-2"]); // Setting info on an artist not in the collection is an error. @@ -399,6 +336,7 @@ mod tests { assert!(music_hoard .merge_artist_info(&artist_id, info.clone()) .is_ok()); + expected.mb_ref = mb_ref.clone(); expected.properties.insert( String::from("property"), vec![String::from("value-1"), String::from("value-2")], @@ -411,6 +349,7 @@ mod tests { // Clearing info. assert!(music_hoard.clear_artist_info(&artist_id).is_ok()); + expected.mb_ref.take(); expected.properties.clear(); assert_eq!(music_hoard.collection[0].meta.info, expected); } diff --git a/src/core/musichoard/library.rs b/src/core/musichoard/library.rs index e1e7f1d..bf2f529 100644 --- a/src/core/musichoard/library.rs +++ b/src/core/musichoard/library.rs @@ -3,7 +3,7 @@ use std::collections::HashMap; use crate::core::{ collection::{ album::{Album, AlbumDate, AlbumId, AlbumMbRef}, - artist::{Artist, ArtistId, ArtistMbRef}, + artist::{Artist, ArtistId}, track::{Track, TrackId, TrackNum, TrackQuality}, Collection, }, @@ -53,7 +53,6 @@ impl MusicHoard { for item in items.into_iter() { let artist_id = ArtistId { name: item.album_artist, - mb_ref: ArtistMbRef::None, }; let artist_sort = item.album_artist_sort; diff --git a/src/external/database/serde/deserialize.rs b/src/external/database/serde/deserialize.rs index 81a1872..d06aad3 100644 --- a/src/external/database/serde/deserialize.rs +++ b/src/external/database/serde/deserialize.rs @@ -120,10 +120,10 @@ impl From for Artist { meta: ArtistMeta { id: ArtistId { name: artist.name, - mb_ref: artist.mb_ref.into(), }, info: ArtistInfo { sort: artist.sort, + mb_ref: artist.mb_ref.into(), properties: artist.properties, }, }, diff --git a/src/external/database/serde/serialize.rs b/src/external/database/serde/serialize.rs index 3213c25..7a529fa 100644 --- a/src/external/database/serde/serialize.rs +++ b/src/external/database/serde/serialize.rs @@ -75,7 +75,7 @@ impl<'a> From<&'a Artist> for SerializeArtist<'a> { fn from(artist: &'a Artist) -> Self { SerializeArtist { name: &artist.meta.id.name, - mb_ref: (&artist.meta.id.mb_ref).into(), + mb_ref: (&artist.meta.info.mb_ref).into(), sort: &artist.meta.info.sort, properties: &artist.meta.info.properties, albums: artist.albums.iter().map(Into::into).collect(), diff --git a/src/testmod/full.rs b/src/testmod/full.rs index bc5f79b..0c00195 100644 --- a/src/testmod/full.rs +++ b/src/testmod/full.rs @@ -5,12 +5,12 @@ macro_rules! full_collection { meta: ArtistMeta { id: ArtistId { name: "Album_Artist ‘A’".to_string(), - mb_ref: ArtistMbRef::Some(MbArtistRef::from_url_str( - "https://musicbrainz.org/artist/00000000-0000-0000-0000-000000000000" - ).unwrap()), }, info: ArtistInfo { sort: None, + mb_ref: ArtistMbRef::Some(MbArtistRef::from_url_str( + "https://musicbrainz.org/artist/00000000-0000-0000-0000-000000000000" + ).unwrap()), properties: HashMap::from([ (String::from("MusicButler"), vec![ String::from("https://www.musicbutler.io/artist-page/000000000"), @@ -131,12 +131,12 @@ macro_rules! full_collection { meta: ArtistMeta { id: ArtistId { name: "Album_Artist ‘B’".to_string(), - mb_ref: ArtistMbRef::Some(MbArtistRef::from_url_str( - "https://musicbrainz.org/artist/11111111-1111-1111-1111-111111111111" - ).unwrap()), }, info: ArtistInfo { sort: None, + mb_ref: ArtistMbRef::Some(MbArtistRef::from_url_str( + "https://musicbrainz.org/artist/11111111-1111-1111-1111-111111111111" + ).unwrap()), properties: HashMap::from([ (String::from("MusicButler"), vec![ String::from("https://www.musicbutler.io/artist-page/111111111"), @@ -324,10 +324,10 @@ macro_rules! full_collection { meta: ArtistMeta { id: ArtistId { name: "The Album_Artist ‘C’".to_string(), - mb_ref: ArtistMbRef::CannotHaveMbid, }, info: ArtistInfo { sort: Some("Album_Artist ‘C’, The".to_string()), + mb_ref: ArtistMbRef::CannotHaveMbid, properties: HashMap::new(), }, }, @@ -416,10 +416,10 @@ macro_rules! full_collection { meta: ArtistMeta { id: ArtistId { name: "Album_Artist ‘D’".to_string(), - mb_ref: ArtistMbRef::None, }, info: ArtistInfo { sort: None, + mb_ref: ArtistMbRef::None, properties: HashMap::new(), }, }, diff --git a/src/testmod/library.rs b/src/testmod/library.rs index c5d0cfb..86eb376 100644 --- a/src/testmod/library.rs +++ b/src/testmod/library.rs @@ -6,10 +6,10 @@ macro_rules! library_collection { meta: ArtistMeta { id: ArtistId { name: "Album_Artist ‘A’".to_string(), - mb_ref: ArtistMbRef::None, }, info: ArtistInfo { sort: None, + mb_ref: ArtistMbRef::None, properties: HashMap::new(), }, }, @@ -113,10 +113,10 @@ macro_rules! library_collection { meta: ArtistMeta { id: ArtistId { name: "Album_Artist ‘B’".to_string(), - mb_ref: ArtistMbRef::None, }, info: ArtistInfo { sort: None, + mb_ref: ArtistMbRef::None, properties: HashMap::new(), }, }, @@ -275,10 +275,10 @@ macro_rules! library_collection { meta: ArtistMeta { id: ArtistId { name: "The Album_Artist ‘C’".to_string(), - mb_ref: ArtistMbRef::None, }, info: ArtistInfo { sort: Some("Album_Artist ‘C’, The".to_string()), + mb_ref: ArtistMbRef::None, properties: HashMap::new(), }, }, @@ -363,10 +363,10 @@ macro_rules! library_collection { meta: ArtistMeta { id: ArtistId { name: "Album_Artist ‘D’".to_string(), - mb_ref: ArtistMbRef::None, }, info: ArtistInfo { sort: None, + mb_ref: ArtistMbRef::None, properties: HashMap::new(), }, }, diff --git a/src/tui/app/machine/fetch_state.rs b/src/tui/app/machine/fetch_state.rs index 55eb380..3c0deeb 100644 --- a/src/tui/app/machine/fetch_state.rs +++ b/src/tui/app/machine/fetch_state.rs @@ -115,14 +115,14 @@ impl AppMachine { let mut requests = Self::search_artist_job(artist); if requests.is_empty() { fetch = FetchState::fetch(rx); - requests = Self::browse_release_group_job(&artist.meta.id.mb_ref); + requests = Self::browse_release_group_job(&artist.meta.info.mb_ref); } else { fetch = FetchState::search(rx); } SubmitJob { fetch, requests } } _ => { - let arid = match artist.meta.id.mb_ref { + let arid = match artist.meta.info.mb_ref { MbRefOption::Some(ref mbref) => mbref, _ => return Err("cannot fetch album: artist has no MBID"), }; @@ -262,7 +262,7 @@ impl AppMachine { } fn search_artist_job(artist: &Artist) -> VecDeque { - match artist.meta.id.mb_ref { + match artist.meta.info.mb_ref { MbRefOption::Some(ref arid) => { Self::search_albums_requests(&artist.meta.id, arid, &artist.albums) } @@ -802,7 +802,7 @@ mod tests { } fn browse_release_group_expectation(artist: &Artist) -> MockIMbJobSender { - let requests = AppMachine::browse_release_group_job(&artist.meta.id.mb_ref); + let requests = AppMachine::browse_release_group_job(&artist.meta.info.mb_ref); let mut mb_job_sender = MockIMbJobSender::new(); mb_job_sender .expect_submit_background_job() diff --git a/src/tui/app/machine/match_state.rs b/src/tui/app/machine/match_state.rs index b6f4f2d..2121bc2 100644 --- a/src/tui/app/machine/match_state.rs +++ b/src/tui/app/machine/match_state.rs @@ -2,7 +2,7 @@ use std::cmp; use musichoard::collection::{ album::{AlbumInfo, AlbumMbRef, AlbumMeta}, - artist::{ArtistInfo, ArtistMbRef, ArtistMeta}, + artist::{ArtistInfo, ArtistMeta}, musicbrainz::{MbRefOption, Mbid}, }; @@ -13,11 +13,6 @@ use crate::tui::app::{ MatchOption, MatchStatePublic, WidgetState, }; -struct ArtistInfoTuple { - mb_ref: ArtistMbRef, - info: ArtistInfo, -} - struct AlbumInfoTuple { mb_ref: AlbumMbRef, info: AlbumInfo, @@ -27,7 +22,7 @@ trait GetInfoMeta { type InfoType; } impl GetInfoMeta for ArtistMeta { - type InfoType = ArtistInfoTuple; + type InfoType = ArtistInfo; } impl GetInfoMeta for AlbumMeta { type InfoType = AlbumInfoTuple; @@ -44,20 +39,18 @@ enum InfoOption { } impl GetInfo for MatchOption { - type InfoType = ArtistInfoTuple; + type InfoType = ArtistInfo; fn get_info(&self) -> InfoOption { - let mb_ref; let mut info = ArtistInfo::default(); match self { MatchOption::Some(option) => { - mb_ref = option.entity.id.mb_ref.clone(); info = option.entity.info.clone(); } - MatchOption::CannotHaveMbid => mb_ref = MbRefOption::CannotHaveMbid, + MatchOption::CannotHaveMbid => info.mb_ref = MbRefOption::CannotHaveMbid, MatchOption::ManualInputMbid => return InfoOption::NeedInput, } - InfoOption::Info(ArtistInfoTuple { mb_ref, info }) + InfoOption::Info(info) } } @@ -205,11 +198,10 @@ impl AppMachine { fn select_artist( inner: &mut AppInner, matches: &ArtistMatches, - tuple: ArtistInfoTuple, + info: ArtistInfo, ) -> Result<(), musichoard::Error> { let mh = &mut inner.music_hoard; - mh.merge_artist_info(&matches.matching.id, tuple.info)?; - mh.set_artist_mb_ref(&matches.matching.id, tuple.mb_ref) + mh.merge_artist_info(&matches.matching.id, info) } fn filter_mb_ref(left: &AlbumMbRef, right: &AlbumMbRef) -> bool { @@ -326,7 +318,7 @@ mod tests { album::{ Album, AlbumDate, AlbumId, AlbumInfo, AlbumMeta, AlbumPrimaryType, AlbumSecondaryType, }, - artist::{Artist, ArtistId, ArtistMeta}, + artist::{Artist, ArtistId, ArtistMbRef, ArtistMeta}, Collection, }; @@ -361,7 +353,7 @@ mod tests { } fn artist_meta() -> ArtistMeta { - ArtistMeta::new(ArtistId::new("Artist").with_mb_ref(ArtistMbRef::Some(mbid().into()))) + ArtistMeta::new(ArtistId::new("Artist")).with_mb_ref(ArtistMbRef::Some(mbid().into())) } fn artist_match() -> EntityMatches { @@ -495,21 +487,12 @@ mod tests { .return_once(|_, _, _| Ok(())); } EntityMatches::Artist(_) => { - let mb_ref = MbRefOption::CannotHaveMbid; - let info = ArtistInfo::default(); + let info = ArtistInfo::default().with_mb_ref(MbRefOption::CannotHaveMbid); - let mut seq = Sequence::new(); music_hoard .expect_merge_artist_info() .with(eq(artist_id.clone()), eq(info)) .times(1) - .in_sequence(&mut seq) - .return_once(|_, _| Ok(())); - music_hoard - .expect_set_artist_mb_ref() - .with(eq(artist_id.clone()), eq(mb_ref)) - .times(1) - .in_sequence(&mut seq) .return_once(|_, _| Ok(())); } } @@ -589,22 +572,12 @@ mod tests { match matches_info { EntityMatches::Album(_) => panic!(), EntityMatches::Artist(_) => { - let mut meta = artist_meta(); - let mb_ref = meta.id.mb_ref.clone(); - meta.clear_mb_ref(); + let meta = artist_meta(); - let mut seq = Sequence::new(); music_hoard .expect_merge_artist_info() .with(eq(meta.id.clone()), eq(meta.info)) .times(1) - .in_sequence(&mut seq) - .return_once(|_, _| Ok(())); - music_hoard - .expect_set_artist_mb_ref() - .with(eq(meta.id.clone()), eq(mb_ref)) - .times(1) - .in_sequence(&mut seq) .return_once(|_, _| Ok(())); } } diff --git a/src/tui/lib/external/musicbrainz/api/mod.rs b/src/tui/lib/external/musicbrainz/api/mod.rs index 49d5c69..578628c 100644 --- a/src/tui/lib/external/musicbrainz/api/mod.rs +++ b/src/tui/lib/external/musicbrainz/api/mod.rs @@ -117,10 +117,10 @@ fn from_mb_artist_meta(meta: MbArtistMeta) -> (ArtistMeta, Option) { ArtistMeta { id: ArtistId { name: meta.name, - mb_ref: ArtistMbRef::Some(meta.id.into()), }, info: ArtistInfo { sort, + mb_ref: ArtistMbRef::Some(meta.id.into()), properties: HashMap::new(), }, }, diff --git a/src/tui/lib/external/musicbrainz/daemon/mod.rs b/src/tui/lib/external/musicbrainz/daemon/mod.rs index 13becb4..da140aa 100644 --- a/src/tui/lib/external/musicbrainz/daemon/mod.rs +++ b/src/tui/lib/external/musicbrainz/daemon/mod.rs @@ -454,7 +454,7 @@ mod tests { } fn search_albums_requests() -> VecDeque { - let mbref = mb_ref_opt_as_ref(&COLLECTION[1].meta.id.mb_ref); + let mbref = mb_ref_opt_as_ref(&COLLECTION[1].meta.info.mb_ref); let arid = mb_ref_opt_unwrap(mbref).mbid().clone(); let artist_id = COLLECTION[1].meta.id.clone(); @@ -468,7 +468,7 @@ mod tests { } fn browse_albums_requests() -> VecDeque { - let mbref = mb_ref_opt_as_ref(&COLLECTION[1].meta.id.mb_ref); + let mbref = mb_ref_opt_as_ref(&COLLECTION[1].meta.info.mb_ref); let arid = mb_ref_opt_unwrap(mbref).mbid().clone(); VecDeque::from([MbParams::browse_release_group(arid)]) } @@ -478,7 +478,7 @@ mod tests { } fn album_arid_expectation() -> Mbid { - let mbref = mb_ref_opt_as_ref(&COLLECTION[1].meta.id.mb_ref); + let mbref = mb_ref_opt_as_ref(&COLLECTION[1].meta.info.mb_ref); mb_ref_opt_unwrap(mbref).mbid().clone() } diff --git a/src/tui/lib/mod.rs b/src/tui/lib/mod.rs index ef67eec..29337ec 100644 --- a/src/tui/lib/mod.rs +++ b/src/tui/lib/mod.rs @@ -4,7 +4,7 @@ pub mod interface; use musichoard::{ collection::{ album::{AlbumId, AlbumInfo, AlbumMbRef, AlbumMeta}, - artist::{ArtistId, ArtistInfo, ArtistMbRef}, + artist::{ArtistId, ArtistInfo}, Collection, }, interface::{database::IDatabase, library::ILibrary}, @@ -33,11 +33,6 @@ pub trait IMusicHoard { album_id: &AlbumId, ) -> Result<(), musichoard::Error>; - fn set_artist_mb_ref( - &mut self, - artist_id: &ArtistId, - mb_ref: ArtistMbRef, - ) -> Result<(), musichoard::Error>; fn merge_artist_info( &mut self, id: &ArtistId, @@ -91,14 +86,6 @@ impl IMusicHoard for MusicHoard::remove_album(self, artist_id, album_id) } - fn set_artist_mb_ref( - &mut self, - artist_id: &ArtistId, - mb_ref: ArtistMbRef, - ) -> Result<(), musichoard::Error> { - ::set_artist_mb_ref(self, artist_id, mb_ref) - } - fn merge_artist_info( &mut self, id: &ArtistId, diff --git a/src/tui/ui/info_state.rs b/src/tui/ui/info_state.rs index 218c9ca..234e33d 100644 --- a/src/tui/ui/info_state.rs +++ b/src/tui/ui/info_state.rs @@ -76,7 +76,7 @@ impl<'a> ArtistOverlay<'a> { Properties: {}", artist.map(|a| a.meta.id.name.as_str()).unwrap_or(""), artist - .map(|a| UiDisplay::display_mb_ref_option_as_url(&a.meta.id.mb_ref)) + .map(|a| UiDisplay::display_mb_ref_option_as_url(&a.meta.info.mb_ref)) .unwrap_or_default(), Self::opt_hashmap_to_string( artist.map(|a| &a.meta.info.properties), diff --git a/tests/testlib.rs b/tests/testlib.rs index a244c31..2664f41 100644 --- a/tests/testlib.rs +++ b/tests/testlib.rs @@ -18,12 +18,12 @@ pub static COLLECTION: Lazy> = Lazy::new(|| -> Collection { meta: ArtistMeta { id: ArtistId { name: String::from("Аркона"), - mb_ref: ArtistMbRef::Some(MbArtistRef::from_url_str( - "https://musicbrainz.org/artist/baad262d-55ef-427a-83c7-f7530964f212" - ).unwrap()), }, info: ArtistInfo { sort: Some(String::from("Arkona")), + mb_ref: ArtistMbRef::Some(MbArtistRef::from_url_str( + "https://musicbrainz.org/artist/baad262d-55ef-427a-83c7-f7530964f212" + ).unwrap()), properties: HashMap::from([ (String::from("MusicButler"), vec![ String::from("https://www.musicbutler.io/artist-page/283448581"), @@ -210,12 +210,12 @@ pub static COLLECTION: Lazy> = Lazy::new(|| -> Collection { meta: ArtistMeta { id: ArtistId { name: String::from("Eluveitie"), - mb_ref: ArtistMbRef::Some(MbArtistRef::from_url_str( - "https://musicbrainz.org/artist/8000598a-5edb-401c-8e6d-36b167feaf38" - ).unwrap()), }, info: ArtistInfo { sort: None, + mb_ref: ArtistMbRef::Some(MbArtistRef::from_url_str( + "https://musicbrainz.org/artist/8000598a-5edb-401c-8e6d-36b167feaf38" + ).unwrap()), properties: HashMap::from([ (String::from("MusicButler"), vec![ String::from("https://www.musicbutler.io/artist-page/269358403"), @@ -459,12 +459,12 @@ pub static COLLECTION: Lazy> = Lazy::new(|| -> Collection { meta: ArtistMeta { id: ArtistId { name: String::from("Frontside"), - mb_ref: ArtistMbRef::Some(MbArtistRef::from_url_str( - "https://musicbrainz.org/artist/3a901353-fccd-4afd-ad01-9c03f451b490" - ).unwrap()), }, info: ArtistInfo { sort: None, + mb_ref: ArtistMbRef::Some(MbArtistRef::from_url_str( + "https://musicbrainz.org/artist/3a901353-fccd-4afd-ad01-9c03f451b490" + ).unwrap()), properties: HashMap::from([ (String::from("MusicButler"), vec![ String::from("https://www.musicbutler.io/artist-page/826588800"), @@ -615,12 +615,12 @@ pub static COLLECTION: Lazy> = Lazy::new(|| -> Collection { meta: ArtistMeta { id: ArtistId { name: String::from("Heaven’s Basement"), - mb_ref: ArtistMbRef::Some(MbArtistRef::from_url_str( - "https://musicbrainz.org/artist/c2c4d56a-d599-4a18-bd2f-ae644e2198cc" - ).unwrap()), }, info: ArtistInfo { sort: Some(String::from("Heaven’s Basement")), + mb_ref: ArtistMbRef::Some(MbArtistRef::from_url_str( + "https://musicbrainz.org/artist/c2c4d56a-d599-4a18-bd2f-ae644e2198cc" + ).unwrap()), properties: HashMap::from([ (String::from("MusicButler"), vec![ String::from("https://www.musicbutler.io/artist-page/291158685"), @@ -749,12 +749,12 @@ pub static COLLECTION: Lazy> = Lazy::new(|| -> Collection { meta: ArtistMeta { id: ArtistId { name: String::from("Metallica"), - mb_ref: ArtistMbRef::Some(MbArtistRef::from_url_str( - "https://musicbrainz.org/artist/65f4f0c5-ef9e-490c-aee3-909e7ae6b2ab" - ).unwrap()), }, info: ArtistInfo { sort: None, + mb_ref: ArtistMbRef::Some(MbArtistRef::from_url_str( + "https://musicbrainz.org/artist/65f4f0c5-ef9e-490c-aee3-909e7ae6b2ab" + ).unwrap()), properties: HashMap::from([ (String::from("MusicButler"), vec![ String::from("https://www.musicbutler.io/artist-page/3996865"), -- 2.47.1 From 3b0fa28dfcedafb92898af91e44e3c61879c143b Mon Sep 17 00:00:00 2001 From: Wojciech Kozlowski Date: Sun, 19 Jan 2025 10:22:48 +0100 Subject: [PATCH 11/12] WIP: use unique artist id --- .gitea/workflows/gitea-ci.yaml | 6 +- src/core/collection/album.rs | 19 +- src/core/collection/artist.rs | 248 +++++++-------- src/core/collection/merge.rs | 29 +- src/core/interface/database/mod.rs | 9 +- src/core/musichoard/base.rs | 291 ++++++++++-------- src/core/musichoard/builder.rs | 10 +- src/core/musichoard/database.rs | 52 ++-- src/core/musichoard/library.rs | 68 ++-- src/core/musichoard/mod.rs | 44 ++- src/external/database/serde/deserialize.rs | 8 +- src/external/database/serde/serialize.rs | 25 +- src/external/database/sql/backend.rs | 41 ++- src/external/database/sql/mod.rs | 34 +- src/external/database/sql/testmod.rs | 4 + src/testmod/full.rs | 28 +- src/testmod/library.rs | 28 +- src/tui/app/machine/fetch_state.rs | 131 ++++---- src/tui/app/machine/match_state.rs | 74 +++-- src/tui/app/machine/mod.rs | 14 +- src/tui/app/machine/search_state.rs | 4 +- src/tui/app/mod.rs | 42 ++- src/tui/app/selection/artist.rs | 10 +- src/tui/lib/external/musicbrainz/api/mod.rs | 27 +- .../lib/external/musicbrainz/daemon/mod.rs | 82 ++--- src/tui/lib/interface/musicbrainz/api/mod.rs | 15 +- .../lib/interface/musicbrainz/daemon/mod.rs | 40 ++- src/tui/ui/browse_state.rs | 2 +- src/tui/ui/display.rs | 10 +- src/tui/ui/info_state.rs | 2 +- src/tui/ui/match_state.rs | 6 +- src/tui/ui/mod.rs | 40 ++- tests/testlib.rs | 35 +-- 33 files changed, 874 insertions(+), 604 deletions(-) diff --git a/.gitea/workflows/gitea-ci.yaml b/.gitea/workflows/gitea-ci.yaml index 597d4e3..1d2793c 100644 --- a/.gitea/workflows/gitea-ci.yaml +++ b/.gitea/workflows/gitea-ci.yaml @@ -22,11 +22,11 @@ jobs: steps: - uses: actions/checkout@v3 - run: cargo build --no-default-features --all-targets - - run: cargo test --no-default-features --all-targets --no-fail-fast + - run: cargo test --no-default-features --all-targets --no-fail-fast -- --include-ignored - run: cargo build --all-targets - - run: cargo test --all-targets --no-fail-fast + - run: cargo test --all-targets --no-fail-fast -- --include-ignored - run: cargo build --all-features --all-targets - - run: cargo test --all-features --all-targets --no-fail-fast + - run: cargo test --all-features --all-targets --no-fail-fast -- --include-ignored - run: >- grcov target/debug/profraw --binary-path target/debug/ diff --git a/src/core/collection/album.rs b/src/core/collection/album.rs index 7945187..05f4c43 100644 --- a/src/core/collection/album.rs +++ b/src/core/collection/album.rs @@ -1,7 +1,7 @@ use std::mem; use crate::core::collection::{ - merge::{Merge, MergeSorted}, + merge::{IntoId, Merge, MergeSorted, WithId}, musicbrainz::{MbAlbumRef, MbRefOption}, track::{Track, TrackFormat}, }; @@ -208,6 +208,23 @@ impl Ord for Album { } } +impl WithId for Album { + type Id = AlbumId; + + fn id(&self) -> &Self::Id { + &self.meta.id + } +} + +impl IntoId for Album { + type Id = AlbumId; + type IdSelf = Album; + + fn into_id(self, _: &Self::Id) -> Self::IdSelf { + self + } +} + impl Merge for Album { fn merge_in_place(&mut self, other: Self) { self.meta.merge_in_place(other.meta); diff --git a/src/core/collection/artist.rs b/src/core/collection/artist.rs index 76c8222..3acefd0 100644 --- a/src/core/collection/artist.rs +++ b/src/core/collection/artist.rs @@ -11,56 +11,57 @@ use crate::core::collection::{ string::{self, NormalString}, }; +use super::merge::WithId; + /// An artist. #[derive(Clone, Debug, PartialEq, Eq)] pub struct Artist { + pub id: ArtistId, pub meta: ArtistMeta, pub albums: Vec, } -/// Artist metadata. #[derive(Clone, Debug, PartialEq, Eq)] pub struct ArtistMeta { - pub id: ArtistId, + pub name: ArtistName, + pub sort: Option, pub info: ArtistInfo, } -/// Artist non-identifier metadata. #[derive(Clone, Debug, Default, PartialEq, Eq)] pub struct ArtistInfo { - pub sort: Option, pub mb_ref: ArtistMbRef, pub properties: HashMap>, } /// The artist identifier. -#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)] -pub struct ArtistId { - pub name: String, +#[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)] +pub struct ArtistId(pub usize); + +impl From for ArtistId { + fn from(value: usize) -> Self { + ArtistId(value) + } } +impl AsRef for ArtistId { + fn as_ref(&self) -> &ArtistId { + self + } +} + +impl Display for ArtistId { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + write!(f, "{}", self.0) + } +} + +/// The artist name. +pub type ArtistName = String; + /// Unique database identifier. Use MBID for this purpose. pub type ArtistMbRef = MbRefOption; -impl PartialOrd for Artist { - fn partial_cmp(&self, other: &Self) -> Option { - Some(self.cmp(other)) - } -} - -impl Ord for Artist { - fn cmp(&self, other: &Self) -> std::cmp::Ordering { - self.meta.cmp(&other.meta) - } -} - -impl Merge for Artist { - fn merge_in_place(&mut self, other: Self) { - self.meta.merge_in_place(other.meta); - self.albums = MergeAlbums::merge_albums(mem::take(&mut self.albums), other.albums); - } -} - #[derive(Debug, Default)] struct MergeAlbums { primary_by_lib_id: HashMap, @@ -74,10 +75,10 @@ impl MergeAlbums { fn merge_albums(primary_albums: Vec, secondary_albums: Vec) -> Vec { let mut cache = MergeAlbums::new(primary_albums); cache.merge_albums_by_lib_id(secondary_albums); - cache.merged.extend(MergeCollections::merge_by_name( - cache.primary_by_title, - cache.secondary_by_title, - )); + let (merged, left) = + MergeCollections::merge_by_name(cache.primary_by_title, cache.secondary_by_title); + cache.merged.extend(merged); + cache.merged.extend(left); cache.merged.sort_unstable(); cache.merged } @@ -140,55 +141,65 @@ impl MergeAlbums { impl Artist { /// Create new [`Artist`] with the given [`ArtistId`]. - pub fn new>(id: Id) -> Self { + pub fn new, Name: Into>(id: Id, name: Name) -> Self { Artist { - meta: ArtistMeta::new(id), + id: id.into(), + meta: ArtistMeta::new(name), albums: vec![], } } } impl ArtistMeta { - pub fn new>(id: Id) -> Self { + pub fn new>(name: Name) -> Self { ArtistMeta { - id: id.into(), + name: name.into(), + sort: None, info: ArtistInfo::default(), } } - // TODO: move to info once name moves there too. pub fn compatible(&self, other: &ArtistMeta) -> bool { let names_compatible = - string::normalize_string(&self.id.name) == string::normalize_string(&other.id.name); + string::normalize_string(&self.name) == string::normalize_string(&other.name); let mb_ref_compatible = self.info.mb_ref.is_none() || other.info.mb_ref.is_none() || (self.info.mb_ref == other.info.mb_ref); names_compatible && mb_ref_compatible } + pub fn get_sort_key(&self) -> (&str,) { + (self.sort.as_ref().unwrap_or(&self.name),) + } + + pub fn with_sort>(mut self, name: S) -> Self { + self.sort = Some(name.into()); + self + } + pub fn with_mb_ref(mut self, mb_ref: ArtistMbRef) -> Self { self.info.set_mb_ref(mb_ref); self } - - pub fn set_mb_ref(&mut self, mb_ref: ArtistMbRef) { - self.info.set_mb_ref(mb_ref); - } - - pub fn clear_mb_ref(&mut self) { - self.info.clear_mb_ref(); - } - - pub fn get_sort_key(&self) -> (&str,) { - (self.info.sort.as_ref().unwrap_or(&self.id.name),) - } } impl ArtistInfo { + pub fn with_mb_ref(mut self, mb_ref: ArtistMbRef) -> Self { + self.set_mb_ref(mb_ref); + self + } + + pub fn set_mb_ref(&mut self, mb_ref: ArtistMbRef) { + self.mb_ref = mb_ref; + } + + pub fn clear_mb_ref(&mut self) { + self.mb_ref.take(); + } + // In the functions below, it would be better to use `contains` instead of `iter().any`, but for // type reasons that does not work: // https://stackoverflow.com/questions/48985924/why-does-a-str-not-coerce-to-a-string-when-using-veccontains - pub fn add_to_property + Into>(&mut self, property: S, values: Vec) { match self.properties.get_mut(property.as_ref()) { Some(container) => { @@ -209,19 +220,6 @@ impl ArtistInfo { } } - pub fn with_mb_ref(mut self, mb_ref: ArtistMbRef) -> Self { - self.set_mb_ref(mb_ref); - self - } - - pub fn set_mb_ref(&mut self, mb_ref: ArtistMbRef) { - self.mb_ref = mb_ref; - } - - pub fn clear_mb_ref(&mut self) { - self.mb_ref.take(); - } - pub fn remove_from_property>(&mut self, property: S, values: Vec) { if let Some(container) = self.properties.get_mut(property.as_ref()) { container.retain(|val| !values.iter().any(|x| x.as_ref() == val)); @@ -243,6 +241,49 @@ impl ArtistInfo { } } +impl WithId for Artist { + type Id = ArtistId; + + fn id(&self) -> &Self::Id { + &self.id + } +} + +impl Merge for Artist { + fn merge_in_place(&mut self, other: Self) { + assert_eq!(self.id, other.id); + self.meta.merge_in_place(other.meta); + self.albums = MergeAlbums::merge_albums(mem::take(&mut self.albums), other.albums); + } +} + +impl Merge for ArtistMeta { + fn merge_in_place(&mut self, other: Self) { + assert!(self.compatible(&other)); + // No merge for name - always keep original. + self.info.merge_in_place(other.info); + } +} + +impl Merge for ArtistInfo { + fn merge_in_place(&mut self, other: Self) { + self.mb_ref = self.mb_ref.take().or(other.mb_ref); + self.properties.merge_in_place(other.properties); + } +} + +impl PartialOrd for Artist { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } +} + +impl Ord for Artist { + fn cmp(&self, other: &Self) -> std::cmp::Ordering { + self.meta.cmp(&other.meta) + } +} + impl PartialOrd for ArtistMeta { fn partial_cmp(&self, other: &Self) -> Option { Some(self.cmp(other)) @@ -255,45 +296,6 @@ impl Ord for ArtistMeta { } } -impl Merge for ArtistMeta { - fn merge_in_place(&mut self, other: Self) { - assert!(self.compatible(&other)); - self.info.merge_in_place(other.info); - } -} - -impl Merge for ArtistInfo { - fn merge_in_place(&mut self, other: Self) { - self.sort = self.sort.take().or(other.sort); - self.mb_ref = self.mb_ref.take().or(other.mb_ref); - self.properties.merge_in_place(other.properties); - } -} - -impl> From for ArtistId { - fn from(value: S) -> Self { - ArtistId::new(value) - } -} - -impl AsRef for ArtistId { - fn as_ref(&self) -> &ArtistId { - self - } -} - -impl ArtistId { - pub fn new>(name: S) -> ArtistId { - ArtistId { name: name.into() } - } -} - -impl Display for ArtistId { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - write!(f, "{}", self.name) - } -} - #[cfg(test)] mod tests { use crate::{ @@ -315,40 +317,39 @@ mod tests { #[test] fn set_clear_musicbrainz_url() { - let mut artist = Artist::new(ArtistId::new("an artist")); + let mut info = ArtistInfo::default(); let mut expected: MbRefOption = MbRefOption::None; - assert_eq!(artist.meta.info.mb_ref, expected); + assert_eq!(info.mb_ref, expected); - // Setting a URL on an artist. - artist.meta.info.set_mb_ref(MbRefOption::Some( + // Setting a URL on an info. + info.set_mb_ref(MbRefOption::Some( MbArtistRef::from_url_str(MUSICBRAINZ).unwrap(), )); expected.replace(MbArtistRef::from_url_str(MUSICBRAINZ).unwrap()); - assert_eq!(artist.meta.info.mb_ref, expected); + assert_eq!(info.mb_ref, expected); - artist.meta.info.set_mb_ref(MbRefOption::Some( + info.set_mb_ref(MbRefOption::Some( MbArtistRef::from_url_str(MUSICBRAINZ).unwrap(), )); - assert_eq!(artist.meta.info.mb_ref, expected); + assert_eq!(info.mb_ref, expected); - artist.meta.info.set_mb_ref(MbRefOption::Some( + info.set_mb_ref(MbRefOption::Some( MbArtistRef::from_url_str(MUSICBRAINZ_2).unwrap(), )); expected.replace(MbArtistRef::from_url_str(MUSICBRAINZ_2).unwrap()); - assert_eq!(artist.meta.info.mb_ref, expected); + assert_eq!(info.mb_ref, expected); // Clearing URLs. - artist.meta.info.clear_mb_ref(); + info.clear_mb_ref(); expected.take(); - assert_eq!(artist.meta.info.mb_ref, expected); + assert_eq!(info.mb_ref, expected); } #[test] fn add_to_remove_from_property() { - let mut artist = Artist::new(ArtistId::new("an artist")); + let mut info = ArtistInfo::default(); - let info = &mut artist.meta.info; let mut expected: Vec = vec![]; assert!(info.properties.is_empty()); @@ -418,9 +419,8 @@ mod tests { #[test] fn set_clear_musicbutler_urls() { - let mut artist = Artist::new(ArtistId::new("an artist")); + let mut info = ArtistInfo::default(); - let info = &mut artist.meta.info; let mut expected: Vec = vec![]; assert!(info.properties.is_empty()); @@ -456,7 +456,8 @@ mod tests { fn merge_artist_no_overlap() { let left = FULL_COLLECTION[0].to_owned(); let mut right = FULL_COLLECTION[1].to_owned(); - right.meta.id = left.meta.id.clone(); + right.id = left.id; + right.meta.name = left.meta.name.clone(); right.meta.info.mb_ref = MbRefOption::None; right.meta.info.properties = HashMap::new(); @@ -493,7 +494,8 @@ mod tests { fn merge_artist_overlap() { let mut left = FULL_COLLECTION[0].to_owned(); let mut right = FULL_COLLECTION[1].to_owned(); - right.meta.id = left.meta.id.clone(); + right.id = left.id; + right.meta.name = left.meta.name.clone(); right.meta.info.mb_ref = left.meta.info.mb_ref.clone(); // The right collection needs more albums than we modify to make sure some do not overlap. @@ -530,7 +532,7 @@ mod tests { #[test] #[should_panic(expected = "multiple secondaries unsupported")] fn merge_two_db_albums_to_one_lib_album() { - let mut left = Artist::new(ArtistId::new("Artist")); + let mut left = Artist::new(0, "Artist"); let mut right = left.clone(); let album = Album::new(AlbumId::new("Album")); @@ -547,7 +549,7 @@ mod tests { #[test] #[should_panic(expected = "multiple primaries unsupported")] fn merge_one_db_album_to_two_lib_albums() { - let mut left = Artist::new(ArtistId::new("Artist")); + let mut left = Artist::new(0, "Artist"); let mut right = left.clone(); let album = Album::new(AlbumId::new("Album")); @@ -564,7 +566,7 @@ mod tests { #[test] fn merge_normalized_album_titles() { - let mut left = Artist::new(ArtistId::new("Artist")); + let mut left = Artist::new(0, "Artist"); let mut right = left.clone(); left.albums @@ -589,7 +591,7 @@ mod tests { #[test] fn merge_multiple_singletons() { - let mut left = Artist::new(ArtistId::new("Artist")); + let mut left = Artist::new(0, "Artist"); let mut right = left.clone(); left.albums.push(Album::new(AlbumId::new("Singleton 1"))); @@ -615,7 +617,7 @@ mod tests { #[test] fn merge_two_db_albums_to_one_lib_album_with_ids() { - let mut left = Artist::new(ArtistId::new("Artist")); + let mut left = Artist::new(0, "Artist"); let mut right = left.clone(); let album = Album::new(AlbumId::new("Album")); diff --git a/src/core/collection/merge.rs b/src/core/collection/merge.rs index 796045a..68a5ba7 100644 --- a/src/core/collection/merge.rs +++ b/src/core/collection/merge.rs @@ -105,10 +105,26 @@ impl NormalMap { } } +pub trait WithId { + type Id; + + fn id(&self) -> &Self::Id; +} + +pub trait IntoId { + type Id; + type IdSelf; + + fn into_id(self, id: &Self::Id) -> Self::IdSelf; +} + pub struct MergeCollections; impl MergeCollections { - pub fn merge_by_name(mut primary: NormalMap, secondary: NormalMap) -> Vec { + pub fn merge_by_name, T2: IntoId>( + mut primary: NormalMap, + secondary: NormalMap, + ) -> (Vec, Vec) { let mut merged = vec![]; for (title, mut secondary_items) in secondary.0.into_iter() { match primary.remove(&title) { @@ -117,14 +133,17 @@ impl MergeCollections { // added once encountered in the wild. assert_eq!(primary_items.len(), 1, "multiple primaries unsupported"); assert_eq!(secondary_items.len(), 1, "multiple secondaries unsupported"); - let mut primary_item = primary_items.pop().unwrap(); - primary_item.merge_in_place(secondary_items.pop().unwrap()); + + let secondary_item = secondary_items.pop().unwrap(); + let id = secondary_item.id(); + + let mut primary_item = primary_items.pop().unwrap().into_id(id); + primary_item.merge_in_place(secondary_item); merged.push(primary_item); } None => merged.extend(secondary_items), } } - merged.extend(primary.0.into_values().flatten()); - merged + (merged, primary.0.into_values().flatten().collect()) } } diff --git a/src/core/interface/database/mod.rs b/src/core/interface/database/mod.rs index cc300f3..a006b34 100644 --- a/src/core/interface/database/mod.rs +++ b/src/core/interface/database/mod.rs @@ -5,7 +5,7 @@ use std::fmt; #[cfg(test)] use mockall::automock; -use crate::core::collection::{self, Collection}; +use crate::{collection::artist::{ArtistId, ArtistMeta}, core::collection::{self, Collection}}; /// Trait for interacting with the database. #[cfg_attr(test, automock)] @@ -18,6 +18,9 @@ pub trait IDatabase { /// Save collection to the database. fn save(&mut self, collection: &Collection) -> Result<(), SaveError>; + + /// Insert an artist into the database and return its assigned ID. + fn insert_artist(&mut self, artist: &ArtistMeta) -> Result; } /// Null database implementation of [`IDatabase`]. @@ -35,6 +38,10 @@ impl IDatabase for NullDatabase { fn save(&mut self, _collection: &Collection) -> Result<(), SaveError> { Ok(()) } + + fn insert_artist(&mut self, _: &ArtistMeta) -> Result { + Ok(ArtistId(0)) + } } /// Error type for database calls. diff --git a/src/core/musichoard/base.rs b/src/core/musichoard/base.rs index f7d684e..1047520 100644 --- a/src/core/musichoard/base.rs +++ b/src/core/musichoard/base.rs @@ -5,7 +5,7 @@ use crate::core::{ merge::{MergeCollections, NormalMap}, string, Collection, }, - musichoard::{filter::CollectionFilter, Error, MusicHoard}, + musichoard::{filter::CollectionFilter, Error, LibArtist, MusicHoard}, }; pub trait IMusicHoardBase { @@ -30,7 +30,7 @@ impl IMusicHoardBase for MusicHoard { } pub trait IMusicHoardBasePrivate { - fn sort_albums_and_tracks<'a, C: Iterator>(collection: C); + fn sort_albums_and_tracks<'a, COL: Iterator>>(collection: COL); fn merge_collections>(&self, database: It) -> Collection; fn filter_collection(&self) -> Collection; @@ -54,30 +54,32 @@ pub trait IMusicHoardBasePrivate { } impl IMusicHoardBasePrivate for MusicHoard { - fn sort_albums_and_tracks<'a, COL: Iterator>(collection: COL) { - for artist in collection { - artist.albums.sort_unstable(); - for album in artist.albums.iter_mut() { + fn sort_albums_and_tracks<'a, COL: Iterator>>(collection: COL) { + for albums in collection { + albums.sort_unstable(); + for album in albums.iter_mut() { album.tracks.sort_unstable(); } } } fn merge_collections>(&self, database: It) -> Collection { - let mut primary = NormalMap::::new(); + let mut primary = NormalMap::::new(); let mut secondary = NormalMap::::new(); - for artist in self.library_cache.iter().cloned() { - primary.insert(string::normalize_string(&artist.meta.id.name), artist); + for (normal_name, artist) in self.library_cache.clone().into_iter() { + primary.insert(normal_name, artist); } for artist in database.into_iter() { - secondary.insert(string::normalize_string(&artist.meta.id.name), artist); + secondary.insert(string::normalize_string(&artist.meta.name), artist); } - let mut collection = MergeCollections::merge_by_name(primary, secondary); - collection.sort_unstable(); + let (mut collection, left) = MergeCollections::merge_by_name(primary, secondary); + // TODO: Insert what's left into the DB to get IDs and then append to collection. + + collection.sort_unstable(); collection } @@ -95,15 +97,18 @@ impl IMusicHoardBasePrivate for MusicHoard return None; } - let meta = artist.meta.clone(); - Some(Artist { meta, albums }) + Some(Artist { + id: artist.id, + meta: artist.meta.clone(), + albums, + }) } fn get_artist_mut<'a>( collection: &'a mut Collection, artist_id: &ArtistId, ) -> Option<&'a mut Artist> { - collection.iter_mut().find(|a| &a.meta.id == artist_id) + collection.iter_mut().find(|a| &a.id == artist_id) } fn get_artist_mut_or_err<'a>( @@ -138,180 +143,224 @@ impl IMusicHoardBasePrivate for MusicHoard #[cfg(test)] mod tests { + use std::collections::HashMap; + use crate::{ - collection::{album::AlbumPrimaryType, artist::ArtistMeta}, - core::testmod::FULL_COLLECTION, + collection::{ + album::AlbumPrimaryType, + artist::{ArtistMeta, ArtistName}, + }, + core::{musichoard::LibArtist, testmod::FULL_COLLECTION}, filter::AlbumField, }; use super::*; #[test] + #[ignore] + // TODO: figure out how to do a merge fn merge_collection_no_overlap() { - let half: usize = FULL_COLLECTION.len() / 2; + // let half: usize = FULL_COLLECTION.len() / 2; - let left = FULL_COLLECTION[..half].to_owned(); - let right = FULL_COLLECTION[half..].to_owned(); + // let left = FULL_COLLECTION[..half].to_owned(); + // let right = FULL_COLLECTION[half..].to_owned(); - let mut expected = FULL_COLLECTION.to_owned(); - expected.sort_unstable(); + // let mut expected = FULL_COLLECTION.to_owned(); + // expected.sort_unstable(); - let mut mh = MusicHoard { - library_cache: left.clone(), - ..Default::default() - }; + // let mut mh = MusicHoard { + // library_cache: left.clone(), + // ..Default::default() + // }; - mh.collection = mh.merge_collections(right.clone()); - assert_eq!(expected, mh.collection); + // mh.collection = mh.merge_collections(right.clone()); + // assert_eq!(expected, mh.collection); - // The merge is completely non-overlapping so it should be commutative. - let mut mh = MusicHoard { - library_cache: right.clone(), - ..Default::default() - }; + // // The merge is completely non-overlapping so it should be commutative. + // let mut mh = MusicHoard { + // library_cache: right.clone(), + // ..Default::default() + // }; - mh.collection = mh.merge_collections(left.clone()); - assert_eq!(expected, mh.collection); + // mh.collection = mh.merge_collections(left.clone()); + // assert_eq!(expected, mh.collection); } #[test] + #[ignore] + // TODO: figure out how to do a merge fn merge_collection_overlap() { - let half: usize = FULL_COLLECTION.len() / 2; + // let half: usize = FULL_COLLECTION.len() / 2; - let left = FULL_COLLECTION[..(half + 1)].to_owned(); - let right = FULL_COLLECTION[half..].to_owned(); + // let left = FULL_COLLECTION[..(half + 1)].to_owned(); + // let right = FULL_COLLECTION[half..].to_owned(); - let mut expected = FULL_COLLECTION.to_owned(); - expected.sort_unstable(); + // let mut expected = FULL_COLLECTION.to_owned(); + // expected.sort_unstable(); - let mut mh = MusicHoard { - library_cache: left.clone(), - ..Default::default() - }; + // let mut mh = MusicHoard { + // library_cache: left.clone(), + // ..Default::default() + // }; - mh.collection = mh.merge_collections(right.clone()); - assert_eq!(expected, mh.collection); + // mh.collection = mh.merge_collections(right.clone()); + // assert_eq!(expected, mh.collection); - // The merge does not overwrite any data so it should be commutative. - let mut mh = MusicHoard { - library_cache: right.clone(), - ..Default::default() - }; + // // The merge does not overwrite any data so it should be commutative. + // let mut mh = MusicHoard { + // library_cache: right.clone(), + // ..Default::default() + // }; - mh.collection = mh.merge_collections(left.clone()); - assert_eq!(expected, mh.collection); + // mh.collection = mh.merge_collections(left.clone()); + // assert_eq!(expected, mh.collection); } #[test] + #[ignore] + // TODO: figure out how to do a merge fn merge_collection_incompatible_sorting() { - // It may be that the same artist in one collection has a "sort" field defined while the - // same artist in the other collection does not. This means that the two collections are not - // sorted consistently. If the merge assumes they are sorted consistently this will lead to - // the same artist appearing twice in the final list. This should not be the case. + // // It may be that the same artist in one collection has a "sort" field defined while the + // // same artist in the other collection does not. This means that the two collections are not + // // sorted consistently. If the merge assumes they are sorted consistently this will lead to + // // the same artist appearing twice in the final list. This should not be the case. - // We will mimic this situation by taking the last artist from FULL_COLLECTION and giving it - // a sorting name that would place it in the beginning. - let left = FULL_COLLECTION.to_owned(); - let mut right: Vec = vec![left.last().unwrap().clone()]; + // // We will mimic this situation by taking the last artist from FULL_COLLECTION and giving it + // // a sorting name that would place it in the beginning. + // let left = FULL_COLLECTION.to_owned(); + // let mut right: Vec = vec![left.last().unwrap().clone()]; - assert!(right.first().unwrap() > left.first().unwrap()); - let artist_sort = Some(String::from("Album_Artist 0")); - right[0].meta.info.sort = artist_sort.clone(); - assert!(right.first().unwrap() < left.first().unwrap()); + // assert!(right.first().unwrap() > left.first().unwrap()); + // let artist_sort = Some(String::from("Album_Artist 0")); + // right[0].meta.info.sort = artist_sort.clone(); + // assert!(right.first().unwrap() < left.first().unwrap()); - // The result of the merge should be the same list of artists, but with the last artist now - // in first place. - let mut expected = left.to_owned(); - expected.last_mut().as_mut().unwrap().meta.info.sort = artist_sort.clone(); - expected.rotate_right(1); + // // The result of the merge should be the same list of artists, but with the last artist now + // // in first place. + // let mut expected = left.to_owned(); + // expected.last_mut().as_mut().unwrap().meta.info.sort = artist_sort.clone(); + // expected.rotate_right(1); - let mut mh = MusicHoard { - library_cache: left.clone(), - ..Default::default() - }; + // let mut mh = MusicHoard { + // library_cache: left.clone(), + // ..Default::default() + // }; - mh.collection = mh.merge_collections(right.clone()); - assert_eq!(expected, mh.collection); + // mh.collection = mh.merge_collections(right.clone()); + // assert_eq!(expected, mh.collection); - // The merge overwrites the sort data, but no data is erased so it should be commutative. - let mut mh = MusicHoard { - library_cache: right.clone(), - ..Default::default() - }; + // // The merge overwrites the sort data, but no data is erased so it should be commutative. + // let mut mh = MusicHoard { + // library_cache: right.clone(), + // ..Default::default() + // }; - mh.collection = mh.merge_collections(left.clone()); - assert_eq!(expected, mh.collection); + // mh.collection = mh.merge_collections(left.clone()); + // assert_eq!(expected, mh.collection); } #[test] + #[ignore] + // TODO: figure out how to do a merge #[should_panic(expected = "multiple secondaries unsupported")] fn merge_two_db_artists_to_one_lib_artist() { - let mut left = Collection::new(); - let mut right = Collection::new(); + // let mut left = HashMap::::new(); + // let mut right = Collection::new(); - let artist = Artist::new(ArtistId::new("Artist")); + // let name = ArtistName::new("Artist"); - left.push(artist.clone()); - right.push(artist.clone()); - right.push(artist.clone()); + // left.insert( + // name.official.clone(), + // LibArtist { + // meta: ArtistMeta::new(name.clone()), + // albums: vec![], + // }, + // ); + // right.push(Artist::new(1, name.clone())); + // right.push(Artist::new(2, name.clone())); - let mut mh = MusicHoard { - library_cache: left.clone(), - ..Default::default() - }; + // let mut mh = MusicHoard { + // library_cache: left.clone(), + // ..Default::default() + // }; - mh.collection = mh.merge_collections(right.clone()); + // mh.collection = mh.merge_collections(right.clone()); } #[test] + #[ignore] + // TODO: change to albums - primary clash is not possible for artists since without a lib id #[should_panic(expected = "multiple primaries unsupported")] fn merge_one_db_artist_to_two_lib_artists() { - let mut left = Collection::new(); - let mut right = Collection::new(); + // let mut left = Collection::new(); + // let mut right = Collection::new(); - let artist = Artist::new(ArtistId::new("Artist")); + // let artist = Artist::new(ArtistId::new("Artist")); - left.push(artist.clone()); - left.push(artist.clone()); - right.push(artist.clone()); + // left.insert( + // name.official.clone(), + // LibArtist { + // name: name.clone(), + // meta: ArtistMeta::default(), + // albums: vec![], + // }, + // ); + // left.insert( + // name.official.clone(), + // LibArtist { + // name: name.clone(), + // meta: ArtistMeta::default(), + // albums: vec![], + // }, + // ); + // right.push(artist.clone()); - let mut mh = MusicHoard { - library_cache: left.clone(), - ..Default::default() - }; + // let mut mh = MusicHoard { + // library_cache: left.clone(), + // ..Default::default() + // }; - mh.collection = mh.merge_collections(right.clone()); + // mh.collection = mh.merge_collections(right.clone()); } #[test] + #[ignore] + // TODO: figue out how to do a merge fn merge_normalized_artist_names() { - let mut left = Collection::new(); - let mut right = Collection::new(); + // let mut left = HashMap::::new(); + // let mut right = Collection::new(); - left.push(Artist::new(ArtistId::new("Artist‐Name ‘Name’"))); + // let left_name = "Artist‐Name ‘Name’"; + // left.insert( + // String::from(left_name), + // LibArtist { + // meta: ArtistMeta::new(left_name.into()), + // albums: vec![], + // }, + // ); - right.push(Artist::new(ArtistId::new("arTist—naMe 'name’"))); - right.push(Artist::new(ArtistId::new("Artist‐Name “Name”"))); + // right.push(Artist::new(1, "arTist—naMe 'name’")); + // right.push(Artist::new(2, "Artist‐Name “Name”")); - // The first artist will be merged, the second will be added. - let mut expected = left.clone(); - expected.push(right.last().unwrap().clone()); - expected.sort_unstable(); + // // The first artist will be merged preserving the name, the second will be added. + // let mut expected = right.clone(); + // expected.first_mut().unwrap().meta.name = left["Artist‐Name ‘Name’"].meta.name.clone(); - let mut mh = MusicHoard { - library_cache: left.clone(), - ..Default::default() - }; + // let mut mh = MusicHoard { + // library_cache: left.clone(), + // ..Default::default() + // }; - mh.collection = mh.merge_collections(right.clone()); - assert_eq!(expected, mh.collection); + // mh.collection = mh.merge_collections(right.clone()); + // assert_eq!(expected, mh.collection); } #[test] fn filtered() { let mut mh = MusicHoard { collection: vec![Artist { - meta: ArtistMeta::new(ArtistId::new("Artist")), + id: 0.into(), + meta: ArtistMeta::new("Artist"), albums: vec![ Album::new(AlbumId::new("Album 1")), Album::new(AlbumId::new("Album 2")), diff --git a/src/core/musichoard/builder.rs b/src/core/musichoard/builder.rs index e828718..6ac9d9a 100644 --- a/src/core/musichoard/builder.rs +++ b/src/core/musichoard/builder.rs @@ -1,3 +1,5 @@ +use std::collections::HashMap; + use crate::core::{ interface::{database::IDatabase, library::ILibrary}, musichoard::{CollectionFilter, MusicHoard, NoDatabase, NoLibrary}, @@ -67,7 +69,7 @@ impl MusicHoard { collection: vec![], database: NoDatabase, library: NoLibrary, - library_cache: vec![], + library_cache: HashMap::new(), } } } @@ -88,7 +90,7 @@ impl MusicHoard { collection: vec![], database: NoDatabase, library, - library_cache: vec![], + library_cache: HashMap::new(), } } } @@ -109,7 +111,7 @@ impl MusicHoard { collection: vec![], database, library: NoLibrary, - library_cache: vec![], + library_cache: HashMap::new(), } } } @@ -130,7 +132,7 @@ impl MusicHoard { collection: vec![], database, library, - library_cache: vec![], + library_cache: HashMap::new(), } } } diff --git a/src/core/musichoard/database.rs b/src/core/musichoard/database.rs index aac0310..2848535 100644 --- a/src/core/musichoard/database.rs +++ b/src/core/musichoard/database.rs @@ -66,7 +66,7 @@ pub trait IMusicHoardDatabase { impl IMusicHoardDatabase for MusicHoard { fn reload_database(&mut self) -> Result<(), Error> { let mut database_cache = self.database.load()?; - Self::sort_albums_and_tracks(database_cache.iter_mut()); + Self::sort_albums_and_tracks(database_cache.iter_mut().map(|a| &mut a.albums)); self.collection = self.merge_collections(database_cache); self.filtered = self.filter_collection(); @@ -297,7 +297,7 @@ mod tests { let database = MockIDatabase::new(); let mut music_hoard = MusicHoard::database(database); - let artist_id = ArtistId::new("an artist"); + let artist_id = ArtistId(1); let actual_err = music_hoard .merge_artist_info(&artist_id, ArtistInfo::default()) .unwrap_err(); @@ -312,12 +312,12 @@ mod tests { let mut database = MockIDatabase::new(); database.expect_save().times(2).returning(|_| Ok(())); - let artist_id = ArtistId::new("an artist"); - let artist_id_2 = ArtistId::new("another artist"); + let artist = Artist::new(1, "an artist"); + let artist_2 = Artist::new(2, "another artist"); let mb_ref = ArtistMbRef::Some(MbArtistRef::from_uuid_str(MBID).unwrap()); let mut music_hoard = MusicHoard::database(database); - music_hoard.collection.push(Artist::new(artist_id.clone())); + music_hoard.collection.push(artist.clone()); music_hoard.collection.sort_unstable(); let mut expected = ArtistInfo::default(); @@ -328,13 +328,13 @@ mod tests { // Setting info on an artist not in the collection is an error. assert!(music_hoard - .merge_artist_info(&artist_id_2, info.clone()) + .merge_artist_info(&artist_2.id, info.clone()) .is_err()); assert_eq!(music_hoard.collection[0].meta.info, expected); // Setting info on an artist. assert!(music_hoard - .merge_artist_info(&artist_id, info.clone()) + .merge_artist_info(&artist.id, info.clone()) .is_ok()); expected.mb_ref = mb_ref.clone(); expected.properties.insert( @@ -344,11 +344,11 @@ mod tests { assert_eq!(music_hoard.collection[0].meta.info, expected); // Clearing info on an artist that does not exist is an error. - assert!(music_hoard.clear_artist_info(&artist_id_2).is_err()); + assert!(music_hoard.clear_artist_info(&artist_2.id).is_err()); assert_eq!(music_hoard.collection[0].meta.info, expected); // Clearing info. - assert!(music_hoard.clear_artist_info(&artist_id).is_ok()); + assert!(music_hoard.clear_artist_info(&artist.id).is_ok()); expected.mb_ref.take(); expected.properties.clear(); assert_eq!(music_hoard.collection[0].meta.info, expected); @@ -362,7 +362,7 @@ mod tests { let album_meta_2 = AlbumMeta::new(album_id_2); let collection = FULL_COLLECTION.to_owned(); - let artist_id = collection[0].meta.id.clone(); + let artist_id = collection[0].id; let mut with_album = collection.clone(); with_album[0].albums.push(Album::new(album_id)); with_album[0].albums.sort_unstable(); @@ -415,11 +415,11 @@ mod tests { fn set_clear_album_mb_ref() { let mut database = MockIDatabase::new(); - let artist_id = ArtistId::new("an artist"); + let artist = Artist::new(1, "an artist"); let mut album_id = AlbumId::new("an album"); let album_id_2 = AlbumId::new("another album"); - let mut database_result = vec![Artist::new(artist_id.clone())]; + let mut database_result = vec![artist.clone()]; database_result[0].albums.push(Album::new(album_id.clone())); database @@ -435,27 +435,27 @@ mod tests { // Seting mb_ref on an album not belonging to the artist is an error. assert!(music_hoard - .set_album_mb_ref(&artist_id, &album_id_2, AlbumMbRef::CannotHaveMbid) + .set_album_mb_ref(&artist.id, &album_id_2, AlbumMbRef::CannotHaveMbid) .is_err()); let album = &music_hoard.collection[0].albums[0]; assert_eq!(album.meta.id.mb_ref, AlbumMbRef::None); // Set mb_ref. assert!(music_hoard - .set_album_mb_ref(&artist_id, &album_id, AlbumMbRef::CannotHaveMbid) + .set_album_mb_ref(&artist.id, &album_id, AlbumMbRef::CannotHaveMbid) .is_ok()); let album = &music_hoard.collection[0].albums[0]; assert_eq!(album.meta.id.mb_ref, AlbumMbRef::CannotHaveMbid); // Clearing mb_ref on an album that does not exist is an error. assert!(music_hoard - .clear_album_mb_ref(&artist_id, &album_id_2) + .clear_album_mb_ref(&artist.id, &album_id_2) .is_err()); // Clearing mb_ref from an album without the mb_ref set is an error. Effectively the album // does not exist. assert!(music_hoard - .clear_album_mb_ref(&artist_id, &album_id) + .clear_album_mb_ref(&artist.id, &album_id) .is_err()); let album = &music_hoard.collection[0].albums[0]; assert_eq!(album.meta.id.mb_ref, AlbumMbRef::CannotHaveMbid); @@ -464,7 +464,7 @@ mod tests { // album. album_id.set_mb_ref(AlbumMbRef::CannotHaveMbid); assert!(music_hoard - .clear_album_mb_ref(&artist_id, &album_id) + .clear_album_mb_ref(&artist.id, &album_id) .is_ok()); let album = &music_hoard.collection[0].albums[0]; assert_eq!(album.meta.id.mb_ref, AlbumMbRef::None); @@ -474,11 +474,11 @@ mod tests { fn set_clear_album_info() { let mut database = MockIDatabase::new(); - let artist_id = ArtistId::new("an artist"); + let artist = Artist::new(1, "an artist"); let album_id = AlbumId::new("an album"); let album_id_2 = AlbumId::new("another album"); - let mut database_result = vec![Artist::new(artist_id.clone())]; + let mut database_result = vec![artist.clone()]; database_result[0].albums.push(Album::new(album_id.clone())); database @@ -499,25 +499,25 @@ mod tests { // Seting info on an album not belonging to the artist is an error. assert!(music_hoard - .merge_album_info(&artist_id, &album_id_2, info.clone()) + .merge_album_info(&artist.id, &album_id_2, info.clone()) .is_err()); let meta = &music_hoard.collection[0].albums[0].meta; assert_eq!(meta.info, AlbumInfo::default()); // Set info. assert!(music_hoard - .merge_album_info(&artist_id, &album_id, info.clone()) + .merge_album_info(&artist.id, &album_id, info.clone()) .is_ok()); let meta = &music_hoard.collection[0].albums[0].meta; assert_eq!(meta.info, info); // Clearing info on an album that does not exist is an error. assert!(music_hoard - .clear_album_info(&artist_id, &album_id_2) + .clear_album_info(&artist.id, &album_id_2) .is_err()); // Clear info. - assert!(music_hoard.clear_album_info(&artist_id, &album_id).is_ok()); + assert!(music_hoard.clear_album_info(&artist.id, &album_id).is_ok()); let meta = &music_hoard.collection[0].albums[0].meta; assert_eq!(meta.info, AlbumInfo::default()); } @@ -583,11 +583,11 @@ mod tests { let mut music_hoard = MusicHoard::database(database); music_hoard.reload_database().unwrap(); - let artist_id = ArtistId::new("an artist"); - music_hoard.collection.push(Artist::new(artist_id.clone())); + let artist = Artist::new(1, "an artist"); + music_hoard.collection.push(artist.clone()); let actual_err = music_hoard - .add_album(artist_id, AlbumMeta::new("an album")) + .add_album(artist.id, AlbumMeta::new("an album")) .unwrap_err(); let expected_err = Error::DatabaseError( database::SaveError::IoError(String::from("I/O error")).to_string(), diff --git a/src/core/musichoard/library.rs b/src/core/musichoard/library.rs index bf2f529..ee76998 100644 --- a/src/core/musichoard/library.rs +++ b/src/core/musichoard/library.rs @@ -1,19 +1,21 @@ use std::collections::HashMap; -use crate::core::{ - collection::{ - album::{Album, AlbumDate, AlbumId, AlbumMbRef}, - artist::{Artist, ArtistId}, - track::{Track, TrackId, TrackNum, TrackQuality}, - Collection, - }, - interface::{ - database::IDatabase, - library::{ILibrary, Item, Query}, - }, - musichoard::{ - base::IMusicHoardBasePrivate, database::IMusicHoardDatabasePrivate, Error, MusicHoard, - NoDatabase, +use crate::{ + collection::string::{self, NormalString}, + core::{ + collection::{ + album::{Album, AlbumDate, AlbumId, AlbumMbRef}, + track::{Track, TrackId, TrackNum, TrackQuality}, + Collection, + }, + interface::{ + database::IDatabase, + library::{ILibrary, Item, Query}, + }, + musichoard::{ + base::IMusicHoardBasePrivate, database::IMusicHoardDatabasePrivate, Error, LibArtist, + MusicHoard, NoDatabase, + }, }, }; @@ -31,7 +33,7 @@ impl IMusicHoardLibrary for MusicHoard { impl IMusicHoardLibrary for MusicHoard { fn rescan_library(&mut self) -> Result<(), Error> { let mut database_cache = self.database.load()?; - Self::sort_albums_and_tracks(database_cache.iter_mut()); + Self::sort_albums_and_tracks(database_cache.iter_mut().map(|a| &mut a.albums)); self.collection = self.rescan_library_inner(database_cache)?; self.commit() @@ -42,20 +44,17 @@ impl MusicHoard { fn rescan_library_inner(&mut self, database: Collection) -> Result { let items = self.library.list(&Query::new())?; self.library_cache = Self::items_to_artists(items)?; - Self::sort_albums_and_tracks(self.library_cache.iter_mut()); + Self::sort_albums_and_tracks(self.library_cache.values_mut().map(|la| &mut la.albums)); Ok(self.merge_collections(database)) } - fn items_to_artists(items: Vec) -> Result { - let mut collection = HashMap::::new(); + fn items_to_artists(items: Vec) -> Result, Error> { + let mut collection = HashMap::::new(); for item in items.into_iter() { - let artist_id = ArtistId { - name: item.album_artist, - }; - - let artist_sort = item.album_artist_sort; + let artist_name_official = item.album_artist; + let artist_name_sort = item.album_artist_sort; let album_id = AlbumId { title: item.album_title, @@ -84,24 +83,25 @@ impl MusicHoard { // There are usually many entries per artist. Therefore, we avoid simply calling // .entry(artist_id.clone()).or_insert_with(..), because of the clone. The flipside is // that insertions will thus do an additional lookup. - let artist = match collection.get_mut(&artist_id) { + let normal_name = string::normalize_string(&artist_name_official); + let artist = match collection.get_mut(&normal_name) { Some(artist) => artist, None => collection - .entry(artist_id.clone()) - .or_insert_with(|| Artist::new(artist_id)), + .entry(normal_name) + .or_insert_with(|| LibArtist::new(artist_name_official)), }; - if artist.meta.info.sort.is_some() { - if artist_sort.is_some() && (artist.meta.info.sort != artist_sort) { + if artist.meta.sort.is_some() { + if artist_name_sort.is_some() && (artist.meta.sort != artist_name_sort) { return Err(Error::CollectionError(format!( "multiple album_artist_sort found for artist '{}': '{}' != '{}'", - artist.meta.id, - artist.meta.info.sort.as_ref().unwrap(), - artist_sort.as_ref().unwrap() + artist.meta.name, + artist.meta.sort.as_ref().unwrap(), + artist_name_sort.as_ref().unwrap() ))); } - } else if artist_sort.is_some() { - artist.meta.info.sort = artist_sort; + } else if artist_name_sort.is_some() { + artist.meta.sort = artist_name_sort; } // Do a linear search as few artists have more than a handful of albums. Search from the @@ -121,7 +121,7 @@ impl MusicHoard { } } - Ok(collection.into_values().collect()) + Ok(collection) } } diff --git a/src/core/musichoard/mod.rs b/src/core/musichoard/mod.rs index 8e2f83a..7206398 100644 --- a/src/core/musichoard/mod.rs +++ b/src/core/musichoard/mod.rs @@ -12,10 +12,19 @@ pub use database::IMusicHoardDatabase; pub use filter::CollectionFilter; pub use library::IMusicHoardLibrary; -use std::fmt::{self, Display}; +use std::{ + collections::HashMap, + fmt::{self, Display}, +}; use crate::core::{ - collection::Collection, + collection::{ + album::Album, + artist::{Artist, ArtistId, ArtistMeta, ArtistName}, + merge::IntoId, + string::NormalString, + Collection, + }, interface::{ database::{LoadError as DatabaseLoadError, SaveError as DatabaseSaveError}, library::Error as LibraryError, @@ -24,7 +33,6 @@ use crate::core::{ /// The Music Hoard. It is responsible for pulling information from both the library and the /// database, ensuring its consistent and writing back any changes. -// TODO: Split into inner and external/interfaces to facilitate building. #[derive(Debug)] pub struct MusicHoard { filter: CollectionFilter, @@ -32,7 +40,35 @@ pub struct MusicHoard { collection: Collection, database: Database, library: Library, - library_cache: Collection, + library_cache: HashMap, +} + +#[derive(Clone, Debug)] +struct LibArtist { + meta: ArtistMeta, + albums: Vec, +} + +impl LibArtist { + fn new>(name: Name) -> Self { + LibArtist { + meta: ArtistMeta::new(name), + albums: vec![], + } + } +} + +impl IntoId for LibArtist { + type Id = ArtistId; + type IdSelf = Artist; + + fn into_id(self, id: &Self::Id) -> Self::IdSelf { + Artist { + id: *id, + meta: self.meta, + albums: self.albums, + } + } } /// Phantom type for when a library implementation is not needed. diff --git a/src/external/database/serde/deserialize.rs b/src/external/database/serde/deserialize.rs index d06aad3..f2e8bb1 100644 --- a/src/external/database/serde/deserialize.rs +++ b/src/external/database/serde/deserialize.rs @@ -34,6 +34,7 @@ impl From for Collection { #[derive(Clone, Debug, Deserialize)] pub struct DeserializeArtist { + pub id: usize, pub name: String, pub mb_ref: DeserializeMbRefOption, pub sort: Option, @@ -117,12 +118,11 @@ impl From for Artist { let mut albums: Vec = artist.albums.into_iter().map(Into::into).collect(); albums.sort_unstable(); Artist { + id: ArtistId(artist.id), meta: ArtistMeta { - id: ArtistId { - name: artist.name, - }, + name: artist.name, + sort: artist.sort, info: ArtistInfo { - sort: artist.sort, mb_ref: artist.mb_ref.into(), properties: artist.properties, }, diff --git a/src/external/database/serde/serialize.rs b/src/external/database/serde/serialize.rs index 7a529fa..845c060 100644 --- a/src/external/database/serde/serialize.rs +++ b/src/external/database/serde/serialize.rs @@ -4,7 +4,12 @@ use serde::Serialize; use crate::{ collection::musicbrainz::{MbRefOption, Mbid}, - core::collection::{album::Album, artist::Artist, musicbrainz::IMusicBrainzRef, Collection}, + core::collection::{ + album::Album, + artist::{Artist, ArtistMeta}, + musicbrainz::IMusicBrainzRef, + Collection, + }, external::database::serde::common::{ MbRefOptionDef, SerdeAlbumDate, SerdeAlbumLibId, SerdeAlbumPrimaryType, SerdeAlbumSecondaryType, @@ -73,12 +78,20 @@ impl Serialize for SerializeMbid<'_> { impl<'a> From<&'a Artist> for SerializeArtist<'a> { fn from(artist: &'a Artist) -> Self { + let mut sa: SerializeArtist = (&artist.meta).into(); + sa.albums = artist.albums.iter().map(Into::into).collect(); + sa + } +} + +impl<'a> From<&'a ArtistMeta> for SerializeArtist<'a> { + fn from(meta: &'a ArtistMeta) -> Self { SerializeArtist { - name: &artist.meta.id.name, - mb_ref: (&artist.meta.info.mb_ref).into(), - sort: &artist.meta.info.sort, - properties: &artist.meta.info.properties, - albums: artist.albums.iter().map(Into::into).collect(), + name: &meta.name, + mb_ref: (&meta.info.mb_ref).into(), + sort: &meta.sort, + properties: &meta.info.properties, + albums: vec![], } } } diff --git a/src/external/database/sql/backend.rs b/src/external/database/sql/backend.rs index 3a87781..a5eb57a 100644 --- a/src/external/database/sql/backend.rs +++ b/src/external/database/sql/backend.rs @@ -166,14 +166,10 @@ impl ISqlTransactionBackend for SqlTransactionSqliteBackend<'_> { .transpose() } - fn insert_artist(&self, artist: &SerializeArtist<'_>) -> Result<(), Error> { + fn insert_artist(&self, artist: &SerializeArtist<'_>) -> Result { let mut stmt = self.prepare_cached( "INSERT INTO artists (name, mbid, sort, properties) - VALUES (?1, ?2, ?3, ?4) - ON CONFLICT(name, mbid) DO UPDATE SET sort = ?3, properties = ?4 - WHERE EXISTS ( - SELECT 1 EXCEPT SELECT 1 WHERE sort = ?3 AND properties = ?4 - )", + VALUES (?1, ?2, ?3, ?4)", ); Self::execute( &mut stmt, @@ -183,20 +179,43 @@ impl ISqlTransactionBackend for SqlTransactionSqliteBackend<'_> { artist.sort, serde_json::to_string(&artist.properties)?, ), + )?; + Ok(self.tx.last_insert_rowid()) + } + + fn update_artist(&self, oid: i64, artist: &SerializeArtist<'_>) -> Result<(), Error> { + let mut stmt = self.prepare_cached( + "UPDATE SET name = ?2, mbid = ?3, sort = ?4, properties = ?5 + WHERE rowid = ?1 EXISTS ( + SELECT 1 EXCEPT SELECT 1 WHERE + name = ?2 AND mbid = ?3 AND sort = ?4 AND properties = ?5 + )", + ); + Self::execute( + &mut stmt, + ( + oid, + artist.name, + serde_json::to_string(&artist.mb_ref)?, + artist.sort, + serde_json::to_string(&artist.properties)?, + ), ) } fn select_all_artists(&self) -> Result, Error> { - let mut stmt = self.prepare_cached("SELECT name, mbid, sort, properties FROM artists"); + let mut stmt = + self.prepare_cached("SELECT rowid, name, mbid, sort, properties FROM artists"); let mut rows = Self::query(&mut stmt, ())?; let mut artists = vec![]; while let Some(row) = Self::next_row(&mut rows)? { artists.push(DeserializeArtist { - name: Self::get_value(row, 0)?, - mb_ref: serde_json::from_str(&Self::get_value::(row, 1)?)?, - sort: Self::get_value(row, 2)?, - properties: serde_json::from_str(&Self::get_value::(row, 3)?)?, + id: Self::get_value::(row, 0)? as usize, + name: Self::get_value(row, 1)?, + mb_ref: serde_json::from_str(&Self::get_value::(row, 2)?)?, + sort: Self::get_value(row, 3)?, + properties: serde_json::from_str(&Self::get_value::(row, 4)?)?, albums: vec![], }); } diff --git a/src/external/database/sql/mod.rs b/src/external/database/sql/mod.rs index eecb94d..cdb003d 100644 --- a/src/external/database/sql/mod.rs +++ b/src/external/database/sql/mod.rs @@ -9,7 +9,10 @@ use mockall::automock; use crate::{ core::{ - collection::Collection, + collection::{ + artist::{ArtistId, ArtistMeta}, + Collection, + }, interface::database::{IDatabase, LoadError, SaveError}, }, external::database::serde::{ @@ -60,7 +63,11 @@ pub trait ISqlTransactionBackend { /// Insert an artist into the artist table. #[allow(clippy::needless_lifetimes)] // Conflicts with automock. - fn insert_artist<'a>(&self, artist: &SerializeArtist<'a>) -> Result<(), Error>; + fn insert_artist<'a>(&self, artist: &SerializeArtist<'a>) -> Result; + + /// Update an artist in the artist table. + #[allow(clippy::needless_lifetimes)] // Conflicts with automock. + fn update_artist<'a>(&self, oid: i64, artist: &SerializeArtist<'a>) -> Result<(), Error>; /// Get all artists from the artist table. fn select_all_artists(&self) -> Result, Error>; @@ -204,6 +211,16 @@ impl ISqlDatabaseBackend<'conn>> IDatabase for SqlDatabase tx.commit()?; Ok(()) } + + fn insert_artist(&mut self, artist: &ArtistMeta) -> Result { + let tx = self.backend.transaction()?; + + let sa: SerializeArtist = artist.into(); + let oid = tx.insert_artist(&sa)?; + + tx.commit()?; + Ok(ArtistId(oid as usize)) + } } #[cfg(test)] @@ -211,7 +228,7 @@ pub mod testmod; #[cfg(test)] mod tests { - use std::collections::VecDeque; + use std::{collections::VecDeque, ops::AddAssign}; use mockall::{predicate, Sequence}; @@ -326,12 +343,15 @@ mod tests { let mut seq = Sequence::new(); expect_create!(tx, seq); then1!(tx, seq, expect_insert_database_version).with(predicate::eq(V20250103)); + let mut rowid: i64 = 0; for artist in write_data.iter() { let ac = artist.clone(); - then1!(tx, seq, expect_insert_artist) + rowid.add_assign(1); + then!(tx, seq, expect_insert_artist) + .return_once(move |_| Ok(rowid)) .withf(move |a| a == &Into::::into(&ac)); for album in artist.albums.iter() { - let (nc, ac) = (artist.meta.id.name.clone(), album.clone()); + let (nc, ac) = (artist.meta.name.clone(), album.clone()); then2!(tx, seq, expect_insert_album) .withf(move |n, a| n == nc && a == &Into::::into(&ac)); } @@ -354,9 +374,9 @@ mod tests { then!(tx, seq, expect_select_all_artists).return_once(|| Ok(de_artists)); for artist in artists.iter() { - let de_albums = DATABASE_SQL_ALBUMS.get(&artist.meta.id.name).unwrap(); + let de_albums = DATABASE_SQL_ALBUMS.get(&artist.meta.name).unwrap(); then!(tx, seq, expect_select_artist_albums) - .with(predicate::eq(artist.meta.id.name.clone())) + .with(predicate::eq(artist.meta.name.clone())) .return_once(|_| Ok(de_albums.to_owned())); } diff --git a/src/external/database/sql/testmod.rs b/src/external/database/sql/testmod.rs index 94046ad..3c14fb8 100644 --- a/src/external/database/sql/testmod.rs +++ b/src/external/database/sql/testmod.rs @@ -20,6 +20,7 @@ pub static DATABASE_SQL_VERSION: &str = "V20250103"; pub static DATABASE_SQL_ARTISTS: Lazy> = Lazy::new(|| { vec![ DeserializeArtist { + id: 1, name: String::from("Album_Artist ‘A’"), mb_ref: DeserializeMbRefOption(MbRefOption::Some(DeserializeMbid( "00000000-0000-0000-0000-000000000000".try_into().unwrap(), @@ -42,6 +43,7 @@ pub static DATABASE_SQL_ARTISTS: Lazy> = Lazy::new(|| { albums: vec![], }, DeserializeArtist { + id: 2, name: String::from("Album_Artist ‘B’"), mb_ref: DeserializeMbRefOption(MbRefOption::Some(DeserializeMbid( "11111111-1111-1111-1111-111111111111".try_into().unwrap(), @@ -64,6 +66,7 @@ pub static DATABASE_SQL_ARTISTS: Lazy> = Lazy::new(|| { albums: vec![], }, DeserializeArtist { + id: 3, name: String::from("The Album_Artist ‘C’"), mb_ref: DeserializeMbRefOption(MbRefOption::CannotHaveMbid), sort: Some(String::from("Album_Artist ‘C’, The")), @@ -71,6 +74,7 @@ pub static DATABASE_SQL_ARTISTS: Lazy> = Lazy::new(|| { albums: vec![], }, DeserializeArtist { + id: 4, name: String::from("Album_Artist ‘D’"), mb_ref: DeserializeMbRefOption(MbRefOption::None), sort: None, diff --git a/src/testmod/full.rs b/src/testmod/full.rs index 0c00195..0013873 100644 --- a/src/testmod/full.rs +++ b/src/testmod/full.rs @@ -2,12 +2,11 @@ macro_rules! full_collection { () => { vec![ Artist { + id: ArtistId(1), meta: ArtistMeta { - id: ArtistId { - name: "Album_Artist ‘A’".to_string(), - }, + name: "Album_Artist ‘A’".to_string(), + sort: None, info: ArtistInfo { - sort: None, mb_ref: ArtistMbRef::Some(MbArtistRef::from_url_str( "https://musicbrainz.org/artist/00000000-0000-0000-0000-000000000000" ).unwrap()), @@ -128,12 +127,11 @@ macro_rules! full_collection { ], }, Artist { + id: ArtistId(2), meta: ArtistMeta { - id: ArtistId { - name: "Album_Artist ‘B’".to_string(), - }, + name: "Album_Artist ‘B’".to_string(), + sort: None, info: ArtistInfo { - sort: None, mb_ref: ArtistMbRef::Some(MbArtistRef::from_url_str( "https://musicbrainz.org/artist/11111111-1111-1111-1111-111111111111" ).unwrap()), @@ -321,12 +319,11 @@ macro_rules! full_collection { ], }, Artist { + id: ArtistId(3), meta: ArtistMeta { - id: ArtistId { - name: "The Album_Artist ‘C’".to_string(), - }, + name: "The Album_Artist ‘C’".to_string(), + sort: Some("Album_Artist ‘C’, The".to_string()), info: ArtistInfo { - sort: Some("Album_Artist ‘C’, The".to_string()), mb_ref: ArtistMbRef::CannotHaveMbid, properties: HashMap::new(), }, @@ -413,12 +410,11 @@ macro_rules! full_collection { ], }, Artist { + id: ArtistId(4), meta: ArtistMeta { - id: ArtistId { - name: "Album_Artist ‘D’".to_string(), - }, + name: "Album_Artist ‘D’".to_string(), + sort: None, info: ArtistInfo { - sort: None, mb_ref: ArtistMbRef::None, properties: HashMap::new(), }, diff --git a/src/testmod/library.rs b/src/testmod/library.rs index 86eb376..f2abd8f 100644 --- a/src/testmod/library.rs +++ b/src/testmod/library.rs @@ -3,12 +3,11 @@ macro_rules! library_collection { () => { vec![ Artist { + id: ArtistId(0), meta: ArtistMeta { - id: ArtistId { - name: "Album_Artist ‘A’".to_string(), - }, + name: "Album_Artist ‘A’".to_string(), + sort: None, info: ArtistInfo { - sort: None, mb_ref: ArtistMbRef::None, properties: HashMap::new(), }, @@ -110,12 +109,11 @@ macro_rules! library_collection { ], }, Artist { + id: ArtistId(0), meta: ArtistMeta { - id: ArtistId { - name: "Album_Artist ‘B’".to_string(), - }, + name: "Album_Artist ‘B’".to_string(), + sort: None, info: ArtistInfo { - sort: None, mb_ref: ArtistMbRef::None, properties: HashMap::new(), }, @@ -272,12 +270,11 @@ macro_rules! library_collection { ], }, Artist { + id: ArtistId(0), meta: ArtistMeta { - id: ArtistId { - name: "The Album_Artist ‘C’".to_string(), - }, + name: "The Album_Artist ‘C’".to_string(), + sort: Some("Album_Artist ‘C’, The".to_string()), info: ArtistInfo { - sort: Some("Album_Artist ‘C’, The".to_string()), mb_ref: ArtistMbRef::None, properties: HashMap::new(), }, @@ -360,12 +357,11 @@ macro_rules! library_collection { ], }, Artist { + id: ArtistId(0), meta: ArtistMeta { - id: ArtistId { - name: "Album_Artist ‘D’".to_string(), - }, + name: "Album_Artist ‘D’".to_string(), + sort: None, info: ArtistInfo { - sort: None, mb_ref: ArtistMbRef::None, properties: HashMap::new(), }, diff --git a/src/tui/app/machine/fetch_state.rs b/src/tui/app/machine/fetch_state.rs index 3c0deeb..abb4e25 100644 --- a/src/tui/app/machine/fetch_state.rs +++ b/src/tui/app/machine/fetch_state.rs @@ -6,7 +6,7 @@ use std::{ use musichoard::collection::{ album::{Album, AlbumId, AlbumMeta}, - artist::{Artist, ArtistId, ArtistMeta}, + artist::{Artist, ArtistId}, musicbrainz::{IMusicBrainzRef, MbArtistRef, MbRefOption, Mbid}, }; @@ -14,7 +14,7 @@ use crate::tui::{ app::{ machine::{match_state::MatchState, App, AppInner, AppMachine}, selection::KeySelection, - AppPublicState, AppState, Category, IAppEventFetch, IAppInteractFetch, + AppPublicState, AppState, ArtistMatching, Category, IAppEventFetch, IAppInteractFetch, }, lib::interface::musicbrainz::daemon::{ EntityList, Error as DaemonError, IMbJobSender, MbApiResult, MbParams, MbReturn, @@ -115,7 +115,7 @@ impl AppMachine { let mut requests = Self::search_artist_job(artist); if requests.is_empty() { fetch = FetchState::fetch(rx); - requests = Self::browse_release_group_job(&artist.meta.info.mb_ref); + requests = Self::browse_release_group_job(artist.id, &artist.meta.info.mb_ref); } else { fetch = FetchState::search(rx); } @@ -130,10 +130,9 @@ impl AppMachine { Some(album_state) => &artist.albums[album_state.index], None => return Err("cannot fetch album: no album selected"), }; - let artist_id = &artist.meta.id; SubmitJob { fetch: FetchState::search(rx), - requests: Self::search_release_group_job(artist_id, arid, album), + requests: Self::search_release_group_job(artist.id, arid, album), } } }; @@ -191,9 +190,9 @@ impl AppMachine { let selection = KeySelection::get(coll, &inner.selection); // Find the artist in the full collection to correctly identify already existing albums. - let artist_id = artist.meta.id.clone(); + let artist_id = artist.id.clone(); let coll = inner.music_hoard.get_collection(); - let artist = coll.iter().find(|a| a.meta.id == artist_id).unwrap(); + let artist = coll.iter().find(|a| a.id == artist_id).unwrap(); for new in Self::new_albums(fetch_albums, &artist.albums).into_iter() { inner.music_hoard.add_album(&artist_id, new)?; @@ -223,11 +222,11 @@ impl AppMachine { pub fn app_lookup_artist( inner: AppInner, fetch: FetchState, - artist: &ArtistMeta, + matching: ArtistMatching, mbid: Mbid, ) -> App { let f = Self::submit_lookup_artist_job; - Self::app_lookup(f, inner, fetch, artist, mbid) + Self::app_lookup(f, inner, fetch, matching, mbid) } pub fn app_lookup_album( @@ -243,18 +242,18 @@ impl AppMachine { Self::app_lookup(f, inner, fetch, album_id, mbid) } - fn app_lookup( + fn app_lookup( submit: F, inner: AppInner, mut fetch: FetchState, - meta: Meta, + matching: Matching, mbid: Mbid, ) -> App where - F: FnOnce(&dyn IMbJobSender, ResultSender, Meta, Mbid) -> Result<(), DaemonError>, + F: FnOnce(&dyn IMbJobSender, ResultSender, Matching, Mbid) -> Result<(), DaemonError>, { let (lookup_tx, lookup_rx) = mpsc::channel::(); - if let Err(err) = submit(&*inner.musicbrainz, lookup_tx, meta, mbid) { + if let Err(err) = submit(&*inner.musicbrainz, lookup_tx, matching, mbid) { return AppMachine::error_state(inner, err.to_string()).into(); } fetch.lookup_rx.replace(lookup_rx); @@ -264,58 +263,70 @@ impl AppMachine { fn search_artist_job(artist: &Artist) -> VecDeque { match artist.meta.info.mb_ref { MbRefOption::Some(ref arid) => { - Self::search_albums_requests(&artist.meta.id, arid, &artist.albums) + Self::search_albums_requests(artist.id, arid, &artist.albums) } MbRefOption::CannotHaveMbid => VecDeque::new(), - MbRefOption::None => Self::search_artist_request(&artist.meta), + MbRefOption::None => Self::search_artist_request(ArtistMatching::new( + artist.id, + artist.meta.name.clone(), + )), } } fn search_release_group_job( - artist_id: &ArtistId, + artist_id: ArtistId, artist_mbid: &MbArtistRef, album: &Album, ) -> VecDeque { Self::search_albums_requests(artist_id, artist_mbid, slice::from_ref(album)) } - fn search_artist_request(meta: &ArtistMeta) -> VecDeque { - VecDeque::from([MbParams::search_artist(meta.clone())]) + fn search_artist_request(matching: ArtistMatching) -> VecDeque { + VecDeque::from([MbParams::search_artist(matching)]) } fn search_albums_requests( - artist: &ArtistId, - arid: &MbArtistRef, + artist_id: ArtistId, + artist_mbid: &MbArtistRef, albums: &[Album], ) -> VecDeque { - let arid = arid.mbid(); + let arid = artist_mbid.mbid(); albums .iter() .filter(|album| album.meta.id.mb_ref.is_none()) .map(|album| { - MbParams::search_release_group(artist.clone(), arid.clone(), album.meta.clone()) + MbParams::search_release_group(artist_id, arid.clone(), album.meta.clone()) }) .collect() } - fn browse_release_group_job(mbopt: &MbRefOption) -> VecDeque { + fn browse_release_group_job( + artist_id: ArtistId, + mbopt: &MbRefOption, + ) -> VecDeque { match mbopt { - MbRefOption::Some(mbref) => Self::browse_release_group_request(mbref), + MbRefOption::Some(mbref) => Self::browse_release_group_request(artist_id, mbref), _ => VecDeque::new(), } } - fn browse_release_group_request(mbref: &MbArtistRef) -> VecDeque { - VecDeque::from([MbParams::browse_release_group(mbref.mbid().clone())]) + fn browse_release_group_request( + artist_id: ArtistId, + mbref: &MbArtistRef, + ) -> VecDeque { + VecDeque::from([MbParams::browse_release_group( + artist_id, + mbref.mbid().clone(), + )]) } fn submit_lookup_artist_job( musicbrainz: &dyn IMbJobSender, result_sender: ResultSender, - artist: &ArtistMeta, + matching: ArtistMatching, mbid: Mbid, ) -> Result<(), DaemonError> { - let requests = VecDeque::from([MbParams::lookup_artist(artist.clone(), mbid)]); + let requests = VecDeque::from([MbParams::lookup_artist(matching, mbid)]); musicbrainz.submit_foreground_job(result_sender, requests) } @@ -395,16 +406,18 @@ mod tests { let mut fetch = FetchState::search(fetch_rx); fetch.lookup_rx.replace(lookup_rx); - let artist = COLLECTION[3].meta.clone(); + let id = COLLECTION[3].id; + let meta = COLLECTION[3].meta.clone(); + let matching = ArtistMatching::new(id, meta.name.clone()); let matches: Vec> = vec![]; - let fetch_result = MbReturn::Match(EntityMatches::artist_search(artist.clone(), matches)); + let fetch_result = MbReturn::Match(EntityMatches::artist_search(matching.clone(), matches)); fetch_tx.send(Ok(fetch_result.clone())).unwrap(); assert_eq!(fetch.try_recv(), Err(TryRecvError::Empty)); - let lookup = Entity::new(artist.clone()); - let lookup_result = MbReturn::Match(EntityMatches::artist_lookup(artist.clone(), lookup)); + let lookup = Entity::new(meta.clone()); + let lookup_result = MbReturn::Match(EntityMatches::artist_lookup(matching.clone(), lookup)); lookup_tx.send(Ok(lookup_result.clone())).unwrap(); assert_eq!(fetch.try_recv(), Ok(Ok(lookup_result))); @@ -445,7 +458,7 @@ mod tests { fn fetch_single_album() { let mut mb_job_sender = MockIMbJobSender::new(); - let artist_id = COLLECTION[1].meta.id.clone(); + let artist_id = COLLECTION[1].id; let artist_mbid: Mbid = "11111111-1111-1111-1111-111111111111".try_into().unwrap(); let album_meta = COLLECTION[1].albums[0].meta.clone(); @@ -520,7 +533,7 @@ mod tests { fn fetch_albums() { let mut mb_job_sender = MockIMbJobSender::new(); - let artist_id = COLLECTION[1].meta.id.clone(); + let artist_id = COLLECTION[1].id; let artist_mbid: Mbid = "11111111-1111-1111-1111-111111111111".try_into().unwrap(); let album_1_meta = COLLECTION[1].albums[0].meta.clone(); @@ -566,7 +579,7 @@ mod tests { fn lookup_album() { let mut mb_job_sender = MockIMbJobSender::new(); - let artist_id = COLLECTION[1].meta.id.clone(); + let artist_id = COLLECTION[1].id; let album_id = COLLECTION[1].albums[0].meta.id.clone(); lookup_album_expectation(&mut mb_job_sender, &artist_id, &album_id); @@ -579,8 +592,8 @@ mod tests { AppMachine::app_lookup_album(inner, fetch, &artist_id, &album_id, mbid()); } - fn search_artist_expectation(job_sender: &mut MockIMbJobSender, artist: &ArtistMeta) { - let requests = VecDeque::from([MbParams::search_artist(artist.clone())]); + fn search_artist_expectation(job_sender: &mut MockIMbJobSender, artist: ArtistMatching) { + let requests = VecDeque::from([MbParams::search_artist(artist)]); job_sender .expect_submit_background_job() .with(predicate::always(), predicate::eq(requests)) @@ -592,8 +605,10 @@ mod tests { fn fetch_artist() { let mut mb_job_sender = MockIMbJobSender::new(); - let artist = COLLECTION[3].meta.clone(); - search_artist_expectation(&mut mb_job_sender, &artist); + let id = COLLECTION[3].id; + let name = COLLECTION[3].meta.name.clone(); + let matching = ArtistMatching::new(id, name); + search_artist_expectation(&mut mb_job_sender, matching); let music_hoard = music_hoard(COLLECTION.to_owned()); let inner = AppInner::new(music_hoard, mb_job_sender); @@ -608,8 +623,8 @@ mod tests { assert!(matches!(app, AppState::Fetch(_))); } - fn lookup_artist_expectation(job_sender: &mut MockIMbJobSender, artist: &ArtistMeta) { - let requests = VecDeque::from([MbParams::lookup_artist(artist.clone(), mbid())]); + fn lookup_artist_expectation(job_sender: &mut MockIMbJobSender, artist: ArtistMatching) { + let requests = VecDeque::from([MbParams::lookup_artist(artist, mbid())]); job_sender .expect_submit_foreground_job() .with(predicate::always(), predicate::eq(requests)) @@ -621,8 +636,10 @@ mod tests { fn lookup_artist() { let mut mb_job_sender = MockIMbJobSender::new(); - let artist = COLLECTION[3].meta.clone(); - lookup_artist_expectation(&mut mb_job_sender, &artist); + let id = COLLECTION[3].id; + let name = COLLECTION[3].meta.name.clone(); + let matching = ArtistMatching::new(id, name); + lookup_artist_expectation(&mut mb_job_sender, matching.clone()); let music_hoard = music_hoard(COLLECTION.to_owned()); let inner = AppInner::new(music_hoard, mb_job_sender); @@ -630,7 +647,7 @@ mod tests { let (_fetch_tx, fetch_rx) = mpsc::channel(); let fetch = FetchState::search(fetch_rx); - AppMachine::app_lookup_artist(inner, fetch, &artist, mbid()); + AppMachine::app_lookup_artist(inner, fetch, matching, mbid()); } #[test] @@ -671,7 +688,9 @@ mod tests { .expect_submit_foreground_job() .return_once(|_, _| Err(DaemonError::JobChannelDisconnected)); - let artist = COLLECTION[3].meta.clone(); + let id = COLLECTION[3].id; + let name = COLLECTION[3].meta.name.clone(); + let matching = ArtistMatching::new(id, name); let music_hoard = music_hoard(COLLECTION.to_owned()); let inner = AppInner::new(music_hoard, mb_job_sender); @@ -679,7 +698,7 @@ mod tests { let (_fetch_tx, fetch_rx) = mpsc::channel(); let fetch = FetchState::search(fetch_rx); - let app = AppMachine::app_lookup_artist(inner, fetch, &artist, mbid()); + let app = AppMachine::app_lookup_artist(inner, fetch, matching, mbid()); assert!(matches!(app, AppState::Error(_))); } @@ -687,10 +706,12 @@ mod tests { fn recv_ok_match_ok() { let (tx, rx) = mpsc::channel::(); - let artist = COLLECTION[3].meta.clone(); + let id = COLLECTION[3].id; + let name = COLLECTION[3].meta.name.clone(); + let matching = ArtistMatching::new(id, name); let artist_match = Entity::with_score(COLLECTION[2].meta.clone(), 80); let artist_match_info = - EntityMatches::artist_search(artist.clone(), vec![artist_match.clone()]); + EntityMatches::artist_search(matching.clone(), vec![artist_match.clone()]); let fetch_result = Ok(MbReturn::Match(artist_match_info)); tx.send(fetch_result).unwrap(); @@ -706,7 +727,7 @@ mod tests { MatchOption::CannotHaveMbid, MatchOption::ManualInputMbid, ]; - let expected = EntityMatches::artist_search(artist, match_options); + let expected = EntityMatches::artist_search(matching, match_options); assert_eq!(match_state.matches, &expected); } @@ -730,7 +751,7 @@ mod tests { let (tx, rx) = mpsc::channel::(); let fetch = FetchState::fetch(rx); - let artist_id = collection[0].meta.id.clone(); + let artist_id = collection[0].id; let old_album = collection[0].albums[0].meta.clone(); let new_album = AlbumMeta::new(AlbumId::new("some new album")); @@ -764,7 +785,7 @@ mod tests { let (tx, rx) = mpsc::channel::(); let fetch = FetchState::fetch(rx); - let artist_id = collection[0].meta.id.clone(); + let artist_id = collection[0].id; let old_album = collection[0].albums[0].meta.clone(); let new_album = AlbumMeta::new(AlbumId::new("some new album")); @@ -802,7 +823,7 @@ mod tests { } fn browse_release_group_expectation(artist: &Artist) -> MockIMbJobSender { - let requests = AppMachine::browse_release_group_job(&artist.meta.info.mb_ref); + let requests = AppMachine::browse_release_group_job(artist.id, &artist.meta.info.mb_ref); let mut mb_job_sender = MockIMbJobSender::new(); mb_job_sender .expect_submit_background_job() @@ -858,8 +879,10 @@ mod tests { let app = AppMachine::app_fetch_next(inner, fetch); assert!(matches!(app, AppState::Fetch(_))); - let artist = COLLECTION[3].meta.clone(); - let match_info = EntityMatches::artist_search::>(artist, vec![]); + let id = COLLECTION[3].id; + let name = COLLECTION[3].meta.name.clone(); + let matching = ArtistMatching::new(id, name); + let match_info = EntityMatches::artist_search::>(matching, vec![]); let fetch_result = Ok(MbReturn::Match(match_info)); tx.send(fetch_result).unwrap(); diff --git a/src/tui/app/machine/match_state.rs b/src/tui/app/machine/match_state.rs index 2121bc2..90c5c5c 100644 --- a/src/tui/app/machine/match_state.rs +++ b/src/tui/app/machine/match_state.rs @@ -173,11 +173,11 @@ impl AppMachine { }; match self.state.current { EntityMatches::Artist(artist_matches) => { - let matching = &artist_matches.matching; + let matching = artist_matches.matching; AppMachine::app_lookup_artist(self.inner, self.state.fetch, matching, mbid) } EntityMatches::Album(album_matches) => { - let artist_id = &album_matches.artist; + let artist_id = &album_matches.artist_id; let matching = &album_matches.matching; AppMachine::app_lookup_album( self.inner, @@ -215,7 +215,7 @@ impl AppMachine { ) -> Result<(), musichoard::Error> { let coll = inner.music_hoard.get_collection(); let mut clashing = vec![]; - if let Some(artist) = coll.iter().find(|artist| artist.meta.id == matches.artist) { + if let Some(artist) = coll.iter().find(|artist| artist.id == matches.artist_id) { // While we expect only one, there is nothing stopping anybody from having multiple // different albums with the same MBID. let iter = artist.albums.iter(); @@ -229,15 +229,15 @@ impl AppMachine { let coll = inner.music_hoard.get_filtered(); let selection = KeySelection::get(coll, &inner.selection); - inner.music_hoard.remove_album(&matches.artist, &album)?; + inner.music_hoard.remove_album(&matches.artist_id, &album)?; let coll = inner.music_hoard.get_filtered(); inner.selection.select_by_id(coll, selection); } let mh = &mut inner.music_hoard; - mh.merge_album_info(&matches.artist, &matches.matching, tuple.info)?; - mh.set_album_mb_ref(&matches.artist, &matches.matching, tuple.mb_ref) + mh.merge_album_info(&matches.artist_id, &matches.matching, tuple.info)?; + mh.set_album_mb_ref(&matches.artist_id, &matches.matching, tuple.mb_ref) } } @@ -285,11 +285,11 @@ impl IAppInteractMatch for AppMachine { let inner = &mut self.inner; let result = match self.state.current { EntityMatches::Artist(ref mut matches) => match matches.list.extract_info(index) { - InfoOption::Info(tuple) => Self::select_artist(inner, matches, tuple), + InfoOption::Info(info) => Self::select_artist(inner, matches, info), InfoOption::NeedInput => return self.get_input(), }, EntityMatches::Album(ref mut matches) => match matches.list.extract_info(index) { - InfoOption::Info(tuple) => Self::select_album(inner, matches, tuple), + InfoOption::Info(info) => Self::select_album(inner, matches, info), InfoOption::NeedInput => return self.get_input(), }, }; @@ -318,14 +318,14 @@ mod tests { album::{ Album, AlbumDate, AlbumId, AlbumInfo, AlbumMeta, AlbumPrimaryType, AlbumSecondaryType, }, - artist::{Artist, ArtistId, ArtistMbRef, ArtistMeta}, + artist::{Artist, ArtistId, ArtistMbRef, ArtistMeta, ArtistName}, Collection, }; use crate::tui::{ app::{ machine::tests::{inner, inner_with_mb, input_event, music_hoard}, - IApp, IAppAccess, IAppInput, + ArtistMatching, IApp, IAppAccess, IAppInput, }, lib::{ interface::musicbrainz::{ @@ -352,30 +352,41 @@ mod tests { "00000000-0000-0000-0000-000000000000".try_into().unwrap() } - fn artist_meta() -> ArtistMeta { - ArtistMeta::new(ArtistId::new("Artist")).with_mb_ref(ArtistMbRef::Some(mbid().into())) + fn artist_id() -> ArtistId { + ArtistId(1) + } + + fn artist_name() -> ArtistName { + "Artist".into() + } + + fn artist_meta>(name: Name) -> ArtistMeta { + ArtistMeta::new(name).with_mb_ref(ArtistMbRef::Some(mbid().into())) } fn artist_match() -> EntityMatches { - let mut artist = artist_meta(); + let id = artist_id(); + let name = artist_name(); + let meta = artist_meta(name.clone()); - let artist_1 = artist.clone(); + let artist_1 = meta.clone(); let artist_match_1 = Entity::with_score(artist_1, 100); - let artist_2 = artist.clone(); + let artist_2 = meta.clone(); let mut artist_match_2 = Entity::with_score(artist_2, 100); artist_match_2.disambiguation = Some(String::from("some disambiguation")); - artist.clear_mb_ref(); let list = vec![artist_match_1.clone(), artist_match_2.clone()]; - EntityMatches::artist_search(artist, list) + EntityMatches::artist_search(ArtistMatching::new(id, name), list) } fn artist_lookup() -> EntityMatches { - let mut artist = artist_meta(); - artist.clear_mb_ref(); + let id = artist_id(); + let name = artist_name(); + let artist = artist_meta(name.clone()); + let lookup = Entity::new(artist.clone()); - EntityMatches::artist_lookup(artist, lookup) + EntityMatches::artist_lookup(ArtistMatching::new(id, name), lookup) } fn album_id() -> AlbumId { @@ -396,7 +407,7 @@ mod tests { } fn album_match() -> EntityMatches { - let artist_id = ArtistId::new("Artist"); + let artist_id = ArtistId(1); let mut album_id = album_id(); let album_meta = album_meta(album_id.clone()); @@ -414,7 +425,7 @@ mod tests { } fn album_lookup() -> EntityMatches { - let artist_id = ArtistId::new("Artist"); + let artist_id = ArtistId(1); let mut album_id = album_id(); let album_meta = album_meta(album_id.clone()); @@ -460,7 +471,7 @@ mod tests { let collection = vec![]; let mut music_hoard = music_hoard(collection.clone()); - let artist_id = ArtistId::new("Artist"); + let artist_id = ArtistId(0); match matches_info { EntityMatches::Album(_) => { let album_id = AlbumId::new("Album"); @@ -572,11 +583,12 @@ mod tests { match matches_info { EntityMatches::Album(_) => panic!(), EntityMatches::Artist(_) => { - let meta = artist_meta(); + let id = artist_id(); + let meta = artist_meta(artist_name()); music_hoard .expect_merge_artist_info() - .with(eq(meta.id.clone()), eq(meta.info)) + .with(eq(id), eq(meta.info)) .times(1) .return_once(|_, _| Ok(())); } @@ -596,7 +608,7 @@ mod tests { let meta = album_meta(album_id.clone()); let mb_ref = album_id.mb_ref.clone(); album_id.clear_mb_ref(); - let artist = matches.artist.clone(); + let artist = matches.artist_id; let mut seq = Sequence::new(); mh.expect_get_collection() @@ -650,7 +662,7 @@ mod tests { // matching album_id. // (1) Same artist as matches_info. - let mut artist = Artist::new(ArtistId::new("Artist")); + let mut artist = Artist::new(1, "Artist"); // (2) An album with the same album_id as the selected one. artist.albums.push(Album::new(AlbumId::new("Album"))); @@ -818,15 +830,15 @@ mod tests { #[test] fn select_manual_input_artist() { let mut mb_job_sender = MockIMbJobSender::new(); - let artist = ArtistMeta::new(ArtistId::new("Artist")); - let requests = VecDeque::from([MbParams::lookup_artist(artist.clone(), mbid())]); + let matching = ArtistMatching::new(artist_id(), artist_name()); + let requests = VecDeque::from([MbParams::lookup_artist(matching.clone(), mbid())]); mb_job_sender .expect_submit_foreground_job() .with(predicate::always(), predicate::eq(requests)) .return_once(|_, _| Ok(())); let matches_vec: Vec> = vec![]; - let artist_match = EntityMatches::artist_search(artist.clone(), matches_vec); + let artist_match = EntityMatches::artist_search(matching.clone(), matches_vec); let matches = AppMachine::match_state( inner_with_mb(music_hoard(vec![]), mb_job_sender), match_state(artist_match), @@ -846,7 +858,7 @@ mod tests { #[test] fn select_manual_input_album() { let mut mb_job_sender = MockIMbJobSender::new(); - let artist_id = ArtistId::new("Artist"); + let artist_id = artist_id(); let album = AlbumMeta::new("Album").with_date(1990); let requests = VecDeque::from([MbParams::lookup_release_group( artist_id.clone(), diff --git a/src/tui/app/machine/mod.rs b/src/tui/app/machine/mod.rs index 6f630a5..8fb20da 100644 --- a/src/tui/app/machine/mod.rs +++ b/src/tui/app/machine/mod.rs @@ -225,7 +225,10 @@ mod tests { }; use crate::tui::{ - app::{AppState, EntityMatches, IApp, IAppInput, IAppInteractBrowse, InputEvent}, + app::{ + AppState, ArtistMatching, EntityMatches, IApp, IAppInput, IAppInteractBrowse, + InputEvent, + }, lib::{ interface::musicbrainz::{api::Entity, daemon::MockIMbJobSender}, MockIMusicHoard, @@ -519,8 +522,13 @@ mod tests { let (_, rx) = mpsc::channel(); let fetch = FetchState::search(rx); - let artist = ArtistMeta::new(ArtistId::new("Artist")); - let info = EntityMatches::artist_lookup(artist.clone(), Entity::new(artist.clone())); + + let id = ArtistId(1); + let name = String::from("Artist"); + let meta = ArtistMeta::new(name.clone()); + let matching = ArtistMatching::new(id, name); + + let info = EntityMatches::artist_lookup(matching, Entity::new(meta)); app = AppMachine::match_state(app.unwrap_browse().inner, MatchState::new(info, fetch)).into(); diff --git a/src/tui/app/machine/search_state.rs b/src/tui/app/machine/search_state.rs index a7f5701..c2c0fc3 100644 --- a/src/tui/app/machine/search_state.rs +++ b/src/tui/app/machine/search_state.rs @@ -159,10 +159,10 @@ impl IAppInteractSearchPrivate for AppMachine { } fn predicate_artists(case_sens: bool, char_sens: bool, search: &str, probe: &Artist) -> bool { - let name = string::normalize_string_with(&probe.meta.id.name, !case_sens, !char_sens); + let name = string::normalize_string_with(&probe.meta.name, !case_sens, !char_sens); let mut result = name.string.starts_with(search); - if let Some(ref probe_sort) = probe.meta.info.sort { + if let Some(ref probe_sort) = probe.meta.sort { if !result { let name = string::normalize_string_with(probe_sort, !case_sens, !char_sens); result = name.string.starts_with(search); diff --git a/src/tui/app/mod.rs b/src/tui/app/mod.rs index bce93ce..57c253d 100644 --- a/src/tui/app/mod.rs +++ b/src/tui/app/mod.rs @@ -7,7 +7,7 @@ pub use selection::{Category, Selection}; use musichoard::collection::{ album::{AlbumId, AlbumMeta}, - artist::{ArtistId, ArtistMeta}, + artist::{ArtistId, ArtistMeta, ArtistName}, Collection, }; @@ -228,13 +228,28 @@ impl From> for MatchOption { #[derive(Clone, Debug, PartialEq, Eq)] pub struct ArtistMatches { - pub matching: ArtistMeta, + pub matching: ArtistMatching, pub list: Vec>, } +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct ArtistMatching { + pub id: ArtistId, + pub name: ArtistName, +} + +impl ArtistMatching { + pub fn new>(id: ArtistId, name: Name) -> Self { + ArtistMatching { + id, + name: name.into(), + } + } +} + #[derive(Clone, Debug, PartialEq, Eq)] pub struct AlbumMatches { - pub artist: ArtistId, + pub artist_id: ArtistId, pub matching: AlbumId, pub list: Vec>, } @@ -246,40 +261,43 @@ pub enum EntityMatches { } impl EntityMatches { - pub fn artist_search>>( - matching: ArtistMeta, - list: Vec, - ) -> Self { + pub fn artist_search(matching: ArtistMatching, list: Vec) -> Self + where + M: Into>, + { let list = list.into_iter().map(Into::into).collect(); EntityMatches::Artist(ArtistMatches { matching, list }) } pub fn album_search>>( - artist: ArtistId, + artist_id: ArtistId, matching: AlbumId, list: Vec, ) -> Self { let list = list.into_iter().map(Into::into).collect(); EntityMatches::Album(AlbumMatches { - artist, + artist_id, matching, list, }) } - pub fn artist_lookup>>(matching: ArtistMeta, item: M) -> Self { + pub fn artist_lookup(matching: ArtistMatching, item: M) -> Self + where + M: Into>, + { let list = vec![item.into()]; EntityMatches::Artist(ArtistMatches { matching, list }) } pub fn album_lookup>>( - artist: ArtistId, + artist_id: ArtistId, matching: AlbumId, item: M, ) -> Self { let list = vec![item.into()]; EntityMatches::Album(AlbumMatches { - artist, + artist_id, matching, list, }) diff --git a/src/tui/app/selection/artist.rs b/src/tui/app/selection/artist.rs index 851bd52..5a2dd62 100644 --- a/src/tui/app/selection/artist.rs +++ b/src/tui/app/selection/artist.rs @@ -1,10 +1,6 @@ use std::cmp; -use musichoard::collection::{ - album::Album, - artist::{Artist, ArtistId}, - track::Track, -}; +use musichoard::collection::{album::Album, artist::Artist, track::Track}; use crate::tui::app::{ selection::{ @@ -198,7 +194,7 @@ impl ArtistSelection { } pub struct KeySelectArtist { - key: (ArtistId,), + key: (String,), album: Option, } @@ -215,7 +211,7 @@ impl KeySelectArtist { } pub fn get_sort_key(&self) -> (&str,) { - (&self.key.0.name,) + (&self.key.0,) } } diff --git a/src/tui/lib/external/musicbrainz/api/mod.rs b/src/tui/lib/external/musicbrainz/api/mod.rs index 578628c..22648b4 100644 --- a/src/tui/lib/external/musicbrainz/api/mod.rs +++ b/src/tui/lib/external/musicbrainz/api/mod.rs @@ -5,7 +5,7 @@ use std::collections::HashMap; use musichoard::{ collection::{ album::{AlbumDate, AlbumId, AlbumInfo, AlbumLibId, AlbumMbRef, AlbumMeta, AlbumSeq}, - artist::{ArtistId, ArtistInfo, ArtistMbRef, ArtistMeta}, + artist::{ArtistInfo, ArtistMbRef, ArtistMeta, ArtistName}, musicbrainz::Mbid, }, external::musicbrainz::{ @@ -55,8 +55,8 @@ impl IMusicBrainz for MusicBrainz { Ok(from_lookup_release_group_response(mb_response)) } - fn search_artist(&mut self, artist: &ArtistMeta) -> Result>, Error> { - let query = SearchArtistRequest::new().string(&artist.id.name); + fn search_artist(&mut self, name: &ArtistName) -> Result>, Error> { + let query = SearchArtistRequest::new().string(name); let paging = PageSettings::default(); let mb_response = self.client.search_artist(&query, &paging)?; @@ -70,19 +70,19 @@ impl IMusicBrainz for MusicBrainz { fn search_release_group( &mut self, - arid: &Mbid, - album: &AlbumMeta, + artist_mbid: &Mbid, + meta: &AlbumMeta, ) -> Result>, Error> { // Some release groups may have a promotional early release messing up the search. Searching // with just the year should be enough anyway. - let date = AlbumDate::new(album.info.date.year, None, None); + let date = AlbumDate::new(meta.info.date.year, None, None); let query = SearchReleaseGroupRequest::new() - .arid(arid) + .arid(artist_mbid) .and() .first_release_date(&date) .and() - .release_group(&album.id.title); + .release_group(&meta.id.title); let paging = PageSettings::default(); let mb_response = self.client.search_release_group(&query, &paging)?; @@ -96,10 +96,11 @@ impl IMusicBrainz for MusicBrainz { fn browse_release_group( &mut self, - artist: &Mbid, + artist_mbid: &Mbid, paging: &mut Option, ) -> Result>, Error> { - let request = BrowseReleaseGroupRequest::artist(artist).filter_status_website_default(); + let request = + BrowseReleaseGroupRequest::artist(artist_mbid).filter_status_website_default(); let page = paging.take().unwrap_or_default(); let mb_response = self.client.browse_release_group(&request, &page)?; @@ -115,11 +116,9 @@ fn from_mb_artist_meta(meta: MbArtistMeta) -> (ArtistMeta, Option) { let sort = Some(meta.sort_name).filter(|s| s != &meta.name); ( ArtistMeta { - id: ArtistId { - name: meta.name, - }, + name: meta.name, + sort, info: ArtistInfo { - sort, mb_ref: ArtistMbRef::Some(meta.id.into()), properties: HashMap::new(), }, diff --git a/src/tui/lib/external/musicbrainz/daemon/mod.rs b/src/tui/lib/external/musicbrainz/daemon/mod.rs index da140aa..56db4cb 100644 --- a/src/tui/lib/external/musicbrainz/daemon/mod.rs +++ b/src/tui/lib/external/musicbrainz/daemon/mod.rs @@ -256,30 +256,26 @@ impl JobInstance { MbParams::Lookup(lookup) => match lookup { LookupParams::Artist(p) => musicbrainz .lookup_artist(&p.mbid) - .map(|rv| EntityMatches::artist_lookup(p.artist.clone(), rv)), - LookupParams::ReleaseGroup(p) => { - musicbrainz.lookup_release_group(&p.mbid).map(|rv| { - EntityMatches::album_lookup(p.artist_id.clone(), p.album_id.clone(), rv) - }) - } + .map(|rv| EntityMatches::artist_lookup(p.matching.clone(), rv)), + LookupParams::ReleaseGroup(p) => musicbrainz + .lookup_release_group(&p.mbid) + .map(|rv| EntityMatches::album_lookup(p.artist_id, p.id.clone(), rv)), } .map(MbReturn::Match), MbParams::Search(search) => match search { SearchParams::Artist(p) => musicbrainz - .search_artist(&p.artist) - .map(|rv| EntityMatches::artist_search(p.artist.clone(), rv)), + .search_artist(&p.matching.name) + .map(|rv| EntityMatches::artist_search(p.matching.clone(), rv)), SearchParams::ReleaseGroup(p) => musicbrainz - .search_release_group(&p.artist_mbid, &p.album) - .map(|rv| { - EntityMatches::album_search(p.artist_id.clone(), p.album.id.clone(), rv) - }), + .search_release_group(&p.artist_mbid, &p.meta) + .map(|rv| EntityMatches::album_search(p.artist_id, p.meta.id.clone(), rv)), } .map(MbReturn::Match), MbParams::Browse(browse) => match browse { BrowseParams::ReleaseGroup(params) => { Self::init_paging_if_none(paging); musicbrainz - .browse_release_group(¶ms.artist, paging) + .browse_release_group(¶ms.artist_mbid, paging) .map(|rv| EntityList::Album(rv.into_iter().map(|rg| rg.entity).collect())) } } @@ -350,11 +346,12 @@ mod tests { use mockall::{predicate, Sequence}; use musichoard::collection::{ album::AlbumMeta, - artist::{ArtistId, ArtistMeta}, + artist::{ArtistId, ArtistMeta, ArtistName}, musicbrainz::{IMusicBrainzRef, MbRefOption, Mbid}, }; use crate::tui::{ + app::ArtistMatching, event::{Event, EventError, MockIFetchCompleteEventSender}, lib::interface::musicbrainz::api::{Entity, MockIMusicBrainz}, testmod::COLLECTION, @@ -426,38 +423,43 @@ mod tests { } fn lookup_artist_requests() -> VecDeque { - let artist = COLLECTION[3].meta.clone(); + let id = COLLECTION[3].id; + let name = COLLECTION[3].meta.name.clone(); + let matching = ArtistMatching::new(id, name); let mbid = mbid(); - VecDeque::from([MbParams::lookup_artist(artist, mbid)]) + VecDeque::from([MbParams::lookup_artist(matching, mbid)]) } fn lookup_release_group_requests() -> VecDeque { - let artist_id = COLLECTION[1].meta.id.clone(); + let artist_id = COLLECTION[1].id; let album_id = COLLECTION[1].albums[0].meta.id.clone(); let mbid = mbid(); VecDeque::from([MbParams::lookup_release_group(artist_id, album_id, mbid)]) } fn search_artist_requests() -> VecDeque { - let artist = COLLECTION[3].meta.clone(); - VecDeque::from([MbParams::search_artist(artist)]) + let id = COLLECTION[3].id; + let name = COLLECTION[3].meta.name.clone(); + let matching = ArtistMatching::new(id, name); + VecDeque::from([MbParams::search_artist(matching)]) } - fn search_artist_expectations() -> (ArtistMeta, Vec>) { - let artist = COLLECTION[3].meta.clone(); + fn search_artist_expectations() -> (ArtistName, Vec>) { + let name = COLLECTION[3].meta.name.clone(); + let meta = COLLECTION[3].meta.clone(); - let artist_match_1 = Entity::with_score(artist.clone(), 100); - let artist_match_2 = Entity::with_score(artist.clone(), 50); + let artist_match_1 = Entity::with_score(meta.clone(), 100); + let artist_match_2 = Entity::with_score(meta.clone(), 50); let matches = vec![artist_match_1.clone(), artist_match_2.clone()]; - (artist, matches) + (name, matches) } fn search_albums_requests() -> VecDeque { let mbref = mb_ref_opt_as_ref(&COLLECTION[1].meta.info.mb_ref); let arid = mb_ref_opt_unwrap(mbref).mbid().clone(); - let artist_id = COLLECTION[1].meta.id.clone(); + let artist_id = COLLECTION[1].id; let album_1 = COLLECTION[1].albums[0].meta.clone(); let album_4 = COLLECTION[1].albums[3].meta.clone(); @@ -470,11 +472,15 @@ mod tests { fn browse_albums_requests() -> VecDeque { let mbref = mb_ref_opt_as_ref(&COLLECTION[1].meta.info.mb_ref); let arid = mb_ref_opt_unwrap(mbref).mbid().clone(); - VecDeque::from([MbParams::browse_release_group(arid)]) + VecDeque::from([MbParams::browse_release_group(album_artist_id(), arid)]) + } + + fn artist_id() -> ArtistId { + ArtistId(1) } fn album_artist_id() -> ArtistId { - COLLECTION[1].meta.id.clone() + COLLECTION[1].id } fn album_arid_expectation() -> Mbid { @@ -594,8 +600,12 @@ mod tests { fn execute_lookup_artist() { let mut musicbrainz = musicbrainz(); let mbid = mbid(); - let artist = COLLECTION[3].meta.clone(); - let lookup = Entity::new(artist.clone()); + let id = COLLECTION[3].id; + let name = COLLECTION[3].meta.name.clone(); + let meta = COLLECTION[3].meta.clone(); + let matching = ArtistMatching::new(id, name); + + let lookup = Entity::new(meta.clone()); lookup_artist_expectation(&mut musicbrainz, &mbid, &lookup); let mut event_sender = event_sender(); @@ -622,7 +632,7 @@ mod tests { assert_eq!( result, Ok(MbReturn::Match(EntityMatches::artist_lookup( - artist, lookup + matching, lookup ))) ); } @@ -681,13 +691,13 @@ mod tests { fn search_artist_expectation( musicbrainz: &mut MockIMusicBrainz, - artist: &ArtistMeta, + name: &ArtistName, matches: &[Entity], ) { let result = Ok(matches.to_owned()); musicbrainz .expect_search_artist() - .with(predicate::eq(artist.clone())) + .with(predicate::eq(name.clone())) .times(1) .return_once(|_| result); } @@ -695,8 +705,9 @@ mod tests { #[test] fn execute_search_artist() { let mut musicbrainz = musicbrainz(); - let (artist, matches) = search_artist_expectations(); - search_artist_expectation(&mut musicbrainz, &artist, &matches); + let id = artist_id(); + let (name, matches) = search_artist_expectations(); + search_artist_expectation(&mut musicbrainz, &name, &matches); let mut event_sender = event_sender(); fetch_complete_expectation(&mut event_sender, 1); @@ -719,10 +730,11 @@ mod tests { assert_eq!(result, Err(JobError::JobQueueEmpty)); let result = result_receiver.try_recv().unwrap(); + let matching = ArtistMatching::new(id, name); assert_eq!( result, Ok(MbReturn::Match(EntityMatches::artist_search( - artist, matches + matching, matches ))) ); } diff --git a/src/tui/lib/interface/musicbrainz/api/mod.rs b/src/tui/lib/interface/musicbrainz/api/mod.rs index 11889fc..099a61e 100644 --- a/src/tui/lib/interface/musicbrainz/api/mod.rs +++ b/src/tui/lib/interface/musicbrainz/api/mod.rs @@ -4,7 +4,11 @@ use mockall::automock; use musichoard::{ - collection::{album::AlbumMeta, artist::ArtistMeta, musicbrainz::Mbid}, + collection::{ + album::AlbumMeta, + artist::{ArtistMeta, ArtistName}, + musicbrainz::Mbid, + }, external::musicbrainz::api::PageSettings, }; @@ -12,15 +16,16 @@ use musichoard::{ pub trait IMusicBrainz { fn lookup_artist(&mut self, mbid: &Mbid) -> Result, Error>; fn lookup_release_group(&mut self, mbid: &Mbid) -> Result, Error>; - fn search_artist(&mut self, artist: &ArtistMeta) -> Result>, Error>; + fn search_artist(&mut self, name: &ArtistName) -> Result>, Error>; + // TODO: AlbumMeta -> AlbumTitle fn search_release_group( &mut self, - arid: &Mbid, - album: &AlbumMeta, + artist_mbid: &Mbid, + meta: &AlbumMeta, ) -> Result>, Error>; fn browse_release_group( &mut self, - artist: &Mbid, + artist_mbid: &Mbid, paging: &mut Option, ) -> Result>, Error>; } diff --git a/src/tui/lib/interface/musicbrainz/daemon/mod.rs b/src/tui/lib/interface/musicbrainz/daemon/mod.rs index 42eabaa..e786628 100644 --- a/src/tui/lib/interface/musicbrainz/daemon/mod.rs +++ b/src/tui/lib/interface/musicbrainz/daemon/mod.rs @@ -2,11 +2,14 @@ use std::{collections::VecDeque, fmt, sync::mpsc}; use musichoard::collection::{ album::{AlbumId, AlbumMeta}, - artist::{ArtistId, ArtistMeta}, + artist::ArtistId, musicbrainz::Mbid, }; -use crate::tui::{app::EntityMatches, lib::interface::musicbrainz::api::Error as MbApiError}; +use crate::tui::{ + app::{ArtistMatching, EntityMatches}, + lib::interface::musicbrainz::api::Error as MbApiError, +}; #[cfg(test)] use mockall::automock; @@ -74,14 +77,14 @@ pub enum LookupParams { #[derive(Clone, Debug, PartialEq, Eq)] pub struct LookupArtistParams { - pub artist: ArtistMeta, + pub matching: ArtistMatching, pub mbid: Mbid, } #[derive(Clone, Debug, PartialEq, Eq)] pub struct LookupReleaseGroupParams { pub artist_id: ArtistId, - pub album_id: AlbumId, + pub id: AlbumId, pub mbid: Mbid, } @@ -93,14 +96,15 @@ pub enum SearchParams { #[derive(Clone, Debug, PartialEq, Eq)] pub struct SearchArtistParams { - pub artist: ArtistMeta, + pub matching: ArtistMatching, } #[derive(Clone, Debug, PartialEq, Eq)] pub struct SearchReleaseGroupParams { pub artist_id: ArtistId, pub artist_mbid: Mbid, - pub album: AlbumMeta, + // TODO: probably needs AlbumId when we get there + pub meta: AlbumMeta, } #[derive(Clone, Debug, PartialEq, Eq)] @@ -110,37 +114,39 @@ pub enum BrowseParams { #[derive(Clone, Debug, PartialEq, Eq)] pub struct BrowseReleaseGroupParams { - pub artist: Mbid, + pub artist_id: ArtistId, + pub artist_mbid: Mbid, } impl MbParams { - pub fn lookup_artist(artist: ArtistMeta, mbid: Mbid) -> Self { - MbParams::Lookup(LookupParams::Artist(LookupArtistParams { artist, mbid })) + pub fn lookup_artist(matching: ArtistMatching, mbid: Mbid) -> Self { + MbParams::Lookup(LookupParams::Artist(LookupArtistParams { matching, mbid })) } - pub fn lookup_release_group(artist_id: ArtistId, album_id: AlbumId, mbid: Mbid) -> Self { + pub fn lookup_release_group(artist_id: ArtistId, id: AlbumId, mbid: Mbid) -> Self { MbParams::Lookup(LookupParams::ReleaseGroup(LookupReleaseGroupParams { artist_id, - album_id, + id, mbid, })) } - pub fn search_artist(artist: ArtistMeta) -> Self { - MbParams::Search(SearchParams::Artist(SearchArtistParams { artist })) + pub fn search_artist(matching: ArtistMatching) -> Self { + MbParams::Search(SearchParams::Artist(SearchArtistParams { matching })) } - pub fn search_release_group(artist_id: ArtistId, artist_mbid: Mbid, album: AlbumMeta) -> Self { + pub fn search_release_group(artist_id: ArtistId, artist_mbid: Mbid, meta: AlbumMeta) -> Self { MbParams::Search(SearchParams::ReleaseGroup(SearchReleaseGroupParams { artist_id, artist_mbid, - album, + meta, })) } - pub fn browse_release_group(artist: Mbid) -> Self { + pub fn browse_release_group(artist_id: ArtistId, artist_mbid: Mbid) -> Self { MbParams::Browse(BrowseParams::ReleaseGroup(BrowseReleaseGroupParams { - artist, + artist_id, + artist_mbid, })) } } diff --git a/src/tui/ui/browse_state.rs b/src/tui/ui/browse_state.rs index b53e5b1..3e9ba02 100644 --- a/src/tui/ui/browse_state.rs +++ b/src/tui/ui/browse_state.rs @@ -126,7 +126,7 @@ impl<'a, 'b> ArtistState<'a, 'b> { let list = List::new( artists .iter() - .map(|a| ListItem::new(a.meta.id.name.as_str())) + .map(|a| ListItem::new(a.meta.name.as_str())) .collect::>(), ); diff --git a/src/tui/ui/display.rs b/src/tui/ui/display.rs index efdb03d..a640b43 100644 --- a/src/tui/ui/display.rs +++ b/src/tui/ui/display.rs @@ -3,7 +3,7 @@ use musichoard::collection::{ AlbumDate, AlbumId, AlbumLibId, AlbumMeta, AlbumOwnership, AlbumPrimaryType, AlbumSecondaryType, AlbumSeq, }, - artist::ArtistMeta, + artist::{ArtistMeta, ArtistName}, musicbrainz::{IMusicBrainzRef, MbRefOption}, track::{TrackFormat, TrackQuality}, }; @@ -118,8 +118,8 @@ impl UiDisplay { } } - pub fn display_artist_matching(artist: &ArtistMeta) -> String { - format!("Matching artist: {}", &artist.id.name) + pub fn display_artist_matching(name: &ArtistName) -> String { + format!("Matching artist: {}", name) } pub fn display_album_matching(album: &AlbumId) -> String { @@ -128,7 +128,7 @@ impl UiDisplay { pub fn display_matching_info(info: &EntityMatches) -> String { match info { - EntityMatches::Artist(m) => UiDisplay::display_artist_matching(&m.matching), + EntityMatches::Artist(m) => UiDisplay::display_artist_matching(&m.matching.name), EntityMatches::Album(m) => UiDisplay::display_album_matching(&m.matching), } } @@ -159,7 +159,7 @@ impl UiDisplay { fn display_option_artist(artist: &ArtistMeta, disambiguation: &Option) -> String { format!( "{}{}", - artist.id.name, + artist.name, disambiguation .as_ref() .filter(|s| !s.is_empty()) diff --git a/src/tui/ui/info_state.rs b/src/tui/ui/info_state.rs index 234e33d..8fe8902 100644 --- a/src/tui/ui/info_state.rs +++ b/src/tui/ui/info_state.rs @@ -74,7 +74,7 @@ impl<'a> ArtistOverlay<'a> { "Artist: {}\n\n{item_indent}\ MusicBrainz: {}\n{item_indent}\ Properties: {}", - artist.map(|a| a.meta.id.name.as_str()).unwrap_or(""), + artist.map(|a| a.meta.name.as_str()).unwrap_or(""), artist .map(|a| UiDisplay::display_mb_ref_option_as_url(&a.meta.info.mb_ref)) .unwrap_or_default(), diff --git a/src/tui/ui/match_state.rs b/src/tui/ui/match_state.rs index 99725b1..1468946 100644 --- a/src/tui/ui/match_state.rs +++ b/src/tui/ui/match_state.rs @@ -1,6 +1,6 @@ use musichoard::collection::{ album::{AlbumId, AlbumMeta}, - artist::ArtistMeta, + artist::{ArtistMeta, ArtistName}, }; use ratatui::widgets::{List, ListItem}; @@ -18,13 +18,13 @@ pub struct MatchOverlay<'a, 'b> { impl<'a, 'b> MatchOverlay<'a, 'b> { pub fn new(info: &'a EntityMatches, state: &'b mut WidgetState) -> Self { match info { - EntityMatches::Artist(m) => Self::artists(&m.matching, &m.list, state), + EntityMatches::Artist(m) => Self::artists(&m.matching.name, &m.list, state), EntityMatches::Album(m) => Self::albums(&m.matching, &m.list, state), } } fn artists( - matching: &ArtistMeta, + matching: &ArtistName, matches: &'a [MatchOption], state: &'b mut WidgetState, ) -> Self { diff --git a/src/tui/ui/mod.rs b/src/tui/ui/mod.rs index b281816..e8ee315 100644 --- a/src/tui/ui/mod.rs +++ b/src/tui/ui/mod.rs @@ -200,11 +200,11 @@ impl IUi for Ui { mod tests { use musichoard::collection::{ album::{AlbumDate, AlbumId, AlbumInfo, AlbumMeta, AlbumPrimaryType, AlbumSecondaryType}, - artist::{Artist, ArtistId, ArtistMeta}, + artist::{Artist, ArtistId, ArtistMeta, ArtistName}, }; use crate::tui::{ - app::{AppPublic, AppPublicInner, Delta, MatchStatePublic}, + app::{AppPublic, AppPublicInner, ArtistMatching, Delta, MatchStatePublic}, lib::interface::musicbrainz::api::Entity, testmod::COLLECTION, tests::terminal, @@ -287,7 +287,7 @@ mod tests { #[test] fn empty_album() { - let mut artists: Vec = vec![Artist::new(ArtistId::new("An artist"))]; + let mut artists: Vec = vec![Artist::new(0, "An artist")]; artists[0].albums.push(Album::new("An album")); let mut selection = Selection::new(&artists); @@ -324,33 +324,49 @@ mod tests { draw_test_suite(artists, &mut selection); } - fn artist_meta() -> ArtistMeta { - ArtistMeta::new(ArtistId::new("an artist")) + fn artist_id() -> ArtistId { + ArtistId(1) + } + + fn artist_name() -> ArtistName { + "an artist".into() + } + + fn artist_meta>(name: Name) -> ArtistMeta { + ArtistMeta::new(name) } fn artist_matches() -> EntityMatches { - let artist = artist_meta(); - let artist_match = Entity::with_score(artist.clone(), 80); + let id = artist_id(); + let name = artist_name(); + let meta = artist_meta(name.clone()); + let matching = ArtistMatching::new(id, name); + + let artist_match = Entity::with_score(meta, 80); let list = vec![artist_match.clone(), artist_match.clone()]; - let mut info = EntityMatches::artist_search(artist, list); + let mut info = EntityMatches::artist_search(matching, list); info.push_cannot_have_mbid(); info.push_manual_input_mbid(); info } fn artist_lookup() -> EntityMatches { - let artist = artist_meta(); - let artist_lookup = Entity::new(artist.clone()); + let id = artist_id(); + let name = artist_name(); + let meta = artist_meta(name.clone()); + let matching = ArtistMatching::new(id, name); - let mut info = EntityMatches::artist_lookup(artist, artist_lookup); + let artist_lookup = Entity::new(meta.clone()); + + let mut info = EntityMatches::artist_lookup(matching, artist_lookup); info.push_cannot_have_mbid(); info.push_manual_input_mbid(); info } fn album_artist_id() -> ArtistId { - ArtistId::new("Artist") + ArtistId(1) } fn album_id() -> AlbumId { diff --git a/tests/testlib.rs b/tests/testlib.rs index 2664f41..4ef98b7 100644 --- a/tests/testlib.rs +++ b/tests/testlib.rs @@ -15,12 +15,11 @@ use musichoard::collection::{ pub static COLLECTION: Lazy> = Lazy::new(|| -> Collection { vec![ Artist { + id: ArtistId(1), meta: ArtistMeta { - id: ArtistId { - name: String::from("Аркона"), - }, + name: String::from("Аркона"), + sort: Some(String::from("Arkona")), info: ArtistInfo { - sort: Some(String::from("Arkona")), mb_ref: ArtistMbRef::Some(MbArtistRef::from_url_str( "https://musicbrainz.org/artist/baad262d-55ef-427a-83c7-f7530964f212" ).unwrap()), @@ -207,12 +206,11 @@ pub static COLLECTION: Lazy> = Lazy::new(|| -> Collection { }], }, Artist { + id: ArtistId(2), meta: ArtistMeta { - id: ArtistId { - name: String::from("Eluveitie"), - }, + name: String::from("Eluveitie"), + sort: None, info: ArtistInfo { - sort: None, mb_ref: ArtistMbRef::Some(MbArtistRef::from_url_str( "https://musicbrainz.org/artist/8000598a-5edb-401c-8e6d-36b167feaf38" ).unwrap()), @@ -456,12 +454,11 @@ pub static COLLECTION: Lazy> = Lazy::new(|| -> Collection { ], }, Artist { + id: ArtistId(3), meta: ArtistMeta { - id: ArtistId { - name: String::from("Frontside"), - }, + name: String::from("Frontside"), + sort: None, info: ArtistInfo { - sort: None, mb_ref: ArtistMbRef::Some(MbArtistRef::from_url_str( "https://musicbrainz.org/artist/3a901353-fccd-4afd-ad01-9c03f451b490" ).unwrap()), @@ -612,12 +609,11 @@ pub static COLLECTION: Lazy> = Lazy::new(|| -> Collection { }], }, Artist { + id: ArtistId(4), meta: ArtistMeta { - id: ArtistId { - name: String::from("Heaven’s Basement"), - }, + name: String::from("Heaven’s Basement"), + sort: Some(String::from("Heaven’s Basement")), info: ArtistInfo { - sort: Some(String::from("Heaven’s Basement")), mb_ref: ArtistMbRef::Some(MbArtistRef::from_url_str( "https://musicbrainz.org/artist/c2c4d56a-d599-4a18-bd2f-ae644e2198cc" ).unwrap()), @@ -746,12 +742,11 @@ pub static COLLECTION: Lazy> = Lazy::new(|| -> Collection { }], }, Artist { + id: ArtistId(5), meta: ArtistMeta { - id: ArtistId { - name: String::from("Metallica"), - }, + name: String::from("Metallica"), + sort: None, info: ArtistInfo { - sort: None, mb_ref: ArtistMbRef::Some(MbArtistRef::from_url_str( "https://musicbrainz.org/artist/65f4f0c5-ef9e-490c-aee3-909e7ae6b2ab" ).unwrap()), -- 2.47.1 From 38a2e1f33c86e76da951e691c84bc23361681666 Mon Sep 17 00:00:00 2001 From: Wojciech Kozlowski Date: Sun, 19 Jan 2025 20:43:16 +0100 Subject: [PATCH 12/12] Merge figured out --- src/core/musichoard/base.rs | 26 ++------------------- src/core/musichoard/database.rs | 40 ++++++++++++++++++++++++++++----- src/core/musichoard/library.rs | 32 +++++++++++++++++--------- 3 files changed, 57 insertions(+), 41 deletions(-) diff --git a/src/core/musichoard/base.rs b/src/core/musichoard/base.rs index 1047520..0372b4a 100644 --- a/src/core/musichoard/base.rs +++ b/src/core/musichoard/base.rs @@ -2,10 +2,9 @@ use crate::core::{ collection::{ album::{Album, AlbumId}, artist::{Artist, ArtistId}, - merge::{MergeCollections, NormalMap}, - string, Collection, + Collection, }, - musichoard::{filter::CollectionFilter, Error, LibArtist, MusicHoard}, + musichoard::{filter::CollectionFilter, Error, MusicHoard}, }; pub trait IMusicHoardBase { @@ -32,7 +31,6 @@ impl IMusicHoardBase for MusicHoard { pub trait IMusicHoardBasePrivate { fn sort_albums_and_tracks<'a, COL: Iterator>>(collection: COL); - fn merge_collections>(&self, database: It) -> Collection; fn filter_collection(&self) -> Collection; fn filter_artist(&self, artist: &Artist) -> Option; @@ -63,26 +61,6 @@ impl IMusicHoardBasePrivate for MusicHoard } } - fn merge_collections>(&self, database: It) -> Collection { - let mut primary = NormalMap::::new(); - let mut secondary = NormalMap::::new(); - - for (normal_name, artist) in self.library_cache.clone().into_iter() { - primary.insert(normal_name, artist); - } - - for artist in database.into_iter() { - secondary.insert(string::normalize_string(&artist.meta.name), artist); - } - - let (mut collection, left) = MergeCollections::merge_by_name(primary, secondary); - - // TODO: Insert what's left into the DB to get IDs and then append to collection. - - collection.sort_unstable(); - collection - } - fn filter_collection(&self) -> Collection { let iter = self.collection.iter(); iter.flat_map(|a| self.filter_artist(a)).collect() diff --git a/src/core/musichoard/database.rs b/src/core/musichoard/database.rs index 2848535..62b8d2a 100644 --- a/src/core/musichoard/database.rs +++ b/src/core/musichoard/database.rs @@ -4,7 +4,8 @@ use crate::{ collection::{ album::{AlbumInfo, AlbumMbRef, AlbumMeta}, artist::ArtistInfo, - merge::Merge, + merge::{Merge, MergeCollections, NormalMap}, + string, }, core::{ collection::{ @@ -13,11 +14,15 @@ use crate::{ Collection, }, interface::database::IDatabase, - musichoard::{base::IMusicHoardBasePrivate, Error, MusicHoard, NoDatabase}, + musichoard::{ + base::IMusicHoardBasePrivate, Error, IntoId, LibArtist, MusicHoard, NoDatabase, + }, }, }; pub trait IMusicHoardDatabase { + fn merge_collections(&mut self) -> Result; + fn reload_database(&mut self) -> Result<(), Error>; fn merge_artist_info>( @@ -64,11 +69,34 @@ pub trait IMusicHoardDatabase { } impl IMusicHoardDatabase for MusicHoard { - fn reload_database(&mut self) -> Result<(), Error> { - let mut database_cache = self.database.load()?; - Self::sort_albums_and_tracks(database_cache.iter_mut().map(|a| &mut a.albums)); + fn merge_collections(&mut self) -> Result { + let mut database = self.database.load()?; + Self::sort_albums_and_tracks(database.iter_mut().map(|a| &mut a.albums)); - self.collection = self.merge_collections(database_cache); + let mut primary = NormalMap::::new(); + for (normal_name, artist) in self.library_cache.clone().into_iter() { + primary.insert(normal_name, artist); + } + + let mut secondary = NormalMap::::new(); + for artist in database.into_iter() { + secondary.insert(string::normalize_string(&artist.meta.name), artist); + } + + let (mut collection, lib_artists) = MergeCollections::merge_by_name(primary, secondary); + + for lib_artist in lib_artists.into_iter() { + let id = self.database.insert_artist(&lib_artist.meta)?; + collection.push(lib_artist.into_id(&id)); + } + + collection.sort_unstable(); + + Ok(collection) + } + + fn reload_database(&mut self) -> Result<(), Error> { + self.collection = self.merge_collections()?; self.filtered = self.filter_collection(); Ok(()) diff --git a/src/core/musichoard/library.rs b/src/core/musichoard/library.rs index ee76998..584b1fc 100644 --- a/src/core/musichoard/library.rs +++ b/src/core/musichoard/library.rs @@ -1,20 +1,24 @@ use std::collections::HashMap; use crate::{ - collection::string::{self, NormalString}, + collection::{ + artist::ArtistId, + merge::IntoId, + string::{self, NormalString}, + }, core::{ collection::{ album::{Album, AlbumDate, AlbumId, AlbumMbRef}, track::{Track, TrackId, TrackNum, TrackQuality}, - Collection, }, interface::{ database::IDatabase, library::{ILibrary, Item, Query}, }, musichoard::{ - base::IMusicHoardBasePrivate, database::IMusicHoardDatabasePrivate, Error, LibArtist, - MusicHoard, NoDatabase, + base::IMusicHoardBasePrivate, + database::{IMusicHoardDatabase, IMusicHoardDatabasePrivate}, + Error, LibArtist, MusicHoard, NoDatabase, }, }, }; @@ -25,28 +29,34 @@ pub trait IMusicHoardLibrary { impl IMusicHoardLibrary for MusicHoard { fn rescan_library(&mut self) -> Result<(), Error> { - self.collection = self.rescan_library_inner(vec![])?; + self.rescan_library_inner()?; + self.collection = self + .library_cache + .values() + .cloned() + .enumerate() + .map(|(ix, la)| la.into_id(&ArtistId(ix))) + .collect(); + self.collection.sort_unstable(); self.commit() } } impl IMusicHoardLibrary for MusicHoard { fn rescan_library(&mut self) -> Result<(), Error> { - let mut database_cache = self.database.load()?; - Self::sort_albums_and_tracks(database_cache.iter_mut().map(|a| &mut a.albums)); + self.rescan_library_inner()?; + self.collection = self.merge_collections()?; - self.collection = self.rescan_library_inner(database_cache)?; self.commit() } } impl MusicHoard { - fn rescan_library_inner(&mut self, database: Collection) -> Result { + fn rescan_library_inner(&mut self) -> Result<(), Error> { let items = self.library.list(&Query::new())?; self.library_cache = Self::items_to_artists(items)?; Self::sort_albums_and_tracks(self.library_cache.values_mut().map(|la| &mut la.albums)); - - Ok(self.merge_collections(database)) + Ok(()) } fn items_to_artists(items: Vec) -> Result, Error> { -- 2.47.1