From 4d2ea77da93b93b3f19c338630849a9656cf119f Mon Sep 17 00:00:00 2001 From: Wojciech Kozlowski Date: Fri, 1 Mar 2024 09:00:52 +0100 Subject: [PATCH] Ensure consistency between in-memory and database state (#146) Closes #120 Reviewed-on: https://git.thenineworlds.net/wojtek/musichoard/pulls/146 --- src/bin/musichoard-edit.rs | 37 +- src/core/collection/artist.rs | 88 +-- src/core/musichoard/musichoard.rs | 638 ++++++++++++---------- src/core/musichoard/musichoard_builder.rs | 50 +- src/main.rs | 2 +- src/tui/app/machine/browse.rs | 38 +- src/tui/app/machine/mod.rs | 9 +- src/tui/app/machine/reload.rs | 6 +- src/tui/app/mod.rs | 2 +- src/tui/handler.rs | 2 +- src/tui/lib.rs | 11 +- src/tui/mod.rs | 2 +- tests/lib.rs | 8 +- 13 files changed, 442 insertions(+), 451 deletions(-) diff --git a/src/bin/musichoard-edit.rs b/src/bin/musichoard-edit.rs index 6b86875..00de904 100644 --- a/src/bin/musichoard-edit.rs +++ b/src/bin/musichoard-edit.rs @@ -77,10 +77,6 @@ struct ArtistSortValue { #[derive(StructOpt, Debug)] enum MusicBrainzCommand { - #[structopt(about = "Add a MusicBrainz URL without overwriting the existing value")] - Add(MusicBrainzValue), - #[structopt(about = "Remove the MusicBrainz URL")] - Remove(MusicBrainzValue), #[structopt(about = "Set the MusicBrainz URL overwriting any existing value")] Set(MusicBrainzValue), #[structopt(about = "Clear the MusicBrainz URL)")] @@ -129,10 +125,14 @@ impl ArtistCommand { fn handle(self, music_hoard: &mut MH) { match self { ArtistCommand::Add(artist_value) => { - music_hoard.add_artist(ArtistId::new(artist_value.artist)); + music_hoard + .add_artist(ArtistId::new(artist_value.artist)) + .expect("failed to add artist"); } ArtistCommand::Remove(artist_value) => { - music_hoard.remove_artist(ArtistId::new(artist_value.artist)); + music_hoard + .remove_artist(ArtistId::new(artist_value.artist)) + .expect("failed to remove artist"); } ArtistCommand::Sort(sort_command) => { sort_command.handle(music_hoard); @@ -166,18 +166,6 @@ impl SortCommand { impl MusicBrainzCommand { fn handle(self, music_hoard: &mut MH) { match self { - MusicBrainzCommand::Add(musicbrainz_value) => music_hoard - .add_musicbrainz_url( - ArtistId::new(musicbrainz_value.artist), - musicbrainz_value.url, - ) - .expect("failed to add MusicBrainz URL"), - MusicBrainzCommand::Remove(musicbrainz_value) => music_hoard - .remove_musicbrainz_url( - ArtistId::new(musicbrainz_value.artist), - musicbrainz_value.url, - ) - .expect("failed to remove MusicBrainz URL"), MusicBrainzCommand::Set(musicbrainz_value) => music_hoard .set_musicbrainz_url( ArtistId::new(musicbrainz_value.artist), @@ -227,14 +215,9 @@ fn main() { let db = JsonDatabase::new(JsonDatabaseFileBackend::new(&opt.database_file_path)); - let mut music_hoard = MusicHoardBuilder::default().set_database(db).build(); - music_hoard - .load_from_database() - .expect("failed to load database"); - + let mut music_hoard = MusicHoardBuilder::default() + .set_database(db) + .build() + .expect("failed to initialise MusicHoard"); opt.category.handle(&mut music_hoard); - - music_hoard - .save_to_database() - .expect("failed to save database"); } diff --git a/src/core/collection/artist.rs b/src/core/collection/artist.rs index 437ab71..c2a09fa 100644 --- a/src/core/collection/artist.rs +++ b/src/core/collection/artist.rs @@ -53,38 +53,8 @@ impl Artist { _ = self.sort.take(); } - pub fn add_musicbrainz_url>(&mut self, url: S) -> Result<(), Error> { - let url: MusicBrainz = url.as_ref().try_into()?; - - match &self.musicbrainz { - Some(current) => { - if current != &url { - return Err(Error::UrlError(format!( - "artist already has a different URL: {current}" - ))); - } - } - None => { - _ = self.musicbrainz.insert(url); - } - } - - Ok(()) - } - - pub fn remove_musicbrainz_url>(&mut self, url: S) -> Result<(), Error> { - let url = url.as_ref().try_into()?; - - if self.musicbrainz == Some(url) { - _ = self.musicbrainz.take(); - } - - Ok(()) - } - - pub fn set_musicbrainz_url>(&mut self, url: S) -> Result<(), Error> { - _ = self.musicbrainz.insert(url.as_ref().try_into()?); - Ok(()) + pub fn set_musicbrainz_url(&mut self, url: MusicBrainz) { + _ = self.musicbrainz.insert(url); } pub fn clear_musicbrainz_url(&mut self) { @@ -227,12 +197,6 @@ impl TryFrom<&str> for MusicBrainz { } } -impl Display for MusicBrainz { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - write!(f, "{}", self.0) - } -} - impl IMbid for MusicBrainz { fn mbid(&self) -> &str { // The URL is assumed to have been validated. @@ -326,40 +290,12 @@ mod tests { } #[test] - fn add_remove_musicbrainz_url() { - let mut artist = Artist::new(ArtistId::new("an artist")); + fn musicbrainz_url() { + let result: Result = MUSICBUTLER.try_into(); + assert!(result.is_err()); - let mut expected: Option = None; - assert_eq!(artist.musicbrainz, expected); - - // Adding incorect URL is an error. - assert!(artist.add_musicbrainz_url(MUSICBUTLER).is_err()); - assert_eq!(artist.musicbrainz, expected); - - // Adding URL to artist. - assert!(artist.add_musicbrainz_url(MUSICBRAINZ).is_ok()); - _ = expected.insert(MusicBrainz::new(MUSICBRAINZ).unwrap()); - assert_eq!(artist.musicbrainz, expected); - - // Adding the same URL again is ok, but does not do anything. - assert!(artist.add_musicbrainz_url(MUSICBRAINZ).is_ok()); - assert_eq!(artist.musicbrainz, expected); - - // Adding further URLs is an error. - assert!(artist.add_musicbrainz_url(MUSICBRAINZ_2).is_err()); - assert_eq!(artist.musicbrainz, expected); - - // Removing a URL not in the collection is okay, but does not do anything. - assert!(artist.remove_musicbrainz_url(MUSICBRAINZ_2).is_ok()); - assert_eq!(artist.musicbrainz, expected); - - // Removing a URL in the collection removes it. - assert!(artist.remove_musicbrainz_url(MUSICBRAINZ).is_ok()); - _ = expected.take(); - assert_eq!(artist.musicbrainz, expected); - - assert!(artist.remove_musicbrainz_url(MUSICBRAINZ).is_ok()); - assert_eq!(artist.musicbrainz, expected); + let result: Result = MUSICBRAINZ.try_into(); + assert!(result.is_ok()); } #[test] @@ -369,19 +305,15 @@ mod tests { let mut expected: Option = None; assert_eq!(artist.musicbrainz, expected); - // Setting an incorrect URL is an error. - assert!(artist.set_musicbrainz_url(MUSICBUTLER).is_err()); - assert_eq!(artist.musicbrainz, expected); - // Setting a URL on an artist. - assert!(artist.set_musicbrainz_url(MUSICBRAINZ).is_ok()); + artist.set_musicbrainz_url(MUSICBRAINZ.try_into().unwrap()); _ = expected.insert(MusicBrainz::new(MUSICBRAINZ).unwrap()); assert_eq!(artist.musicbrainz, expected); - assert!(artist.set_musicbrainz_url(MUSICBRAINZ).is_ok()); + artist.set_musicbrainz_url(MUSICBRAINZ.try_into().unwrap()); assert_eq!(artist.musicbrainz, expected); - assert!(artist.set_musicbrainz_url(MUSICBRAINZ_2).is_ok()); + artist.set_musicbrainz_url(MUSICBRAINZ_2.try_into().unwrap()); _ = expected.insert(MusicBrainz::new(MUSICBRAINZ_2).unwrap()); assert_eq!(artist.musicbrainz, expected); diff --git a/src/core/musichoard/musichoard.rs b/src/core/musichoard/musichoard.rs index 4c2515a..fcb5828 100644 --- a/src/core/musichoard/musichoard.rs +++ b/src/core/musichoard/musichoard.rs @@ -14,169 +14,51 @@ 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. +#[derive(Debug)] pub struct MusicHoard { collection: Collection, + pre_commit: Collection, library: LIB, database: DB, - // There is no database cache since the database contains the entirety of the `collection` - // itself. Therefore, [`collection`] also represents the last state of the database. library_cache: HashMap, + database_cache: Collection, } /// Phantom type for when a library implementation is not needed. +#[derive(Debug)] pub struct NoLibrary; /// Phantom type for when a database implementation is not needed. +#[derive(Debug)] pub struct NoDatabase; impl Default for MusicHoard { /// Create a new [`MusicHoard`] without any library or database. fn default() -> Self { - MusicHoard::new(NoLibrary, NoDatabase) + MusicHoard::empty() + } +} + +impl MusicHoard { + /// Create a new [`MusicHoard`] without any library or database. + pub fn empty() -> Self { + MusicHoard { + collection: vec![], + pre_commit: vec![], + library: NoLibrary, + database: NoDatabase, + library_cache: HashMap::new(), + database_cache: vec![], + } } } impl MusicHoard { - /// Create a new [`MusicHoard`] with the provided [`ILibrary`] and [`IDatabase`]. - pub fn new(library: LIB, database: DB) -> Self { - MusicHoard { - collection: vec![], - library, - database, - library_cache: HashMap::new(), - } - } - /// Retrieve the [`Collection`]. pub fn get_collection(&self) -> &Collection { &self.collection } - pub fn add_artist>(&mut self, artist_id: ID) { - let artist_id: ArtistId = artist_id.into(); - - if self.get_artist(&artist_id).is_none() { - self.collection.push(Artist::new(artist_id)); - Self::sort_artists(&mut self.collection); - } - } - - pub fn remove_artist>(&mut self, artist_id: ID) { - let index_opt = self - .collection - .iter() - .position(|a| &a.id == artist_id.as_ref()); - - if let Some(index) = index_opt { - self.collection.remove(index); - } - } - - pub fn set_artist_sort, SORT: Into>( - &mut self, - artist_id: ID, - artist_sort: SORT, - ) -> Result<(), Error> { - self.get_artist_mut_or_err(artist_id.as_ref())? - .set_sort_key(artist_sort); - Self::sort(&mut self.collection); - Ok(()) - } - - pub fn clear_artist_sort>(&mut self, artist_id: ID) -> Result<(), Error> { - self.get_artist_mut_or_err(artist_id.as_ref())? - .clear_sort_key(); - Self::sort(&mut self.collection); - Ok(()) - } - - pub fn add_musicbrainz_url, S: AsRef>( - &mut self, - artist_id: ID, - url: S, - ) -> Result<(), Error> { - Ok(self - .get_artist_mut_or_err(artist_id.as_ref())? - .add_musicbrainz_url(url)?) - } - - pub fn remove_musicbrainz_url, S: AsRef>( - &mut self, - artist_id: ID, - url: S, - ) -> Result<(), Error> { - Ok(self - .get_artist_mut_or_err(artist_id.as_ref())? - .remove_musicbrainz_url(url)?) - } - - pub fn set_musicbrainz_url, S: AsRef>( - &mut self, - artist_id: ID, - url: S, - ) -> Result<(), Error> { - Ok(self - .get_artist_mut_or_err(artist_id.as_ref())? - .set_musicbrainz_url(url)?) - } - - pub fn clear_musicbrainz_url>( - &mut self, - artist_id: ID, - ) -> Result<(), Error> { - self.get_artist_mut_or_err(artist_id.as_ref())? - .clear_musicbrainz_url(); - Ok(()) - } - - pub fn add_to_property, S: AsRef + Into>( - &mut self, - artist_id: ID, - property: S, - values: Vec, - ) -> Result<(), Error> { - self.get_artist_mut_or_err(artist_id.as_ref())? - .add_to_property(property, values); - Ok(()) - } - - pub fn remove_from_property, S: AsRef>( - &mut self, - artist_id: ID, - property: S, - values: Vec, - ) -> Result<(), Error> { - self.get_artist_mut_or_err(artist_id.as_ref())? - .remove_from_property(property, values); - Ok(()) - } - - pub fn set_property, S: AsRef + Into>( - &mut self, - artist_id: ID, - property: S, - values: Vec, - ) -> Result<(), Error> { - self.get_artist_mut_or_err(artist_id.as_ref())? - .set_property(property, values); - Ok(()) - } - - pub fn clear_property, S: AsRef>( - &mut self, - artist_id: ID, - property: S, - ) -> Result<(), Error> { - self.get_artist_mut_or_err(artist_id.as_ref())? - .clear_property(property); - Ok(()) - } - - fn sort(collection: &mut [Artist]) { - Self::sort_artists(collection); - Self::sort_albums_and_tracks(collection.iter_mut()); - } - fn sort_artists(collection: &mut [Artist]) { collection.sort_unstable(); } @@ -192,15 +74,18 @@ impl MusicHoard { fn merge_collections(&mut self) { let mut primary = self.library_cache.clone(); - for secondary_artist in self.collection.drain(..) { + for secondary_artist in self.database_cache.iter().cloned() { if let Some(ref mut primary_artist) = primary.get_mut(&secondary_artist.id) { primary_artist.merge_in_place(secondary_artist); } else { primary.insert(secondary_artist.id.clone(), secondary_artist); } } - self.collection.extend(primary.into_values()); + + self.collection = primary.into_values().collect(); Self::sort_artists(&mut self.collection); + + self.pre_commit = self.collection.clone(); } fn items_to_artists(items: Vec) -> Result, Error> { @@ -272,21 +157,41 @@ impl MusicHoard { Ok(collection) } - fn get_artist(&self, artist_id: &ArtistId) -> Option<&Artist> { - self.collection.iter().find(|a| &a.id == artist_id) + fn get_artist<'a>(collection: &'a Collection, artist_id: &ArtistId) -> Option<&'a Artist> { + collection.iter().find(|a| &a.id == artist_id) } - fn get_artist_mut(&mut self, artist_id: &ArtistId) -> Option<&mut Artist> { - self.collection.iter_mut().find(|a| &a.id == artist_id) + fn get_artist_mut<'a>( + collection: &'a mut Collection, + artist_id: &ArtistId, + ) -> Option<&'a mut Artist> { + collection.iter_mut().find(|a| &a.id == artist_id) } - fn get_artist_mut_or_err(&mut self, artist_id: &ArtistId) -> Result<&mut Artist, Error> { - self.get_artist_mut(artist_id).ok_or_else(|| { + fn get_artist_mut_or_err<'a>( + collection: &'a mut Collection, + artist_id: &ArtistId, + ) -> Result<&'a mut Artist, Error> { + Self::get_artist_mut(collection, artist_id).ok_or_else(|| { Error::CollectionError(format!("artist '{}' is not in the collection", artist_id)) }) } } +impl MusicHoard { + /// Create a new [`MusicHoard`] with the provided [`ILibrary`] and no database. + pub fn library(library: LIB) -> Self { + MusicHoard { + collection: vec![], + pre_commit: vec![], + library, + database: NoDatabase, + library_cache: HashMap::new(), + database_cache: vec![], + } + } +} + impl MusicHoard { /// Rescan the library and merge with the in-memory collection. pub fn rescan_library(&mut self) -> Result<(), Error> { @@ -299,26 +204,188 @@ impl MusicHoard { } } +impl MusicHoard { + /// Create a new [`MusicHoard`] with the provided [`IDatabase`] and no library. + pub fn database(database: DB) -> Result { + let mut mh = MusicHoard { + collection: vec![], + pre_commit: vec![], + library: NoLibrary, + database, + library_cache: HashMap::new(), + database_cache: vec![], + }; + mh.reload_database()?; + Ok(mh) + } +} + impl MusicHoard { /// Load the database and merge with the in-memory collection. - pub fn load_from_database(&mut self) -> Result<(), Error> { - self.collection = self.database.load()?; - Self::sort_albums_and_tracks(self.collection.iter_mut()); + pub fn reload_database(&mut self) -> Result<(), Error> { + self.database_cache = self.database.load()?; + Self::sort_albums_and_tracks(self.database_cache.iter_mut()); self.merge_collections(); Ok(()) } - /// Save the in-memory collection to the database. - pub fn save_to_database(&mut self) -> Result<(), Error> { - self.database.save(&self.collection)?; + 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(); + } Ok(()) } + + fn update_collection(&mut self, func: F) -> Result<(), Error> + where + F: FnOnce(&mut Collection), + { + func(&mut self.pre_commit); + self.commit() + } + + fn update_artist_and, F1, F2>( + &mut self, + artist_id: ID, + f1: F1, + f2: F2, + ) -> Result<(), Error> + where + F1: FnOnce(&mut Artist), + F2: FnOnce(&mut Collection), + { + f1(Self::get_artist_mut_or_err( + &mut self.pre_commit, + artist_id.as_ref(), + )?); + self.update_collection(f2) + } + + fn update_artist, F>(&mut self, artist_id: ID, func: F) -> Result<(), Error> + where + F: FnOnce(&mut Artist), + { + self.update_artist_and(artist_id, func, |_| {}) + } + + pub fn add_artist>(&mut self, artist_id: ID) -> 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); + } + }) + } + + pub fn remove_artist>(&mut self, artist_id: ID) -> Result<(), Error> { + self.update_collection(|collection| { + let index_opt = collection.iter().position(|a| &a.id == artist_id.as_ref()); + if let Some(index) = index_opt { + collection.remove(index); + } + }) + } + + pub fn set_artist_sort, SORT: Into>( + &mut self, + artist_id: ID, + artist_sort: SORT, + ) -> Result<(), Error> { + self.update_artist_and( + artist_id, + |artist| artist.set_sort_key(artist_sort), + |collection| Self::sort_artists(collection), + ) + } + + pub fn clear_artist_sort>(&mut self, artist_id: ID) -> Result<(), Error> { + self.update_artist_and( + artist_id, + |artist| artist.clear_sort_key(), + |collection| Self::sort_artists(collection), + ) + } + + pub fn set_musicbrainz_url, S: AsRef>( + &mut self, + artist_id: ID, + url: S, + ) -> Result<(), Error> { + let url = url.as_ref().try_into()?; + self.update_artist(artist_id, |artist| artist.set_musicbrainz_url(url)) + } + + pub fn clear_musicbrainz_url>( + &mut self, + artist_id: ID, + ) -> Result<(), Error> { + self.update_artist(artist_id, |artist| artist.clear_musicbrainz_url()) + } + + pub fn add_to_property, S: AsRef + Into>( + &mut self, + artist_id: ID, + property: S, + values: Vec, + ) -> Result<(), Error> { + self.update_artist(artist_id, |artist| artist.add_to_property(property, values)) + } + + pub fn remove_from_property, S: AsRef>( + &mut self, + artist_id: ID, + property: S, + values: Vec, + ) -> Result<(), Error> { + self.update_artist(artist_id, |artist| { + artist.remove_from_property(property, values) + }) + } + + pub fn set_property, S: AsRef + Into>( + &mut self, + artist_id: ID, + property: S, + values: Vec, + ) -> Result<(), Error> { + self.update_artist(artist_id, |artist| artist.set_property(property, values)) + } + + pub fn clear_property, S: AsRef>( + &mut self, + artist_id: ID, + property: S, + ) -> Result<(), Error> { + self.update_artist(artist_id, |artist| artist.clear_property(property)) + } +} + +impl MusicHoard { + /// Create a new [`MusicHoard`] with the provided [`ILibrary`] and [`IDatabase`]. + pub fn new(library: LIB, database: DB) -> Result { + let mut mh = MusicHoard { + collection: vec![], + pre_commit: vec![], + library, + database, + library_cache: HashMap::new(), + database_cache: vec![], + }; + mh.reload_database()?; + Ok(mh) + } } #[cfg(test)] mod tests { - use mockall::predicate; + use mockall::{predicate, Sequence}; use crate::core::{ collection::artist::{ArtistId, MusicBrainz}, @@ -338,28 +405,56 @@ mod tests { fn artist_new_delete() { let artist_id = ArtistId::new("an artist"); let artist_id_2 = ArtistId::new("another artist"); - let mut music_hoard = MusicHoard::default(); - let mut expected: Vec = vec![]; + let collection = FULL_COLLECTION.to_owned(); + let mut with_artist = collection.clone(); + with_artist.push(Artist::new(artist_id.clone())); - music_hoard.add_artist(artist_id.clone()); - expected.push(Artist::new(artist_id.clone())); - assert_eq!(music_hoard.collection, expected); + 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(1) + .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(())); - music_hoard.add_artist(artist_id.clone()); - assert_eq!(music_hoard.collection, expected); + let mut music_hoard = MusicHoard::database(database).unwrap(); + assert_eq!(music_hoard.collection, collection); - music_hoard.remove_artist(&artist_id_2); - assert_eq!(music_hoard.collection, expected); + assert!(music_hoard.add_artist(artist_id.clone()).is_ok()); + assert_eq!(music_hoard.collection, with_artist); - music_hoard.remove_artist(&artist_id); - _ = expected.pop(); - assert_eq!(music_hoard.collection, expected); + 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 music_hoard = MusicHoard::default(); + let mut database = MockIDatabase::new(); + database.expect_load().times(1).returning(|| Ok(vec![])); + database.expect_save().times(4).returning(|_| Ok(())); + + type MH = MusicHoard; + let mut music_hoard: MH = MusicHoard::database(database).unwrap(); let artist_1_id = ArtistId::new("the artist"); let artist_1_sort = ArtistId::new("artist, the"); @@ -370,11 +465,11 @@ mod tests { assert!(artist_1_sort < artist_2_id); assert!(artist_2_id < artist_1_id); - music_hoard.add_artist(artist_1_id.clone()); - music_hoard.add_artist(artist_2_id.clone()); + 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 = music_hoard.get_artist(&artist_1_id).unwrap(); - let artist_2: &Artist = music_hoard.get_artist(&artist_2_id).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); @@ -385,8 +480,8 @@ mod tests { .set_artist_sort(artist_1_id.as_ref(), artist_1_sort.clone()) .unwrap(); - let artist_1: &Artist = music_hoard.get_artist(&artist_1_id).unwrap(); - let artist_2: &Artist = music_hoard.get_artist(&artist_2_id).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); @@ -395,8 +490,8 @@ mod tests { music_hoard.clear_artist_sort(artist_1_id.as_ref()).unwrap(); - let artist_1: &Artist = music_hoard.get_artist(&artist_1_id).unwrap(); - let artist_2: &Artist = music_hoard.get_artist(&artist_2_id).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); @@ -406,12 +501,16 @@ mod tests { #[test] fn collection_error() { + let mut database = MockIDatabase::new(); + database.expect_load().times(1).returning(|| Ok(vec![])); + database.expect_save().times(1).returning(|_| Ok(())); + let artist_id = ArtistId::new("an artist"); - let mut music_hoard = MusicHoard::default(); - music_hoard.add_artist(artist_id.clone()); + let mut music_hoard = MusicHoard::database(database).unwrap(); + assert!(music_hoard.add_artist(artist_id.clone()).is_ok()); let actual_err = music_hoard - .add_musicbrainz_url(&artist_id, MUSICBUTLER) + .set_musicbrainz_url(&artist_id, MUSICBUTLER) .unwrap_err(); let expected_err = Error::CollectionError(format!( "an error occurred when processing a URL: invalid MusicBrainz URL: {MUSICBUTLER}" @@ -420,51 +519,17 @@ mod tests { assert_eq!(actual_err.to_string(), expected_err.to_string()); } - #[test] - fn add_remove_musicbrainz_url() { - let artist_id = ArtistId::new("an artist"); - let artist_id_2 = ArtistId::new("another artist"); - let mut music_hoard = MusicHoard::default(); - - music_hoard.add_artist(artist_id.clone()); - - let mut expected: Option = None; - assert_eq!(music_hoard.collection[0].musicbrainz, expected); - - // Adding URL to an artist not in the collection is an error. - assert!(music_hoard - .add_musicbrainz_url(&artist_id_2, MUSICBRAINZ) - .is_err()); - assert_eq!(music_hoard.collection[0].musicbrainz, expected); - - // Adding URL to artist. - assert!(music_hoard - .add_musicbrainz_url(&artist_id, MUSICBRAINZ) - .is_ok()); - _ = expected.insert(MusicBrainz::new(MUSICBRAINZ).unwrap()); - assert_eq!(music_hoard.collection[0].musicbrainz, expected); - - // Removing a URL from an artist not in the collection is an error. - assert!(music_hoard - .remove_musicbrainz_url(&artist_id_2, MUSICBRAINZ) - .is_err()); - assert_eq!(music_hoard.collection[0].musicbrainz, expected); - - // Removing a URL in the collection removes it. - assert!(music_hoard - .remove_musicbrainz_url(&artist_id, MUSICBRAINZ) - .is_ok()); - _ = expected.take(); - assert_eq!(music_hoard.collection[0].musicbrainz, expected); - } - #[test] fn set_clear_musicbrainz_url() { + let mut database = MockIDatabase::new(); + database.expect_load().times(1).returning(|| Ok(vec![])); + 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::default(); + let mut music_hoard = MusicHoard::database(database).unwrap(); - music_hoard.add_artist(artist_id.clone()); + assert!(music_hoard.add_artist(artist_id.clone()).is_ok()); let mut expected: Option = None; assert_eq!(music_hoard.collection[0].musicbrainz, expected); @@ -494,11 +559,15 @@ mod tests { #[test] fn add_to_remove_from_property() { + let mut database = MockIDatabase::new(); + database.expect_load().times(1).returning(|| Ok(vec![])); + 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::default(); + let mut music_hoard = MusicHoard::database(database).unwrap(); - music_hoard.add_artist(artist_id.clone()); + assert!(music_hoard.add_artist(artist_id.clone()).is_ok()); let mut expected: Vec = vec![]; assert!(music_hoard.collection[0].properties.is_empty()); @@ -539,11 +608,15 @@ mod tests { #[test] fn set_clear_property() { + let mut database = MockIDatabase::new(); + database.expect_load().times(1).returning(|| Ok(vec![])); + 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::default(); + let mut music_hoard = MusicHoard::database(database).unwrap(); - music_hoard.add_artist(artist_id.clone()); + assert!(music_hoard.add_artist(artist_id.clone()).is_ok()); let mut expected: Vec = vec![]; assert!(music_hoard.collection[0].properties.is_empty()); @@ -590,12 +663,12 @@ mod tests { expected.sort_unstable(); let mut mh = MusicHoard { - collection: right.clone(), library_cache: left .clone() .into_iter() .map(|a| (a.id.clone(), a)) .collect(), + database_cache: right.clone(), ..Default::default() }; @@ -604,12 +677,12 @@ mod tests { // The merge is completely non-overlapping so it should be commutative. let mut mh = MusicHoard { - collection: left.clone(), library_cache: right .clone() .into_iter() .map(|a| (a.id.clone(), a)) .collect(), + database_cache: left.clone(), ..Default::default() }; @@ -628,12 +701,12 @@ mod tests { expected.sort_unstable(); let mut mh = MusicHoard { - collection: right.clone(), library_cache: left .clone() .into_iter() .map(|a| (a.id.clone(), a)) .collect(), + database_cache: right.clone(), ..Default::default() }; @@ -642,12 +715,12 @@ mod tests { // The merge does not overwrite any data so it should be commutative. let mut mh = MusicHoard { - collection: left.clone(), library_cache: right .clone() .into_iter() .map(|a| (a.id.clone(), a)) .collect(), + database_cache: left.clone(), ..Default::default() }; @@ -679,12 +752,12 @@ mod tests { expected.rotate_right(1); let mut mh = MusicHoard { - collection: right.clone(), library_cache: left .clone() .into_iter() .map(|a| (a.id.clone(), a)) .collect(), + database_cache: right.clone(), ..Default::default() }; @@ -693,12 +766,12 @@ mod tests { // The merge overwrites the sort data, but no data is erased so it should be commutative. let mut mh = MusicHoard { - collection: left.clone(), library_cache: right .clone() .into_iter() .map(|a| (a.id.clone(), a)) .collect(), + database_cache: left.clone(), ..Default::default() }; @@ -709,7 +782,6 @@ mod tests { #[test] fn rescan_library_ordered() { let mut library = MockILibrary::new(); - let database = MockIDatabase::new(); let library_input = Query::new(); let library_result = Ok(LIBRARY_ITEMS.to_owned()); @@ -720,16 +792,59 @@ mod tests { .times(1) .return_once(|_| library_result); - let mut music_hoard = MusicHoard::new(library, database); + let mut music_hoard = MusicHoard::library(library); music_hoard.rescan_library().unwrap(); assert_eq!(music_hoard.get_collection(), &*LIBRARY_COLLECTION); } + #[test] + fn rescan_library_changed() { + let mut library = MockILibrary::new(); + let mut seq = Sequence::new(); + + let library_input = Query::new(); + let library_result = Ok(LIBRARY_ITEMS.to_owned()); + + library + .expect_list() + .with(predicate::eq(library_input)) + .times(1) + .in_sequence(&mut seq) + .return_once(|_| library_result); + + let library_input = Query::new(); + let library_result = Ok(LIBRARY_ITEMS + .iter() + .filter(|item| item.album_title != "album_title a.a") + .cloned() + .collect()); + + library + .expect_list() + .with(predicate::eq(library_input)) + .times(1) + .in_sequence(&mut seq) + .return_once(|_| library_result); + + let mut music_hoard = MusicHoard::library(library); + + music_hoard.rescan_library().unwrap(); + assert!(music_hoard.get_collection()[0] + .albums + .iter() + .any(|album| album.id.title == "album_title a.a")); + + music_hoard.rescan_library().unwrap(); + assert!(!music_hoard.get_collection()[0] + .albums + .iter() + .any(|album| album.id.title == "album_title a.a")); + } + #[test] fn rescan_library_unordered() { let mut library = MockILibrary::new(); - let database = MockIDatabase::new(); let library_input = Query::new(); let mut library_result = Ok(LIBRARY_ITEMS.to_owned()); @@ -744,7 +859,7 @@ mod tests { .times(1) .return_once(|_| library_result); - let mut music_hoard = MusicHoard::new(library, database); + let mut music_hoard = MusicHoard::library(library); music_hoard.rescan_library().unwrap(); assert_eq!(music_hoard.get_collection(), &*LIBRARY_COLLECTION); @@ -753,7 +868,6 @@ mod tests { #[test] fn rescan_library_album_title_year_clash() { let mut library = MockILibrary::new(); - let database = MockIDatabase::new(); let mut expected = LIBRARY_COLLECTION.to_owned(); let removed_album_id = expected[0].albums[0].id.clone(); @@ -778,7 +892,7 @@ mod tests { .times(1) .return_once(|_| library_result); - let mut music_hoard = MusicHoard::new(library, database); + let mut music_hoard = MusicHoard::library(library); music_hoard.rescan_library().unwrap(); assert_eq!(music_hoard.get_collection(), &expected); @@ -787,7 +901,6 @@ mod tests { #[test] fn rescan_library_album_artist_sort_clash() { let mut library = MockILibrary::new(); - let database = MockIDatabase::new(); let library_input = Query::new(); let mut library_items = LIBRARY_ITEMS.to_owned(); @@ -811,7 +924,7 @@ mod tests { .times(1) .return_once(|_| library_result); - let mut music_hoard = MusicHoard::new(library, database); + let mut music_hoard = MusicHoard::library(library); assert!(music_hoard.rescan_library().is_err()); } @@ -826,46 +939,14 @@ mod tests { .times(1) .return_once(|| Ok(FULL_COLLECTION.to_owned())); - let mut music_hoard = MusicHoard::new(library, database); + let music_hoard = MusicHoard::new(library, database).unwrap(); - music_hoard.load_from_database().unwrap(); assert_eq!(music_hoard.get_collection(), &*FULL_COLLECTION); } - #[test] - fn rescan_get_save() { - let mut library = MockILibrary::new(); - let mut database = MockIDatabase::new(); - - let library_input = Query::new(); - let library_result = Ok(LIBRARY_ITEMS.to_owned()); - - let database_input = LIBRARY_COLLECTION.to_owned(); - let database_result = Ok(()); - - library - .expect_list() - .with(predicate::eq(library_input)) - .times(1) - .return_once(|_| library_result); - - database - .expect_save() - .with(predicate::eq(database_input)) - .times(1) - .return_once(|_: &Collection| database_result); - - let mut music_hoard = MusicHoard::new(library, database); - - music_hoard.rescan_library().unwrap(); - assert_eq!(music_hoard.get_collection(), &*LIBRARY_COLLECTION); - music_hoard.save_to_database().unwrap(); - } - #[test] fn library_error() { let mut library = MockILibrary::new(); - let database = MockIDatabase::new(); let library_result = Err(library::Error::Invalid(String::from("invalid data"))); @@ -874,7 +955,7 @@ mod tests { .times(1) .return_once(|_| library_result); - let mut music_hoard = MusicHoard::new(library, database); + let mut music_hoard = MusicHoard::library(library); let actual_err = music_hoard.rescan_library().unwrap_err(); let expected_err = @@ -886,7 +967,6 @@ mod tests { #[test] fn database_load_error() { - let library = MockILibrary::new(); let mut database = MockIDatabase::new(); let database_result = Err(database::LoadError::IoError(String::from("I/O error"))); @@ -896,9 +976,7 @@ mod tests { .times(1) .return_once(|| database_result); - let mut music_hoard = MusicHoard::new(library, database); - - let actual_err = music_hoard.load_from_database().unwrap_err(); + let actual_err = MusicHoard::database(database).unwrap_err(); let expected_err = Error::DatabaseError( database::LoadError::IoError(String::from("I/O error")).to_string(), ); @@ -909,19 +987,21 @@ mod tests { #[test] fn database_save_error() { - let library = MockILibrary::new(); let mut database = MockIDatabase::new(); let database_result = Err(database::SaveError::IoError(String::from("I/O error"))); + database.expect_load().return_once(|| Ok(vec![])); database .expect_save() .times(1) .return_once(|_: &Collection| database_result); - let mut music_hoard = MusicHoard::new(library, database); + let mut music_hoard = MusicHoard::database(database).unwrap(); - let actual_err = music_hoard.save_to_database().unwrap_err(); + let actual_err = music_hoard + .add_artist(ArtistId::new("an artist")) + .unwrap_err(); let expected_err = Error::DatabaseError( database::SaveError::IoError(String::from("I/O error")).to_string(), ); @@ -929,4 +1009,10 @@ mod tests { assert_eq!(actual_err, expected_err); assert_eq!(actual_err.to_string(), expected_err.to_string()); } + + #[test] + fn empty() { + let music_hoard = MusicHoard::empty(); + assert!(!format!("{music_hoard:?}").is_empty()); + } } diff --git a/src/core/musichoard/musichoard_builder.rs b/src/core/musichoard/musichoard_builder.rs index 346a592..af48922 100644 --- a/src/core/musichoard/musichoard_builder.rs +++ b/src/core/musichoard/musichoard_builder.rs @@ -1,7 +1,10 @@ -use crate::core::{ - database::IDatabase, - library::ILibrary, - musichoard::musichoard::{MusicHoard, NoDatabase, NoLibrary}, +use crate::{ + core::{ + database::IDatabase, + library::ILibrary, + musichoard::musichoard::{MusicHoard, NoDatabase, NoLibrary}, + }, + Error, }; /// Builder for [`MusicHoard`]. Its purpose is to make it easier to set various combinations of @@ -44,9 +47,32 @@ impl MusicHoardBuilder { database, } } +} +impl MusicHoardBuilder { /// Build [`MusicHoard`] with the currently set library and database. - pub fn build(self) -> MusicHoard { + pub fn build(self) -> MusicHoard { + MusicHoard::empty() + } +} + +impl MusicHoardBuilder { + /// Build [`MusicHoard`] with the currently set library and database. + pub fn build(self) -> MusicHoard { + MusicHoard::library(self.library) + } +} + +impl MusicHoardBuilder { + /// Build [`MusicHoard`] with the currently set library and database. + pub fn build(self) -> Result, Error> { + MusicHoard::database(self.database) + } +} + +impl MusicHoardBuilder { + /// Build [`MusicHoard`] with the currently set library and database. + pub fn build(self) -> Result, Error> { MusicHoard::new(self.library, self.database) } } @@ -59,7 +85,7 @@ mod tests { #[test] fn no_library_no_database() { - MusicHoardBuilder::default(); + MusicHoardBuilder::default().build(); } #[test] @@ -74,9 +100,9 @@ mod tests { fn no_library_with_database() { let mut mh = MusicHoardBuilder::default() .set_database(NullDatabase) - .build(); - assert!(mh.load_from_database().is_ok()); - assert!(mh.save_to_database().is_ok()); + .build() + .unwrap(); + assert!(mh.reload_database().is_ok()); } #[test] @@ -84,9 +110,9 @@ mod tests { let mut mh = MusicHoardBuilder::default() .set_library(NullLibrary) .set_database(NullDatabase) - .build(); + .build() + .unwrap(); assert!(mh.rescan_library().is_ok()); - assert!(mh.load_from_database().is_ok()); - assert!(mh.save_to_database().is_ok()); + assert!(mh.reload_database().is_ok()); } } diff --git a/src/main.rs b/src/main.rs index 63afadd..5c3dfa5 100644 --- a/src/main.rs +++ b/src/main.rs @@ -61,7 +61,7 @@ struct DbOpt { } fn with(builder: MusicHoardBuilder) { - let music_hoard = builder.build(); + let music_hoard = builder.build().expect("failed to initialise MusicHoard"); // Initialize the terminal user interface. let backend = CrosstermBackend::new(io::stdout()); diff --git a/src/tui/app/machine/browse.rs b/src/tui/app/machine/browse.rs index f327ee3..481ca32 100644 --- a/src/tui/app/machine/browse.rs +++ b/src/tui/app/machine/browse.rs @@ -36,14 +36,9 @@ impl<'a, MH: IMusicHoard> From<&'a mut AppMachine> for AppPublic< impl IAppInteractBrowse for AppMachine { type APP = App; - fn save_and_quit(mut self) -> Self::APP { - match self.inner.music_hoard.save_to_database() { - Ok(_) => { - self.inner.running = false; - self.into() - } - Err(err) => AppMachine::error(self.inner, err.to_string()).into(), - } + fn quit(mut self) -> Self::APP { + self.inner.running = false; + self.into() } fn increment_category(mut self) -> Self::APP { @@ -104,37 +99,16 @@ mod tests { use super::*; #[test] - fn save_and_quit() { - let mut music_hoard = music_hoard(vec![]); - - music_hoard - .expect_save_to_database() - .times(1) - .return_once(|| Ok(())); + fn quit() { + let music_hoard = music_hoard(vec![]); let browse = AppMachine::browse(inner(music_hoard)); - let app = browse.save_and_quit(); + let app = browse.quit(); assert!(!app.is_running()); app.unwrap_browse(); } - #[test] - fn save_and_quit_error() { - let mut music_hoard = music_hoard(vec![]); - - music_hoard - .expect_save_to_database() - .times(1) - .return_once(|| Err(musichoard::Error::DatabaseError(String::from("get rekt")))); - - let browse = AppMachine::browse(inner(music_hoard)); - - let app = browse.save_and_quit(); - assert!(app.is_running()); - app.unwrap_error(); - } - #[test] fn increment_decrement() { let mut browse = AppMachine::browse(inner(music_hoard(COLLECTION.to_owned()))); diff --git a/src/tui/app/machine/mod.rs b/src/tui/app/machine/mod.rs index a35e7cd..e4e9c60 100644 --- a/src/tui/app/machine/mod.rs +++ b/src/tui/app/machine/mod.rs @@ -48,7 +48,6 @@ impl App { } fn init(music_hoard: &mut MH) -> Result<(), musichoard::Error> { - music_hoard.load_from_database()?; music_hoard.rescan_library()?; Ok(()) } @@ -196,10 +195,6 @@ mod tests { fn music_hoard_init(collection: Collection) -> MockIMusicHoard { let mut music_hoard = music_hoard(collection); - music_hoard - .expect_load_from_database() - .times(1) - .return_once(|| Ok(())); music_hoard .expect_rescan_library() .times(1) @@ -323,9 +318,9 @@ mod tests { let mut music_hoard = MockIMusicHoard::new(); music_hoard - .expect_load_from_database() + .expect_rescan_library() .times(1) - .return_once(|| Err(musichoard::Error::DatabaseError(String::from("get rekt")))); + .return_once(|| Err(musichoard::Error::LibraryError(String::from("get rekt")))); music_hoard.expect_get_collection().return_const(vec![]); let app = App::new(music_hoard); diff --git a/src/tui/app/machine/reload.rs b/src/tui/app/machine/reload.rs index 79564f8..ee6a57f 100644 --- a/src/tui/app/machine/reload.rs +++ b/src/tui/app/machine/reload.rs @@ -49,7 +49,7 @@ impl IAppInteractReload for AppMachine { self.inner.music_hoard.get_collection(), &self.inner.selection, ); - let result = self.inner.music_hoard.load_from_database(); + let result = self.inner.music_hoard.reload_database(); self.refresh(previous, result) } @@ -98,7 +98,7 @@ mod tests { let mut music_hoard = music_hoard(vec![]); music_hoard - .expect_load_from_database() + .expect_reload_database() .times(1) .return_once(|| Ok(())); @@ -126,7 +126,7 @@ mod tests { let mut music_hoard = music_hoard(vec![]); music_hoard - .expect_load_from_database() + .expect_reload_database() .times(1) .return_once(|| Err(musichoard::Error::DatabaseError(String::from("get rekt")))); diff --git a/src/tui/app/mod.rs b/src/tui/app/mod.rs index 66f612a..8f86587 100644 --- a/src/tui/app/mod.rs +++ b/src/tui/app/mod.rs @@ -33,7 +33,7 @@ pub trait IAppInteract { pub trait IAppInteractBrowse { type APP: IAppInteract; - fn save_and_quit(self) -> Self::APP; + fn quit(self) -> Self::APP; fn increment_category(self) -> Self::APP; fn decrement_category(self) -> Self::APP; diff --git a/src/tui/handler.rs b/src/tui/handler.rs index a9d73d6..3e06caf 100644 --- a/src/tui/handler.rs +++ b/src/tui/handler.rs @@ -83,7 +83,7 @@ impl IEventHandlerPrivate for EventHandler { fn handle_browse_key_event(app: ::BS, key_event: KeyEvent) -> APP { match key_event.code { // Exit application on `ESC` or `q`. - KeyCode::Esc | KeyCode::Char('q') | KeyCode::Char('Q') => app.save_and_quit(), + KeyCode::Esc | KeyCode::Char('q') | KeyCode::Char('Q') => app.quit(), // Category change. KeyCode::Left => app.decrement_category(), KeyCode::Right => app.increment_category(), diff --git a/src/tui/lib.rs b/src/tui/lib.rs index 23349ce..f85b7c3 100644 --- a/src/tui/lib.rs +++ b/src/tui/lib.rs @@ -6,8 +6,7 @@ use mockall::automock; #[cfg_attr(test, automock)] pub trait IMusicHoard { fn rescan_library(&mut self) -> Result<(), musichoard::Error>; - fn load_from_database(&mut self) -> Result<(), musichoard::Error>; - fn save_to_database(&mut self) -> Result<(), musichoard::Error>; + fn reload_database(&mut self) -> Result<(), musichoard::Error>; fn get_collection(&self) -> &Collection; } @@ -17,12 +16,8 @@ impl IMusicHoard for MusicHoard { MusicHoard::rescan_library(self) } - fn load_from_database(&mut self) -> Result<(), musichoard::Error> { - MusicHoard::load_from_database(self) - } - - fn save_to_database(&mut self) -> Result<(), musichoard::Error> { - MusicHoard::save_to_database(self) + fn reload_database(&mut self) -> Result<(), musichoard::Error> { + MusicHoard::reload_database(self) } fn get_collection(&self) -> &Collection { diff --git a/src/tui/mod.rs b/src/tui/mod.rs index e11acb6..f442ce6 100644 --- a/src/tui/mod.rs +++ b/src/tui/mod.rs @@ -193,7 +193,7 @@ mod tests { fn music_hoard(collection: Collection) -> MockIMusicHoard { let mut music_hoard = MockIMusicHoard::new(); - music_hoard.expect_load_from_database().returning(|| Ok(())); + music_hoard.expect_reload_database().returning(|| Ok(())); music_hoard.expect_rescan_library().returning(|| Ok(())); music_hoard.expect_get_collection().return_const(collection); diff --git a/tests/lib.rs b/tests/lib.rs index 4cf757c..5074fcf 100644 --- a/tests/lib.rs +++ b/tests/lib.rs @@ -29,10 +29,10 @@ fn merge_library_then_database() { let backend = JsonDatabaseFileBackend::new(&*database::json::DATABASE_TEST_FILE); let database = JsonDatabase::new(backend); - let mut music_hoard = MusicHoard::new(library, database); + let mut music_hoard = MusicHoard::new(library, database).unwrap(); music_hoard.rescan_library().unwrap(); - music_hoard.load_from_database().unwrap(); + music_hoard.reload_database().unwrap(); assert_eq!(music_hoard.get_collection(), &*COLLECTION); } @@ -52,9 +52,9 @@ fn merge_database_then_library() { let backend = JsonDatabaseFileBackend::new(&*database::json::DATABASE_TEST_FILE); let database = JsonDatabase::new(backend); - let mut music_hoard = MusicHoard::new(library, database); + let mut music_hoard = MusicHoard::new(library, database).unwrap(); - music_hoard.load_from_database().unwrap(); + music_hoard.reload_database().unwrap(); music_hoard.rescan_library().unwrap(); assert_eq!(music_hoard.get_collection(), &*COLLECTION);