From f2bcfcaa605ccd38be15d87aab15dd1606cb80a6 Mon Sep 17 00:00:00 2001 From: Henrik Friedrichsen Date: Fri, 5 Mar 2021 23:34:29 +0100 Subject: [PATCH] fix: more resilient playlist track deletion Introduction of the sort command with #328 broke the deletion of playlist items, because sorted track indices got out of sync with their actual index within the playlist at the Spotify backend. The new approach is not perfect, as it doesn't update the list index of items after deletion, but by supplying the playlist snapshot id the backend is able to apply the requested changes. This should still be improved in the future, though. --- src/library.rs | 1 + src/playlist.rs | 71 ++++++++++++++++++++++++++++++++++++++++------ src/spotify.rs | 9 ++++-- src/track.rs | 4 +++ src/ui/listview.rs | 56 +----------------------------------- src/ui/playlist.rs | 43 +++++++++++++++------------- 6 files changed, 99 insertions(+), 85 deletions(-) diff --git a/src/library.rs b/src/library.rs index 5aa9fc6..1ff849e 100644 --- a/src/library.rs +++ b/src/library.rs @@ -272,6 +272,7 @@ impl Library { if self.needs_download(remote) { info!("updating playlist {} (index: {})", remote.name, index); let mut playlist: Playlist = remote.into(); + playlist.tracks = None; playlist.load_tracks(self.spotify.clone()); self.append_or_update(&playlist); // trigger redraw diff --git a/src/playlist.rs b/src/playlist.rs index c49cd62..7fc2127 100644 --- a/src/playlist.rs +++ b/src/playlist.rs @@ -1,15 +1,15 @@ -use std::iter::Iterator; use std::sync::Arc; +use std::{cmp::Ordering, iter::Iterator}; use rspotify::model::playlist::{FullPlaylist, SimplifiedPlaylist}; -use crate::library::Library; use crate::playable::Playable; use crate::queue::Queue; use crate::spotify::Spotify; use crate::track::Track; use crate::traits::{IntoBoxedViewExt, ListItem, ViewExt}; use crate::ui::playlist::PlaylistView; +use crate::{command::SortDirection, command::SortKey, library::Library}; #[derive(Clone, Debug, Deserialize, Serialize)] pub struct Playlist { @@ -36,9 +36,10 @@ impl Playlist { let mut tracks_result = spotify.user_playlist_tracks(&self.id, 100, 0); while let Some(ref tracks) = tracks_result.clone() { - for listtrack in &tracks.items { + for (index, listtrack) in tracks.items.iter().enumerate() { if let Some(track) = &listtrack.track { let mut t: Track = track.into(); + t.list_index = index; t.added_at = Some(listtrack.added_at); collected_tracks.push(t); } @@ -70,19 +71,19 @@ impl Playlist { }) } - pub fn delete_tracks( + pub fn delete_track( &mut self, - track_pos_pairs: &[(Track, usize)], + index: usize, spotify: Arc, library: Arc, ) -> bool { - match spotify.delete_tracks(&self.id, track_pos_pairs) { + let track = self.tracks.as_ref().unwrap()[index].clone(); + debug!("deleting track: {} {:?}", index, track); + match spotify.delete_tracks(&self.id, &self.snapshot_id, &[(&track, track.list_index)]) { false => false, true => { if let Some(tracks) = &mut self.tracks { - for (_track, pos) in track_pos_pairs { - tracks.remove(*pos); - } + tracks.remove(index); library.playlist_update(&self); } @@ -117,6 +118,58 @@ impl Playlist { library.playlist_update(self); } } + + pub fn sort(&mut self, key: &SortKey, direction: &SortDirection) { + fn compare_artists(a: Vec, b: Vec) -> Ordering { + let sanitize_artists_name = |x: Vec| -> Vec { + x.iter() + .map(|x| { + x.to_lowercase() + .split(' ') + .skip_while(|x| x == &"the") + .collect() + }) + .collect() + }; + + let a = sanitize_artists_name(a); + let b = sanitize_artists_name(b); + + a.cmp(&b) + } + + if let Some(c) = self.tracks.as_mut() { + c.sort_by(|a, b| match (a.track(), b.track()) { + (Some(a), Some(b)) => match (key, direction) { + (SortKey::Title, SortDirection::Ascending) => { + a.title.to_lowercase().cmp(&b.title.to_lowercase()) + } + (SortKey::Title, SortDirection::Descending) => { + b.title.to_lowercase().cmp(&a.title.to_lowercase()) + } + (SortKey::Duration, SortDirection::Ascending) => a.duration.cmp(&b.duration), + (SortKey::Duration, SortDirection::Descending) => b.duration.cmp(&a.duration), + (SortKey::Album, SortDirection::Ascending) => a + .album + .map(|x| x.to_lowercase()) + .cmp(&b.album.map(|x| x.to_lowercase())), + (SortKey::Album, SortDirection::Descending) => b + .album + .map(|x| x.to_lowercase()) + .cmp(&a.album.map(|x| x.to_lowercase())), + (SortKey::Added, SortDirection::Ascending) => a.added_at.cmp(&b.added_at), + (SortKey::Added, SortDirection::Descending) => b.added_at.cmp(&a.added_at), + (SortKey::Artist, SortDirection::Ascending) => { + compare_artists(a.artists, b.artists) + } + (SortKey::Artist, SortDirection::Descending) => { + compare_artists(b.artists, a.artists) + } + }, + _ => std::cmp::Ordering::Equal, + }) + } + } } impl From<&SimplifiedPlaylist> for Playlist { diff --git a/src/spotify.rs b/src/spotify.rs index d6c20bb..77390e0 100644 --- a/src/spotify.rs +++ b/src/spotify.rs @@ -566,7 +566,12 @@ impl Spotify { .is_some() } - pub fn delete_tracks(&self, playlist_id: &str, track_pos_pairs: &[(Track, usize)]) -> bool { + pub fn delete_tracks( + &self, + playlist_id: &str, + snapshot_id: &str, + track_pos_pairs: &[(&Track, usize)], + ) -> bool { let mut tracks = Vec::new(); for (track, pos) in track_pos_pairs { let track_occurrence = json!({ @@ -581,7 +586,7 @@ impl Spotify { self.user.as_ref().unwrap(), playlist_id, tracks.clone(), - None, + Some(snapshot_id.to_string()), ) }) .is_some() diff --git a/src/track.rs b/src/track.rs index f347f82..01cc430 100644 --- a/src/track.rs +++ b/src/track.rs @@ -29,6 +29,7 @@ pub struct Track { pub cover_url: Option, pub url: String, pub added_at: Option>, + pub list_index: usize, } impl Track { @@ -65,6 +66,7 @@ impl Track { cover_url: album.images.get(0).map(|img| img.url.clone()), url: track.uri.clone(), added_at: None, + list_index: 0, } } @@ -104,6 +106,7 @@ impl From<&SimplifiedTrack> for Track { cover_url: None, url: track.uri.clone(), added_at: None, + list_index: 0, } } } @@ -143,6 +146,7 @@ impl From<&FullTrack> for Track { cover_url: track.album.images.get(0).map(|img| img.url.clone()), url: track.uri.clone(), added_at: None, + list_index: 0, } } } diff --git a/src/ui/listview.rs b/src/ui/listview.rs index c4da511..6a1bbc9 100644 --- a/src/ui/listview.rs +++ b/src/ui/listview.rs @@ -10,9 +10,7 @@ use cursive::{Cursive, Printer, Rect, Vec2}; use unicode_width::UnicodeWidthStr; use crate::artist::Artist; -use crate::command::{ - Command, GotoMode, JumpMode, MoveAmount, MoveMode, SortDirection, SortKey, TargetMode, -}; +use crate::command::{Command, GotoMode, JumpMode, MoveAmount, MoveMode, TargetMode}; use crate::commands::CommandResult; use crate::episode::Episode; use crate::library::Library; @@ -135,58 +133,6 @@ impl ListView { let mut c = self.content.write().unwrap(); c.remove(index); } - - pub fn sort(&self, key: &SortKey, direction: &SortDirection) { - fn compare_artists(a: Vec, b: Vec) -> Ordering { - let sanitize_artists_name = |x: Vec| -> Vec { - x.iter() - .map(|x| { - x.to_lowercase() - .split(' ') - .skip_while(|x| x == &"the") - .collect() - }) - .collect() - }; - - let a = sanitize_artists_name(a); - let b = sanitize_artists_name(b); - - a.cmp(&b) - } - - let mut c = self.content.write().unwrap(); - - c.sort_by(|a, b| match (a.track(), b.track()) { - (Some(a), Some(b)) => match (key, direction) { - (SortKey::Title, SortDirection::Ascending) => { - a.title.to_lowercase().cmp(&b.title.to_lowercase()) - } - (SortKey::Title, SortDirection::Descending) => { - b.title.to_lowercase().cmp(&a.title.to_lowercase()) - } - (SortKey::Duration, SortDirection::Ascending) => a.duration.cmp(&b.duration), - (SortKey::Duration, SortDirection::Descending) => b.duration.cmp(&a.duration), - (SortKey::Album, SortDirection::Ascending) => a - .album - .map(|x| x.to_lowercase()) - .cmp(&b.album.map(|x| x.to_lowercase())), - (SortKey::Album, SortDirection::Descending) => b - .album - .map(|x| x.to_lowercase()) - .cmp(&a.album.map(|x| x.to_lowercase())), - (SortKey::Added, SortDirection::Ascending) => a.added_at.cmp(&b.added_at), - (SortKey::Added, SortDirection::Descending) => b.added_at.cmp(&a.added_at), - (SortKey::Artist, SortDirection::Ascending) => { - compare_artists(a.artists, b.artists) - } - (SortKey::Artist, SortDirection::Descending) => { - compare_artists(b.artists, a.artists) - } - }, - _ => std::cmp::Ordering::Equal, - }) - } } impl View for ListView { diff --git a/src/ui/playlist.rs b/src/ui/playlist.rs index 26e8ae5..9ff8e1c 100644 --- a/src/ui/playlist.rs +++ b/src/ui/playlist.rs @@ -18,6 +18,7 @@ pub struct PlaylistView { list: ListView, spotify: Arc, library: Arc, + queue: Arc, } impl PlaylistView { @@ -25,6 +26,10 @@ impl PlaylistView { let mut playlist = playlist.clone(); playlist.load_tracks(queue.get_spotify()); + if let Some(order) = library.cfg.state().playlist_orders.get(&playlist.id) { + playlist.sort(&order.key, &order.direction); + } + let tracks = if let Some(t) = playlist.tracks.as_ref() { t.clone() } else { @@ -32,16 +37,18 @@ impl PlaylistView { }; let spotify = queue.get_spotify(); - let list = ListView::new(Arc::new(RwLock::new(tracks)), queue, library.clone()); - if let Some(order) = library.cfg.state().playlist_orders.get(&playlist.id) { - list.sort(&order.key, &order.direction); - } + let list = ListView::new( + Arc::new(RwLock::new(tracks)), + queue.clone(), + library.clone(), + ); Self { playlist, list, spotify, library, + queue, } } } @@ -58,20 +65,11 @@ impl ViewExt for PlaylistView { fn on_command(&mut self, s: &mut Cursive, cmd: &Command) -> Result { if let Command::Delete = cmd { let pos = self.list.get_selected_index(); - let tracks = if let Some(t) = self.playlist.tracks.as_ref() { - t.clone() - } else { - Vec::new() - }; - let track = tracks.get(pos); - if let Some(t) = track { - if self.playlist.delete_tracks( - &[(t.clone(), pos)], - self.spotify.clone(), - self.library.clone(), - ) { - self.list.remove(pos); - } + if self + .playlist + .delete_track(pos, self.spotify.clone(), self.library.clone()) + { + self.list.remove(pos); } return Ok(CommandResult::Consumed(None)); } @@ -86,7 +84,14 @@ impl ViewExt for PlaylistView { .playlist_orders .insert(self.playlist.id.clone(), order); }); - self.list.sort(key, direction); + + self.playlist.sort(key, direction); + let tracks = self.playlist.tracks.as_ref().unwrap_or(&Vec::new()).clone(); + self.list = ListView::new( + Arc::new(RwLock::new(tracks)), + self.queue.clone(), + self.library.clone(), + ); return Ok(CommandResult::Consumed(None)); }