From 638211e5cf8e6308b1acb52e7ee9b676cd5ecf95 Mon Sep 17 00:00:00 2001 From: Vlad S Date: Tue, 9 Jun 2026 15:24:01 +0200 Subject: [PATCH] fix spotatui native device and selector behavior --- CHANGELOG.md | 1 + src/core/app.rs | 42 ++++++++++++++++++ src/infra/network/playback.rs | 49 ++++++++++++++++++++- src/infra/network/user.rs | 72 ++++++++++++++++++++++++++++--- src/infra/player/events.rs | 6 ++- src/infra/player/streaming.rs | 5 +++ src/runtime.rs | 5 +++ src/tui/handlers/mod.rs | 60 ++++++++++++++++++++++++-- src/tui/handlers/select_device.rs | 38 ++++++++++------ 9 files changed, 254 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ab7bbac9..bc059473 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,7 @@ ### Fixed +- **Native device selection and playback startup**: Made the local `spotatui` Connect device selectable when Spotify's devices API omits it, recovered stale native streaming sessions after long idle, fixed `Esc`/`Enter` handling in the device selector, and avoided `NO_ACTIVE_DEVICE` playback failures when native streaming is connected but no Spotify playback context is active ([#292](https://github.com/LargeModGames/spotatui/issues/292)). - **Search box no longer traps focus on submit**: Pressing `Enter` to run a search now always moves focus to the results list, including when re-searching while already on the Search screen (previously focus stayed stuck in the input box) ([#191](https://github.com/LargeModGames/spotatui/issues/191)). - **Cover-art load failures are non-fatal**: A failed album-image fetch is now logged and ignored instead of surfacing a blocking error and aborting the now-playing update, so playback metadata keeps updating when artwork can't be loaded ([#142](https://github.com/LargeModGames/spotatui/issues/142)). diff --git a/src/core/app.rs b/src/core/app.rs index 06ec6546..aefffabc 100644 --- a/src/core/app.rs +++ b/src/core/app.rs @@ -935,6 +935,10 @@ pub struct App { /// Reference to the native streaming player for direct control (bypasses event channel) #[cfg(feature = "streaming")] pub streaming_player: Option>, + /// Sender used to recover native streaming when a stale/disconnected player is detected. + #[cfg(feature = "streaming")] + pub streaming_recovery_tx: + Option>, /// Reference to MPRIS manager for emitting Seeked signals after native seeks #[cfg(all(feature = "mpris", target_os = "linux"))] pub mpris_manager: Option>, @@ -1151,6 +1155,8 @@ impl Default for App { last_dispatched_volume: None, #[cfg(feature = "streaming")] streaming_player: None, + #[cfg(feature = "streaming")] + streaming_recovery_tx: None, #[cfg(all(feature = "mpris", target_os = "linux"))] mpris_manager: None, #[cfg(feature = "cover-art")] @@ -1384,6 +1390,42 @@ impl App { self.status_message_expires_at = Some(Instant::now() + Duration::from_secs(ttl_secs)); } + #[cfg(feature = "streaming")] + pub fn request_native_streaming_recovery_if_disconnected( + &mut self, + reselect_device: bool, + ) -> bool { + let Some(player) = self.streaming_player.as_ref() else { + return false; + }; + + if player.is_connected() { + return false; + } + + self.streaming_player = None; + self.is_streaming_active = false; + self.native_activation_pending = false; + self.native_device_id = None; + self.native_is_playing = Some(false); + self.native_track_info = None; + self.native_playback_origin = None; + self.song_progress_ms = 0; + self.last_track_id = None; + self.last_device_activation = None; + self.seek_ms = None; + if reselect_device { + self.current_playback_context = None; + } + + self.set_status_message("Native streaming disconnected; attempting recovery.", 8); + if let Some(tx) = &self.streaming_recovery_tx { + let _ = tx.send(crate::infra::player::StreamingRecoveryRequest { reselect_device }); + } + self.dispatch(IoEvent::GetCurrentPlayback); + true + } + pub fn playlist_is_editable(&self, playlist: &SimplifiedPlaylist) -> bool { let Some(user) = &self.user else { return false; diff --git a/src/infra/network/playback.rs b/src/infra/network/playback.rs index bb15fd9c..48d8749a 100644 --- a/src/infra/network/playback.rs +++ b/src/infra/network/playback.rs @@ -241,6 +241,40 @@ async fn is_native_streaming_active_for_playback(network: &Network) -> bool { false } +#[cfg(feature = "streaming")] +async fn should_activate_native_streaming_for_playback(network: &Network) -> bool { + let saved_device_id = network.client_config.device_id.as_ref(); + let app = network.app.lock().await; + let Some(player) = app.streaming_player.as_ref() else { + return false; + }; + + if !player.is_connected() || app.current_playback_context.is_some() { + return false; + } + + let Some(saved_device_id) = saved_device_id else { + return true; + }; + + if app.native_device_id.as_ref() == Some(saved_device_id) { + return true; + } + + app.devices.as_ref().is_some_and(|payload| { + payload.devices.iter().any(|device| { + device.id.as_ref() == Some(saved_device_id) + && device.name.eq_ignore_ascii_case(player.device_name()) + }) + }) +} + +#[cfg(feature = "streaming")] +async fn request_native_streaming_recovery_if_disconnected(network: &Network) -> bool { + let mut app = network.app.lock().await; + app.request_native_streaming_recovery_if_disconnected(true) +} + #[cfg(feature = "streaming")] async fn requested_native_playback_origin( network: &Network, @@ -624,7 +658,14 @@ impl PlaybackNetwork for Network { // Check if we should use native streaming for playback #[cfg(feature = "streaming")] - if is_native_streaming_active_for_playback(self).await { + if request_native_streaming_recovery_if_disconnected(self).await { + return; + } + + #[cfg(feature = "streaming")] + if is_native_streaming_active_for_playback(self).await + || should_activate_native_streaming_for_playback(self).await + { if let Some(player) = current_streaming_player(self).await { let requested_origin = requested_native_playback_origin(self, &context_id, &uris).await; let native_route = resolve_native_playback_route(self, &context_id).await; @@ -1088,6 +1129,7 @@ impl PlaybackNetwork for Network { app.is_streaming_active = true; app.native_activation_pending = true; app.native_playback_origin = None; + app.native_device_id = Some(device_id.clone()); // Drop the stale previous-device context so playback routing follows the // native intent (is_streaming_active) until the next poll repopulates it // — mirrors the non-native transfer branch below. Without this, the first @@ -1095,6 +1137,11 @@ impl PlaybackNetwork for Network { app.current_playback_context = None; app.last_device_activation = Some(Instant::now()); app.instant_since_last_current_playback_poll = Instant::now() - Duration::from_secs(6); + if persist_device_id { + if let Err(e) = self.client_config.set_device_id(device_id) { + app.handle_error(anyhow!(e)); + } + } return; } } diff --git a/src/infra/network/user.rs b/src/infra/network/user.rs index 75346ff2..b6d0a6bf 100644 --- a/src/infra/network/user.rs +++ b/src/infra/network/user.rs @@ -12,6 +12,8 @@ use rspotify::model::{ track::FullTrack, user::PrivateUser, }; +#[cfg(feature = "streaming")] +use rspotify::model::{enums::DeviceType, Device}; use rspotify::prelude::*; use serde::Deserialize; use std::time::{Duration, Instant}; @@ -21,6 +23,44 @@ struct ArtistTopTracksResponse { tracks: Vec, } +#[cfg(feature = "streaming")] +fn include_native_streaming_device(app: &crate::core::app::App, payload: &mut DevicePayload) { + let Some(player) = app.streaming_player.as_ref() else { + return; + }; + + if !player.is_connected() { + return; + } + + let device_name = player.device_name(); + let device_id = app + .native_device_id + .clone() + .unwrap_or_else(|| player.device_id()); + + if let Some(device) = payload + .devices + .iter_mut() + .find(|device| device.name.eq_ignore_ascii_case(device_name)) + { + if device.id.is_none() { + device.id = Some(device_id); + } + return; + } + + payload.devices.push(Device { + id: Some(device_id), + is_active: app.is_streaming_active, + is_private_session: false, + is_restricted: false, + name: device_name.to_string(), + _type: DeviceType::Computer, + volume_percent: Some(player.get_volume().into()), + }); +} + pub trait UserNetwork { async fn get_user(&mut self); async fn get_devices(&mut self); @@ -53,16 +93,36 @@ impl UserNetwork for Network { } async fn get_devices(&mut self) { - if let Ok(result) = self + match self .spotify_get_typed::("me/player/devices", &[]) .await { - let mut app = self.app.lock().await; - app.push_navigation_stack(RouteId::SelectedDevice, ActiveBlock::SelectDevice); - if !result.devices.is_empty() { + Ok(result) => { + let mut app = self.app.lock().await; + app.push_navigation_stack(RouteId::SelectedDevice, ActiveBlock::SelectDevice); + + #[cfg(feature = "streaming")] + let mut result = result; + #[cfg(feature = "streaming")] + { + let recovering = app.request_native_streaming_recovery_if_disconnected(true); + if !recovering { + include_native_streaming_device(&app, &mut result); + } + } + + app.selected_device_index = if result.devices.is_empty() { + None + } else { + app + .selected_device_index + .filter(|index| *index < result.devices.len()) + .or(Some(0)) + }; app.devices = Some(result); - // Select the first device in the list - app.selected_device_index = Some(0); + } + Err(e) => { + self.handle_error(anyhow!(e)).await; } } } diff --git a/src/infra/player/events.rs b/src/infra/player/events.rs index c1e7f4d9..7d9f4415 100644 --- a/src/infra/player/events.rs +++ b/src/infra/player/events.rs @@ -122,7 +122,11 @@ async fn handle_streaming_recovery(mut ctx: StreamingRecoveryContext) { /// Get the currently active streaming player (if any). pub async fn active_streaming_player(app: &Arc>) -> Option> { let app_lock = app.lock().await; - app_lock.streaming_player.clone() + app_lock + .streaming_player + .as_ref() + .filter(|player| player.is_connected()) + .cloned() } pub fn spawn_player_event_handler(ctx: PlayerEventContext) { diff --git a/src/infra/player/streaming.rs b/src/infra/player/streaming.rs index b25da3f7..276b2dbd 100644 --- a/src/infra/player/streaming.rs +++ b/src/infra/player/streaming.rs @@ -495,6 +495,11 @@ impl StreamingPlayer { &self.config.device_name } + /// Get the Spotify Connect device id for this session. + pub fn device_id(&self) -> String { + self.session.device_id().to_string() + } + /// Check if the session is connected pub fn is_connected(&self) -> bool { self.spirc_alive.load(Ordering::Relaxed) diff --git a/src/runtime.rs b/src/runtime.rs index 6d0999b0..4326b4ff 100644 --- a/src/runtime.rs +++ b/src/runtime.rs @@ -896,6 +896,11 @@ screens more often and cost more CPU. Animation-heavy views keep their separate #[cfg(feature = "streaming")] let (streaming_recovery_tx, streaming_recovery_rx) = tokio::sync::mpsc::unbounded_channel::(); + #[cfg(feature = "streaming")] + { + let mut app_mut = app.lock().await; + app_mut.streaming_recovery_tx = Some(streaming_recovery_tx.clone()); + } // Initialize MPRIS D-Bus integration for desktop media control // This registers spotatui as a controllable media player on the session bus diff --git a/src/tui/handlers/mod.rs b/src/tui/handlers/mod.rs index 5489b33b..43596400 100644 --- a/src/tui/handlers/mod.rs +++ b/src/tui/handlers/mod.rs @@ -467,11 +467,14 @@ fn handle_escape(app: &mut App) { ActiveBlock::Party => { app.pop_navigation_stack(); } - ActiveBlock::LyricsView | ActiveBlock::CoverArtView | ActiveBlock::MiniPlayer => { + ActiveBlock::SelectDevice + | ActiveBlock::LyricsView + | ActiveBlock::CoverArtView + | ActiveBlock::MiniPlayer => { app.pop_navigation_stack(); } - // These are global views that have no active/inactive distinction so do nothing - ActiveBlock::SelectDevice | ActiveBlock::Analysis => {} + // This is a global view that has no active/inactive distinction so do nothing + ActiveBlock::Analysis => {} // Announcement prompt must be dismissed with Enter/Esc, not global escape ActiveBlock::AnnouncementPrompt => {} @@ -567,6 +570,7 @@ mod tests { use chrono::Utc; use rspotify::model::{ context::{Actions, CurrentPlaybackContext}, + device::DevicePayload, enums::{DeviceType, RepeatState}, idtypes::PlaylistId, CurrentlyPlayingType, Device, PlayableId, PlayableItem, @@ -619,6 +623,56 @@ mod tests { assert_eq!(app.get_current_route().active_block, ActiveBlock::Empty); } + #[test] + fn escape_exits_device_selector() { + let mut app = App::default(); + app.push_navigation_stack(RouteId::SelectedDevice, ActiveBlock::SelectDevice); + + handle_app(Key::Esc, &mut app); + + assert_ne!( + app.get_current_route().active_block, + ActiveBlock::SelectDevice + ); + } + + #[test] + fn enter_on_device_selector_dispatches_transfer_and_exits() { + let (tx, rx) = channel(); + let mut app = App::new(tx, UserConfig::new(), SystemTime::now()); + app.devices = Some(DevicePayload { + devices: vec![Device { + id: Some("device-1".to_string()), + is_active: false, + is_private_session: false, + is_restricted: false, + name: "Desk Speaker".to_string(), + _type: DeviceType::Computer, + volume_percent: Some(42), + }], + }); + app.selected_device_index = Some(0); + app.push_navigation_stack(RouteId::SelectedDevice, ActiveBlock::SelectDevice); + + handle_app(Key::Enter, &mut app); + + match rx.recv().unwrap() { + IoEvent::TransferPlaybackToDevice(device_id, persist_device_id) => { + assert_eq!(device_id, "device-1"); + assert!(persist_device_id); + } + _ => panic!("unexpected event"), + } + assert_ne!( + app.get_current_route().active_block, + ActiveBlock::SelectDevice + ); + assert_eq!( + app.status_message.as_deref(), + Some("Switching playback to Desk Speaker") + ); + } + #[test] fn global_shift_f_likes_current_track_from_anywhere() { let (tx, rx) = channel(); diff --git a/src/tui/handlers/select_device.rs b/src/tui/handlers/select_device.rs index 7bf25ef2..801e6674 100644 --- a/src/tui/handlers/select_device.rs +++ b/src/tui/handlers/select_device.rs @@ -1,13 +1,10 @@ use super::common_key_events; -use crate::core::app::{ActiveBlock, App}; +use crate::core::app::App; use crate::infra::network::IoEvent; use crate::tui::event::Key; pub fn handler(key: Key, app: &mut App) { match key { - Key::Esc => { - app.set_current_route_state(Some(ActiveBlock::Library), None); - } k if common_key_events::down_event(k) => { if let Some(p) = &app.devices { if let Some(selected_device_index) = app.selected_device_index { @@ -51,15 +48,30 @@ pub fn handler(key: Key, app: &mut App) { }; } Key::Enter => { - if let Some(index) = app.selected_device_index { - if let Some(devices) = &app.devices { - if let Some(device) = devices.devices.get(index) { - if let Some(device_id) = &device.id { - app.dispatch(IoEvent::TransferPlaybackToDevice(device_id.clone(), true)); - } - } - } - } + let Some(index) = app.selected_device_index else { + app.set_status_message("No playback device selected", 4); + return; + }; + + let Some(devices) = &app.devices else { + app.set_status_message("No playback devices found", 4); + return; + }; + + let Some(device) = devices.devices.get(index) else { + app.set_status_message("Selected playback device is no longer available", 4); + return; + }; + + let Some(device_id) = &device.id else { + app.set_status_message("Selected playback device has no Spotify device id", 4); + return; + }; + + let device_name = device.name.clone(); + app.dispatch(IoEvent::TransferPlaybackToDevice(device_id.clone(), true)); + app.set_status_message(format!("Switching playback to {}", device_name), 4); + app.pop_navigation_stack(); } _ => {} }