From b53d04e0360827ed47c4a2c513ca98a56e78d409 Mon Sep 17 00:00:00 2001 From: Wojciech Kozlowski Date: Sat, 4 Jan 2025 00:01:57 +0100 Subject: [PATCH] Implement and test --- src/tui/app/machine/match_state.rs | 112 +++++++++++++++++++++++++---- src/tui/lib/mod.rs | 13 ++++ 2 files changed, 111 insertions(+), 14 deletions(-) diff --git a/src/tui/app/machine/match_state.rs b/src/tui/app/machine/match_state.rs index b25e669..17eec10 100644 --- a/src/tui/app/machine/match_state.rs +++ b/src/tui/app/machine/match_state.rs @@ -6,10 +6,13 @@ use musichoard::collection::{ musicbrainz::{MbRefOption, Mbid}, }; -use crate::tui::app::{ - machine::{fetch_state::FetchState, input::Input, App, AppInner, AppMachine}, - AlbumMatches, AppPublicState, AppState, ArtistMatches, Delta, EntityMatches, IAppInteractMatch, - MatchOption, MatchStatePublic, WidgetState, +use crate::tui::{ + app::{ + machine::{fetch_state::FetchState, input::Input, App, AppInner, AppMachine}, + AlbumMatches, AppPublicState, AppState, ArtistMatches, Delta, EntityMatches, + IAppInteractMatch, MatchOption, MatchStatePublic, WidgetState, + }, + lib::IMusicHoard, }; struct ArtistInfoTuple { @@ -200,6 +203,39 @@ impl AppMachine { self.input.replace(Input::default()); self.into() } + + fn select_artist( + mh: &mut Box, + matches: &ArtistMatches, + tuple: ArtistInfoTuple, + ) -> Result<(), musichoard::Error> { + mh.merge_artist_info(&matches.matching.id, tuple.info)?; + mh.set_artist_mb_ref(&matches.matching.id, tuple.mb_ref) + } + + fn select_album( + mh: &mut Box, + matches: &AlbumMatches, + tuple: AlbumInfoTuple, + ) -> Result<(), musichoard::Error> { + let coll = mh.get_collection(); + let mut clashing = vec![]; + if let Some(artist) = coll.iter().find(|artist| artist.meta.id == matches.artist) { + // While we expect only one, there is nothing stopping anybody from having multiple + // different albums with the same MBID. + let iter = artist.albums.iter(); + let filtered = iter + .filter(|album| album.meta.id != matches.matching) + .filter(|album| album.meta.id.mb_ref == tuple.mb_ref); + clashing.extend(filtered.map(|a| &a.meta.id).cloned()) + } + + for album in clashing.into_iter() { + mh.remove_album(&matches.artist, &album)?; + } + mh.merge_album_info(&matches.artist, &matches.matching, tuple.info)?; + mh.set_album_mb_ref(&matches.artist, &matches.matching, tuple.mb_ref) + } } impl From> for App { @@ -246,17 +282,11 @@ 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(tuple) => mh - .merge_artist_info(&matches.matching.id, tuple.info) - .and_then(|()| mh.set_artist_mb_ref(&matches.matching.id, tuple.mb_ref)), + InfoOption::Info(tuple) => Self::select_artist(mh, matches, tuple), InfoOption::NeedInput => return self.get_input(), }, EntityMatches::Album(ref mut matches) => match matches.list.extract_info(index) { - InfoOption::Info(tuple) => mh - .merge_album_info(&matches.artist, &matches.matching, tuple.info) - .and_then(|()| { - mh.set_album_mb_ref(&matches.artist, &matches.matching, tuple.mb_ref) - }), + InfoOption::Info(tuple) => Self::select_album(mh, matches, tuple), InfoOption::NeedInput => return self.get_input(), }, }; @@ -282,8 +312,10 @@ mod tests { Sequence, }; use musichoard::collection::{ - album::{AlbumDate, AlbumId, AlbumInfo, AlbumMeta, AlbumPrimaryType, AlbumSecondaryType}, - artist::{ArtistId, ArtistMeta}, + album::{ + Album, AlbumDate, AlbumId, AlbumInfo, AlbumMeta, AlbumPrimaryType, AlbumSecondaryType, + }, + artist::{Artist, ArtistId, ArtistMeta}, }; use crate::tui::{ @@ -593,6 +625,58 @@ mod tests { matches.select().unwrap_fetch(); } + #[test] + fn set_album_info_with_clashing_mbid() { + let matches_info = album_match(); + + // The collection below should trigger MH to remove the album with the same MBID but not + // matching album_id. + + // (1) Same artist as matches_info. + let mut artist = Artist::new(ArtistId::new("Artist")); + + // (2) An album with the same album_id as the selected one. + artist.albums.push(Album::new(AlbumId::new("Album"))); + + // (3) An album with a different album_id than the selected one. + // (4) This album has an MBID that is identical to that of the selected match. + let mb_ref = AlbumMbRef::Some(mbid().into()); + let removed = Album::new(AlbumId::new("Not the Same").with_mb_ref(mb_ref)); + artist.albums.push(removed.clone()); + + let (_tx, rx) = mpsc::channel(); + let app_matches = MatchState::new(matches_info.clone(), FetchState::search(rx)); + + let mut music_hoard = music_hoard(vec![artist]); + match matches_info { + EntityMatches::Artist(_) => panic!(), + EntityMatches::Album(matches) => { + let artist = matches.artist.clone(); + + let mut seq = Sequence::new(); + music_hoard + .expect_remove_album() + .times(1) + .with(eq(artist), eq(removed.meta.id.clone())) + .in_sequence(&mut seq) + .return_once(|_, _| Ok(())); + music_hoard + .expect_merge_album_info() + .times(1) + .in_sequence(&mut seq) + .return_once(|_, _, _| Ok(())); + music_hoard + .expect_set_album_mb_ref() + .times(1) + .in_sequence(&mut seq) + .return_once(|_, _, _| Ok(())); + } + } + + let matches = AppMachine::match_state(inner(music_hoard), app_matches); + matches.select().unwrap_fetch(); + } + #[test] fn set_info_error() { let matches_info = artist_match(); diff --git a/src/tui/lib/mod.rs b/src/tui/lib/mod.rs index b45e662..c08eb3a 100644 --- a/src/tui/lib/mod.rs +++ b/src/tui/lib/mod.rs @@ -25,6 +25,11 @@ pub trait IMusicHoard { artist_id: &ArtistId, album_meta: AlbumMeta, ) -> Result<(), musichoard::Error>; + fn remove_album( + &mut self, + artist_id: &ArtistId, + album_id: &AlbumId, + ) -> Result<(), musichoard::Error>; fn set_artist_mb_ref( &mut self, @@ -72,6 +77,14 @@ impl IMusicHoard for MusicHoard::add_album(self, artist_id, album_meta) } + fn remove_album( + &mut self, + artist_id: &ArtistId, + album_id: &AlbumId, + ) -> Result<(), musichoard::Error> { + ::remove_album(self, artist_id, album_id) + } + fn set_artist_mb_ref( &mut self, artist_id: &ArtistId, -- 2.47.1