Remove empty matches

2024-09-29 11:29:17 +02:00 · 2024-09-29 11:29:17 +02:00 · 96b0d47ce4
commit 96b0d47ce4
parent 0b0599318e
8 changed files with 100 additions and 173 deletions
--- a/src/tui/app/machine/fetch_state.rs
+++ b/src/tui/app/machine/fetch_state.rs
@ -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]
--- 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<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));
-            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<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,46 +253,41 @@ 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 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<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.
--- 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(_)));
--- 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,
 }

--- 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<usize>) -> Self {
+        self.list = self.list.with_selected(selected);
+        self
+    }
+}
+
 pub enum Delta {
    Line,
    Page,
--- 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),
        }
    }

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

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