From 829bb896966c0ecad06d7145ac5569ece527f7fd Mon Sep 17 00:00:00 2001 From: Wojciech Kozlowski Date: Sun, 5 Jan 2025 11:19:35 +0100 Subject: [PATCH 1/2] Code fix --- src/tui/app/machine/match_state.rs | 35 ++++++++++++++++++------------ 1 file changed, 21 insertions(+), 14 deletions(-) diff --git a/src/tui/app/machine/match_state.rs b/src/tui/app/machine/match_state.rs index 4c8e83c..8d7dc6d 100644 --- a/src/tui/app/machine/match_state.rs +++ b/src/tui/app/machine/match_state.rs @@ -6,13 +6,11 @@ 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, - }, - lib::IMusicHoard, +use crate::tui::app::{ + machine::{fetch_state::FetchState, input::Input, App, AppInner, AppMachine}, + selection::KeySelection, + AlbumMatches, AppPublicState, AppState, ArtistMatches, Delta, EntityMatches, IAppInteractMatch, + MatchOption, MatchStatePublic, WidgetState, }; struct ArtistInfoTuple { @@ -205,20 +203,21 @@ impl AppMachine { } fn select_artist( - mh: &mut Box, + inner: &mut AppInner, matches: &ArtistMatches, tuple: ArtistInfoTuple, ) -> 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) } fn select_album( - mh: &mut Box, + inner: &mut AppInner, matches: &AlbumMatches, tuple: AlbumInfoTuple, ) -> Result<(), musichoard::Error> { - let coll = mh.get_collection(); + let coll = inner.music_hoard.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 @@ -231,8 +230,16 @@ impl AppMachine { } for album in clashing.into_iter() { - mh.remove_album(&matches.artist, &album)?; + let coll = inner.music_hoard.get_filtered(); + let selection = KeySelection::get(coll, &inner.selection); + + inner.music_hoard.remove_album(&matches.artist, &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) } @@ -279,14 +286,14 @@ impl IAppInteractMatch for AppMachine { fn select(mut self) -> Self::APP { let index = self.state.state.list.selected().unwrap(); - let mh = &mut self.inner.music_hoard; + 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(mh, matches, tuple), + InfoOption::Info(tuple) => Self::select_artist(inner, matches, tuple), InfoOption::NeedInput => return self.get_input(), }, EntityMatches::Album(ref mut matches) => match matches.list.extract_info(index) { - InfoOption::Info(tuple) => Self::select_album(mh, matches, tuple), + InfoOption::Info(tuple) => Self::select_album(inner, matches, tuple), InfoOption::NeedInput => return self.get_input(), }, }; -- 2.47.1 From 38fe036a4bf0368552a0fa370be41b1da4ce4a60 Mon Sep 17 00:00:00 2001 From: Wojciech Kozlowski Date: Sun, 5 Jan 2025 12:00:28 +0100 Subject: [PATCH 2/2] Unit test --- src/tui/app/machine/match_state.rs | 57 +++++++++++++++++++++++++----- 1 file changed, 49 insertions(+), 8 deletions(-) diff --git a/src/tui/app/machine/match_state.rs b/src/tui/app/machine/match_state.rs index 8d7dc6d..7c2f202 100644 --- a/src/tui/app/machine/match_state.rs +++ b/src/tui/app/machine/match_state.rs @@ -330,9 +330,12 @@ mod tests { machine::tests::{inner, inner_with_mb, input_event, music_hoard}, IApp, IAppAccess, IAppInput, }, - lib::interface::musicbrainz::{ - api::Entity, - daemon::{MbParams, MockIMbJobSender}, + lib::{ + interface::musicbrainz::{ + api::Entity, + daemon::{MbParams, MockIMbJobSender}, + }, + MockIMusicHoard, }, }; @@ -660,14 +663,34 @@ mod tests { // (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)); + let removed = Album::new(AlbumId::new("Album: Not the Same").with_mb_ref(mb_ref.clone())); artist.albums.push(removed.clone()); + // (5) An album after the one that will be removed. Selection must remain on it. + let selected = Album::new(AlbumId::new("Album: Z")); + artist.albums.push(selected.clone()); + let (_tx, rx) = mpsc::channel(); let app_matches = MatchState::new(matches_info.clone(), FetchState::search(rx)); let collection = vec![artist]; - let mut music_hoard = music_hoard(collection.clone()); + let mut reduced = collection.clone(); + reduced[0].albums.retain(|a| a.meta.id.mb_ref != mb_ref); + + // We need a custom music_hoard to override the get_filtered expectations. + let mut music_hoard = MockIMusicHoard::new(); + let mut seq = Sequence::new(); + music_hoard + .expect_get_filtered() + .times(2) + .in_sequence(&mut seq) + .return_const(collection.clone()); + music_hoard + .expect_get_filtered() + .times(1) + .in_sequence(&mut seq) + .return_const(reduced.clone()); + match matches_info { EntityMatches::Artist(_) => panic!(), EntityMatches::Album(matches) => { @@ -678,7 +701,7 @@ mod tests { .expect_get_collection() .times(1) .in_sequence(&mut seq) - .return_const(collection); + .return_const(collection.clone()); music_hoard .expect_remove_album() .times(1) @@ -698,8 +721,26 @@ mod tests { } } - let matches = AppMachine::match_state(inner(music_hoard), app_matches); - matches.select().unwrap_fetch(); + let mut matches = AppMachine::match_state(inner(music_hoard), app_matches); + + // Make sure the last album is selected. + let selection = &mut matches.inner.selection; + selection.increment_category(); + selection.increment_selection(&collection, Delta::Line); + selection.increment_selection(&collection, Delta::Line); + + // Check that the correct album is selected. + let state = selection.state_album(&collection).unwrap(); + let album = &collection[0].albums[state.index]; + assert_eq!(album.meta, selected.meta); + + let mut fetch = matches.select().unwrap_fetch(); + + // Check that the correct album is still selected in the reduced collection. + let selection = &mut fetch.inner.selection; + let state = selection.state_album(&reduced).unwrap(); + let album = &reduced[0].albums[state.index]; + assert_eq!(album.meta, selected.meta); } #[test] -- 2.47.1