Playlist completely server side

[hasezoey]

This PR refactors the playlist handling to have all playlist state handling completely done server-side, no more ReloadPlaylist send from the TUI and no more touching playlist.log from the TUI. Some more details:

• doing this required that Playlist be shared between GeneralPlayer(player thread) and the grpc service, which introduced a lot of .write(), .read() & drop(playlist) noise
• change cryptic function PlaySelected to PlaySpecific with a index attached, this makes it a lot clearer what this function should do

I know this is quite a big PR but i didnt want to split it into multiple parts as it would otherwise be mixed in terms of calling conventions.

Known Issues:

• events from the TUI are asynchronously handled, as in: the event is send, but the TUI is not blocked until the event has been received via the stream, which can result in some more specific cases:
• if a user sends a PlaySpecific request, and then moves the selected / highlighted list item before the event is received, the selected list item will become the currently playing index again (making for a little bad user experience) Fixed in a later commit
• on shuffle, currently the TUI gets the playlist twice, once from executing the shuffle command, and once when getting the shuffle event as there is currently no way to know if both are for the same state (more discussion on this necessary) fixed in a later commit
• there can be a little lag between the user in the TUI executing a event and before the finished result of that action is reflected in the TUI, while still being able to navigate & execute more events from the TUI (ie the user deletes a track from the playlist, moves to another track, then sometime later gets the event that one track was removed)

Most of these known issues should not really be a problem for the way termusic currently has worked, as it still requires to run on the same machine, which will make the lag for now negligible. (also because indexes are send and verified via track ids to be the same before applying any event)

Please note that this PR is marked as a DRAFT to facilitate some more testing before merging.

re #152

[hasezoey]

on shuffle, currently the TUI gets the playlist twice, once from executing the shuffle command, and once when getting the shuffle event as there is currently no way to know if both are for the same state (more discussion on this necessary)

Some more discussion on that, for now i have a Proof-of-Concept with a monotonically increasing u64 each time a shuffle (and potentially other events) are executed for the playlist, which is send along the main data and the highest is stored in the tui.
Alternatively i have also though about just adding something like a uuid(v4) to each event that can be send along, which the tui may need to keep track of, but the server can just send-and-forget about those uuids.
EDIT: some more alternatives i could think of:

• make the sync state value based on time
• make the TUI keep track of what events it sends and then check that it matches the order it gets the events back, if it does, ignore the events from the stream (as to not re-request data), if it does not match, re-request the data

@tramhao which of those 2 solutions would likely be the best, or do you have any suggestions regarding that?
Also i would appriciate if you could test this PR and provide feedback before merging it.

[hasezoey]

Actually i have now moved the playlist tracks getting for after a shuffle to be in the event instead of returned in the ShufflePlaylist grpc function, which simplifies the keeping track and not duplicating work, making my last comment irrelevant.

[tramhao]

on shuffle, currently the TUI gets the playlist twice, once from executing the shuffle command, and once when getting the shuffle event as there is currently no way to know if both are for the same state (more discussion on this necessary)

Some more discussion on that, for now i have a Proof-of-Concept with a monotonically increasing u64 each time a shuffle (and potentially other events) are executed for the playlist, which is send along the main data and the highest is stored in the tui. Alternatively i have also though about just adding something like a uuid(v4) to each event that can be send along, which the tui may need to keep track of, but the server can just send-and-forget about those uuids. EDIT: some more alternatives i could think of:

• make the sync state value based on time
• make the TUI keep track of what events it sends and then check that it matches the order it gets the events back, if it does, ignore the events from the stream (as to not re-request data), if it does not match, re-request the data

@tramhao which of those 2 solutions would likely be the best, or do you have any suggestions regarding that? Also i would appriciate if you could test this PR and provide feedback before merging it.

In my mind, there are two shuffle modes. One is when playing next track, the other is reordering the whole list. I think if you keep the sync state, it'll be more complicated as we need to assume that the server can work without TUI. If you ask TUI to keep track of what events it sends, it seems that it's more like a procedure thing, not event-driven. What do you think?

[hasezoey]

In my mind, there are two shuffle modes. One is when playing next track, the other is reordering the whole list.

I kinda forgot, i should have mentioned it. This PR only affect the Playlist::shuffle code / what you get when you press r to shuffle the whole playlist. LoopMode::Random is unmodified by this PR.
I also just tested LoopMode::Random, it has worked before this PR and also still works with this PR without problems on the server side.

I think if you keep the sync state, it'll be more complicated as we need to assume that the server can work without TUI

The Server can currently work without the TUI and the sync_state i was proposing was just a "change value" that does not affect the server, but may have needed to be kept by the server to populate it to clients on specific requests.
I dont know what this pattern / value may otherwise be called, but it also exists in something like mongoose as __v, if you are familiar with that.

If you ask TUI to keep track of what events it sends, it seems that it's more like a procedure thing, not event-driven

I do not quite understand what you mean with procedure thing, could you elaborate?
What i was proposing was so that we could maybe return the data directly from rpc calls such as ShufflePlaylist, while also giving out events like PlaylistShuffled, which would trigger clients to re-sync the playlist (other than the client that called that specific shuffle, as that already got the data), this event and the previous rpc call return would contain the same "sync state" variable.
It would likely also be useful for if we would use a "shadow state" (i dont know what its actually called), ie apply the change in the TUI before the server confirms that change, that way the TUI would be more responsive instead of either changing stuff later when the user has moved-on or blocking the user-input until the server has confirmed the event.
I was kinda trying so that the events would not contain the whole playlists, as those can get quite big (lets say in the thousands, maybe more), and only have clients ask for the playlist if they actually need it.

As for the TUI keeping track of the events, it was so that it keeps a list of events itself sends, and if they come in in that order, everything is fine (for example for the "shadow state"), if they contain events not in that order, the playlist (and possibly other data) should be re-synced. This could happen when another client triggers a event.

POC of what i was proposing

diff --git a/lib/proto/player.proto b/lib/proto/player.proto
index 26d7758b..e833054a 100644
--- a/lib/proto/player.proto
+++ b/lib/proto/player.proto
@@ -166,6 +166,8 @@ message PlaylistPlaySpecific {
 message PlaylistTracks {
   uint64 current_track_index = 1;
   repeated PlaylistAddTrack tracks = 2;
+  
+  uint64 sync_state = 3;
 }
 
 message UpdatePlaylist {
@@ -263,7 +265,7 @@ message PlaylistTracksToRemoveClear {
 
 /// Indicate that the playlist has been shuffled and should be re-fetched
 message PlaylistShuffled {
-  // empty as there are no values, but not using "Empty" to have a unique message id
+  uint64 sync_state = 1;
 }
 
 // A Identifier for a track.
diff --git a/lib/src/player.rs b/lib/src/player.rs
index 03c1b277..d9ffab08 100644
--- a/lib/src/player.rs
+++ b/lib/src/player.rs
@@ -204,6 +204,11 @@ pub struct PlaylistSwapInfo {
     pub index_b: u64,
 }
 
+#[derive(Debug, Clone, PartialEq)]
+pub struct PlaylistShuffledInfo {
+    pub sync_state: u64,
+}
+
 /// Separate nested enum to handle all playlist related events
 #[derive(Debug, Clone, PartialEq)]
 pub enum UpdatePlaylistEvents {
@@ -212,7 +217,7 @@ pub enum UpdatePlaylistEvents {
     PlaylistCleared,
     PlaylistLoopMode(PlaylistLoopModeInfo),
     PlaylistSwapTracks(PlaylistSwapInfo),
-    PlaylistShuffled,
+    PlaylistShuffled(PlaylistShuffledInfo),
 }
 
 type PPlaylistTypes = protobuf::update_playlist::Type;
@@ -247,8 +252,10 @@ impl From<UpdatePlaylistEvents> for protobuf::UpdatePlaylist {
                     index_b: vals.index_b,
                 })
             }
-            UpdatePlaylistEvents::PlaylistShuffled => {
-                PPlaylistTypes::Shuffled(protobuf::PlaylistShuffled {})
+            UpdatePlaylistEvents::PlaylistShuffled(vals) => {
+                PPlaylistTypes::Shuffled(protobuf::PlaylistShuffled {
+                    sync_state: vals.sync_state,
+                })
             }
         };
 
@@ -293,7 +300,9 @@ impl TryFrom<protobuf::UpdatePlaylist> for UpdatePlaylistEvents {
                 index_a: ev.index_a,
                 index_b: ev.index_b,
             }),
-            PPlaylistTypes::Shuffled(_) => Self::PlaylistShuffled,
+            PPlaylistTypes::Shuffled(ev) => Self::PlaylistShuffled(PlaylistShuffledInfo {
+                sync_state: ev.sync_state,
+            }),
         };
 
         Ok(res)
diff --git a/playback/src/playlist.rs b/playback/src/playlist.rs
index fc99b6fc..c40e9a73 100644
--- a/playback/src/playlist.rs
+++ b/playback/src/playlist.rs
@@ -17,6 +17,7 @@ use termusiclib::player::playlist_helpers::PlaylistSwapTrack;
 use termusiclib::player::playlist_helpers::PlaylistTrackSource;
 use termusiclib::player::playlist_helpers::{PlaylistAddTrack, PlaylistRemoveTrackIndexed};
 use termusiclib::player::PlaylistLoopModeInfo;
+use termusiclib::player::PlaylistShuffledInfo;
 use termusiclib::player::PlaylistSwapInfo;
 use termusiclib::player::PlaylistTracks;
 use termusiclib::player::UpdateEvents;
@@ -89,6 +90,11 @@ pub struct Playlist {
     /// Indicator if the playlist should advance the `current_*` and `next_*` values
     need_proceed_to_next: bool,
     stream_tx: StreamTX,
+
+    /// For the lack of a better way, keep track of some sync points (like after a shuffle) that will be send along with events
+    /// that way, some clients may need to do less work
+    // TODO: consider replacing this with something else in the future
+    sync_state_counter: u64,
 }
 
 impl Playlist {
@@ -108,6 +114,7 @@ impl Playlist {
             next_track_index: None,
             need_proceed_to_next: false,
             stream_tx,
+            sync_state_counter: 0,
         }
     }
 
@@ -911,8 +918,13 @@ impl Playlist {
         let current_track_file = self.get_current_track();
 
         self.tracks.shuffle(&mut thread_rng());
+        self.sync_state_counter += 1;
 
-        self.send_stream_ev(UpdatePlaylistEvents::PlaylistShuffled);
+        self.send_stream_ev(UpdatePlaylistEvents::PlaylistShuffled(
+            PlaylistShuffledInfo {
+                sync_state: self.sync_state_counter,
+            },
+        ));
 
         if let Some(current_track_file) = current_track_file {
             if let Some(index) = self.find_index_from_file(&current_track_file) {
@@ -1058,6 +1070,15 @@ impl Playlist {
         self.next_track_index.is_some()
     }
 
+    /// Get the current sync state.
+    ///
+    /// A Sync state is monotonically increasing, and the same state indicates that no event happened inbetween
+    /// and so may not need to reload the playlist
+    #[must_use]
+    pub fn get_sync_state(&self) -> u64 {
+        self.sync_state_counter
+    }
+
     /// Send stream events with consistent error handling
     fn send_stream_ev(&self, ev: UpdatePlaylistEvents) {
         // there is only one error case: no receivers
diff --git a/server/src/music_player_service.rs b/server/src/music_player_service.rs
index 9f5532c0..ad291464 100644
--- a/server/src/music_player_service.rs
+++ b/server/src/music_player_service.rs
@@ -375,5 +375,6 @@ fn playlist_to_grpc_tracks(playlist: &Playlist) -> PlaylistTracks {
     PlaylistTracks {
         current_track_index: u64::try_from(playlist.get_current_track_index()).unwrap(),
         tracks,
+        sync_state: playlist.get_sync_state(),
     }
 }
diff --git a/tui/src/ui/mod.rs b/tui/src/ui/mod.rs
index ee563ec3..00f6bceb 100644
--- a/tui/src/ui/mod.rs
+++ b/tui/src/ui/mod.rs
@@ -63,6 +63,8 @@ pub struct UI {
     model: Model,
     playback: Playback,
     cmd_rx: UnboundedReceiver<TuiCmd>,
+
+    last_playlist_sync_state: u64,
 }
 
 impl UI {
@@ -84,6 +86,8 @@ impl UI {
             model,
             playback,
             cmd_rx,
+
+            last_playlist_sync_state: 0,
         })
     }
 
@@ -347,6 +351,7 @@ impl UI {
             }
             PlaylistCmd::Shuffle => {
                 let new_tracks = self.playback.shuffle_playlist().await?;
+                self.last_playlist_sync_state = new_tracks.sync_state;
                 self.model
                     .playlist
                     .load_from_grpc(new_tracks, &self.model.podcast.db_podcast)?;
@@ -451,11 +456,13 @@ impl UI {
             UpdatePlaylistEvents::PlaylistSwapTracks(swapped_tracks) => {
                 self.model.handle_playlist_swap_tracks(&swapped_tracks)?;
             }
-            UpdatePlaylistEvents::PlaylistShuffled => {
-                // NOTE: the current implementation will reload the playlist on this client, even if this client was
-                // the one that had requested the shuffle and already applied the returned playlist values
-                self.model
-                    .command(TuiCmd::Playlist(PlaylistCmd::SelfReloadPlaylist));
+            UpdatePlaylistEvents::PlaylistShuffled(shuffled) => {
+                if shuffled.sync_state > self.last_playlist_sync_state {
+                    self.model
+                        .command(TuiCmd::Playlist(PlaylistCmd::SelfReloadPlaylist));
+                } else {
+                    debug!("Recieved PlaylistShuffled event, but sync_state {} is not more than current", shuffled.sync_state);
+                }
             }
         }
 
@@ -466,6 +473,7 @@ impl UI {
     async fn load_playlist(&mut self) -> Result<()> {
         info!("Requesting Playlist from server");
         let tracks = self.playback.get_playlist().await?;
+        self.last_playlist_sync_state = tracks.sync_state;
         self.model
             .playlist
             .load_from_grpc(tracks, &self.model.podcast.db_podcast)?;

But all that does not really apply anymore to this PR, as with 70ba03e i changed the rpc call ShufflePlaylist to not return anything anymore, and the event PlaylistShuffled to contain the whole playlist.

[hasezoey]

Added some changes to not re-select the new "current track index" in the TUI playlist table/list component, if the previous "current track index" was not selected. (ie a user has selected another item or is moving through the playlist, while a track change happens, it now does not reset to the "current track index")
Also re-select the selected track in the TUI playlist component after a shuffle (best effort, as Tracks are not unique currently).

Known Issues:

• if the current playing track index advances to what the user has selected, then another track change happens, the next "current track index" will be selected as the TUI currently does not know if the selected index was because of user input or automatic "current track index" setting) (ie playing track is 0, user has selected 1, track change happens, next track is 1 (also selected), user does not move but next track happens, playing track is 2 an selected track is 2)

[hasezoey]

Fixed accidentally sending 2 removal events and the TUI blindly applying the index changes without checking the ids, leading to playlist de-syncs.

[tramhao]

It would likely also be useful for if we would use a "shadow state" (i dont know what its actually called), ie apply the change in the TUI before the server confirms that change, that way the TUI would be more responsive instead of either changing stuff later when the user has moved-on or blocking the user-input until the server has confirmed the event.
I was kinda trying so that the events would not contain the whole playlists, as those can get quite big (lets say in the thousands, maybe more), and only have clients ask for the playlist if they actually need it.

This is great and I love this way:)

[hasezoey]

This is great and I love this way:)

Just to confirm, is this regarding the "shadow state" or event not containing the whole playlist?

---

Also just though about instead of a "shadow state", as in showing the state after the event, how about showing it differently (either through different colors, italics or a added symbol) that something is happening regarding that playlist entry? (for example we could on add make the entry "less colorful"(like grey) and on removal either make it "red" or make it also "less colorful"(would likely conflict with add) and / or add a loading symbol)
This would likely still cause some kind of disruption, but it would show the user that something is happening and that some action may not be fully complete yet.

[tramhao]

event not containing the whole playlist

[hasezoey]

Just as a FYI, i would like to delay merging this PR until after the next version (#439) releases, afterwards this PR could be merged to get some more wide testing.
Aside from that, unless new problems are discovered, i would say this PR is feature-complete, anything else (like removing the full playlist from the event again), would be a follow-up PR.

[hasezoey]

Btw, i just tried this PR with multiple clients, and it actually works quite good, compared to the before de-syncs that could happen. (i sometimes do multiple clients to test terminal-specific things, like cover debugging)

[tramhao]

Just as a FYI, i would like to delay merging this PR until after the next version (#439) releases, afterwards this PR could be merged to get some more wide testing. Aside from that, unless new problems are discovered, i would say this PR is feature-complete, anything else (like removing the full playlist from the event again), would be a follow-up PR.

No problem.