Make fetch asynchronous (#210)

Closes #187

Reviewed-on: #210

src/main.rs

@@ -73,7 +73,7 @@ struct DbOpt {
 fn with<Database: IDatabase + 'static, Library: ILibrary + 'static>(
     builder: MusicHoardBuilder<Database, Library>,
 ) {
-    let music_hoard = Box::new(builder.build().expect("failed to initialise MusicHoard"));
+    let music_hoard = builder.build().expect("failed to initialise MusicHoard");
 
     // Initialize the terminal user interface.
     let backend = CrosstermBackend::new(io::stdout());
@@ -86,7 +86,7 @@ fn with<Database: IDatabase + 'static, Library: ILibrary + 'static>(
     let http =
         MusicBrainzHttp::new(MUSICHOARD_HTTP_USER_AGENT).expect("failed to initialise HTTP client");
     let client = MusicBrainzClient::new(http);
-    let musicbrainz = Box::new(MusicBrainz::new(client));
+    let musicbrainz = MusicBrainz::new(client);
 
     let app = App::new(music_hoard, musicbrainz);
     let ui = Ui;

src/tui/app/machine/browse.rs

@@ -1,11 +1,21 @@
-use std::{sync::mpsc, thread, time};
+use std::{
+    sync::{mpsc, Arc, Mutex},
+    thread, time,
+};
 
-use musichoard::collection::musicbrainz::IMusicBrainzRef;
+use musichoard::collection::{
+    album::AlbumMeta,
+    artist::ArtistMeta,
+    musicbrainz::{IMusicBrainzRef, Mbid},
+};
 
-use crate::tui::app::{
-    machine::{App, AppInner, AppMachine},
-    selection::{Delta, ListSelection},
-    AppMatchesInfo, AppPublic, AppState, IAppInteractBrowse,
+use crate::tui::{
+    app::{
+        machine::{App, AppInner, AppMachine},
+        selection::{Delta, ListSelection},
+        AppMatchesInfo, AppPublic, AppState, IAppInteractBrowse,
+    },
+    lib::interface::musicbrainz::{Error as MbError, IMusicBrainz},
 };
 
 pub struct AppBrowse;
@@ -82,7 +92,7 @@ impl IAppInteractBrowse for AppMachine<AppBrowse> {
         AppMachine::search(self.inner, orig).into()
     }
 
-    fn fetch_musicbrainz(mut self) -> Self::APP {
+    fn fetch_musicbrainz(self) -> Self::APP {
         let coll = self.inner.music_hoard.get_collection();
         let artist = match self.inner.selection.state_artist(coll) {
             Some(artist_state) => &coll[artist_state.index],
@@ -91,43 +101,23 @@ impl IAppInteractBrowse for AppMachine<AppBrowse> {
             }
         };
 
-        let (matches_tx, matches_rx) = mpsc::channel::<AppMatchesInfo>();
+        let (matches_tx, matches_rx) = mpsc::channel::<FetchResult>();
 
         match artist.meta.musicbrainz {
-            Some(ref mbid) => {
-                let arid = mbid.mbid();
-
-                let mut album_iter = artist.albums.iter().peekable();
-                while let Some(album) = album_iter.next() {
-                    if album.meta.musicbrainz.is_some() {
-                        continue;
-                    }
-
-                    match self
-                        .inner
-                        .musicbrainz
-                        .search_release_group(arid, &album.meta)
-                    {
-                        Ok(list) => matches_tx
-                            .send(AppMatchesInfo::album(album.meta.clone(), list))
-                            .expect("send fails only if receiver is disconnected"),
-                        Err(err) => return AppMachine::error(self.inner, err.to_string()).into(),
-                    }
-
-                    if album_iter.peek().is_some() {
-                        thread::sleep(time::Duration::from_secs(1));
-                    }
-                }
+            Some(ref arid) => {
+                let musicbrainz = Arc::clone(&self.inner.musicbrainz);
+                let arid = arid.mbid().clone();
+                let albums = artist.albums.iter().map(|a| &a.meta).cloned().collect();
+                thread::spawn(|| Self::fetch_albums(musicbrainz, matches_tx, arid, albums));
+            }
+            None => {
+                let musicbrainz = Arc::clone(&self.inner.musicbrainz);
+                let artist = artist.meta.clone();
+                thread::spawn(|| Self::fetch_artist(musicbrainz, matches_tx, artist));
             }
-            None => match self.inner.musicbrainz.search_artist(&artist.meta) {
-                Ok(list) => matches_tx
-                    .send(AppMatchesInfo::artist(artist.meta.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_rx).into()
+        AppMachine::app_matches(self.inner, matches_rx)
     }
 
     fn no_op(self) -> Self::APP {
@@ -135,6 +125,63 @@ impl IAppInteractBrowse for AppMachine<AppBrowse> {
     }
 }
 
+pub type FetchError = MbError;
+pub type FetchResult = Result<AppMatchesInfo, FetchError>;
+pub type FetchSender = mpsc::Sender<FetchResult>;
+pub type FetchReceiver = mpsc::Receiver<FetchResult>;
+
+trait IAppInteractBrowsePrivate {
+    fn fetch_artist(
+        musicbrainz: Arc<Mutex<dyn IMusicBrainz + Send>>,
+        matches_tx: FetchSender,
+        artist: ArtistMeta,
+    );
+    fn fetch_albums(
+        musicbrainz: Arc<Mutex<dyn IMusicBrainz + Send>>,
+        matches_tx: FetchSender,
+        arid: Mbid,
+        albums: Vec<AlbumMeta>,
+    );
+}
+
+impl IAppInteractBrowsePrivate for AppMachine<AppBrowse> {
+    fn fetch_artist(
+        musicbrainz: Arc<Mutex<dyn IMusicBrainz + Send>>,
+        matches_tx: FetchSender,
+        artist: ArtistMeta,
+    ) {
+        let result = musicbrainz.lock().unwrap().search_artist(&artist);
+        let result = result.map(|list| AppMatchesInfo::artist(artist, list));
+        matches_tx.send(result).ok();
+    }
+
+    fn fetch_albums(
+        musicbrainz: Arc<Mutex<dyn IMusicBrainz + Send>>,
+        matches_tx: FetchSender,
+        arid: Mbid,
+        albums: Vec<AlbumMeta>,
+    ) {
+        let mut musicbrainz = musicbrainz.lock().unwrap();
+        let mut album_iter = albums.into_iter().peekable();
+        while let Some(album) = album_iter.next() {
+            if album.musicbrainz.is_some() {
+                continue;
+            }
+
+            let result = musicbrainz.search_release_group(&arid, &album);
+            let result = result.map(|list| AppMatchesInfo::album(album, list));
+            if matches_tx.send(result).is_err() {
+                // If receiver disconnects just drop the rest.
+                return;
+            }
+
+            if album_iter.peek().is_some() {
+                thread::sleep(time::Duration::from_secs(1));
+            }
+        }
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use mockall::{predicate, Sequence};
@@ -224,7 +271,7 @@ mod tests {
 
     #[test]
     fn fetch_musicbrainz() {
-        let mut mb_api = Box::new(MockIMusicBrainz::new());
+        let mut mb_api = MockIMusicBrainz::new();
 
         let arid: Mbid = "11111111-1111-1111-1111-111111111111".try_into().unwrap();
         let album_1 = COLLECTION[1].albums[0].meta.clone();
@@ -305,7 +352,7 @@ mod tests {
 
     #[test]
     fn fetch_musicbrainz_no_artist_mbid() {
-        let mut mb_api = Box::new(MockIMusicBrainz::new());
+        let mut mb_api = MockIMusicBrainz::new();
 
         let artist = COLLECTION[3].meta.clone();
 
@@ -349,7 +396,7 @@ mod tests {
 
     #[test]
     fn fetch_musicbrainz_artist_api_error() {
-        let mut mb_api = Box::new(MockIMusicBrainz::new());
+        let mut mb_api = MockIMusicBrainz::new();
 
         let error = Err(musicbrainz::Error::RateLimit);
 
@@ -371,7 +418,7 @@ mod tests {
 
     #[test]
     fn fetch_musicbrainz_album_api_error() {
-        let mut mb_api = Box::new(MockIMusicBrainz::new());
+        let mut mb_api = MockIMusicBrainz::new();
 
         let error = Err(musicbrainz::Error::RateLimit);
 
@@ -386,6 +433,45 @@ mod tests {
         app.unwrap_error();
     }
 
+    #[test]
+    fn fetch_musicbrainz_artist_receiver_disconnect() {
+        let (tx, rx) = mpsc::channel::<FetchResult>();
+        drop(rx);
+
+        let mut mb_api = MockIMusicBrainz::new();
+
+        mb_api
+            .expect_search_artist()
+            .times(1)
+            .return_once(|_| Ok(vec![]));
+
+        // We only check it does not panic and that it doesn't call the API more than once.
+        AppMachine::fetch_artist(Arc::new(Mutex::new(mb_api)), tx, COLLECTION[3].meta.clone());
+    }
+
+    #[test]
+    fn fetch_musicbrainz_albums_receiver_disconnect() {
+        let (tx, rx) = mpsc::channel::<FetchResult>();
+        drop(rx);
+
+        let mut mb_api = MockIMusicBrainz::new();
+
+        mb_api
+            .expect_search_release_group()
+            .times(1)
+            .return_once(|_, _| Ok(vec![]));
+
+        // We only check it does not panic and that it doesn't call the API more than once.
+        let mbref = &COLLECTION[1].meta.musicbrainz;
+        let albums = &COLLECTION[1].albums;
+        AppMachine::fetch_albums(
+            Arc::new(Mutex::new(mb_api)),
+            tx,
+            mbref.as_ref().unwrap().mbid().clone(),
+            albums.iter().map(|a| &a.meta).cloned().collect(),
+        );
+    }
+
     #[test]
     fn no_op() {
         let browse = AppMachine::browse(inner(music_hoard(vec![])));

src/tui/app/machine/matches.rs

@@ -1,7 +1,7 @@
-use std::{cmp, sync::mpsc};
+use std::cmp;
 
 use crate::tui::app::{
-    machine::{App, AppInner, AppMachine},
+    machine::{browse::FetchReceiver, App, AppInner, AppMachine},
     AppAlbumMatches, AppArtistMatches, AppMatchesInfo, AppPublic, AppPublicMatches, AppState,
     IAppInteractMatches, MatchOption, WidgetState,
 };
@@ -43,21 +43,29 @@ impl AppMatchesInfo {
 }
 
 pub struct AppMatches {
-    matches_rx: mpsc::Receiver<AppMatchesInfo>,
+    matches_rx: FetchReceiver,
     current: Option<AppMatchesInfo>,
     state: WidgetState,
 }
 
-impl AppMachine<AppMatches> {
-    pub fn matches(inner: AppInner, matches_rx: mpsc::Receiver<AppMatchesInfo>) -> Self {
-        let mut state = AppMatches {
+impl AppMatches {
+    fn empty(matches_rx: FetchReceiver) -> Self {
+        AppMatches {
             matches_rx,
             current: None,
             state: WidgetState::default(),
-        };
-        state.next_matches_info();
+        }
+    }
+}
+
+impl AppMachine<AppMatches> {
+    fn matches(inner: AppInner, state: AppMatches) -> Self {
         AppMachine { inner, state }
     }
+
+    pub fn app_matches(inner: AppInner, matches_rx: FetchReceiver) -> App {
+        AppMachine::matches(inner, AppMatches::empty(matches_rx)).fetch_first()
+    }
 }
 
 impl From<AppMachine<AppMatches>> for App {
@@ -104,12 +112,8 @@ impl IAppInteractMatches for AppMachine<AppMatches> {
         self.into()
     }
 
-    fn select(mut self) -> Self::APP {
-        self.state.next_matches_info();
-        match self.state.current {
-            Some(_) => self.into(),
-            None => AppMachine::browse(self.inner).into(),
-        }
+    fn select(self) -> Self::APP {
+        self.fetch_next()
     }
 
     fn abort(self) -> Self::APP {
@@ -121,28 +125,51 @@ impl IAppInteractMatches for AppMachine<AppMatches> {
     }
 }
 
-trait IAppInteractMatchesPrivate {
-    fn next_matches_info(&mut self);
+trait IAppInteractMatchesPrivate
+where
+    Self: Sized,
+{
+    fn fetch_first(self) -> App;
+    fn fetch_next(self) -> App;
+    fn fetch(self, first: bool) -> App;
 }
 
-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(mut next_match) => {
-                next_match.push_cannot_have_mbid();
-                let mut state = WidgetState::default();
-                state.list.select(Some(0));
-                (Some(next_match), state)
+impl IAppInteractMatchesPrivate for AppMachine<AppMatches> {
+    fn fetch_first(self) -> App {
+        self.fetch(true)
+    }
+
+    fn fetch_next(self) -> App {
+        self.fetch(false)
+    }
+
+    fn fetch(mut self, first: bool) -> App {
+        match self.state.matches_rx.recv() {
+            Ok(fetch_result) => match fetch_result {
+                Ok(mut next_match) => {
+                    next_match.push_cannot_have_mbid();
+                    self.state.current = Some(next_match);
+                    self.state.state.list.select(Some(0));
+                    AppMachine::matches(self.inner, self.state).into()
+                }
+                Err(err) => AppMachine::error(self.inner, format!("fetch failed: {err}")).into(),
+            },
+            // only happens when the sender disconnects which means it finished its job
+            Err(_) => {
+                if first {
+                    AppMachine::matches(self.inner, AppMatches::empty(self.state.matches_rx)).into()
+                } else {
+                    AppMachine::browse(self.inner).into()
+                }
             }
-            Err(_) => (None, WidgetState::default()),
         }
     }
 }
 
 #[cfg(test)]
 mod tests {
-    use mpsc::Receiver;
+    use std::sync::mpsc;
+
     use musichoard::collection::{
         album::{AlbumDate, AlbumId, AlbumMeta, AlbumPrimaryType, AlbumSecondaryType},
         artist::{ArtistId, ArtistMeta},
@@ -227,10 +254,10 @@ mod tests {
         vec![matches_info_1, matches_info_2]
     }
 
-    fn receiver(matches_info_vec: Vec<AppMatchesInfo>) -> Receiver<AppMatchesInfo> {
+    fn receiver(matches_info_vec: Vec<AppMatchesInfo>) -> FetchReceiver {
         let (tx, rx) = mpsc::channel();
         for matches_info in matches_info_vec.into_iter() {
-            tx.send(matches_info).unwrap();
+            tx.send(Ok(matches_info)).unwrap();
         }
         rx
     }
@@ -243,7 +270,8 @@ mod tests {
 
     #[test]
     fn create_empty() {
-        let matches = AppMachine::matches(inner(music_hoard(vec![])), receiver(vec![]));
+        let matches =
+            AppMachine::app_matches(inner(music_hoard(vec![])), receiver(vec![])).unwrap_matches();
 
         let widget_state = WidgetState::default();
 
@@ -261,10 +289,11 @@ mod tests {
     #[test]
     fn create_nonempty() {
         let mut matches_info_vec = album_matches_info_vec();
-        let matches = AppMachine::matches(
+        let matches = AppMachine::app_matches(
             inner(music_hoard(vec![])),
             receiver(matches_info_vec.clone()),
-        );
+        )
+        .unwrap_matches();
         push_cannot_have_mbid(&mut matches_info_vec);
 
         let mut widget_state = WidgetState::default();
@@ -282,10 +311,11 @@ mod tests {
     }
 
     fn matches_flow(mut matches_info_vec: Vec<AppMatchesInfo>) {
-        let matches = AppMachine::matches(
+        let matches = AppMachine::app_matches(
             inner(music_hoard(vec![])),
             receiver(matches_info_vec.clone()),
-        );
+        )
+        .unwrap_matches();
         push_cannot_have_mbid(&mut matches_info_vec);
 
         let mut widget_state = WidgetState::default();
@@ -337,10 +367,11 @@ mod tests {
     #[test]
     fn matches_abort() {
         let mut matches_info_vec = album_matches_info_vec();
-        let matches = AppMachine::matches(
+        let matches = AppMachine::app_matches(
             inner(music_hoard(vec![])),
             receiver(matches_info_vec.clone()),
-        );
+        )
+        .unwrap_matches();
         push_cannot_have_mbid(&mut matches_info_vec);
 
         let mut widget_state = WidgetState::default();
@@ -354,7 +385,8 @@ mod tests {
 
     #[test]
     fn matches_select_empty() {
-        let matches = AppMachine::matches(inner(music_hoard(vec![])), receiver(vec![]));
+        let matches =
+            AppMachine::app_matches(inner(music_hoard(vec![])), receiver(vec![])).unwrap_matches();
 
         assert_eq!(matches.state.current, None);
 
@@ -363,7 +395,8 @@ mod tests {
 
     #[test]
     fn no_op() {
-        let matches = AppMachine::matches(inner(music_hoard(vec![])), receiver(vec![]));
+        let matches =
+            AppMachine::app_matches(inner(music_hoard(vec![])), receiver(vec![])).unwrap_matches();
         let app = matches.no_op();
         app.unwrap_matches();
     }

src/tui/app/machine/mod.rs

@@ -6,6 +6,8 @@ mod matches;
 mod reload;
 mod search;
 
+use std::sync::{Arc, Mutex};
+
 use crate::tui::{
     app::{selection::Selection, AppPublic, AppPublicInner, AppState, IAppAccess, IAppInteract},
     lib::{interface::musicbrainz::IMusicBrainz, IMusicHoard},
@@ -37,21 +39,24 @@ pub struct AppMachine<STATE> {
 pub struct AppInner {
     running: bool,
     music_hoard: Box<dyn IMusicHoard>,
-    musicbrainz: Box<dyn IMusicBrainz>,
+    musicbrainz: Arc<Mutex<dyn IMusicBrainz + Send>>,
     selection: Selection,
 }
 
 impl App {
-    pub fn new(mut music_hoard: Box<dyn IMusicHoard>, mb_api: Box<dyn IMusicBrainz>) -> Self {
+    pub fn new<MH: IMusicHoard + 'static, MB: IMusicBrainz + Send + 'static>(
+        mut music_hoard: MH,
+        musicbrainz: MB,
+    ) -> Self {
         let init_result = Self::init(&mut music_hoard);
-        let inner = AppInner::new(music_hoard, mb_api);
+        let inner = AppInner::new(music_hoard, musicbrainz);
         match init_result {
             Ok(()) => AppMachine::browse(inner).into(),
             Err(err) => AppMachine::critical(inner, err.to_string()).into(),
         }
     }
 
-    fn init(music_hoard: &mut Box<dyn IMusicHoard>) -> Result<(), musichoard::Error> {
+    fn init<MH: IMusicHoard>(music_hoard: &mut MH) -> Result<(), musichoard::Error> {
         music_hoard.rescan_library()?;
         Ok(())
     }
@@ -121,12 +126,15 @@ impl IAppAccess for App {
 }
 
 impl AppInner {
-    pub fn new(music_hoard: Box<dyn IMusicHoard>, musicbrainz: Box<dyn IMusicBrainz>) -> Self {
+    pub fn new<MH: IMusicHoard + 'static, MB: IMusicBrainz + Send + 'static>(
+        music_hoard: MH,
+        musicbrainz: MB,
+    ) -> Self {
         let selection = Selection::new(music_hoard.get_collection());
         AppInner {
             running: true,
-            music_hoard,
-            musicbrainz,
+            music_hoard: Box::new(music_hoard),
+            musicbrainz: Arc::new(Mutex::new(musicbrainz)),
             selection,
         }
     }
@@ -205,14 +213,14 @@ mod tests {
         }
     }
 
-    pub fn music_hoard(collection: Collection) -> Box<MockIMusicHoard> {
-        let mut music_hoard = Box::new(MockIMusicHoard::new());
+    pub fn music_hoard(collection: Collection) -> MockIMusicHoard {
+        let mut music_hoard = MockIMusicHoard::new();
         music_hoard.expect_get_collection().return_const(collection);
 
         music_hoard
     }
 
-    fn music_hoard_init(collection: Collection) -> Box<MockIMusicHoard> {
+    fn music_hoard_init(collection: Collection) -> MockIMusicHoard {
         let mut music_hoard = music_hoard(collection);
 
         music_hoard
@@ -223,18 +231,15 @@ mod tests {
         music_hoard
     }
 
-    fn mb_api() -> Box<MockIMusicBrainz> {
-        Box::new(MockIMusicBrainz::new())
+    fn mb_api() -> MockIMusicBrainz {
+        MockIMusicBrainz::new()
     }
 
-    pub fn inner(music_hoard: Box<MockIMusicHoard>) -> AppInner {
+    pub fn inner(music_hoard: MockIMusicHoard) -> AppInner {
         AppInner::new(music_hoard, mb_api())
     }
 
-    pub fn inner_with_mb(
-        music_hoard: Box<MockIMusicHoard>,
-        mb_api: Box<MockIMusicBrainz>,
-    ) -> AppInner {
+    pub fn inner_with_mb(music_hoard: MockIMusicHoard, mb_api: MockIMusicBrainz) -> AppInner {
         AppInner::new(music_hoard, mb_api)
     }
 
@@ -314,7 +319,7 @@ mod tests {
         assert!(app.is_running());
 
         let (_, rx) = mpsc::channel();
-        app = AppMachine::matches(app.unwrap_browse().inner, rx).into();
+        app = AppMachine::app_matches(app.unwrap_browse().inner, rx);
 
         let state = app.state();
         assert!(matches!(state, AppState::Matches(_)));
@@ -365,7 +370,7 @@ mod tests {
 
     #[test]
     fn init_error() {
-        let mut music_hoard = Box::new(MockIMusicHoard::new());
+        let mut music_hoard = MockIMusicHoard::new();
 
         music_hoard
             .expect_rescan_library()

src/tui/mod.rs

@@ -192,8 +192,8 @@ mod tests {
         Terminal::new(backend).unwrap()
     }
 
-    fn music_hoard(collection: Collection) -> Box<MockIMusicHoard> {
-        let mut music_hoard = Box::new(MockIMusicHoard::new());
+    fn music_hoard(collection: Collection) -> MockIMusicHoard {
+        let mut music_hoard = MockIMusicHoard::new();
 
         music_hoard.expect_reload_database().returning(|| Ok(()));
         music_hoard.expect_rescan_library().returning(|| Ok(()));
@@ -203,7 +203,7 @@ mod tests {
     }
 
     fn app(collection: Collection) -> App {
-        App::new(music_hoard(collection), Box::new(MockIMusicBrainz::new()))
+        App::new(music_hoard(collection), MockIMusicBrainz::new())
     }
 
     fn listener() -> MockIEventListener {