From 96b0d47ce4a7ada20eedca60885635a7bb5e220a Mon Sep 17 00:00:00 2001 From: Wojciech Kozlowski Date: Sun, 29 Sep 2024 11:29:17 +0200 Subject: [PATCH] Remove empty matches --- src/tui/app/machine/fetch_state.rs | 23 ++--- src/tui/app/machine/match_state.rs | 161 +++++++++++------------------ src/tui/app/machine/mod.rs | 16 ++- src/tui/app/mod.rs | 2 +- src/tui/app/selection/mod.rs | 8 ++ src/tui/ui/display.rs | 15 +-- src/tui/ui/match_state.rs | 17 +-- src/tui/ui/mod.rs | 31 +----- 8 files changed, 100 insertions(+), 173 deletions(-) diff --git a/src/tui/app/machine/fetch_state.rs b/src/tui/app/machine/fetch_state.rs index a8efb7a..74c9b2c 100644 --- a/src/tui/app/machine/fetch_state.rs +++ b/src/tui/app/machine/fetch_state.rs @@ -63,10 +63,10 @@ impl AppMachine { } pub fn app_fetch_first(inner: AppInner) -> App { - Self::app_fetch_new(inner, true) + Self::app_fetch_new(inner) } - fn app_fetch_new(inner: AppInner, first: bool) -> App { + fn app_fetch_new(inner: AppInner) -> App { let coll = inner.music_hoard.get_collection(); let artist = match inner.selection.state_artist(coll) { Some(artist_state) => &coll[artist_state.index], @@ -79,13 +79,7 @@ impl AppMachine { let fetch = FetchState::new(fetch_rx); match Self::submit_fetch_job(&*inner.musicbrainz, fetch_tx, artist) { Ok(()) => AppMachine::fetch_state(inner, fetch).into(), - Err(FetchError::NothingToFetch) => { - if first { - AppMachine::match_state(inner, MatchState::new(None, fetch)).into() - } else { - AppMachine::browse_state(inner).into() - } - } + Err(FetchError::NothingToFetch) => AppMachine::browse_state(inner).into(), Err(FetchError::SubmitError(daemon_err)) => { AppMachine::error_state(inner, daemon_err.to_string()).into() } @@ -96,8 +90,7 @@ impl AppMachine { match fetch.try_recv() { Ok(fetch_result) => match fetch_result { Ok(next_match) => { - let current = Some(next_match); - AppMachine::match_state(inner, MatchState::new(current, fetch)).into() + AppMachine::match_state(inner, MatchState::new(next_match, fetch)).into() } Err(fetch_err) => { AppMachine::error_state(inner, format!("fetch failed: {fetch_err}")).into() @@ -105,7 +98,7 @@ impl AppMachine { }, Err(recv_err) => match recv_err { TryRecvError::Empty => AppMachine::fetch_state(inner, fetch).into(), - TryRecvError::Disconnected => Self::app_fetch_new(inner, false), + TryRecvError::Disconnected => Self::app_fetch_new(inner), }, } } @@ -452,7 +445,7 @@ mod tests { let app = browse.increment_selection(Delta::Line); let app = app.unwrap_browse().fetch_musicbrainz(); - assert!(matches!(app, AppState::Match(_))); + assert!(matches!(app, AppState::Browse(_))); } #[test] @@ -515,7 +508,7 @@ mod tests { MatchOption::ManualInputMbid, ]; let expected = MatchStateInfo::artist_search(artist, match_options); - assert_eq!(match_state.info, Some(expected).as_ref()); + assert_eq!(match_state.info, &expected); } #[test] @@ -547,7 +540,7 @@ mod tests { collection[0].albums.clear(); let app = AppMachine::app_fetch_first(inner(music_hoard(collection))); - assert!(matches!(app, AppState::Match(_))); + assert!(matches!(app, AppState::Browse(_))); } #[test] diff --git a/src/tui/app/machine/match_state.rs b/src/tui/app/machine/match_state.rs index 5db46a0..de68e30 100644 --- a/src/tui/app/machine/match_state.rs +++ b/src/tui/app/machine/match_state.rs @@ -170,19 +170,18 @@ impl MatchStateInfo { } pub struct MatchState { - current: Option, + current: MatchStateInfo, state: WidgetState, fetch: FetchState, } impl MatchState { - pub fn new(mut current: Option, fetch: FetchState) -> Self { - let mut state = WidgetState::default(); - if let Some(ref mut current) = current { - state.list.select(Some(0)); - current.push_cannot_have_mbid(); - current.push_manual_input_mbid(); - } + pub fn new(mut current: MatchStateInfo, fetch: FetchState) -> Self { + current.push_cannot_have_mbid(); + current.push_manual_input_mbid(); + + let state = WidgetState::default().with_selected(Some(0)); + MatchState { current, state, @@ -201,7 +200,7 @@ impl AppMachine { Ok(mbid) => mbid, Err(err) => return AppMachine::error_state(self.inner, err.to_string()).into(), }; - match self.state.current.as_ref().unwrap() { + match self.state.current { MatchStateInfo::Artist(artist_matches) => { let matching = &artist_matches.matching; AppMachine::app_lookup_artist(self.inner, self.state.fetch, matching, mbid) @@ -235,7 +234,7 @@ impl From> for App { impl<'a> From<&'a mut MatchState> for AppPublicState<'a> { fn from(state: &'a mut MatchState) -> Self { AppState::Match(MatchStatePublic { - info: state.current.as_ref().map(Into::into), + info: &state.current, state: &mut state.state, }) } @@ -254,46 +253,41 @@ impl IAppInteractMatch for AppMachine { } fn next_match(mut self) -> Self::APP { - if let Some(index) = self.state.state.list.selected() { - let result = index.saturating_add(1); - let to = cmp::min( - result, - // selected() implies current exists - self.state.current.as_ref().unwrap().len().saturating_sub(1), - ); - self.state.state.list.select(Some(to)); - } + let index = self.state.state.list.selected().unwrap(); + let to = cmp::min( + index.saturating_add(1), + self.state.current.len().saturating_sub(1), + ); + self.state.state.list.select(Some(to)); self.into() } fn select(mut self) -> Self::APP { - if let Some(index) = self.state.state.list.selected() { - // selected() implies current exists + let index = self.state.state.list.selected().unwrap(); - let mh = &mut self.inner.music_hoard; - let result = match self.state.current.as_mut().unwrap() { - MatchStateInfo::Artist(ref mut matches) => { - let info: ArtistInfo = matches.matching.info.clone(); - match matches.list.extract_info(index, info) { - InfoOption::Info(info) => mh.set_artist_info(&matches.matching.id, info), - InfoOption::NeedInput => return self.get_input(), - } + let mh = &mut self.inner.music_hoard; + let result = match self.state.current { + MatchStateInfo::Artist(ref mut matches) => { + let info: ArtistInfo = matches.matching.info.clone(); + match matches.list.extract_info(index, info) { + InfoOption::Info(info) => mh.set_artist_info(&matches.matching.id, info), + InfoOption::NeedInput => return self.get_input(), } - MatchStateInfo::Album(matches) => { - let info: AlbumInfo = matches.matching.info.clone(); - match matches.list.extract_info(index, info) { - InfoOption::Info(info) => { - mh.set_album_info(&matches.artist, &matches.matching.id, info) - } - InfoOption::NeedInput => return self.get_input(), - } - } - }; - - if let Err(err) = result { - return AppMachine::error_state(self.inner, err.to_string()).into(); } + MatchStateInfo::Album(ref mut matches) => { + let info: AlbumInfo = matches.matching.info.clone(); + match matches.list.extract_info(index, info) { + InfoOption::Info(info) => { + mh.set_album_info(&matches.artist, &matches.matching.id, info) + } + InfoOption::NeedInput => return self.get_input(), + } + } + }; + + if let Err(err) = result { + return AppMachine::error_state(self.inner, err.to_string()).into(); } AppMachine::app_fetch_next(self.inner, self.state.fetch) @@ -416,55 +410,35 @@ mod tests { FetchState::new(rx) } - fn match_state(match_state_info: Option) -> MatchState { + fn match_state(match_state_info: MatchStateInfo) -> MatchState { MatchState::new(match_state_info, fetch_state()) } #[test] - fn create_empty() { - let matches = AppMachine::match_state(inner(music_hoard(vec![])), match_state(None)); - - let widget_state = WidgetState::default(); - - assert_eq!(matches.state.current, None); - assert_eq!(matches.state.state, widget_state); - - let mut app: App = matches.into(); - let public = app.get(); - let public_matches = public.state.unwrap_match(); - - assert_eq!(public_matches.info, None); - assert_eq!(public_matches.state, &widget_state); - } - - #[test] - fn create_nonempty() { + fn create() { let mut album_match = album_match(); - let matches = AppMachine::match_state( - inner(music_hoard(vec![])), - match_state(Some(album_match.clone())), - ); + let matches = + AppMachine::match_state(inner(music_hoard(vec![])), match_state(album_match.clone())); album_match.push_cannot_have_mbid(); album_match.push_manual_input_mbid(); - let mut widget_state = WidgetState::default(); - widget_state.list.select(Some(0)); + let widget_state = WidgetState::default().with_selected(Some(0)); - assert_eq!(matches.state.current.as_ref(), Some(&album_match)); + assert_eq!(matches.state.current, album_match); assert_eq!(matches.state.state, widget_state); let mut app: App = matches.into(); let public = app.get(); let public_matches = public.state.unwrap_match(); - assert_eq!(public_matches.info, Some(&album_match)); + assert_eq!(public_matches.info, &album_match); assert_eq!(public_matches.state, &widget_state); } fn match_state_flow(mut matches_info: MatchStateInfo, len: usize) { // tx must exist for rx to return Empty rather than Disconnected. let (_tx, rx) = mpsc::channel(); - let app_matches = MatchState::new(Some(matches_info.clone()), FetchState::new(rx)); + let app_matches = MatchState::new(matches_info.clone(), FetchState::new(rx)); let mut music_hoard = music_hoard(vec![]); let artist_id = ArtistId::new("Artist"); @@ -494,40 +468,39 @@ mod tests { matches_info.push_cannot_have_mbid(); matches_info.push_manual_input_mbid(); - let mut widget_state = WidgetState::default(); - widget_state.list.select(Some(0)); + let widget_state = WidgetState::default().with_selected(Some(0)); - assert_eq!(matches.state.current.as_ref(), Some(&matches_info)); + assert_eq!(matches.state.current, matches_info); assert_eq!(matches.state.state, widget_state); let matches = matches.prev_match().unwrap_match(); - assert_eq!(matches.state.current.as_ref(), Some(&matches_info)); + assert_eq!(matches.state.current, matches_info); assert_eq!(matches.state.state.list.selected(), Some(0)); let mut matches = matches; for ii in 1..len { matches = matches.next_match().unwrap_match(); - assert_eq!(matches.state.current.as_ref(), Some(&matches_info)); + assert_eq!(matches.state.current, matches_info); assert_eq!(matches.state.state.list.selected(), Some(ii)); } // Next is CannotHaveMBID let matches = matches.next_match().unwrap_match(); - assert_eq!(matches.state.current.as_ref(), Some(&matches_info)); + assert_eq!(matches.state.current, matches_info); assert_eq!(matches.state.state.list.selected(), Some(len)); // Next is ManualInputMbid let matches = matches.next_match().unwrap_match(); - assert_eq!(matches.state.current.as_ref(), Some(&matches_info)); + assert_eq!(matches.state.current, matches_info); assert_eq!(matches.state.state.list.selected(), Some(len + 1)); let matches = matches.next_match().unwrap_match(); - assert_eq!(matches.state.current.as_ref(), Some(&matches_info)); + assert_eq!(matches.state.current, matches_info); assert_eq!(matches.state.state.list.selected(), Some(len + 1)); // Go prev_match first as selecting on manual input does not go back to fetch. @@ -560,7 +533,7 @@ mod tests { let matches_info = artist_match(); let (_tx, rx) = mpsc::channel(); - let app_matches = MatchState::new(Some(matches_info.clone()), FetchState::new(rx)); + let app_matches = MatchState::new(matches_info.clone(), FetchState::new(rx)); let mut music_hoard = music_hoard(vec![]); match matches_info { @@ -584,7 +557,7 @@ mod tests { let matches_info = album_match(); let (_tx, rx) = mpsc::channel(); - let app_matches = MatchState::new(Some(matches_info.clone()), FetchState::new(rx)); + let app_matches = MatchState::new(matches_info.clone(), FetchState::new(rx)); let mut music_hoard = music_hoard(vec![]); match matches_info { @@ -608,7 +581,7 @@ mod tests { let matches_info = artist_match(); let (_tx, rx) = mpsc::channel(); - let app_matches = MatchState::new(Some(matches_info.clone()), FetchState::new(rx)); + let app_matches = MatchState::new(matches_info.clone(), FetchState::new(rx)); let mut music_hoard = music_hoard(vec![]); match matches_info { @@ -627,35 +600,23 @@ mod tests { #[test] fn abort() { let mut album_match = album_match(); - let matches = AppMachine::match_state( - inner(music_hoard(vec![])), - match_state(Some(album_match.clone())), - ); + let matches = + AppMachine::match_state(inner(music_hoard(vec![])), match_state(album_match.clone())); album_match.push_cannot_have_mbid(); album_match.push_manual_input_mbid(); - let mut widget_state = WidgetState::default(); - widget_state.list.select(Some(0)); + let widget_state = WidgetState::default().with_selected(Some(0)); - assert_eq!(matches.state.current.as_ref(), Some(&album_match)); + assert_eq!(matches.state.current, album_match); assert_eq!(matches.state.state, widget_state); matches.abort().unwrap_browse(); } - #[test] - fn select_empty() { - // This test will become obsolete with #203 so it just needs to work well enough for - // coverage. We expect the error state, because after selecting, fetch will be invoked, but - // with an empty collection, an error will be raised. - let matches = AppMachine::match_state(inner(music_hoard(vec![])), match_state(None)); - matches.select().unwrap_error(); - } - #[test] fn select_manual_input_empty() { let matches = - AppMachine::match_state(inner(music_hoard(vec![])), match_state(Some(album_match()))); + AppMachine::match_state(inner(music_hoard(vec![])), match_state(album_match())); // album_match has two matches which means that the fourth option should be manual input. let matches = matches.next_match().unwrap_match(); @@ -692,7 +653,7 @@ mod tests { let artist_match = MatchStateInfo::artist_search(artist.clone(), matches_vec); let matches = AppMachine::match_state( inner_with_mb(music_hoard(vec![]), mb_job_sender), - match_state(Some(artist_match)), + match_state(artist_match), ); // There are no matches which means that the second option should be manual input. @@ -726,7 +687,7 @@ mod tests { MatchStateInfo::album_search(artist_id.clone(), album.clone(), matches_vec); let matches = AppMachine::match_state( inner_with_mb(music_hoard(vec![]), mb_job_sender), - match_state(Some(album_match)), + match_state(album_match), ); // There are no matches which means that the second option should be manual input. diff --git a/src/tui/app/machine/mod.rs b/src/tui/app/machine/mod.rs index 87dfea8..9873c25 100644 --- a/src/tui/app/machine/mod.rs +++ b/src/tui/app/machine/mod.rs @@ -219,11 +219,17 @@ where mod tests { use std::sync::mpsc; - use musichoard::collection::Collection; + use musichoard::collection::{ + artist::{ArtistId, ArtistMeta}, + Collection, + }; use crate::tui::{ - app::{AppState, IApp, IAppInput, IAppInteractBrowse, InputEvent}, - lib::{interface::musicbrainz::daemon::MockIMbJobSender, MockIMusicHoard}, + app::{AppState, IApp, IAppInput, IAppInteractBrowse, InputEvent, MatchStateInfo}, + lib::{ + interface::musicbrainz::{api::Lookup, daemon::MockIMbJobSender}, + MockIMusicHoard, + }, }; use super::*; @@ -513,8 +519,10 @@ mod tests { let (_, rx) = mpsc::channel(); let fetch = FetchState::new(rx); + let artist = ArtistMeta::new(ArtistId::new("Artist")); + let info = MatchStateInfo::artist_lookup(artist.clone(), Lookup::new(artist.clone())); app = - AppMachine::match_state(app.unwrap_browse().inner, MatchState::new(None, fetch)).into(); + AppMachine::match_state(app.unwrap_browse().inner, MatchState::new(info, fetch)).into(); let state = app.state(); assert!(matches!(state, AppState::Match(_))); diff --git a/src/tui/app/mod.rs b/src/tui/app/mod.rs index 98ef76a..a44cc28 100644 --- a/src/tui/app/mod.rs +++ b/src/tui/app/mod.rs @@ -263,7 +263,7 @@ impl MatchStateInfo { } pub struct MatchStatePublic<'app> { - pub info: Option<&'app MatchStateInfo>, + pub info: &'app MatchStateInfo, pub state: &'app mut WidgetState, } diff --git a/src/tui/app/selection/mod.rs b/src/tui/app/selection/mod.rs index 74cfb9c..da31713 100644 --- a/src/tui/app/selection/mod.rs +++ b/src/tui/app/selection/mod.rs @@ -36,6 +36,14 @@ impl PartialEq for WidgetState { } } +impl WidgetState { + #[must_use] + pub const fn with_selected(mut self, selected: Option) -> Self { + self.list = self.list.with_selected(selected); + self + } +} + pub enum Delta { Line, Page, diff --git a/src/tui/ui/display.rs b/src/tui/ui/display.rs index 511217c..30c3950 100644 --- a/src/tui/ui/display.rs +++ b/src/tui/ui/display.rs @@ -122,17 +122,10 @@ impl UiDisplay { ) } - pub fn display_nothing_matching() -> &'static str { - "Matching nothing" - } - - pub fn display_matching_info(info: Option<&MatchStateInfo>) -> String { - match info.as_ref() { - Some(kind) => match kind { - MatchStateInfo::Artist(m) => UiDisplay::display_artist_matching(&m.matching), - MatchStateInfo::Album(m) => UiDisplay::display_album_matching(&m.matching), - }, - None => UiDisplay::display_nothing_matching().to_string(), + pub fn display_matching_info(info: &MatchStateInfo) -> String { + match info { + MatchStateInfo::Artist(m) => UiDisplay::display_artist_matching(&m.matching), + MatchStateInfo::Album(m) => UiDisplay::display_album_matching(&m.matching), } } diff --git a/src/tui/ui/match_state.rs b/src/tui/ui/match_state.rs index 2b6528c..bf611fe 100644 --- a/src/tui/ui/match_state.rs +++ b/src/tui/ui/match_state.rs @@ -13,21 +13,10 @@ pub struct MatchOverlay<'a, 'b> { } impl<'a, 'b> MatchOverlay<'a, 'b> { - pub fn new(info: Option<&'a MatchStateInfo>, state: &'b mut WidgetState) -> Self { + pub fn new(info: &'a MatchStateInfo, state: &'b mut WidgetState) -> Self { match info { - Some(info) => match info { - MatchStateInfo::Artist(m) => Self::artists(&m.matching, &m.list, state), - MatchStateInfo::Album(m) => Self::albums(&m.matching, &m.list, state), - }, - None => Self::empty(state), - } - } - - fn empty(state: &'b mut WidgetState) -> Self { - MatchOverlay { - matching: UiDisplay::display_nothing_matching().to_string(), - list: List::default(), - state, + MatchStateInfo::Artist(m) => Self::artists(&m.matching, &m.list, state), + MatchStateInfo::Album(m) => Self::albums(&m.matching, &m.list, state), } } diff --git a/src/tui/ui/mod.rs b/src/tui/ui/mod.rs index d0f13fd..5cf579c 100644 --- a/src/tui/ui/mod.rs +++ b/src/tui/ui/mod.rs @@ -140,11 +140,7 @@ impl Ui { UiWidget::render_overlay_widget("Fetching", fetch_text, area, false, frame) } - fn render_match_overlay( - info: Option<&MatchStateInfo>, - state: &mut WidgetState, - frame: &mut Frame, - ) { + fn render_match_overlay(info: &MatchStateInfo, state: &mut WidgetState, frame: &mut Frame) { let area = OverlayBuilder::default().build(frame.area()); let st = MatchOverlay::new(info, state); UiWidget::render_overlay_list_widget(&st.matching, st.list, st.state, true, area, frame) @@ -329,26 +325,6 @@ mod tests { draw_test_suite(artists, &mut selection); } - #[test] - fn draw_empty_matches() { - let collection = &COLLECTION; - let mut selection = Selection::new(collection); - - let mut terminal = terminal(); - - let mut widget_state = WidgetState::default(); - - let mut app = AppPublic { - inner: public_inner(collection, &mut selection), - state: AppState::Match(MatchStatePublic { - info: None, - state: &mut widget_state, - }), - input: None, - }; - terminal.draw(|frame| Ui::render(&mut app, frame)).unwrap(); - } - fn artist_meta() -> ArtistMeta { ArtistMeta::new(ArtistId::new("an artist")) } @@ -428,13 +404,12 @@ mod tests { ]; for info in match_state_infos.iter() { - let mut widget_state = WidgetState::default(); - widget_state.list.select(Some(0)); + let mut widget_state = WidgetState::default().with_selected(Some(0)); let mut app = AppPublic { inner: public_inner(collection, &mut selection), state: AppState::Match(MatchStatePublic { - info: Some(info), + info, state: &mut widget_state, }), input: None, -- 2.45.2