From fcb539483779c4a0f9aacdd2a4646953eb73abda Mon Sep 17 00:00:00 2001 From: mierak <47547062+mierak@users.noreply.github.com> Date: Sun, 22 Dec 2024 13:13:50 +0100 Subject: [PATCH] fix: move in playlists sometimes failing (#184) --- src/context.rs | 32 ++++++- src/core/client.rs | 11 ++- src/core/update_loop.rs | 2 +- src/core/work.rs | 2 +- src/shared/events.rs | 7 +- src/shared/mpd_query.rs | 11 +++ src/ui/panes/album_art.rs | 4 +- src/ui/panes/playlists.rs | 49 +++++----- src/ui/panes/playlists/tests.rs | 160 +++++++++----------------------- 9 files changed, 126 insertions(+), 152 deletions(-) diff --git a/src/context.rs b/src/context.rs index a265d42..2579cb9 100644 --- a/src/context.rs +++ b/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( + &self, + on_done: impl FnOnce(&mut Client<'_>) -> Result + Send + 'static, + ) -> Result { + 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::() { + 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"); diff --git a/src/core/client.rs b/src/core/client.rs index 368393a..0f3877e 100644 --- a/src/core/client.rs +++ b/src/core/client.rs @@ -141,7 +141,7 @@ fn client_task(client_rx: &Receiver, event_tx: &Sender, 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 { 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) + } } } diff --git a/src/core/update_loop.rs b/src/core/update_loop.rs index e83493f..3176003 100644 --- a/src/core/update_loop.rs +++ b/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, diff --git a/src/core/work.rs b/src/core/work.rs index 03584e8..73e09a2 100644 --- a/src/core/work.rs +++ b/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) diff --git a/src/shared/events.rs b/src/shared/events.rs index b4d4771..35fc651 100644 --- a/src/shared/events.rs +++ b/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)] diff --git a/src/shared/mpd_query.rs b/src/shared/mpd_query.rs index d323bb0..25770e0 100644 --- a/src/shared/mpd_query.rs +++ b/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) -> Result + Send>, } +#[derive(derive_more::Debug, Builder)] +pub(crate) struct MpdQuerySync { + #[debug(skip)] + pub callback: Box) -> Result + Send>, + pub tx: Sender, +} + #[derive(derive_more::Debug)] pub struct MpdCommand { #[debug(skip)] @@ -68,4 +78,5 @@ pub(crate) enum MpdQueryResult { Outputs(Vec), Decoders(Vec), ExternalCommand(&'static [&'static str], Vec), + Any(Box), } diff --git a/src/ui/panes/album_art.rs b/src/ui/panes/album_art.rs index 7e3a0c0..ee3a86c 100644 --- a/src/ui/panes/album_art.rs +++ b/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), diff --git a/src/ui/panes/playlists.rs b/src/ui/panes/playlists.rs index 5372a8d..dce132c 100644 --- a/src/ui/panes/playlists.rs +++ b/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 diff --git a/src/ui/panes/playlists/tests.rs b/src/ui/panes/playlists/tests.rs index d8b8401..c3bc1ad 100644 --- a/src/ui/panes/playlists/tests.rs +++ b/src/ui/panes/playlists/tests.rs @@ -148,11 +148,7 @@ mod on_idle_event { mod browsing_songs { use crate::{ - config::tabs::PaneType, - shared::{ - events::{ClientRequest, WorkRequest}, - mpd_query::MpdQuery, - }, + shared::events::{ClientRequest, WorkRequest}, tests::fixtures::{client_request_channel, work_request_channel}, }; use crossbeam::channel::{Receiver, Sender}; @@ -200,6 +196,15 @@ mod on_idle_event { } // then + let rx2 = rx.clone(); + let new_songs = vec![song("s2"), song("s3"), song("s4")]; + let new_songs2 = new_songs.clone(); + std::thread::spawn(move || { + let req = rx2.recv().unwrap(); + if let ClientRequest::QuerySync(qry) = req { + qry.tx.send(MpdQueryResult::Any(Box::new(new_songs2))).unwrap(); + }; + }); screen .on_query_finished( REINIT, @@ -211,28 +216,6 @@ mod on_idle_event { ) .unwrap(); assert_eq!(screen.stack.previous().selected(), Some(&dir("pl3"))); - let qry = rx.recv_timeout(Duration::from_millis(100)).unwrap(); - assert!(matches!( - qry, - ClientRequest::MpdQuery(MpdQuery { - id: REINIT, - replace_id: None, - target: Some(PaneType::Playlists), - .. - }) - )); - let new_songs = vec![song("s2"), song("s3"), song("s4")]; - screen - .on_query_finished( - REINIT, - MpdQueryResult::SongsList { - data: new_songs.clone(), - origin_path: None, - }, - &app_context, - ) - .unwrap(); - assert_eq!( screen.stack.current().selected(), Some(&DirOrSong::Song(new_songs[1].clone())) @@ -280,6 +263,15 @@ mod on_idle_event { } // then + let rx2 = rx.clone(); + let new_songs = vec![song("s1"), song("s2")]; + let new_songs2 = new_songs.clone(); + std::thread::spawn(move || { + let req = rx2.recv().unwrap(); + if let ClientRequest::QuerySync(qry) = req { + qry.tx.send(MpdQueryResult::Any(Box::new(new_songs2))).unwrap(); + }; + }); screen .on_query_finished( REINIT, @@ -291,27 +283,6 @@ mod on_idle_event { ) .unwrap(); assert_eq!(screen.stack.previous().selected(), Some(&dir("pl3"))); - let qry = rx.recv_timeout(Duration::from_millis(100)).unwrap(); - assert!(matches!( - qry, - ClientRequest::MpdQuery(MpdQuery { - id: REINIT, - replace_id: None, - target: Some(PaneType::Playlists), - .. - }) - )); - let new_songs = vec![song("s1"), song("s2")]; - screen - .on_query_finished( - REINIT, - MpdQueryResult::SongsList { - data: new_songs.clone(), - origin_path: None, - }, - &app_context, - ) - .unwrap(); assert_eq!( screen.stack.current().selected(), @@ -360,6 +331,15 @@ mod on_idle_event { } // then + let rx2 = rx.clone(); + let new_songs = vec![song("s3"), song("s4")]; + let new_songs2 = new_songs.clone(); + std::thread::spawn(move || { + let req = rx2.recv().unwrap(); + if let ClientRequest::QuerySync(qry) = req { + qry.tx.send(MpdQueryResult::Any(Box::new(new_songs2))).unwrap(); + }; + }); screen .on_query_finished( REINIT, @@ -371,28 +351,6 @@ mod on_idle_event { ) .unwrap(); assert_eq!(screen.stack.previous().selected(), Some(&dir("pl3"))); - let qry = rx.recv_timeout(Duration::from_millis(100)).unwrap(); - assert!(matches!( - qry, - ClientRequest::MpdQuery(MpdQuery { - id: REINIT, - replace_id: None, - target: Some(PaneType::Playlists), - .. - }) - )); - let new_songs = vec![song("s3"), song("s4")]; - screen - .on_query_finished( - REINIT, - MpdQueryResult::SongsList { - data: new_songs.clone(), - origin_path: None, - }, - &app_context, - ) - .unwrap(); - assert_eq!( screen.stack.current().selected(), Some(&DirOrSong::Song(new_songs[0].clone())) @@ -440,6 +398,15 @@ mod on_idle_event { } // then + let rx2 = rx.clone(); + let new_songs = vec![song("s1"), song("s3"), song("s4")]; + let new_songs2 = new_songs.clone(); + std::thread::spawn(move || { + let req = rx2.recv().unwrap(); + if let ClientRequest::QuerySync(qry) = req { + qry.tx.send(MpdQueryResult::Any(Box::new(new_songs2))).unwrap(); + }; + }); screen .on_query_finished( REINIT, @@ -451,28 +418,6 @@ mod on_idle_event { ) .unwrap(); assert_eq!(screen.stack.previous().selected(), Some(&dir("pl3"))); - let qry = rx.recv_timeout(Duration::from_millis(100)).unwrap(); - assert!(matches!( - qry, - ClientRequest::MpdQuery(MpdQuery { - id: REINIT, - replace_id: None, - target: Some(PaneType::Playlists), - .. - }) - )); - let new_songs = vec![song("s1"), song("s3"), song("s4")]; - screen - .on_query_finished( - REINIT, - MpdQueryResult::SongsList { - data: new_songs.clone(), - origin_path: None, - }, - &app_context, - ) - .unwrap(); - assert_eq!( screen.stack.current().selected(), Some(&DirOrSong::Song(new_songs[1].clone())) @@ -521,6 +466,15 @@ mod on_idle_event { } // then + let rx2 = rx.clone(); + let new_songs = vec![song("s1"), song("s3"), song("s4")]; + let new_songs2 = new_songs.clone(); + std::thread::spawn(move || { + let req = rx2.recv().unwrap(); + if let ClientRequest::QuerySync(qry) = req { + qry.tx.send(MpdQueryResult::Any(Box::new(new_songs2))).unwrap(); + }; + }); screen .on_query_finished( REINIT, @@ -532,28 +486,6 @@ mod on_idle_event { ) .unwrap(); assert_eq!(screen.stack.previous().selected(), Some(&dir("pl4"))); - let qry = rx.recv_timeout(Duration::from_millis(100)).unwrap(); - assert!(matches!( - qry, - ClientRequest::MpdQuery(MpdQuery { - id: REINIT, - replace_id: None, - target: Some(PaneType::Playlists), - .. - }) - )); - let new_songs = vec![song("s1"), song("s3"), song("s4")]; - screen - .on_query_finished( - REINIT, - MpdQueryResult::SongsList { - data: new_songs.clone(), - origin_path: None, - }, - &app_context, - ) - .unwrap(); - assert_eq!( screen.stack.current().selected(), Some(&DirOrSong::Song(new_songs[1].clone()))