diff --git a/.gitattributes b/.gitattributes index b1f76c8..d126110 100644 --- a/.gitattributes +++ b/.gitattributes @@ -1 +1 @@ - src/external/** linguist-vendored=false + **/external/** linguist-vendored=false diff --git a/src/core/collection/album.rs b/src/core/collection/album.rs index 66c45e6..b69f820 100644 --- a/src/core/collection/album.rs +++ b/src/core/collection/album.rs @@ -4,7 +4,7 @@ use std::{ }; use crate::core::collection::{ - merge::{Merge, MergeSorted}, + merge::{Merge, MergeName, MergeSorted}, musicbrainz::{MbAlbumRef, MbRefOption}, track::{Track, TrackFormat}, }; @@ -57,6 +57,12 @@ impl AlbumLibId { /// Unique database identifier. Use MBID for this purpose. pub type AlbumDbId = MbRefOption; +impl MergeName for Album { + fn name(&self) -> &str { + &self.meta.id.title + } +} + // There are crates for handling dates, but we don't need much complexity beyond year-month-day. /// The album's release date. #[derive(Clone, Debug, Default, PartialEq, Eq, PartialOrd, Ord)] @@ -305,7 +311,7 @@ impl AlbumId { } pub fn clear_db_id(&mut self) { - self.db_id = AlbumDbId::None; + self.db_id.take(); } pub fn compatible(&self, other: &AlbumId) -> bool { diff --git a/src/core/collection/artist.rs b/src/core/collection/artist.rs index 8851470..6b43a0c 100644 --- a/src/core/collection/artist.rs +++ b/src/core/collection/artist.rs @@ -5,7 +5,7 @@ use std::{ use crate::core::collection::{ album::{Album, AlbumLibId}, - merge::{Merge, MergeId}, + merge::{Merge, MergeCollections, MergeName}, musicbrainz::{MbArtistRef, MbRefOption}, }; @@ -25,17 +25,14 @@ pub struct ArtistMeta { } /// Artist non-identifier metadata. -#[derive(Clone, Debug, PartialEq, Eq)] +#[derive(Clone, Debug, Default, PartialEq, Eq)] pub struct ArtistInfo { - pub musicbrainz: MbRefOption, pub properties: HashMap>, } -impl MergeId for Artist { - type Id = ArtistId; - - fn id(&self) -> &Self::Id { - &self.meta.id +impl MergeName for Artist { + fn name(&self) -> &str { + &self.meta.id.name } } @@ -43,8 +40,12 @@ impl MergeId for Artist { #[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)] pub struct ArtistId { pub name: String, + pub db_id: ArtistDbId, } +/// Unique database identifier. Use MBID for this purpose. +pub type ArtistDbId = MbRefOption; + impl Artist { /// Create new [`Artist`] with the given [`ArtistId`]. pub fn new>(id: Id) -> Self { @@ -86,23 +87,6 @@ impl Artist { } secondary_without_id } - - fn merge_albums_by_title( - primary_albums: &mut Vec, - secondary_without_id: HashMap>, - ) { - for (title, mut secondary_albums) in secondary_without_id.into_iter() { - let mut iter = primary_albums.iter_mut(); - match iter.find(|album| album.meta.id.title == title) { - Some(ref mut primary_album) => { - // We do not support merging multiple DB albums with same title yet. - assert_eq!(secondary_albums.len(), 1); - primary_album.merge_in_place(secondary_albums.pop().unwrap()) - } - None => primary_albums.append(&mut secondary_albums), - } - } - } } impl PartialOrd for Artist { @@ -122,7 +106,7 @@ impl Merge for Artist { self.meta.merge_in_place(other.meta); let other_without_id = Artist::merge_albums_by_lib_id(&mut self.albums, other.albums); - Artist::merge_albums_by_title(&mut self.albums, other_without_id); + MergeCollections::merge_by_name(&mut self.albums, other_without_id); self.albums.sort_unstable(); } } @@ -136,6 +120,14 @@ impl ArtistMeta { } } + pub fn set_db_id(&mut self, db_id: ArtistDbId) { + self.id.set_db_id(db_id); + } + + pub fn clear_db_id(&mut self) { + self.id.clear_db_id(); + } + pub fn get_sort_key(&self) -> (&str,) { (self.sort.as_ref().unwrap_or(&self.id.name),) } @@ -149,28 +141,7 @@ impl ArtistMeta { } } -impl Default for ArtistInfo { - fn default() -> Self { - Self::new(MbRefOption::None) - } -} - impl ArtistInfo { - pub fn new(musicbrainz: MbRefOption) -> Self { - ArtistInfo { - musicbrainz, - properties: HashMap::new(), - } - } - - pub fn set_musicbrainz_ref(&mut self, mbref: MbRefOption) { - self.musicbrainz = mbref - } - - pub fn clear_musicbrainz_ref(&mut self) { - self.musicbrainz.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 @@ -230,8 +201,8 @@ impl Ord for ArtistMeta { impl Merge for ArtistMeta { fn merge_in_place(&mut self, other: Self) { - assert_eq!(self.id, other.id); - + assert!(self.id.compatible(&other.id)); + self.id.db_id = self.id.db_id.take().or(other.id.db_id); self.sort = self.sort.take().or(other.sort); self.info.merge_in_place(other.info); } @@ -239,7 +210,6 @@ impl Merge for ArtistMeta { impl Merge for ArtistInfo { fn merge_in_place(&mut self, other: Self) { - self.musicbrainz = self.musicbrainz.take().or(other.musicbrainz); self.properties.merge_in_place(other.properties); } } @@ -258,7 +228,30 @@ impl AsRef for ArtistId { impl ArtistId { pub fn new>(name: S) -> ArtistId { - ArtistId { name: name.into() } + ArtistId { + name: name.into(), + db_id: ArtistDbId::None, + } + } + + pub fn with_db_id(mut self, db_id: ArtistDbId) -> Self { + self.db_id = db_id; + self + } + + pub fn set_db_id(&mut self, db_id: ArtistDbId) { + self.db_id = db_id; + } + + pub fn clear_db_id(&mut self) { + self.db_id.take(); + } + + pub fn compatible(&self, other: &ArtistId) -> bool { + let names_compatible = self.name == other.name; + let db_id_compatible = + self.db_id.is_none() || other.db_id.is_none() || (self.db_id == other.db_id); + names_compatible && db_id_compatible } } @@ -333,30 +326,30 @@ mod tests { let mut artist = Artist::new(ArtistId::new("an artist")); let mut expected: MbRefOption = MbRefOption::None; - assert_eq!(artist.meta.info.musicbrainz, expected); + assert_eq!(artist.meta.id.db_id, expected); // Setting a URL on an artist. - artist.meta.info.set_musicbrainz_ref(MbRefOption::Some( + artist.meta.id.set_db_id(MbRefOption::Some( MbArtistRef::from_url_str(MUSICBRAINZ).unwrap(), )); expected.replace(MbArtistRef::from_url_str(MUSICBRAINZ).unwrap()); - assert_eq!(artist.meta.info.musicbrainz, expected); + assert_eq!(artist.meta.id.db_id, expected); - artist.meta.info.set_musicbrainz_ref(MbRefOption::Some( + artist.meta.id.set_db_id(MbRefOption::Some( MbArtistRef::from_url_str(MUSICBRAINZ).unwrap(), )); - assert_eq!(artist.meta.info.musicbrainz, expected); + assert_eq!(artist.meta.id.db_id, expected); - artist.meta.info.set_musicbrainz_ref(MbRefOption::Some( + artist.meta.id.set_db_id(MbRefOption::Some( MbArtistRef::from_url_str(MUSICBRAINZ_2).unwrap(), )); expected.replace(MbArtistRef::from_url_str(MUSICBRAINZ_2).unwrap()); - assert_eq!(artist.meta.info.musicbrainz, expected); + assert_eq!(artist.meta.id.db_id, expected); // Clearing URLs. - artist.meta.info.clear_musicbrainz_ref(); + artist.meta.id.clear_db_id(); expected.take(); - assert_eq!(artist.meta.info.musicbrainz, expected); + assert_eq!(artist.meta.id.db_id, expected); } #[test] @@ -472,7 +465,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.info.musicbrainz = MbRefOption::None; + right.meta.id.db_id = MbRefOption::None; right.meta.info.properties = HashMap::new(); let mut expected = left.clone(); diff --git a/src/core/collection/merge.rs b/src/core/collection/merge.rs index 42d833a..51b5b79 100644 --- a/src/core/collection/merge.rs +++ b/src/core/collection/merge.rs @@ -80,36 +80,30 @@ where } } -pub trait MergeId { - type Id; - - fn id(&self) -> &Self::Id; +pub trait MergeName { + fn name(&self) -> &str; } -pub struct MergeCollections { - _id: PhantomData, +pub struct MergeCollections { _t: PhantomData, _it: PhantomData, } -impl MergeCollections +impl MergeCollections where - ID: Eq + Hash + Clone, - T: MergeId + Merge + Ord, - IT: IntoIterator, + T: MergeName + Merge + Ord, + IT: IntoIterator)>, { - pub fn merge(mut primary: HashMap, secondary: IT) -> Vec { - for secondary_item in secondary { - if let Some(ref mut primary_item) = primary.get_mut(secondary_item.id()) { - primary_item.merge_in_place(secondary_item); - } else { - primary.insert(secondary_item.id().clone(), secondary_item); + pub fn merge_by_name(primary_items: &mut Vec, secondary: IT) { + for (name, mut secondary_items) in secondary.into_iter() { + match primary_items.iter_mut().find(|item| item.name() == name) { + Some(ref mut primary_item) => { + // We do not support merging multiple DB items with same name yet. + assert_eq!(secondary_items.len(), 1); + primary_item.merge_in_place(secondary_items.pop().unwrap()); + } + None => primary_items.append(&mut secondary_items), } } - - let mut collection: Vec = primary.into_values().collect(); - collection.sort_unstable(); - - collection } } diff --git a/src/core/musichoard/base.rs b/src/core/musichoard/base.rs index 71496e9..32a429c 100644 --- a/src/core/musichoard/base.rs +++ b/src/core/musichoard/base.rs @@ -1,3 +1,5 @@ +use std::collections::HashMap; + use crate::core::{ collection::{ album::{Album, AlbumId}, @@ -57,7 +59,20 @@ impl IMusicHoardBasePrivate for MusicHoard } fn merge_collections(&self) -> Collection { - MergeCollections::merge(self.library_cache.clone(), self.database_cache.clone()) + let mut primary = self.library_cache.clone(); + let mut secondary = HashMap::>::new(); + + for artist in self.database_cache.iter().cloned() { + secondary + .entry(artist.meta.id.name.clone()) + .or_default() + .push(artist); + } + + MergeCollections::merge_by_name(&mut primary, secondary); + primary.sort_unstable(); + + primary } fn get_artist<'a>(collection: &'a Collection, artist_id: &ArtistId) -> Option<&'a Artist> { @@ -118,11 +133,7 @@ mod tests { expected.sort_unstable(); let mut mh = MusicHoard { - library_cache: left - .clone() - .into_iter() - .map(|a| (a.meta.id.clone(), a)) - .collect(), + library_cache: left.clone(), database_cache: right.clone(), ..Default::default() }; @@ -132,11 +143,7 @@ mod tests { // The merge is completely non-overlapping so it should be commutative. let mut mh = MusicHoard { - library_cache: right - .clone() - .into_iter() - .map(|a| (a.meta.id.clone(), a)) - .collect(), + library_cache: right.clone(), database_cache: left.clone(), ..Default::default() }; @@ -156,11 +163,7 @@ mod tests { expected.sort_unstable(); let mut mh = MusicHoard { - library_cache: left - .clone() - .into_iter() - .map(|a| (a.meta.id.clone(), a)) - .collect(), + library_cache: left.clone(), database_cache: right.clone(), ..Default::default() }; @@ -170,11 +173,7 @@ mod tests { // The merge does not overwrite any data so it should be commutative. let mut mh = MusicHoard { - library_cache: right - .clone() - .into_iter() - .map(|a| (a.meta.id.clone(), a)) - .collect(), + library_cache: right.clone(), database_cache: left.clone(), ..Default::default() }; @@ -207,11 +206,7 @@ mod tests { expected.rotate_right(1); let mut mh = MusicHoard { - library_cache: left - .clone() - .into_iter() - .map(|a| (a.meta.id.clone(), a)) - .collect(), + library_cache: left.clone(), database_cache: right.clone(), ..Default::default() }; @@ -221,11 +216,7 @@ mod tests { // The merge overwrites the sort data, but no data is erased so it should be commutative. let mut mh = MusicHoard { - library_cache: right - .clone() - .into_iter() - .map(|a| (a.meta.id.clone(), a)) - .collect(), + library_cache: right.clone(), database_cache: left.clone(), ..Default::default() }; diff --git a/src/core/musichoard/builder.rs b/src/core/musichoard/builder.rs index 41eba10..5ea72ce 100644 --- a/src/core/musichoard/builder.rs +++ b/src/core/musichoard/builder.rs @@ -1,5 +1,3 @@ -use std::collections::HashMap; - use crate::core::{ interface::{database::IDatabase, library::ILibrary}, musichoard::{database::IMusicHoardDatabase, Error, MusicHoard, NoDatabase, NoLibrary}, @@ -69,7 +67,7 @@ impl MusicHoard { database: NoDatabase, database_cache: vec![], library: NoLibrary, - library_cache: HashMap::new(), + library_cache: vec![], } } } @@ -90,7 +88,7 @@ impl MusicHoard { database: NoDatabase, database_cache: vec![], library, - library_cache: HashMap::new(), + library_cache: vec![], } } } @@ -111,7 +109,7 @@ impl MusicHoard { database, database_cache: vec![], library: NoLibrary, - library_cache: HashMap::new(), + library_cache: vec![], }; mh.reload_database()?; Ok(mh) @@ -134,7 +132,7 @@ impl MusicHoard { database, database_cache: vec![], library, - library_cache: HashMap::new(), + library_cache: vec![], }; mh.reload_database()?; Ok(mh) diff --git a/src/core/musichoard/database.rs b/src/core/musichoard/database.rs index 6f7c563..598606a 100644 --- a/src/core/musichoard/database.rs +++ b/src/core/musichoard/database.rs @@ -3,7 +3,7 @@ use std::mem; use crate::{ collection::{ album::{AlbumDbId, AlbumInfo, AlbumMeta}, - artist::ArtistInfo, + artist::{ArtistDbId, ArtistInfo}, merge::Merge, }, core::{ @@ -23,6 +23,13 @@ pub trait IMusicHoardDatabase { fn add_artist>(&mut self, artist_id: IntoId) -> Result<(), Error>; fn remove_artist>(&mut self, artist_id: Id) -> Result<(), Error>; + fn set_artist_db_id>( + &mut self, + artist_id: Id, + db_id: ArtistDbId, + ) -> Result<(), Error>; + fn clear_artist_db_id>(&mut self, artist_id: Id) -> Result<(), Error>; + fn set_artist_sort, S: Into>( &mut self, artist_id: Id, @@ -140,6 +147,26 @@ impl IMusicHoardDatabase for MusicHoard>( + &mut self, + artist_id: Id, + db_id: ArtistDbId, + ) -> Result<(), Error> { + self.update_artist_and( + artist_id.as_ref(), + |artist| artist.meta.set_db_id(db_id), + |collection| Self::sort_artists(collection), + ) + } + + fn clear_artist_db_id>(&mut self, artist_id: Id) -> Result<(), Error> { + self.update_artist_and( + artist_id.as_ref(), + |artist| artist.meta.clear_db_id(), + |collection| Self::sort_artists(collection), + ) + } + fn set_artist_sort, S: Into>( &mut self, artist_id: Id, @@ -427,7 +454,7 @@ mod tests { use crate::{ collection::{ album::{AlbumPrimaryType, AlbumSecondaryType}, - musicbrainz::{MbArtistRef, MbRefOption}, + musicbrainz::MbArtistRef, }, core::{ collection::artist::ArtistId, @@ -557,6 +584,52 @@ mod tests { assert_eq!(actual_err.to_string(), expected_err.to_string()); } + #[test] + fn set_clear_artist_db_id() { + let mut database = MockIDatabase::new(); + database.expect_load().times(1).returning(|| Ok(vec![])); + database.expect_save().times(3).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).unwrap(); + + assert!(music_hoard.add_artist(artist_id.clone()).is_ok()); + + let mut expected = ArtistDbId::None; + assert_eq!(music_hoard.collection[0].meta.id.db_id, expected); + + let db_id = ArtistDbId::Some(MbArtistRef::from_uuid_str(MBID).unwrap()); + + // Setting a db_id on an artist not in the collection is an error. + assert!(music_hoard + .set_artist_db_id(&artist_id_2, db_id.clone()) + .is_err()); + assert_eq!(music_hoard.collection[0].meta.id.db_id, expected); + + // Setting a db_id on an artist. + assert!(music_hoard + .set_artist_db_id(&artist_id, db_id.clone()) + .is_ok()); + expected.replace(MbArtistRef::from_uuid_str(MBID).unwrap()); + assert_eq!(music_hoard.collection[0].meta.id.db_id, expected); + + // Clearing db_id on an artist that does not exist is an error. + assert!(music_hoard.clear_artist_db_id(&artist_id_2).is_err()); + assert_eq!(music_hoard.collection[0].meta.id.db_id, expected); + + // Clearing db_id from an artist without the db_id set is an error. Effectively the album + // does not exist. + assert!(music_hoard.clear_artist_db_id(&artist_id).is_err()); + assert_eq!(music_hoard.collection[0].meta.id.db_id, expected); + + // Clearing db_id. + artist_id.set_db_id(db_id); + assert!(music_hoard.clear_artist_db_id(&artist_id).is_ok()); + expected.take(); + assert_eq!(music_hoard.collection[0].meta.id.db_id, expected); + } + #[test] fn set_clear_artist_info() { let mut database = MockIDatabase::new(); @@ -569,32 +642,36 @@ mod tests { assert!(music_hoard.add_artist(artist_id.clone()).is_ok()); - let mut expected: MbRefOption = MbRefOption::None; - assert_eq!(music_hoard.collection[0].meta.info.musicbrainz, expected); + let mut expected = ArtistInfo::default(); + assert_eq!(music_hoard.collection[0].meta.info, expected); - let info = ArtistInfo::new(MbRefOption::Some(MbArtistRef::from_uuid_str(MBID).unwrap())); + let mut info = ArtistInfo::default(); + info.add_to_property("property", vec!["value-1", "value-2"]); - // Setting a URL on an artist not in the collection is an error. + // Setting info on an artist not in the collection is an error. assert!(music_hoard .merge_artist_info(&artist_id_2, info.clone()) .is_err()); - assert_eq!(music_hoard.collection[0].meta.info.musicbrainz, expected); + assert_eq!(music_hoard.collection[0].meta.info, expected); - // Setting a URL on an artist. + // Setting info on an artist. assert!(music_hoard .merge_artist_info(&artist_id, info.clone()) .is_ok()); - expected.replace(MbArtistRef::from_uuid_str(MBID).unwrap()); - assert_eq!(music_hoard.collection[0].meta.info.musicbrainz, expected); + expected.properties.insert( + String::from("property"), + vec![String::from("value-1"), String::from("value-2")], + ); + assert_eq!(music_hoard.collection[0].meta.info, expected); - // Clearing URLs on an artist that does not exist is an error. + // Clearing info on an artist that does not exist is an error. assert!(music_hoard.clear_artist_info(&artist_id_2).is_err()); - assert_eq!(music_hoard.collection[0].meta.info.musicbrainz, expected); + assert_eq!(music_hoard.collection[0].meta.info, expected); - // Clearing URLs. + // Clearing info. assert!(music_hoard.clear_artist_info(&artist_id).is_ok()); - expected.take(); - assert_eq!(music_hoard.collection[0].meta.info.musicbrainz, expected); + expected.properties.clear(); + assert_eq!(music_hoard.collection[0].meta.info, expected); } #[test] @@ -786,8 +863,8 @@ mod tests { .clear_album_db_id(&artist_id, &album_id_2) .is_err()); - // Clearing db_id from album without the db_id set is an error. Effectively the album does - // not exist. + // Clearing db_id from an album without the db_id set is an error. Effectively the album + // does not exist. assert!(music_hoard .clear_album_db_id(&artist_id, &album_id) .is_err()); diff --git a/src/core/musichoard/library.rs b/src/core/musichoard/library.rs index fb7eed7..d439272 100644 --- a/src/core/musichoard/library.rs +++ b/src/core/musichoard/library.rs @@ -1,7 +1,7 @@ use std::collections::HashMap; use crate::{ - collection::album::AlbumDbId, + collection::{album::AlbumDbId, artist::ArtistDbId}, core::{ collection::{ album::{Album, AlbumDate, AlbumId}, @@ -42,17 +42,18 @@ impl MusicHoard { fn rescan_library_inner(&mut self) -> Result { 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()); + Self::sort_albums_and_tracks(self.library_cache.iter_mut()); Ok(self.merge_collections()) } - fn items_to_artists(items: Vec) -> Result, Error> { + fn items_to_artists(items: Vec) -> Result { let mut collection = HashMap::::new(); for item in items.into_iter() { let artist_id = ArtistId { name: item.album_artist, + db_id: ArtistDbId::None, }; let artist_sort = item.album_artist_sort; @@ -121,7 +122,7 @@ impl MusicHoard { } } - Ok(collection) + Ok(collection.into_values().collect()) } } diff --git a/src/core/musichoard/mod.rs b/src/core/musichoard/mod.rs index 435ec7d..f6ba502 100644 --- a/src/core/musichoard/mod.rs +++ b/src/core/musichoard/mod.rs @@ -10,15 +10,9 @@ pub use base::IMusicHoardBase; pub use database::IMusicHoardDatabase; pub use library::IMusicHoardLibrary; -use std::{ - collections::HashMap, - fmt::{self, Display}, -}; +use std::fmt::{self, Display}; -use crate::core::collection::{ - artist::{Artist, ArtistId}, - Collection, -}; +use crate::core::collection::Collection; use crate::core::interface::{ database::{LoadError as DatabaseLoadError, SaveError as DatabaseSaveError}, @@ -35,7 +29,7 @@ pub struct MusicHoard { database: Database, database_cache: Collection, library: Library, - library_cache: HashMap, + library_cache: Collection, } /// Phantom type for when a library implementation is not needed. diff --git a/src/core/testmod.rs b/src/core/testmod.rs index c44c7c1..38162f0 100644 --- a/src/core/testmod.rs +++ b/src/core/testmod.rs @@ -5,8 +5,8 @@ use crate::core::collection::{ album::{ Album, AlbumDbId, AlbumId, AlbumInfo, AlbumLibId, AlbumMeta, AlbumPrimaryType, AlbumSeq, }, - artist::{Artist, ArtistId, ArtistInfo, ArtistMeta}, - musicbrainz::{MbAlbumRef, MbArtistRef, MbRefOption}, + artist::{Artist, ArtistDbId, ArtistId, ArtistInfo, ArtistMeta}, + musicbrainz::{MbAlbumRef, MbArtistRef}, track::{Track, TrackFormat, TrackId, TrackNum, TrackQuality}, }; use crate::testmod::*; diff --git a/src/external/database/serde/deserialize.rs b/src/external/database/serde/deserialize.rs index a57970b..0528c3d 100644 --- a/src/external/database/serde/deserialize.rs +++ b/src/external/database/serde/deserialize.rs @@ -123,10 +123,12 @@ impl From for Artist { fn from(artist: DeserializeArtist) -> Self { Artist { meta: ArtistMeta { - id: ArtistId::new(artist.name), + id: ArtistId { + name: artist.name, + db_id: artist.musicbrainz.into(), + }, sort: artist.sort, info: ArtistInfo { - musicbrainz: artist.musicbrainz.into(), properties: artist.properties, }, }, diff --git a/src/external/database/serde/serialize.rs b/src/external/database/serde/serialize.rs index 59275d2..a6a7a62 100644 --- a/src/external/database/serde/serialize.rs +++ b/src/external/database/serde/serialize.rs @@ -86,7 +86,7 @@ impl<'a> From<&'a Artist> for SerializeArtist<'a> { SerializeArtist { name: &artist.meta.id.name, sort: artist.meta.sort.as_deref(), - musicbrainz: (&artist.meta.info.musicbrainz).into(), + musicbrainz: (&artist.meta.id.db_id).into(), properties: artist .meta .info diff --git a/src/testmod/full.rs b/src/testmod/full.rs index cbdbec3..f823be1 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(), + db_id: ArtistDbId::Some(MbArtistRef::from_url_str( + "https://musicbrainz.org/artist/00000000-0000-0000-0000-000000000000" + ).unwrap()), }, sort: None, info: ArtistInfo { - musicbrainz: MbRefOption::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"), @@ -135,12 +135,12 @@ macro_rules! full_collection { meta: ArtistMeta { id: ArtistId { name: "Album_Artist ‘B’".to_string(), + db_id: ArtistDbId::Some(MbArtistRef::from_url_str( + "https://musicbrainz.org/artist/11111111-1111-1111-1111-111111111111" + ).unwrap()), }, sort: None, info: ArtistInfo { - musicbrainz: MbRefOption::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"), @@ -336,10 +336,10 @@ macro_rules! full_collection { meta: ArtistMeta { id: ArtistId { name: "The Album_Artist ‘C’".to_string(), + db_id: ArtistDbId::CannotHaveMbid, }, sort: Some("Album_Artist ‘C’, The".to_string()), info: ArtistInfo { - musicbrainz: MbRefOption::CannotHaveMbid, properties: HashMap::new(), }, }, @@ -434,10 +434,10 @@ macro_rules! full_collection { meta: ArtistMeta { id: ArtistId { name: "Album_Artist ‘D’".to_string(), + db_id: ArtistDbId::None, }, sort: None, info: ArtistInfo { - musicbrainz: MbRefOption::None, properties: HashMap::new(), }, }, diff --git a/src/testmod/library.rs b/src/testmod/library.rs index cc326ce..9a9e4e7 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(), + db_id: ArtistDbId::None, }, sort: None, info: ArtistInfo { - musicbrainz: MbRefOption::None, properties: HashMap::new(), }, }, @@ -117,10 +117,10 @@ macro_rules! library_collection { meta: ArtistMeta { id: ArtistId { name: "Album_Artist ‘B’".to_string(), + db_id: ArtistDbId::None, }, sort: None, info: ArtistInfo { - musicbrainz: MbRefOption::None, properties: HashMap::new(), }, }, @@ -287,10 +287,10 @@ macro_rules! library_collection { meta: ArtistMeta { id: ArtistId { name: "The Album_Artist ‘C’".to_string(), + db_id: ArtistDbId::None, }, sort: Some("Album_Artist ‘C’, The".to_string()), info: ArtistInfo { - musicbrainz: MbRefOption::None, properties: HashMap::new(), }, }, @@ -379,10 +379,10 @@ macro_rules! library_collection { meta: ArtistMeta { id: ArtistId { name: "Album_Artist ‘D’".to_string(), + db_id: ArtistDbId::None, }, sort: None, info: ArtistInfo { - musicbrainz: MbRefOption::None, properties: HashMap::new(), }, }, diff --git a/src/tui/app/machine/fetch_state.rs b/src/tui/app/machine/fetch_state.rs index 7443511..39b4120 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.info.musicbrainz); + requests = Self::browse_release_group_job(&artist.meta.id.db_id); } else { fetch = FetchState::search(rx); } SubmitJob { fetch, requests } } _ => { - let arid = match artist.meta.info.musicbrainz { + let arid = match artist.meta.id.db_id { MbRefOption::Some(ref mbref) => mbref, _ => return Err("cannot fetch album: artist has no MBID"), }; @@ -258,7 +258,7 @@ impl AppMachine { } fn search_artist_job(artist: &Artist) -> VecDeque { - match artist.meta.info.musicbrainz { + match artist.meta.id.db_id { MbRefOption::Some(ref arid) => { Self::search_albums_requests(&artist.meta.id, arid, &artist.albums) } @@ -784,7 +784,7 @@ mod tests { } fn browse_release_group_expectation(artist: &Artist) -> MockIMbJobSender { - let requests = AppMachine::browse_release_group_job(&artist.meta.info.musicbrainz); + let requests = AppMachine::browse_release_group_job(&artist.meta.id.db_id); 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 efb51bd..f0a5298 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::{AlbumDbId, AlbumInfo, AlbumMeta}, - artist::{ArtistInfo, ArtistMeta}, + artist::{ArtistDbId, ArtistInfo, ArtistMeta}, musicbrainz::{MbRefOption, Mbid}, }; @@ -12,6 +12,11 @@ use crate::tui::app::{ MatchOption, MatchStatePublic, WidgetState, }; +struct ArtistInfoTuple { + db_id: ArtistDbId, + info: ArtistInfo, +} + struct AlbumInfoTuple { db_id: AlbumDbId, info: AlbumInfo, @@ -21,7 +26,7 @@ trait GetInfoMeta { type InfoType; } impl GetInfoMeta for ArtistMeta { - type InfoType = ArtistInfo; + type InfoType = ArtistInfoTuple; } impl GetInfoMeta for AlbumMeta { type InfoType = AlbumInfoTuple; @@ -38,16 +43,20 @@ enum InfoOption { } impl GetInfo for MatchOption { - type InfoType = ArtistInfo; + type InfoType = ArtistInfoTuple; fn get_info(&self) -> InfoOption { + let db_id; let mut info = ArtistInfo::default(); match self { - MatchOption::Some(option) => info.musicbrainz = option.entity.info.musicbrainz.clone(), - MatchOption::CannotHaveMbid => info.musicbrainz = MbRefOption::CannotHaveMbid, + MatchOption::Some(option) => { + db_id = option.entity.id.db_id.clone(); + info = option.entity.info.clone(); + } + MatchOption::CannotHaveMbid => db_id = MbRefOption::CannotHaveMbid, MatchOption::ManualInputMbid => return InfoOption::NeedInput, } - InfoOption::Info(info) + InfoOption::Info(ArtistInfoTuple { db_id, info }) } } @@ -237,7 +246,9 @@ impl IAppInteractMatch for AppMachine { let mh = &mut self.inner.music_hoard; let result = match self.state.current { EntityMatches::Artist(ref mut matches) => match matches.list.extract_info(index) { - InfoOption::Info(info) => mh.merge_artist_info(&matches.matching.id, info), + InfoOption::Info(tuple) => mh + .merge_artist_info(&matches.matching.id, tuple.info) + .and_then(|()| mh.set_artist_db_id(&matches.matching.id, tuple.db_id)), InfoOption::NeedInput => return self.get_input(), }, EntityMatches::Album(ref mut matches) => match matches.list.extract_info(index) { @@ -303,13 +314,11 @@ mod tests { } fn artist_meta() -> ArtistMeta { - let mut meta = ArtistMeta::new(ArtistId::new("Artist")); - meta.info.musicbrainz = MbRefOption::Some(mbid().into()); - meta + ArtistMeta::new(ArtistId::new("Artist").with_db_id(ArtistDbId::Some(mbid().into()))) } fn artist_match() -> EntityMatches { - let artist = artist_meta(); + let mut artist = artist_meta(); let artist_1 = artist.clone(); let artist_match_1 = Entity::with_score(artist_1, 100); @@ -318,12 +327,14 @@ mod tests { let mut artist_match_2 = Entity::with_score(artist_2, 100); artist_match_2.disambiguation = Some(String::from("some disambiguation")); + artist.clear_db_id(); let list = vec![artist_match_1.clone(), artist_match_2.clone()]; EntityMatches::artist_search(artist, list) } fn artist_lookup() -> EntityMatches { - let artist = artist_meta(); + let mut artist = artist_meta(); + artist.clear_db_id(); let lookup = Entity::new(artist.clone()); EntityMatches::artist_lookup(artist, lookup) } @@ -427,14 +438,21 @@ mod tests { .return_once(|_, _, _| Ok(())); } EntityMatches::Artist(_) => { - let info = ArtistInfo { - musicbrainz: MbRefOption::CannotHaveMbid, - ..Default::default() - }; + let db_id = MbRefOption::CannotHaveMbid; + let info = ArtistInfo::default(); + + 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_db_id() + .with(eq(artist_id.clone()), eq(db_id)) + .times(1) + .in_sequence(&mut seq) .return_once(|_, _| Ok(())); } } @@ -514,11 +532,22 @@ mod tests { match matches_info { EntityMatches::Album(_) => panic!(), EntityMatches::Artist(_) => { - let meta = artist_meta(); + let mut meta = artist_meta(); + let db_id = meta.id.db_id.clone(); + meta.clear_db_id(); + + let mut seq = Sequence::new(); music_hoard .expect_merge_artist_info() - .with(eq(meta.id), eq(meta.info)) + .with(eq(meta.id.clone()), eq(meta.info)) .times(1) + .in_sequence(&mut seq) + .return_once(|_, _| Ok(())); + music_hoard + .expect_set_artist_db_id() + .with(eq(meta.id.clone()), eq(db_id)) + .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 ada894d..d48e24c 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, AlbumDbId, AlbumId, AlbumInfo, AlbumLibId, AlbumMeta, AlbumSeq}, - artist::{ArtistInfo, ArtistMeta}, + artist::{ArtistId, ArtistInfo, ArtistMeta}, musicbrainz::{MbRefOption, Mbid}, }, external::musicbrainz::{ @@ -115,10 +115,12 @@ fn from_mb_artist_meta(meta: MbArtistMeta) -> (ArtistMeta, Option) { let sort = Some(meta.sort_name).filter(|s| s != &meta.name); ( ArtistMeta { - id: meta.name.into(), + id: ArtistId { + name: meta.name, + db_id: MbRefOption::Some(meta.id.into()), + }, sort, info: ArtistInfo { - musicbrainz: MbRefOption::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 744b75a..b7b7211 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.info.musicbrainz); + let mbref = mb_ref_opt_as_ref(&COLLECTION[1].meta.id.db_id); 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.info.musicbrainz); + let mbref = mb_ref_opt_as_ref(&COLLECTION[1].meta.id.db_id); 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.info.musicbrainz); + let mbref = mb_ref_opt_as_ref(&COLLECTION[1].meta.id.db_id); mb_ref_opt_unwrap(mbref).mbid().clone() } diff --git a/src/tui/lib/interface/musicbrainz/daemon/mod.rs b/src/tui/lib/interface/musicbrainz/daemon/mod.rs index eda65c9..42eabaa 100644 --- a/src/tui/lib/interface/musicbrainz/daemon/mod.rs +++ b/src/tui/lib/interface/musicbrainz/daemon/mod.rs @@ -29,6 +29,10 @@ impl fmt::Display for Error { pub type MbApiResult = Result; pub type ResultSender = mpsc::Sender; +// The largest variant is Match(EntityMatches::Album(AlbumMatches)) since that contains both the +// ArtistId and Albumid. However, that is also the most common MbReturn expected. Therefore, we +// allow this large enum variant. +#[allow(clippy::large_enum_variant)] #[derive(Clone, Debug, PartialEq, Eq)] pub enum MbReturn { Match(EntityMatches), diff --git a/src/tui/lib/mod.rs b/src/tui/lib/mod.rs index d8272b5..d99e01f 100644 --- a/src/tui/lib/mod.rs +++ b/src/tui/lib/mod.rs @@ -4,7 +4,7 @@ pub mod interface; use musichoard::{ collection::{ album::{AlbumDbId, AlbumId, AlbumInfo, AlbumMeta}, - artist::{ArtistId, ArtistInfo}, + artist::{ArtistDbId, ArtistId, ArtistInfo}, Collection, }, interface::{database::IDatabase, library::ILibrary}, @@ -26,6 +26,11 @@ pub trait IMusicHoard { album_meta: AlbumMeta, ) -> Result<(), musichoard::Error>; + fn set_artist_db_id( + &mut self, + artist_id: &ArtistId, + db_id: ArtistDbId, + ) -> Result<(), musichoard::Error>; fn merge_artist_info( &mut self, id: &ArtistId, @@ -67,6 +72,14 @@ impl IMusicHoard for MusicHoard::add_album(self, artist_id, album_meta) } + fn set_artist_db_id( + &mut self, + artist_id: &ArtistId, + db_id: ArtistDbId, + ) -> Result<(), musichoard::Error> { + ::set_artist_db_id(self, artist_id, db_id) + } + fn merge_artist_info( &mut self, id: &ArtistId, diff --git a/src/tui/testmod.rs b/src/tui/testmod.rs index fa7e664..52d78c4 100644 --- a/src/tui/testmod.rs +++ b/src/tui/testmod.rs @@ -4,8 +4,8 @@ use musichoard::collection::{ album::{ Album, AlbumDbId, AlbumId, AlbumInfo, AlbumLibId, AlbumMeta, AlbumPrimaryType, AlbumSeq, }, - artist::{Artist, ArtistId, ArtistInfo, ArtistMeta}, - musicbrainz::{MbAlbumRef, MbArtistRef, MbRefOption}, + artist::{Artist, ArtistDbId, ArtistId, ArtistInfo, ArtistMeta}, + musicbrainz::{MbAlbumRef, MbArtistRef}, track::{Track, TrackFormat, TrackId, TrackNum, TrackQuality}, }; use once_cell::sync::Lazy; diff --git a/src/tui/ui/info_state.rs b/src/tui/ui/info_state.rs index be11b12..6238689 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.info.musicbrainz)) + .map(|a| UiDisplay::display_mb_ref_option_as_url(&a.meta.id.db_id)) .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 9bdab89..27457fa 100644 --- a/tests/testlib.rs +++ b/tests/testlib.rs @@ -6,8 +6,8 @@ use musichoard::collection::{ Album, AlbumDbId, AlbumId, AlbumInfo, AlbumLibId, AlbumMeta, AlbumPrimaryType, AlbumSecondaryType, AlbumSeq, }, - artist::{Artist, ArtistId, ArtistInfo, ArtistMeta}, - musicbrainz::{MbArtistRef, MbRefOption}, + artist::{Artist, ArtistDbId, ArtistId, ArtistInfo, ArtistMeta}, + musicbrainz::MbArtistRef, track::{Track, TrackFormat, TrackId, TrackNum, TrackQuality}, Collection, }; @@ -18,12 +18,12 @@ pub static COLLECTION: Lazy> = Lazy::new(|| -> Collection { meta: ArtistMeta { id: ArtistId { name: String::from("Аркона"), + db_id: ArtistDbId::Some(MbArtistRef::from_url_str( + "https://musicbrainz.org/artist/baad262d-55ef-427a-83c7-f7530964f212" + ).unwrap()), }, sort: Some(String::from("Arkona")), info: ArtistInfo { - musicbrainz: MbRefOption::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"), @@ -213,12 +213,12 @@ pub static COLLECTION: Lazy> = Lazy::new(|| -> Collection { meta: ArtistMeta { id: ArtistId { name: String::from("Eluveitie"), + db_id: ArtistDbId::Some(MbArtistRef::from_url_str( + "https://musicbrainz.org/artist/8000598a-5edb-401c-8e6d-36b167feaf38" + ).unwrap()), }, sort: None, info: ArtistInfo { - musicbrainz: MbRefOption::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"), @@ -468,12 +468,12 @@ pub static COLLECTION: Lazy> = Lazy::new(|| -> Collection { meta: ArtistMeta { id: ArtistId { name: String::from("Frontside"), + db_id: ArtistDbId::Some(MbArtistRef::from_url_str( + "https://musicbrainz.org/artist/3a901353-fccd-4afd-ad01-9c03f451b490" + ).unwrap()), }, sort: None, info: ArtistInfo { - musicbrainz: MbRefOption::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"), @@ -627,12 +627,12 @@ pub static COLLECTION: Lazy> = Lazy::new(|| -> Collection { meta: ArtistMeta { id: ArtistId { name: String::from("Heaven’s Basement"), + db_id: ArtistDbId::Some(MbArtistRef::from_url_str( + "https://musicbrainz.org/artist/c2c4d56a-d599-4a18-bd2f-ae644e2198cc" + ).unwrap()), }, sort: Some(String::from("Heaven’s Basement")), info: ArtistInfo { - musicbrainz: MbRefOption::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"), @@ -766,12 +766,12 @@ pub static COLLECTION: Lazy> = Lazy::new(|| -> Collection { meta: ArtistMeta { id: ArtistId { name: String::from("Metallica"), + db_id: ArtistDbId::Some(MbArtistRef::from_url_str( + "https://musicbrainz.org/artist/65f4f0c5-ef9e-490c-aee3-909e7ae6b2ab" + ).unwrap()), }, sort: None, info: ArtistInfo { - musicbrainz: MbRefOption::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"),