Gracefully handle case of nothing being there to match (#222)
All checks were successful
Cargo CI / Build and Test (push) Successful in 1m59s
Cargo CI / Lint (push) Successful in 1m7s

Closes #203

Reviewed-on: #222
This commit is contained in:
Wojciech Kozlowski 2024-09-29 11:33:38 +02:00
parent 0b0599318e
commit e22068e461
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 {
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<FetchState> {
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<FetchState> {
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<FetchState> {
},
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]

View File

@ -170,19 +170,18 @@ impl MatchStateInfo {
}
pub struct MatchState {
current: Option<MatchStateInfo>,
current: MatchStateInfo,
state: WidgetState,
fetch: FetchState,
}
impl MatchState {
pub fn new(mut current: Option<MatchStateInfo>, fetch: FetchState) -> Self {
let mut state = WidgetState::default();
if let Some(ref mut current) = current {
state.list.select(Some(0));
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<MatchState> {
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<AppMachine<MatchState>> 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,25 +253,21 @@ impl IAppInteractMatch for AppMachine<MatchState> {
}
fn next_match(mut self) -> Self::APP {
if let Some(index) = self.state.state.list.selected() {
let result = index.saturating_add(1);
let index = self.state.state.list.selected().unwrap();
let to = cmp::min(
result,
// selected() implies current exists
self.state.current.as_ref().unwrap().len().saturating_sub(1),
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() {
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) {
@ -280,7 +275,7 @@ impl IAppInteractMatch for AppMachine<MatchState> {
InfoOption::NeedInput => return self.get_input(),
}
}
MatchStateInfo::Album(matches) => {
MatchStateInfo::Album(ref mut matches) => {
let info: AlbumInfo = matches.matching.info.clone();
match matches.list.extract_info(index, info) {
InfoOption::Info(info) => {
@ -294,7 +289,6 @@ impl IAppInteractMatch for AppMachine<MatchState> {
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<MatchStateInfo>) -> 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.

View File

@ -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(_)));

View File

@ -263,7 +263,7 @@ impl MatchStateInfo {
}
pub struct MatchStatePublic<'app> {
pub info: Option<&'app MatchStateInfo>,
pub info: &'app MatchStateInfo,
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 {
Line,
Page,

View File

@ -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 {
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),
},
None => UiDisplay::display_nothing_matching().to_string(),
}
}

View File

@ -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,
}
}

View File

@ -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,