From 0f658f87306e114b39ff658511c8c7145748fde2 Mon Sep 17 00:00:00 2001 From: Wojciech Kozlowski Date: Fri, 3 Jan 2025 14:42:09 +0100 Subject: [PATCH] Basic fix working --- src/core/collection/album.rs | 8 +- src/core/collection/artist.rs | 145 +++++++++++++++++++++------------- src/core/collection/merge.rs | 40 +++++----- src/core/musichoard/base.rs | 15 +++- 4 files changed, 124 insertions(+), 84 deletions(-) diff --git a/src/core/collection/album.rs b/src/core/collection/album.rs index cd53346..f872680 100644 --- a/src/core/collection/album.rs +++ b/src/core/collection/album.rs @@ -1,7 +1,7 @@ use std::mem; use crate::core::collection::{ - merge::{Merge, MergeName, MergeSorted}, + merge::{Merge, MergeSorted}, musicbrainz::{MbAlbumRef, MbRefOption}, track::{Track, TrackFormat}, }; @@ -54,12 +54,6 @@ impl AlbumLibId { /// Unique database identifier. Use MBID for this purpose. pub type AlbumMbRef = MbRefOption; -impl MergeName for Album { - fn name(&self) -> &str { - &self.meta.id.title - } -} - // There are crates for handling dates, but we don't need much complexity beyond year-month-day. /// The album's release date. #[derive(Clone, Copy, Debug, Default, PartialEq, Eq, PartialOrd, Ord)] diff --git a/src/core/collection/artist.rs b/src/core/collection/artist.rs index b83240c..49cfe4e 100644 --- a/src/core/collection/artist.rs +++ b/src/core/collection/artist.rs @@ -1,11 +1,12 @@ use std::{ collections::HashMap, fmt::{self, Debug, Display}, + mem, }; use crate::core::collection::{ album::{Album, AlbumLibId}, - merge::{Merge, MergeCollections, MergeName}, + merge::{Merge, MergeCollections}, musicbrainz::{MbArtistRef, MbRefOption}, }; @@ -30,12 +31,6 @@ pub struct ArtistInfo { pub properties: HashMap>, } -impl MergeName for Artist { - fn name(&self) -> &str { - &self.meta.id.name - } -} - /// The artist identifier. #[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)] pub struct ArtistId { @@ -46,49 +41,6 @@ pub struct ArtistId { /// Unique database identifier. Use MBID for this purpose. pub type ArtistMbRef = MbRefOption; -impl Artist { - /// Create new [`Artist`] with the given [`ArtistId`]. - pub fn new>(id: Id) -> Self { - Artist { - meta: ArtistMeta::new(id), - albums: vec![], - } - } - - fn merge_album_by_lib_id( - primary_albums: &mut [Album], - mut secondary_album: Album, - ) -> Option { - let id_opt = secondary_album.meta.id.lib_id; - if let id @ AlbumLibId::Value(_) | id @ AlbumLibId::Singleton = id_opt { - let mut iter = primary_albums.iter_mut(); - if let Some(ref mut primary_album) = iter.find(|a| a.meta.id.lib_id == id) { - primary_album.merge_in_place(secondary_album); - return None; - } - secondary_album.meta.id.lib_id = AlbumLibId::None; - } - Some(secondary_album) - } - - fn merge_albums_by_lib_id( - primary_albums: &mut [Album], - secondary_albums: Vec, - ) -> HashMap> { - let mut secondary_without_id = HashMap::>::new(); - for secondary_album in secondary_albums.into_iter() { - let unmerged = Artist::merge_album_by_lib_id(primary_albums, secondary_album); - if let Some(secondary_album) = unmerged { - secondary_without_id - .entry(secondary_album.meta.id.title.clone()) - .or_default() - .push(secondary_album); - } - } - secondary_without_id - } -} - impl PartialOrd for Artist { fn partial_cmp(&self, other: &Self) -> Option { Some(self.cmp(other)) @@ -104,10 +56,97 @@ impl Ord for Artist { impl Merge for Artist { fn merge_in_place(&mut self, other: Self) { self.meta.merge_in_place(other.meta); + self.albums = MergeAlbums::merge_albums(mem::take(&mut self.albums), other.albums); + } +} - let other_without_id = Artist::merge_albums_by_lib_id(&mut self.albums, other.albums); - MergeCollections::merge_by_name(&mut self.albums, other_without_id); - self.albums.sort_unstable(); +#[derive(Debug, Default)] +struct MergeAlbums { + primary_by_lib_id: HashMap, + primary_by_singleton: HashMap, + primary_by_title: HashMap>, + secondary_by_title: HashMap>, + merged: Vec, +} + +impl MergeAlbums { + fn merge_albums(primary_albums: Vec, secondary_albums: Vec) -> Vec { + let mut cache = MergeAlbums::new(primary_albums); + cache.merge_albums_by_lib_id(secondary_albums); + cache.merged.extend(MergeCollections::merge_by_name( + cache.primary_by_title, + cache.secondary_by_title, + )); + cache.merged.sort_unstable(); + cache.merged + } + + fn new(primary_albums: Vec) -> Self { + let mut cache = MergeAlbums::default(); + for album in primary_albums.into_iter() { + match album.meta.id.lib_id { + AlbumLibId::Value(value) => { + assert!(cache.primary_by_lib_id.insert(value, album).is_none()) + } + AlbumLibId::Singleton => assert!(cache + .primary_by_singleton + .insert(album.meta.id.title.clone(), album) + .is_none()), + _ => unreachable!("primary collection should always have a lib_id"), + } + } + cache + } + + fn merge_albums_by_lib_id(&mut self, secondary_albums: Vec) { + for secondary_album in secondary_albums.into_iter() { + self.merge_album_by_lib_id(secondary_album); + } + for (_, primary_album) in self.primary_by_lib_id.drain() { + self.primary_by_title + .entry(primary_album.meta.id.title.clone()) + .or_default() + .push(primary_album); + } + for (title, primary_album) in self.primary_by_singleton.drain() { + self.primary_by_title + .entry(title) + .or_default() + .push(primary_album); + } + } + + fn merge_album_by_lib_id(&mut self, mut secondary_album: Album) { + let title = &secondary_album.meta.id.title; + let id_opt = secondary_album.meta.id.lib_id; + + let find = match id_opt { + AlbumLibId::Value(value) => self.primary_by_lib_id.remove(&value), + AlbumLibId::Singleton => self.primary_by_singleton.remove(title), + AlbumLibId::None => None, + }; + + if let Some(mut primary_album) = find { + primary_album.merge_in_place(secondary_album); + self.merged.push(primary_album); + return; + } + + secondary_album.meta.id.lib_id = AlbumLibId::None; + self.secondary_by_title + .entry(secondary_album.meta.id.title.clone()) + .or_default() + .push(secondary_album); + } +} + +impl Artist { + /// Create new [`Artist`] with the given [`ArtistId`]. + pub fn new>(id: Id) -> Self { + Artist { + meta: ArtistMeta::new(id), + albums: vec![], + } } } diff --git a/src/core/collection/merge.rs b/src/core/collection/merge.rs index bbfc0fe..a9d0871 100644 --- a/src/core/collection/merge.rs +++ b/src/core/collection/merge.rs @@ -1,4 +1,4 @@ -use std::{cmp::Ordering, collections::HashMap, hash::Hash, iter::Peekable, marker::PhantomData}; +use std::{cmp::Ordering, collections::HashMap, hash::Hash, iter::Peekable}; /// A trait for merging two objects. The merge is asymmetric with the left argument considered to be /// the primary whose properties are to be kept in case of collisions. @@ -80,30 +80,30 @@ where } } -pub trait MergeName { - fn name(&self) -> &str; -} +pub struct MergeCollections; -pub struct MergeCollections { - _t: PhantomData, - _it: PhantomData, -} - -impl MergeCollections -where - T: MergeName + Merge, - IT: IntoIterator)>, -{ - pub fn merge_by_name(primary_items: &mut Vec, secondary: IT) { - for (name, mut secondary_items) in secondary.into_iter() { - match primary_items.iter_mut().find(|item| item.name() == name) { - Some(ref mut primary_item) => { - // We do not support merging multiple DB items with same name yet. +impl MergeCollections { + pub fn merge_by_name(mut primary: HashMap>, secondary: It) -> Vec + where + T: Merge, + It: IntoIterator)>, + { + let mut merged = vec![]; + for (title, mut secondary_items) in secondary.into_iter() { + match primary.remove(&title) { + Some(mut primary_items) => { + // We do not support merging multiple items with same name yet. Support will be + // added once encountered in the wild. + assert_eq!(primary_items.len(), 1); assert_eq!(secondary_items.len(), 1); + let mut primary_item = primary_items.pop().unwrap(); primary_item.merge_in_place(secondary_items.pop().unwrap()); + merged.push(primary_item); } - None => primary_items.extend(secondary_items), + None => merged.extend(secondary_items), } } + merged.extend(primary.into_values().flatten()); + merged } } diff --git a/src/core/musichoard/base.rs b/src/core/musichoard/base.rs index 50c9340..bac53da 100644 --- a/src/core/musichoard/base.rs +++ b/src/core/musichoard/base.rs @@ -59,9 +59,16 @@ impl IMusicHoardBasePrivate for MusicHoard } fn merge_collections(&self) -> Collection { - let mut primary = self.library_cache.clone(); + let mut primary = HashMap::>::new(); let mut secondary = HashMap::>::new(); + for artist in self.library_cache.iter().cloned() { + primary + .entry(artist.meta.id.name.clone()) + .or_default() + .push(artist); + } + for artist in self.database_cache.iter().cloned() { secondary .entry(artist.meta.id.name.clone()) @@ -69,10 +76,10 @@ impl IMusicHoardBasePrivate for MusicHoard .push(artist); } - MergeCollections::merge_by_name(&mut primary, secondary); - primary.sort_unstable(); + let mut collection = MergeCollections::merge_by_name(primary, secondary); + collection.sort_unstable(); - primary + collection } fn get_artist<'a>(collection: &'a Collection, artist_id: &ArtistId) -> Option<&'a Artist> {