Gracefully handle case of nothing being there to match #222

Merged
wojtek merged 1 commits from 203---gracefully-handle-case-of-nothing-being-there-to-match into main 2024-09-29 11:33:38 +02:00
8 changed files with 100 additions and 173 deletions

View File

@ -63,10 +63,10 @@ impl AppMachine<FetchState> {
} }
pub fn app_fetch_first(inner: AppInner) -> App { 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 coll = inner.music_hoard.get_collection();
let artist = match inner.selection.state_artist(coll) { let artist = match inner.selection.state_artist(coll) {
Some(artist_state) => &coll[artist_state.index], Some(artist_state) => &coll[artist_state.index],
@ -79,13 +79,7 @@ impl AppMachine<FetchState> {
let fetch = FetchState::new(fetch_rx); let fetch = FetchState::new(fetch_rx);
match Self::submit_fetch_job(&*inner.musicbrainz, fetch_tx, artist) { match Self::submit_fetch_job(&*inner.musicbrainz, fetch_tx, artist) {
Ok(()) => AppMachine::fetch_state(inner, fetch).into(), Ok(()) => AppMachine::fetch_state(inner, fetch).into(),
Err(FetchError::NothingToFetch) => { Err(FetchError::NothingToFetch) => AppMachine::browse_state(inner).into(),
if first {
AppMachine::match_state(inner, MatchState::new(None, fetch)).into()
} else {
AppMachine::browse_state(inner).into()
}
}
Err(FetchError::SubmitError(daemon_err)) => { Err(FetchError::SubmitError(daemon_err)) => {
AppMachine::error_state(inner, daemon_err.to_string()).into() AppMachine::error_state(inner, daemon_err.to_string()).into()
} }
@ -96,8 +90,7 @@ impl AppMachine<FetchState> {
match fetch.try_recv() { match fetch.try_recv() {
Ok(fetch_result) => match fetch_result { Ok(fetch_result) => match fetch_result {
Ok(next_match) => { Ok(next_match) => {
let current = Some(next_match); AppMachine::match_state(inner, MatchState::new(next_match, fetch)).into()
AppMachine::match_state(inner, MatchState::new(current, fetch)).into()
} }
Err(fetch_err) => { Err(fetch_err) => {
AppMachine::error_state(inner, format!("fetch failed: {fetch_err}")).into() AppMachine::error_state(inner, format!("fetch failed: {fetch_err}")).into()
@ -105,7 +98,7 @@ impl AppMachine<FetchState> {
}, },
Err(recv_err) => match recv_err { Err(recv_err) => match recv_err {
TryRecvError::Empty => AppMachine::fetch_state(inner, fetch).into(), 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 = browse.increment_selection(Delta::Line);
let app = app.unwrap_browse().fetch_musicbrainz(); let app = app.unwrap_browse().fetch_musicbrainz();
assert!(matches!(app, AppState::Match(_))); assert!(matches!(app, AppState::Browse(_)));
} }
#[test] #[test]
@ -515,7 +508,7 @@ mod tests {
MatchOption::ManualInputMbid, MatchOption::ManualInputMbid,
]; ];
let expected = MatchStateInfo::artist_search(artist, match_options); let expected = MatchStateInfo::artist_search(artist, match_options);
assert_eq!(match_state.info, Some(expected).as_ref()); assert_eq!(match_state.info, &expected);
} }
#[test] #[test]
@ -547,7 +540,7 @@ mod tests {
collection[0].albums.clear(); collection[0].albums.clear();
let app = AppMachine::app_fetch_first(inner(music_hoard(collection))); let app = AppMachine::app_fetch_first(inner(music_hoard(collection)));
assert!(matches!(app, AppState::Match(_))); assert!(matches!(app, AppState::Browse(_)));
} }
#[test] #[test]

View File

@ -170,19 +170,18 @@ impl MatchStateInfo {
} }
pub struct MatchState { pub struct MatchState {
current: Option<MatchStateInfo>, current: MatchStateInfo,
state: WidgetState, state: WidgetState,
fetch: FetchState, fetch: FetchState,
} }
impl MatchState { impl MatchState {
pub fn new(mut current: Option<MatchStateInfo>, fetch: FetchState) -> Self { pub fn new(mut current: MatchStateInfo, 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_cannot_have_mbid();
current.push_manual_input_mbid(); current.push_manual_input_mbid();
}
let state = WidgetState::default().with_selected(Some(0));
MatchState { MatchState {
current, current,
state, state,
@ -201,7 +200,7 @@ impl AppMachine<MatchState> {
Ok(mbid) => mbid, Ok(mbid) => mbid,
Err(err) => return AppMachine::error_state(self.inner, err.to_string()).into(), 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) => { MatchStateInfo::Artist(artist_matches) => {
let matching = &artist_matches.matching; let matching = &artist_matches.matching;
AppMachine::app_lookup_artist(self.inner, self.state.fetch, matching, mbid) AppMachine::app_lookup_artist(self.inner, self.state.fetch, matching, mbid)
@ -235,7 +234,7 @@ impl From<AppMachine<MatchState>> for App {
impl<'a> From<&'a mut MatchState> for AppPublicState<'a> { impl<'a> From<&'a mut MatchState> for AppPublicState<'a> {
fn from(state: &'a mut MatchState) -> Self { fn from(state: &'a mut MatchState) -> Self {
AppState::Match(MatchStatePublic { AppState::Match(MatchStatePublic {
info: state.current.as_ref().map(Into::into), info: &state.current,
state: &mut state.state, state: &mut state.state,
}) })
} }
@ -254,25 +253,21 @@ impl IAppInteractMatch for AppMachine<MatchState> {
} }
fn next_match(mut self) -> Self::APP { fn next_match(mut self) -> Self::APP {
if let Some(index) = self.state.state.list.selected() { let index = self.state.state.list.selected().unwrap();
let result = index.saturating_add(1);
let to = cmp::min( let to = cmp::min(
result, index.saturating_add(1),
// selected() implies current exists self.state.current.len().saturating_sub(1),
self.state.current.as_ref().unwrap().len().saturating_sub(1),
); );
self.state.state.list.select(Some(to)); self.state.state.list.select(Some(to));
}
self.into() self.into()
} }
fn select(mut self) -> Self::APP { fn select(mut self) -> Self::APP {
if let Some(index) = self.state.state.list.selected() { let index = self.state.state.list.selected().unwrap();
// selected() implies current exists
let mh = &mut self.inner.music_hoard; let mh = &mut self.inner.music_hoard;
let result = match self.state.current.as_mut().unwrap() { let result = match self.state.current {
MatchStateInfo::Artist(ref mut matches) => { MatchStateInfo::Artist(ref mut matches) => {
let info: ArtistInfo = matches.matching.info.clone(); let info: ArtistInfo = matches.matching.info.clone();
match matches.list.extract_info(index, info) { match matches.list.extract_info(index, info) {
@ -280,7 +275,7 @@ impl IAppInteractMatch for AppMachine<MatchState> {
InfoOption::NeedInput => return self.get_input(), InfoOption::NeedInput => return self.get_input(),
} }
} }
MatchStateInfo::Album(matches) => { MatchStateInfo::Album(ref mut matches) => {
let info: AlbumInfo = matches.matching.info.clone(); let info: AlbumInfo = matches.matching.info.clone();
match matches.list.extract_info(index, info) { match matches.list.extract_info(index, info) {
InfoOption::Info(info) => { InfoOption::Info(info) => {
@ -294,7 +289,6 @@ impl IAppInteractMatch for AppMachine<MatchState> {
if let Err(err) = result { if let Err(err) = result {
return AppMachine::error_state(self.inner, err.to_string()).into(); return AppMachine::error_state(self.inner, err.to_string()).into();
} }
}
AppMachine::app_fetch_next(self.inner, self.state.fetch) AppMachine::app_fetch_next(self.inner, self.state.fetch)
} }
@ -416,55 +410,35 @@ mod tests {
FetchState::new(rx) FetchState::new(rx)
} }
fn match_state(match_state_info: Option<MatchStateInfo>) -> MatchState { fn match_state(match_state_info: MatchStateInfo) -> MatchState {
MatchState::new(match_state_info, fetch_state()) MatchState::new(match_state_info, fetch_state())
} }
#[test] #[test]
fn create_empty() { fn create() {
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() {
let mut album_match = album_match(); let mut album_match = album_match();
let matches = AppMachine::match_state( let matches =
inner(music_hoard(vec![])), AppMachine::match_state(inner(music_hoard(vec![])), match_state(album_match.clone()));
match_state(Some(album_match.clone())),
);
album_match.push_cannot_have_mbid(); album_match.push_cannot_have_mbid();
album_match.push_manual_input_mbid(); album_match.push_manual_input_mbid();
let mut widget_state = WidgetState::default(); let widget_state = WidgetState::default().with_selected(Some(0));
widget_state.list.select(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); assert_eq!(matches.state.state, widget_state);
let mut app: App = matches.into(); let mut app: App = matches.into();
let public = app.get(); let public = app.get();
let public_matches = public.state.unwrap_match(); 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); assert_eq!(public_matches.state, &widget_state);
} }
fn match_state_flow(mut matches_info: MatchStateInfo, len: usize) { fn match_state_flow(mut matches_info: MatchStateInfo, len: usize) {
// tx must exist for rx to return Empty rather than Disconnected. // tx must exist for rx to return Empty rather than Disconnected.
let (_tx, rx) = mpsc::channel(); 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 mut music_hoard = music_hoard(vec![]);
let artist_id = ArtistId::new("Artist"); let artist_id = ArtistId::new("Artist");
@ -494,40 +468,39 @@ mod tests {
matches_info.push_cannot_have_mbid(); matches_info.push_cannot_have_mbid();
matches_info.push_manual_input_mbid(); matches_info.push_manual_input_mbid();
let mut widget_state = WidgetState::default(); let widget_state = WidgetState::default().with_selected(Some(0));
widget_state.list.select(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); assert_eq!(matches.state.state, widget_state);
let matches = matches.prev_match().unwrap_match(); 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)); assert_eq!(matches.state.state.list.selected(), Some(0));
let mut matches = matches; let mut matches = matches;
for ii in 1..len { for ii in 1..len {
matches = matches.next_match().unwrap_match(); 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)); assert_eq!(matches.state.state.list.selected(), Some(ii));
} }
// Next is CannotHaveMBID // Next is CannotHaveMBID
let matches = matches.next_match().unwrap_match(); 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)); assert_eq!(matches.state.state.list.selected(), Some(len));
// Next is ManualInputMbid // Next is ManualInputMbid
let matches = matches.next_match().unwrap_match(); 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)); assert_eq!(matches.state.state.list.selected(), Some(len + 1));
let matches = matches.next_match().unwrap_match(); 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)); 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. // 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 matches_info = artist_match();
let (_tx, rx) = mpsc::channel(); 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 mut music_hoard = music_hoard(vec![]);
match matches_info { match matches_info {
@ -584,7 +557,7 @@ mod tests {
let matches_info = album_match(); let matches_info = album_match();
let (_tx, rx) = mpsc::channel(); 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 mut music_hoard = music_hoard(vec![]);
match matches_info { match matches_info {
@ -608,7 +581,7 @@ mod tests {
let matches_info = artist_match(); let matches_info = artist_match();
let (_tx, rx) = mpsc::channel(); 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 mut music_hoard = music_hoard(vec![]);
match matches_info { match matches_info {
@ -627,35 +600,23 @@ mod tests {
#[test] #[test]
fn abort() { fn abort() {
let mut album_match = album_match(); let mut album_match = album_match();
let matches = AppMachine::match_state( let matches =
inner(music_hoard(vec![])), AppMachine::match_state(inner(music_hoard(vec![])), match_state(album_match.clone()));
match_state(Some(album_match.clone())),
);
album_match.push_cannot_have_mbid(); album_match.push_cannot_have_mbid();
album_match.push_manual_input_mbid(); album_match.push_manual_input_mbid();
let mut widget_state = WidgetState::default(); let widget_state = WidgetState::default().with_selected(Some(0));
widget_state.list.select(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); assert_eq!(matches.state.state, widget_state);
matches.abort().unwrap_browse(); 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] #[test]
fn select_manual_input_empty() { fn select_manual_input_empty() {
let matches = 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. // album_match has two matches which means that the fourth option should be manual input.
let matches = matches.next_match().unwrap_match(); let matches = matches.next_match().unwrap_match();
@ -692,7 +653,7 @@ mod tests {
let artist_match = MatchStateInfo::artist_search(artist.clone(), matches_vec); let artist_match = MatchStateInfo::artist_search(artist.clone(), matches_vec);
let matches = AppMachine::match_state( let matches = AppMachine::match_state(
inner_with_mb(music_hoard(vec![]), mb_job_sender), 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. // 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); MatchStateInfo::album_search(artist_id.clone(), album.clone(), matches_vec);
let matches = AppMachine::match_state( let matches = AppMachine::match_state(
inner_with_mb(music_hoard(vec![]), mb_job_sender), 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. // There are no matches which means that the second option should be manual input.

View File

@ -219,11 +219,17 @@ where
mod tests { mod tests {
use std::sync::mpsc; use std::sync::mpsc;
use musichoard::collection::Collection; use musichoard::collection::{
artist::{ArtistId, ArtistMeta},
Collection,
};
use crate::tui::{ use crate::tui::{
app::{AppState, IApp, IAppInput, IAppInteractBrowse, InputEvent}, app::{AppState, IApp, IAppInput, IAppInteractBrowse, InputEvent, MatchStateInfo},
lib::{interface::musicbrainz::daemon::MockIMbJobSender, MockIMusicHoard}, lib::{
interface::musicbrainz::{api::Lookup, daemon::MockIMbJobSender},
MockIMusicHoard,
},
}; };
use super::*; use super::*;
@ -513,8 +519,10 @@ mod tests {
let (_, rx) = mpsc::channel(); let (_, rx) = mpsc::channel();
let fetch = FetchState::new(rx); let fetch = FetchState::new(rx);
let artist = ArtistMeta::new(ArtistId::new("Artist"));
let info = MatchStateInfo::artist_lookup(artist.clone(), Lookup::new(artist.clone()));
app = 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(); let state = app.state();
assert!(matches!(state, AppState::Match(_))); assert!(matches!(state, AppState::Match(_)));

View File

@ -263,7 +263,7 @@ impl MatchStateInfo {
} }
pub struct MatchStatePublic<'app> { pub struct MatchStatePublic<'app> {
pub info: Option<&'app MatchStateInfo>, pub info: &'app MatchStateInfo,
pub state: &'app mut WidgetState, pub state: &'app mut WidgetState,
} }

View File

@ -36,6 +36,14 @@ impl PartialEq for WidgetState {
} }
} }
impl WidgetState {
#[must_use]
pub const fn with_selected(mut self, selected: Option<usize>) -> Self {
self.list = self.list.with_selected(selected);
self
}
}
pub enum Delta { pub enum Delta {
Line, Line,
Page, Page,

View File

@ -122,17 +122,10 @@ impl UiDisplay {
) )
} }
pub fn display_nothing_matching() -> &'static str { pub fn display_matching_info(info: &MatchStateInfo) -> String {
"Matching nothing" match info {
}
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::Artist(m) => UiDisplay::display_artist_matching(&m.matching),
MatchStateInfo::Album(m) => UiDisplay::display_album_matching(&m.matching), MatchStateInfo::Album(m) => UiDisplay::display_album_matching(&m.matching),
},
None => UiDisplay::display_nothing_matching().to_string(),
} }
} }

View File

@ -13,21 +13,10 @@ pub struct MatchOverlay<'a, 'b> {
} }
impl<'a, 'b> 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 { match info {
Some(info) => match info {
MatchStateInfo::Artist(m) => Self::artists(&m.matching, &m.list, state), MatchStateInfo::Artist(m) => Self::artists(&m.matching, &m.list, state),
MatchStateInfo::Album(m) => Self::albums(&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,
} }
} }

View File

@ -140,11 +140,7 @@ impl Ui {
UiWidget::render_overlay_widget("Fetching", fetch_text, area, false, frame) UiWidget::render_overlay_widget("Fetching", fetch_text, area, false, frame)
} }
fn render_match_overlay( fn render_match_overlay(info: &MatchStateInfo, state: &mut WidgetState, frame: &mut Frame) {
info: Option<&MatchStateInfo>,
state: &mut WidgetState,
frame: &mut Frame,
) {
let area = OverlayBuilder::default().build(frame.area()); let area = OverlayBuilder::default().build(frame.area());
let st = MatchOverlay::new(info, state); let st = MatchOverlay::new(info, state);
UiWidget::render_overlay_list_widget(&st.matching, st.list, st.state, true, area, frame) 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); 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 { fn artist_meta() -> ArtistMeta {
ArtistMeta::new(ArtistId::new("an artist")) ArtistMeta::new(ArtistId::new("an artist"))
} }
@ -428,13 +404,12 @@ mod tests {
]; ];
for info in match_state_infos.iter() { for info in match_state_infos.iter() {
let mut widget_state = WidgetState::default(); let mut widget_state = WidgetState::default().with_selected(Some(0));
widget_state.list.select(Some(0));
let mut app = AppPublic { let mut app = AppPublic {
inner: public_inner(collection, &mut selection), inner: public_inner(collection, &mut selection),
state: AppState::Match(MatchStatePublic { state: AppState::Match(MatchStatePublic {
info: Some(info), info,
state: &mut widget_state, state: &mut widget_state,
}), }),
input: None, input: None,