fix: move in playlists sometimes failing (#184)

Author: mierak

src/context.rs

@@ -11,12 +11,13 @@ use crate::{
         events::ClientRequest,
         lrc::{Lrc, LrcIndex},
         macros::status_warn,
+        mpd_query::MpdQuerySync,
     },
     AppEvent, MpdCommand, MpdQuery, MpdQueryResult, WorkRequest,
 };
 use anyhow::{bail, Result};
 use bon::bon;
-use crossbeam::channel::{SendError, Sender};
+use crossbeam::channel::{bounded, SendError, Sender};
 
 pub struct AppContext {
     pub config: &'static Config,
@@ -78,6 +79,31 @@ impl AppContext {
         self.needs_render.replace(false);
     }
 
+    pub fn query_sync<T: Send + Sync + 'static>(
+        &self,
+        on_done: impl FnOnce(&mut Client<'_>) -> Result<T> + Send + 'static,
+    ) -> Result<T> {
+        let (tx, rx) = bounded(1);
+        let query = MpdQuerySync {
+            callback: Box::new(|client| Ok(MpdQueryResult::Any(Box::new((on_done)(client)?)))),
+            tx,
+        };
+
+        if let Err(err) = self.client_request_sender.send(ClientRequest::QuerySync(query)) {
+            log::error!(error:? = err; "Failed to send query request");
+            bail!("Failed to send sync query request");
+        }
+
+        if let MpdQueryResult::Any(any) = rx.recv()? {
+            if let Ok(val) = any.downcast::<T>() {
+                return Ok(*val);
+            }
+            bail!("Received unknown type answer for sync query request",);
+        }
+
+        bail!("Received unknown MpdQueryResult for sync query request");
+    }
+
     #[builder(finish_fn(name = query))]
     pub fn query(
         &self,
@@ -92,13 +118,13 @@ impl AppContext {
             replace_id,
             callback: Box::new(on_done),
         };
-        if let Err(err) = self.client_request_sender.send(ClientRequest::MpdQuery(query)) {
+        if let Err(err) = self.client_request_sender.send(ClientRequest::Query(query)) {
             log::error!(error:? = err; "Failed to send query request");
         }
     }
 
     pub fn command(&self, callback: impl FnOnce(&mut Client<'_>) -> Result<()> + Send + 'static) {
-        if let Err(err) = self.client_request_sender.send(ClientRequest::MpdCommand(MpdCommand {
+        if let Err(err) = self.client_request_sender.send(ClientRequest::Command(MpdCommand {
             callback: Box::new(callback),
         })) {
             log::error!(error:? = err; "Failed to send command request");

src/core/client.rs

@@ -141,7 +141,7 @@ fn client_task(client_rx: &Receiver<ClientRequest>, event_tx: &Sender<AppEvent>,
                                             buffer.push_back(request);
                                         }
                                         if buffer.iter().any(|request2| {
-                                            if let (ClientRequest::MpdQuery(q1), ClientRequest::MpdQuery(q2)) =
+                                            if let (ClientRequest::Query(q1), ClientRequest::Query(q2)) =
                                                 (&request, &request2)
                                             {
                                                 q1.should_be_skipped(q2)
@@ -283,14 +283,19 @@ fn check_connection(
 
 fn handle_client_request(client: &mut Client<'_>, request: ClientRequest) -> Result<WorkDone> {
     match request {
-        ClientRequest::MpdQuery(query) => Ok(WorkDone::MpdCommandFinished {
+        ClientRequest::Query(query) => Ok(WorkDone::MpdCommandFinished {
             id: query.id,
             target: query.target,
             data: (query.callback)(client)?,
         }),
-        ClientRequest::MpdCommand(command) => {
+        ClientRequest::Command(command) => {
             (command.callback)(client)?;
             Ok(WorkDone::None)
         }
+        ClientRequest::QuerySync(query) => {
+            let result = (query.callback)(client)?;
+            query.tx.send(result)?;
+            Ok(WorkDone::None)
+        }
     }
 }

src/core/update_loop.rs

@@ -48,7 +48,7 @@ impl UpdateLoop {
                 }
 
                 std::thread::sleep(update_interval);
-                if let Err(err) = work_tx.send(ClientRequest::MpdQuery(MpdQuery {
+                if let Err(err) = work_tx.send(ClientRequest::Query(MpdQuery {
                     id: "global_status_update",
                     target: None,
                     replace_id: None,

src/core/work.rs

@@ -39,7 +39,7 @@ fn handle_work_request(
         WorkRequest::Command(command) => {
             let callback = command.execute(config)?; // TODO log
             try_skip!(
-                client_tx.send(ClientRequest::MpdCommand(MpdCommand { callback })),
+                client_tx.send(ClientRequest::Command(MpdCommand { callback })),
                 "Failed to send client request to complete command"
             );
             Ok(WorkDone::None)

src/shared/events.rs

@@ -9,14 +9,15 @@ use crossterm::event::KeyEvent;
 use super::{
     lrc::LrcIndex,
     mouse_event::MouseEvent,
-    mpd_query::{MpdCommand, MpdQuery, MpdQueryResult},
+    mpd_query::{MpdCommand, MpdQuery, MpdQueryResult, MpdQuerySync},
 };
 
 #[derive(Debug)]
 #[allow(unused)]
 pub(crate) enum ClientRequest {
-    MpdQuery(MpdQuery),
-    MpdCommand(MpdCommand),
+    Query(MpdQuery),
+    QuerySync(MpdQuerySync),
+    Command(MpdCommand),
 }
 
 #[derive(Debug)]

src/shared/mpd_query.rs

@@ -1,3 +1,5 @@
+use std::any::Any;
+
 use crate::{
     config::tabs::PaneType,
     mpd::{
@@ -8,6 +10,7 @@ use crate::{
 };
 use anyhow::Result;
 use bon::Builder;
+use crossbeam::channel::Sender;
 use ratatui::widgets::ListItem;
 
 #[derive(derive_more::Debug, Builder)]
@@ -19,6 +22,13 @@ pub(crate) struct MpdQuery {
     pub callback: Box<dyn FnOnce(&mut Client<'_>) -> Result<MpdQueryResult> + Send>,
 }
 
+#[derive(derive_more::Debug, Builder)]
+pub(crate) struct MpdQuerySync {
+    #[debug(skip)]
+    pub callback: Box<dyn FnOnce(&mut Client<'_>) -> Result<MpdQueryResult> + Send>,
+    pub tx: Sender<MpdQueryResult>,
+}
+
 #[derive(derive_more::Debug)]
 pub struct MpdCommand {
     #[debug(skip)]
@@ -68,4 +78,5 @@ pub(crate) enum MpdQueryResult {
     Outputs(Vec<Output>),
     Decoders(Vec<Decoder>),
     ExternalCommand(&'static [&'static str], Vec<Song>),
+    Any(Box<dyn Any + Send + Sync>),
 }

src/ui/panes/album_art.rs

@@ -206,7 +206,7 @@ mod tests {
         if should_search {
             assert!(matches!(
                 rx.recv_timeout(Duration::from_millis(100)).unwrap(),
-                ClientRequest::MpdQuery(MpdQuery {
+                ClientRequest::Query(MpdQuery {
                     id: ALBUM_ART,
                     replace_id: Some(ALBUM_ART),
                     target: Some(PaneType::AlbumArt),
@@ -253,7 +253,7 @@ mod tests {
         if should_search {
             assert!(matches!(
                 rx.recv_timeout(Duration::from_millis(100)).unwrap(),
-                ClientRequest::MpdQuery(MpdQuery {
+                ClientRequest::Query(MpdQuery {
                     id: ALBUM_ART,
                     replace_id: Some(ALBUM_ART),
                     target: Some(PaneType::AlbumArt),

src/ui/panes/playlists.rs

@@ -217,30 +217,6 @@ impl Pane for PlaylistsPane {
                 self.stack = DirStack::new(data);
                 self.prepare_preview(context)?;
             }
-            (REINIT, MpdQueryResult::SongsList { data: songs, .. }) => {
-                // Select the same song by filename or index as before
-                let old_viewport_len = self.stack.current().state.viewport_len();
-
-                self.stack_mut()
-                    .replace(songs.into_iter().map(DirOrSong::Song).collect());
-                self.prepare_preview(context)?;
-                if let Some((idx, song)) = &self.selected_song {
-                    let idx_to_select = self
-                        .stack
-                        .current()
-                        .items
-                        .iter()
-                        .find_position(|item| item.as_path() == song)
-                        .map_or(*idx, |(idx, _)| idx);
-                    self.stack.current_mut().state.set_viewport_len(old_viewport_len);
-                    self.stack
-                        .current_mut()
-                        .state
-                        .select(Some(idx_to_select), context.config.scrolloff);
-                };
-                self.prepare_preview(context)?;
-                context.render()?;
-            }
             (REINIT, MpdQueryResult::DirOrSong { data, .. }) => {
                 let mut new_stack = DirStack::new(data);
                 let old_viewport_len = self.stack.current().state.viewport_len();
@@ -268,10 +244,33 @@ impl Pane for PlaylistsPane {
                         if let Some((idx, DirOrSong::Song(song))) = self.stack().current().selected_with_idx() {
                             self.selected_song = Some((idx, song.as_path().to_owned()));
                         }
+                        let playlist = playlist_name.to_owned();
                         self.stack = new_stack;
-                        self.open_or_play(false, context, REINIT)?;
                         self.stack_mut().current_mut().state.set_content_len(old_content_len);
                         self.stack_mut().current_mut().state.set_viewport_len(old_viewport_len);
+
+                        let songs =
+                            context.query_sync(move |client| Ok(client.list_playlist_info(&playlist, None)?))?;
+
+                        self.stack_mut().push(songs.into_iter().map(DirOrSong::Song).collect());
+                        self.prepare_preview(context)?;
+                        if let Some((idx, song)) = &self.selected_song {
+                            let idx_to_select = self
+                                .stack
+                                .current()
+                                .items
+                                .iter()
+                                .find_position(|item| item.as_path() == song)
+                                .map_or(*idx, |(idx, _)| idx);
+                            self.stack.current_mut().state.set_viewport_len(old_viewport_len);
+                            self.stack
+                                .current_mut()
+                                .state
+                                .select(Some(idx_to_select), context.config.scrolloff);
+                        };
+                        self.stack_mut().clear_preview();
+                        self.prepare_preview(context)?;
+                        context.render()?;
                     }
                     [] => {
                         let Some((selected_idx, selected_playlist)) = self