From acf03b1cd4f3fed3e9397f5b0a201f754e54e09b Mon Sep 17 00:00:00 2001 From: Wojciech Kozlowski Date: Sat, 31 Aug 2024 14:29:00 +0200 Subject: [PATCH 1/2] Replace matches vec with a channel --- src/tui/app/machine/browse.rs | 71 ++++++--------- src/tui/app/machine/matches.rs | 161 ++++++++++++++++----------------- src/tui/app/machine/mod.rs | 5 +- src/tui/app/mod.rs | 16 ---- 4 files changed, 110 insertions(+), 143 deletions(-) diff --git a/src/tui/app/machine/browse.rs b/src/tui/app/machine/browse.rs index 9d5f97e..7ebebba 100644 --- a/src/tui/app/machine/browse.rs +++ b/src/tui/app/machine/browse.rs @@ -1,4 +1,4 @@ -use std::{thread, time}; +use std::{sync::mpsc, thread, time}; use musichoard::collection::musicbrainz::IMusicBrainzRef; @@ -91,7 +91,7 @@ impl IAppInteractBrowse for AppMachine { } }; - let mut matches = vec![]; + let (matches_tx, matches_rx) = mpsc::channel::(); match artist.musicbrainz { Some(ref mbid) => { @@ -104,7 +104,9 @@ impl IAppInteractBrowse for AppMachine { } match self.inner.musicbrainz.search_release_group(arid, album) { - Ok(list) => matches.push(AppMatchesInfo::album(album.clone(), list)), + Ok(list) => matches_tx + .send(AppMatchesInfo::album(album.clone(), list)) + .expect("send fails only if receiver is disconnected"), Err(err) => return AppMachine::error(self.inner, err.to_string()).into(), } @@ -114,12 +116,14 @@ impl IAppInteractBrowse for AppMachine { } } None => match self.inner.musicbrainz.search_artist(artist) { - Ok(list) => matches.push(AppMatchesInfo::artist(artist.clone(), list)), + Ok(list) => matches_tx + .send(AppMatchesInfo::artist(artist.clone(), list)) + .expect("send fails only if receiver is disconnected"), Err(err) => return AppMachine::error(self.inner, err.to_string()).into(), }, }; - AppMachine::matches(self.inner, matches).into() + AppMachine::matches(self.inner, matches_rx).into() } fn no_op(self) -> Self::APP { @@ -135,7 +139,8 @@ mod tests { use crate::tui::{ app::{ machine::tests::{inner, inner_with_mb, music_hoard}, - Category, IAppAccess, IAppInteract, IAppInteractMatches, + AppPublicAlbumMatches, AppPublicArtistMatches, AppPublicMatchesInfo, Category, + IAppAccess, IAppInteract, IAppInteractMatches, }, lib::interface::musicbrainz::{self, Match, MockIMusicBrainz}, testmod::COLLECTION, @@ -258,19 +263,11 @@ mod tests { let public_matches = public.state.unwrap_matches(); - assert_eq!( - public_matches - .matches - .as_ref() - .unwrap() - .album_ref() - .matching, - &album_1 - ); - assert_eq!( - public_matches.matches.as_ref().unwrap().album_ref().list, - matches_1.as_slice() - ); + let expected = Some(AppPublicMatchesInfo::Album(AppPublicAlbumMatches { + matching: &album_1, + list: &matches_1, + })); + assert_eq!(public_matches.matches, expected); let mut app = app.unwrap_matches().select(); @@ -279,19 +276,11 @@ mod tests { let public_matches = public.state.unwrap_matches(); - assert_eq!( - public_matches - .matches - .as_ref() - .unwrap() - .album_ref() - .matching, - &album_4 - ); - assert_eq!( - public_matches.matches.as_ref().unwrap().album_ref().list, - matches_4.as_slice() - ); + let expected = Some(AppPublicMatchesInfo::Album(AppPublicAlbumMatches { + matching: &album_4, + list: &matches_4, + })); + assert_eq!(public_matches.matches, expected); let app = app.unwrap_matches().select(); app.unwrap_browse(); @@ -335,19 +324,11 @@ mod tests { let public_matches = public.state.unwrap_matches(); - assert_eq!( - public_matches - .matches - .as_ref() - .unwrap() - .artist_ref() - .matching, - &artist - ); - assert_eq!( - public_matches.matches.as_ref().unwrap().artist_ref().list, - matches.as_slice() - ); + let expected = Some(AppPublicMatchesInfo::Artist(AppPublicArtistMatches { + matching: &artist, + list: &matches, + })); + assert_eq!(public_matches.matches, expected); let app = app.unwrap_matches().select(); app.unwrap_browse(); diff --git a/src/tui/app/machine/matches.rs b/src/tui/app/machine/matches.rs index 6d028e5..d784db2 100644 --- a/src/tui/app/machine/matches.rs +++ b/src/tui/app/machine/matches.rs @@ -1,4 +1,4 @@ -use std::cmp; +use std::{cmp, sync::mpsc}; use musichoard::collection::{album::Album, artist::Artist}; @@ -101,30 +101,20 @@ impl<'app> From<&'app AppMatchesInfo> for AppPublicMatchesInfo<'app> { } pub struct AppMatches { - matches_info_vec: Vec, - index: Option, + matches_rx: mpsc::Receiver, + current: Option, state: WidgetState, } impl AppMachine { - pub fn matches(inner: AppInner, matches_info_vec: Vec) -> Self { - let mut index = None; - let mut state = WidgetState::default(); - if let Some(matches_info) = matches_info_vec.first() { - index = Some(0); - if !matches_info.is_empty() { - state.list.select(Some(0)); - } - } - - AppMachine { - inner, - state: AppMatches { - matches_info_vec, - index, - state, - }, - } + pub fn matches(inner: AppInner, matches_rx: mpsc::Receiver) -> Self { + let mut state = AppMatches { + matches_rx, + current: None, + state: WidgetState::default(), + }; + state.next_matches_info(); + AppMachine { inner, state } } } @@ -136,15 +126,10 @@ impl From> for App { impl<'a> From<&'a mut AppMachine> for AppPublic<'a> { fn from(machine: &'a mut AppMachine) -> Self { - let matches = machine - .state - .index - .map(|index| (&machine.state.matches_info_vec[index]).into()); - AppPublic { inner: (&mut machine.inner).into(), state: AppState::Matches(AppPublicMatches { - matches, + matches: machine.state.current.as_ref().map(Into::into), state: &mut machine.state.state, }), } @@ -155,8 +140,8 @@ impl IAppInteractMatches for AppMachine { type APP = App; fn prev_match(mut self) -> Self::APP { - if let Some(list_index) = self.state.state.list.selected() { - let result = list_index.saturating_sub(1); + if let Some(index) = self.state.state.list.selected() { + let result = index.saturating_sub(1); self.state.state.list.select(Some(result)); } @@ -164,11 +149,14 @@ impl IAppInteractMatches for AppMachine { } fn next_match(mut self) -> Self::APP { - if let Some(list_index) = self.state.state.list.selected() { - let result = list_index.saturating_add(1); + if let Some(index) = self.state.state.list.selected() { + let result = index.saturating_add(1); let to = cmp::min( result, - self.state.matches_info_vec[self.state.index.unwrap()] + self.state + .current + .as_ref() + .expect("selected() implies current exists") .len() .saturating_sub(1), ); @@ -179,18 +167,17 @@ impl IAppInteractMatches for AppMachine { } fn select(mut self) -> Self::APP { - self.state.index = self.state.index.map(|i| i.saturating_add(1)); - self.state.state = WidgetState::default(); - if let Some(index) = self.state.index { - if let Some(matches_info) = self.state.matches_info_vec.get(index) { + self.state.next_matches_info(); + match self.state.current { + Some(ref matches_info) => { + self.state.state = WidgetState::default(); if !matches_info.is_empty() { self.state.state.list.select(Some(0)); } - return self.into(); + self.into() } + None => AppMachine::browse(self.inner).into(), } - - AppMachine::browse(self.inner).into() } fn abort(self) -> Self::APP { @@ -202,8 +189,29 @@ impl IAppInteractMatches for AppMachine { } } +trait IAppInteractMatchesPrivate { + fn next_matches_info(&mut self); +} + +impl IAppInteractMatchesPrivate for AppMatches { + fn next_matches_info(&mut self) { + // FIXME: try_recv might not be appropriate for asynchronous version. + (self.current, self.state) = match self.matches_rx.try_recv() { + Ok(next_match) => { + let mut state = WidgetState::default(); + if !next_match.is_empty() { + state.list.select(Some(0)); + } + (Some(next_match), state) + } + Err(_) => (None, WidgetState::default()), + } + } +} + #[cfg(test)] mod tests { + use mpsc::Receiver; use musichoard::collection::{ album::{AlbumDate, AlbumId, AlbumPrimaryType, AlbumSecondaryType}, artist::ArtistId, @@ -216,15 +224,6 @@ mod tests { use super::*; - impl AppMatchesInfo { - fn album_ref(&self) -> &AppAlbumMatchesInfo { - match self { - Self::Album(a) => a, - Self::Artist(_) => panic!(), - } - } - } - fn artist_matches_info_vec() -> Vec { let artist_1 = Artist::new(ArtistId::new("Artist 1")); @@ -284,14 +283,21 @@ mod tests { vec![matches_info_1, matches_info_2] } + fn receiver(matches_info_vec: Vec) -> Receiver { + let (tx, rx) = mpsc::channel(); + for matches_info in matches_info_vec.into_iter() { + tx.send(matches_info).unwrap(); + } + rx + } + #[test] fn create_empty() { - let matches = AppMachine::matches(inner(music_hoard(vec![])), vec![]); + let matches = AppMachine::matches(inner(music_hoard(vec![])), receiver(vec![])); let widget_state = WidgetState::default(); - assert_eq!(matches.state.matches_info_vec, vec![]); - assert_eq!(matches.state.index, None); + assert_eq!(matches.state.current, None); assert_eq!(matches.state.state, widget_state); let mut app = matches.no_op(); @@ -305,63 +311,55 @@ mod tests { #[test] fn create_nonempty() { let matches_info_vec = album_matches_info_vec(); - let matches = AppMachine::matches(inner(music_hoard(vec![])), matches_info_vec.clone()); + let matches = AppMachine::matches( + inner(music_hoard(vec![])), + receiver(matches_info_vec.clone()), + ); let mut widget_state = WidgetState::default(); widget_state.list.select(Some(0)); - assert_eq!(matches.state.matches_info_vec, matches_info_vec); - assert_eq!(matches.state.index, Some(0)); + assert_eq!(matches.state.current.as_ref(), Some(&matches_info_vec[0])); assert_eq!(matches.state.state, widget_state); let mut app = matches.no_op(); let public = app.get(); let public_matches = public.state.unwrap_matches(); - assert_eq!( - public_matches - .matches - .as_ref() - .unwrap() - .album_ref() - .matching, - &matches_info_vec[0].album_ref().matching - ); - assert_eq!( - public_matches.matches.as_ref().unwrap().album_ref().list, - matches_info_vec[0].album_ref().list.as_slice() - ); + assert_eq!(public_matches.matches, Some((&matches_info_vec[0]).into())); assert_eq!(public_matches.state, &widget_state); } fn matches_flow(matches_info_vec: Vec) { - let matches = AppMachine::matches(inner(music_hoard(vec![])), matches_info_vec.clone()); + let matches = AppMachine::matches( + inner(music_hoard(vec![])), + receiver(matches_info_vec.clone()), + ); let mut widget_state = WidgetState::default(); widget_state.list.select(Some(0)); - assert_eq!(matches.state.matches_info_vec, matches_info_vec); - assert_eq!(matches.state.index, Some(0)); + assert_eq!(matches.state.current.as_ref(), Some(&matches_info_vec[0])); assert_eq!(matches.state.state, widget_state); let matches = matches.prev_match().unwrap_matches(); - assert_eq!(matches.state.index, Some(0)); + assert_eq!(matches.state.current.as_ref(), Some(&matches_info_vec[0])); assert_eq!(matches.state.state.list.selected(), Some(0)); let matches = matches.next_match().unwrap_matches(); - assert_eq!(matches.state.index, Some(0)); + assert_eq!(matches.state.current.as_ref(), Some(&matches_info_vec[0])); assert_eq!(matches.state.state.list.selected(), Some(1)); let matches = matches.next_match().unwrap_matches(); - assert_eq!(matches.state.index, Some(0)); + assert_eq!(matches.state.current.as_ref(), Some(&matches_info_vec[0])); assert_eq!(matches.state.state.list.selected(), Some(1)); let matches = matches.select().unwrap_matches(); - assert_eq!(matches.state.index, Some(1)); + assert_eq!(matches.state.current.as_ref(), Some(&matches_info_vec[1])); assert_eq!(matches.state.state.list.selected(), Some(0)); // And it's done @@ -381,13 +379,15 @@ mod tests { #[test] fn matches_abort() { let matches_info_vec = album_matches_info_vec(); - let matches = AppMachine::matches(inner(music_hoard(vec![])), matches_info_vec.clone()); + let matches = AppMachine::matches( + inner(music_hoard(vec![])), + receiver(matches_info_vec.clone()), + ); let mut widget_state = WidgetState::default(); widget_state.list.select(Some(0)); - assert_eq!(matches.state.matches_info_vec, matches_info_vec); - assert_eq!(matches.state.index, Some(0)); + assert_eq!(matches.state.current.as_ref(), Some(&matches_info_vec[0])); assert_eq!(matches.state.state, widget_state); matches.abort().unwrap_browse(); @@ -395,17 +395,16 @@ mod tests { #[test] fn matches_select_empty() { - let matches = AppMachine::matches(inner(music_hoard(vec![])), vec![]); + let matches = AppMachine::matches(inner(music_hoard(vec![])), receiver(vec![])); - assert_eq!(matches.state.matches_info_vec, vec![]); - assert_eq!(matches.state.index, None); + assert_eq!(matches.state.current, None); matches.select().unwrap_browse(); } #[test] fn no_op() { - let matches = AppMachine::matches(inner(music_hoard(vec![])), vec![]); + let matches = AppMachine::matches(inner(music_hoard(vec![])), receiver(vec![])); let app = matches.no_op(); app.unwrap_matches(); } diff --git a/src/tui/app/machine/mod.rs b/src/tui/app/machine/mod.rs index f138f67..afdf5e3 100644 --- a/src/tui/app/machine/mod.rs +++ b/src/tui/app/machine/mod.rs @@ -143,6 +143,8 @@ impl<'a> From<&'a mut AppInner> for AppPublicInner<'a> { #[cfg(test)] mod tests { + use std::sync::mpsc; + use musichoard::collection::Collection; use crate::tui::{ @@ -311,7 +313,8 @@ mod tests { let mut app = App::new(music_hoard_init(vec![]), mb_api()); assert!(app.is_running()); - app = AppMachine::matches(app.unwrap_browse().inner, vec![]).into(); + let (_, rx) = mpsc::channel(); + app = AppMachine::matches(app.unwrap_browse().inner, rx).into(); let state = app.state(); assert!(matches!(state, AppState::Matches(_))); diff --git a/src/tui/app/mod.rs b/src/tui/app/mod.rs index b938247..90c49fe 100644 --- a/src/tui/app/mod.rs +++ b/src/tui/app/mod.rs @@ -165,22 +165,6 @@ impl AppState { mod tests { use super::*; - impl<'app> AppPublicMatchesInfo<'app> { - pub fn artist_ref(&self) -> &AppPublicArtistMatches<'app> { - match self { - Self::Artist(m) => m, - _ => panic!(), - } - } - - pub fn album_ref(&self) -> &AppPublicAlbumMatches<'app> { - match self { - Self::Album(m) => m, - _ => panic!(), - } - } - } - #[test] fn app_is_state() { let state = AppPublicState::Search("get rekt"); -- 2.45.2 From 753fbe863dc0a22d5a30324231f7fc074b4d9816 Mon Sep 17 00:00:00 2001 From: Wojciech Kozlowski Date: Sat, 31 Aug 2024 14:37:36 +0200 Subject: [PATCH 2/2] Small nit --- src/tui/app/machine/matches.rs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/tui/app/machine/matches.rs b/src/tui/app/machine/matches.rs index d784db2..80265d4 100644 --- a/src/tui/app/machine/matches.rs +++ b/src/tui/app/machine/matches.rs @@ -153,12 +153,8 @@ impl IAppInteractMatches for AppMachine { let result = index.saturating_add(1); let to = cmp::min( result, - self.state - .current - .as_ref() - .expect("selected() implies current exists") - .len() - .saturating_sub(1), + // selected() implies current exists + self.state.current.as_ref().unwrap().len().saturating_sub(1), ); self.state.state.list.select(Some(to)); } -- 2.45.2