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

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

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<Spotify>,
         library: Arc<Library>,
     ) -> 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(index);
-                        tracks.remove(*pos);
-                    }
                     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<String>, b: Vec<String>) -> Ordering {
+            let sanitize_artists_name = |x: Vec<String>| -> Vec<String> {
+                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 {

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()

src/track.rs

@@ -29,6 +29,7 @@ pub struct Track {
     pub cover_url: Option<String>,
     pub url: String,
     pub added_at: Option<DateTime<Utc>>,
+    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,
         }
     }
 }

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::{
+use crate::command::{Command, GotoMode, JumpMode, MoveAmount, MoveMode, TargetMode};
-    Command, GotoMode, JumpMode, MoveAmount, MoveMode, SortDirection, SortKey, TargetMode,
-};
 use crate::commands::CommandResult;
 use crate::episode::Episode;
 use crate::library::Library;
@@ -135,58 +133,6 @@ impl<I: ListItem> ListView<I> {
         let mut c = self.content.write().unwrap();
         c.remove(index);
     }
-
-    pub fn sort(&self, key: &SortKey, direction: &SortDirection) {
-        fn compare_artists(a: Vec<String>, b: Vec<String>) -> Ordering {
-            let sanitize_artists_name = |x: Vec<String>| -> Vec<String> {
-                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<I: ListItem> View for ListView<I> {

src/ui/playlist.rs

@@ -18,6 +18,7 @@ pub struct PlaylistView {
     list: ListView<Track>,
     spotify: Arc<Spotify>,
     library: Arc<Library>,
+    queue: Arc<Queue>,
 }
 
 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());
+        let list = ListView::new(
-        if let Some(order) = library.cfg.state().playlist_orders.get(&playlist.id) {
+            Arc::new(RwLock::new(tracks)),
-            list.sort(&order.key, &order.direction);
+            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<CommandResult, String> {
         if let Command::Delete = cmd {
             let pos = self.list.get_selected_index();
-            let tracks = if let Some(t) = self.playlist.tracks.as_ref() {
+            if self
-                t.clone()
+                .playlist
-            } else {
+                .delete_track(pos, self.spotify.clone(), self.library.clone())
-                Vec::new()
+            {
-            };
+                self.list.remove(pos);
-            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);
-                }
             }
             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));
         }