src/tui/app/machine/match_state.rs

@@ -212,6 +212,10 @@ impl AppMachine<MatchState> {
         mh.set_artist_mb_ref(&matches.matching.id, tuple.mb_ref)
     }
 
+    fn filter_mb_ref(left: &AlbumMbRef, right: &AlbumMbRef) -> bool {
+        (left == right) && !matches!(left, AlbumMbRef::CannotHaveMbid)
+    }
+
     fn select_album(
         inner: &mut AppInner,
         matches: &AlbumMatches,
@@ -225,7 +229,7 @@ impl AppMachine<MatchState> {
             let iter = artist.albums.iter();
             let filtered = iter
                 .filter(|album| album.meta.id != matches.matching)
-                .filter(|album| album.meta.id.mb_ref == tuple.mb_ref);
+                .filter(|album| Self::filter_mb_ref(&album.meta.id.mb_ref, &tuple.mb_ref));
             clashing.extend(filtered.map(|a| &a.meta.id).cloned())
         }
 
@@ -323,6 +327,7 @@ mod tests {
             Album, AlbumDate, AlbumId, AlbumInfo, AlbumMeta, AlbumPrimaryType, AlbumSecondaryType,
         },
         artist::{Artist, ArtistId, ArtistMeta},
+        Collection,
     };
 
     use crate::tui::{
@@ -604,6 +609,42 @@ mod tests {
         matches.select().unwrap_fetch();
     }
 
+    fn set_album_info_expectations(
+        mh: &mut MockIMusicHoard,
+        collection: Collection,
+        matches: AlbumMatches,
+        mut album_id: AlbumId,
+        removed_id: Option<AlbumId>,
+    ) {
+        let meta = album_meta(album_id.clone());
+        let mb_ref = album_id.mb_ref.clone();
+        album_id.clear_mb_ref();
+        let artist = matches.artist.clone();
+
+        let mut seq = Sequence::new();
+        mh.expect_get_collection()
+            .times(1)
+            .in_sequence(&mut seq)
+            .return_const(collection);
+        if let Some(removed_id) = removed_id {
+            mh.expect_remove_album()
+                .times(1)
+                .with(eq(artist.clone()), eq(removed_id))
+                .in_sequence(&mut seq)
+                .return_once(|_, _| Ok(()));
+        }
+        mh.expect_merge_album_info()
+            .with(eq(artist.clone()), eq(album_id.clone()), eq(meta.info))
+            .times(1)
+            .in_sequence(&mut seq)
+            .return_once(|_, _, _| Ok(()));
+        mh.expect_set_album_mb_ref()
+            .with(eq(artist.clone()), eq(album_id.clone()), eq(mb_ref))
+            .times(1)
+            .in_sequence(&mut seq)
+            .return_once(|_, _, _| Ok(()));
+    }
+
     #[test]
     fn set_album_info() {
         let matches_info = album_match();
@@ -612,45 +653,22 @@ mod tests {
         let app_matches = MatchState::new(matches_info.clone(), FetchState::search(rx));
 
         let collection = vec![];
-        let mut music_hoard = music_hoard(collection.clone());
+        let mut mh = music_hoard(collection.clone());
         match matches_info {
             EntityMatches::Artist(_) => panic!(),
             EntityMatches::Album(matches) => {
-                let mut album_id = album_id();
+                set_album_info_expectations(&mut mh, collection, matches, album_id(), None)
-                let meta = album_meta(album_id.clone());
-                let mb_ref = album_id.mb_ref.clone();
-                album_id.clear_mb_ref();
-                let artist = matches.artist.clone();
-
-                let mut seq = Sequence::new();
-                music_hoard
-                    .expect_get_collection()
-                    .times(1)
-                    .in_sequence(&mut seq)
-                    .return_const(collection);
-                music_hoard
-                    .expect_merge_album_info()
-                    .with(eq(artist.clone()), eq(album_id.clone()), eq(meta.info))
-                    .times(1)
-                    .in_sequence(&mut seq)
-                    .return_once(|_, _, _| Ok(()));
-                music_hoard
-                    .expect_set_album_mb_ref()
-                    .with(eq(artist.clone()), eq(album_id.clone()), eq(mb_ref))
-                    .times(1)
-                    .in_sequence(&mut seq)
-                    .return_once(|_, _, _| Ok(()));
             }
         }
 
-        let matches = AppMachine::match_state(inner(music_hoard), app_matches);
+        let matches = AppMachine::match_state(inner(mh), app_matches);
         matches.select().unwrap_fetch();
     }
 
-    #[test]
+    fn albums_with_clashing_mbid(
-    fn set_album_info_with_clashing_mbid() {
+        clashing_id: AlbumId,
-        let matches_info = album_match();
+        last_id: AlbumId,
-
+    ) -> (Collection, Collection) {
         // The collection below should trigger MH to remove the album with the same MBID but not
         // matching album_id.
 
@@ -662,66 +680,56 @@ mod tests {
 
         // (3) An album with a different album_id than the selected one.
         // (4) This album has an MBID that is identical to that of the selected match.
-        let mb_ref = AlbumMbRef::Some(mbid().into());
+        let mb_ref = clashing_id.mb_ref.clone();
-        let removed = Album::new(AlbumId::new("Album: Not the Same").with_mb_ref(mb_ref.clone()));
+        artist.albums.push(Album::new(clashing_id));
-        artist.albums.push(removed.clone());
 
         // (5) An album after the one that will be removed. Selection must remain on it.
-        let selected = Album::new(AlbumId::new("Album: Z"));
+        artist.albums.push(Album::new(last_id));
-        artist.albums.push(selected.clone());
+
+        let collection = vec![artist];
+
+        let mut reduced = collection.clone();
+        reduced[0].albums.retain(|a| a.meta.id.mb_ref != mb_ref);
+
+        (collection, reduced)
+    }
+
+    #[test]
+    fn set_album_info_with_clashing_mbid() {
+        let matches_info = album_match();
 
         let (_tx, rx) = mpsc::channel();
         let app_matches = MatchState::new(matches_info.clone(), FetchState::search(rx));
 
-        let collection = vec![artist];
+        let mb_ref = AlbumMbRef::Some(mbid().into());
-        let mut reduced = collection.clone();
+        let clashing = AlbumId::new("Album: Not the Same").with_mb_ref(mb_ref.clone());
-        reduced[0].albums.retain(|a| a.meta.id.mb_ref != mb_ref);
+        let selected = AlbumId::new("Album: Z");
+        let (collection, reduced) = albums_with_clashing_mbid(clashing.clone(), selected.clone());
 
         // We need a custom music_hoard to override the get_filtered expectations.
-        let mut music_hoard = MockIMusicHoard::new();
+        let mut mh = MockIMusicHoard::new();
         let mut seq = Sequence::new();
-        music_hoard
+        mh.expect_get_filtered()
-            .expect_get_filtered()
             .times(2)
             .in_sequence(&mut seq)
             .return_const(collection.clone());
-        music_hoard
+        mh.expect_get_filtered()
-            .expect_get_filtered()
             .times(1)
             .in_sequence(&mut seq)
             .return_const(reduced.clone());
 
         match matches_info {
             EntityMatches::Artist(_) => panic!(),
-            EntityMatches::Album(matches) => {
+            EntityMatches::Album(matches) => set_album_info_expectations(
-                let artist = matches.artist.clone();
+                &mut mh,
-
+                collection.clone(),
-                let mut seq = Sequence::new();
+                matches,
-                music_hoard
+                album_id(),
-                    .expect_get_collection()
+                Some(clashing.clone()),
-                    .times(1)
+            ),
-                    .in_sequence(&mut seq)
-                    .return_const(collection.clone());
-                music_hoard
-                    .expect_remove_album()
-                    .times(1)
-                    .with(eq(artist), eq(removed.meta.id.clone()))
-                    .in_sequence(&mut seq)
-                    .return_once(|_, _| Ok(()));
-                music_hoard
-                    .expect_merge_album_info()
-                    .times(1)
-                    .in_sequence(&mut seq)
-                    .return_once(|_, _, _| Ok(()));
-                music_hoard
-                    .expect_set_album_mb_ref()
-                    .times(1)
-                    .in_sequence(&mut seq)
-                    .return_once(|_, _, _| Ok(()));
-            }
         }
 
-        let mut matches = AppMachine::match_state(inner(music_hoard), app_matches);
+        let mut matches = AppMachine::match_state(inner(mh), app_matches);
 
         // Make sure the last album is selected.
         let selection = &mut matches.inner.selection;
@@ -732,7 +740,7 @@ mod tests {
         // Check that the correct album is selected.
         let state = selection.state_album(&collection).unwrap();
         let album = &collection[0].albums[state.index];
-        assert_eq!(album.meta, selected.meta);
+        assert_eq!(album.meta.id, selected);
 
         let mut fetch = matches.select().unwrap_fetch();
 
@@ -740,7 +748,31 @@ mod tests {
         let selection = &mut fetch.inner.selection;
         let state = selection.state_album(&reduced).unwrap();
         let album = &reduced[0].albums[state.index];
-        assert_eq!(album.meta, selected.meta);
+        assert_eq!(album.meta.id, selected);
+    }
+
+    #[test]
+    fn set_album_info_with_clashing_cannot_have_mbid() {
+        let matches_info = album_match();
+
+        let (_tx, rx) = mpsc::channel();
+        let app_matches = MatchState::new(matches_info.clone(), FetchState::search(rx));
+
+        let mb_ref = AlbumMbRef::CannotHaveMbid;
+        let clashing = AlbumId::new("Album: Not the Same").with_mb_ref(mb_ref);
+        let selected = AlbumId::new("Album: Z");
+        let (collection, _) = albums_with_clashing_mbid(clashing.clone(), selected.clone());
+
+        let mut mh = music_hoard(collection.clone());
+        match matches_info {
+            EntityMatches::Artist(_) => panic!(),
+            EntityMatches::Album(matches) => {
+                set_album_info_expectations(&mut mh, collection, matches, album_id(), None)
+            }
+        }
+
+        let matches = AppMachine::match_state(inner(mh), app_matches);
+        matches.select().unwrap_fetch();
     }
 
     #[test]