diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index b49dbb81..d0cd6f4f 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -88,7 +88,7 @@ units under `src/`: | `infra/` | Spotify Web API (`network/`), native librespot streaming (`player/`), alternative sources (`local/`, `subsonic/`, `qobuz/`, `radio/`, `youtube/`, `queue/`), audio viz (`audio/`), Lua scripting (`scripting/`), AI DJ + MCP (`dj/`, `mcp/`), OS integrations (Discord RPC, MPRIS, macOS/Windows media) | | `tui/` | Terminal UI: the event/render loop (`runner.rs`), key plumbing (`event/`), per-block input handlers (`handlers/`), immutable draw fns (`ui/`) | | `cli/` | clap subcommands: playback control, listening history, self-update, MCP relay, plugin management | -| `runtime/` | `mod.rs::run_cli` (entry point + CLI dispatch), `bootstrap.rs::boot` (frontend-neutral config/auth/`App` construction, `run_cli` its sole caller), `cli.rs` (clap assembly + self-update), `pump.rs::start_tokio` (the IoEvent pump), `streaming/` (native-streaming startup every frontend shares: the pure saved-device decision in `mod.rs`, the librespot bring-up in `launch.rs`, gated on `streaming`), `startup.rs` (the UI-launch half, gated on `tui`) | +| `runtime/` | `mod.rs::run_cli` (entry point + CLI dispatch), `bootstrap.rs::boot` (frontend-neutral config/auth/`App` construction, `run_cli` its sole caller, plus the boot auth rule `spotify_auth_mode`: interactive only right after the client wizard or `--reconfigure-auth`, a subcommand needs a cached token, a UI launch tolerates no session), `cli.rs` (clap assembly + self-update), `pump.rs::start_tokio` (the IoEvent pump), `streaming/` (native-streaming startup every frontend shares: the pure saved-device decision in `mod.rs`, the librespot bring-up in `launch.rs`, gated on `streaming`), `startup.rs` (the UI-launch half, gated on `tui`) | ### Data flow @@ -124,6 +124,11 @@ worth knowing before adding an event: (`qobuz:`) → `route_radio_event` (`radio:`) → `route_youtube_event` (`youtube:`) → `Network::handle_network_event`. This is what keeps `infra/network/` Spotify-only. +- **Claim gate**: before the routers, `start_playback_has_taker` drops a + `StartPlayback` whose URI scheme (`core::queue::queue_item_source`) names no + compiled-in source and that no Spotify session can take. The routers' + foreign-start teardown arms therefore only run for a real source-to-source + handoff. - **Service lane**: `Network::runs_on_service_lane` lists events that run on a detached task so slow, source-agnostic work cannot head-of-line-block the serial pump. The service lane's `Network` is built with **no Spotify client** - adding a @@ -187,7 +192,11 @@ fixtures are `pub(super) fn`s in `test_support.rs`, imported as Multiple players share one UI, and the predicate order is the #1 source of regressions. Check in this order: `queue_owns_playback()` / `queue_now_is_spotify()`, then `active_decoded_source()`, then -`is_native_streaming_active_for_playback()`. +`is_native_streaming_active_for_playback()`. `App::playback_owner()` folds them +into one `PlaybackOwner`, and the transport chains (play/pause, +next, previous, shuffle, repeat, volume) end on `dispatch_spotify_fallback`, +which answers "Nothing is playing" instead of a Spotify dispatch when no +session exists. - Starting a decoded source (Local/Subsonic/Qobuz/Radio/YouTube) only **pauses** librespot - the native flag stays true, so driving librespot directly resumes diff --git a/AGENTS.md b/AGENTS.md index b49fd84e..ccfeeeee 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -90,7 +90,7 @@ units under `src/`: | `infra/` | Spotify Web API (`network/`), native librespot streaming (`player/`), alternative sources (`local/`, `subsonic/`, `qobuz/`, `radio/`, `youtube/`, `queue/`), audio viz (`audio/`), Lua scripting (`scripting/`), AI DJ + MCP (`dj/`, `mcp/`), OS integrations (Discord RPC, MPRIS, macOS/Windows media) | | `tui/` | Terminal UI: the event/render loop (`runner.rs`), key plumbing (`event/`), per-block input handlers (`handlers/`), immutable draw fns (`ui/`) | | `cli/` | clap subcommands: playback control, listening history, self-update, MCP relay, plugin management | -| `runtime/` | `mod.rs::run_cli` (entry point + CLI dispatch), `bootstrap.rs::boot` (frontend-neutral config/auth/`App` construction, `run_cli` its sole caller), `cli.rs` (clap assembly + self-update), `pump.rs::start_tokio` (the IoEvent pump), `streaming/` (native-streaming startup every frontend shares: the pure saved-device decision in `mod.rs`, the librespot bring-up in `launch.rs`, gated on `streaming`), `startup.rs` (the UI-launch half, gated on `tui`) | +| `runtime/` | `mod.rs::run_cli` (entry point + CLI dispatch), `bootstrap.rs::boot` (frontend-neutral config/auth/`App` construction, `run_cli` its sole caller, plus the boot auth rule `spotify_auth_mode`: interactive only right after the client wizard or `--reconfigure-auth`, a subcommand needs a cached token, a UI launch tolerates no session), `cli.rs` (clap assembly + self-update), `pump.rs::start_tokio` (the IoEvent pump), `streaming/` (native-streaming startup every frontend shares: the pure saved-device decision in `mod.rs`, the librespot bring-up in `launch.rs`, gated on `streaming`), `startup.rs` (the UI-launch half, gated on `tui`) | ### Data flow @@ -126,6 +126,11 @@ worth knowing before adding an event: (`qobuz:`) → `route_radio_event` (`radio:`) → `route_youtube_event` (`youtube:`) → `Network::handle_network_event`. This is what keeps `infra/network/` Spotify-only. +- **Claim gate**: before the routers, `start_playback_has_taker` drops a + `StartPlayback` whose URI scheme (`core::queue::queue_item_source`) names no + compiled-in source and that no Spotify session can take. The routers' + foreign-start teardown arms therefore only run for a real source-to-source + handoff. - **Service lane**: `Network::runs_on_service_lane` lists events that run on a detached task so slow, source-agnostic work cannot head-of-line-block the serial pump. The service lane's `Network` is built with **no Spotify client** - adding a @@ -189,7 +194,11 @@ fixtures are `pub(super) fn`s in `test_support.rs`, imported as Multiple players share one UI, and the predicate order is the #1 source of regressions. Check in this order: `queue_owns_playback()` / `queue_now_is_spotify()`, then `active_decoded_source()`, then -`is_native_streaming_active_for_playback()`. +`is_native_streaming_active_for_playback()`. `App::playback_owner()` folds them +into one `PlaybackOwner`, and the transport chains (play/pause, +next, previous, shuffle, repeat, volume) end on `dispatch_spotify_fallback`, +which answers "Nothing is playing" instead of a Spotify dispatch when no +session exists. - Starting a decoded source (Local/Subsonic/Qobuz/Radio/YouTube) only **pauses** librespot - the native flag stays true, so driving librespot directly resumes diff --git a/CLAUDE.md b/CLAUDE.md index df6b7311..9533e54b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -90,7 +90,7 @@ units under `src/`: | `infra/` | Spotify Web API (`network/`), native librespot streaming (`player/`), alternative sources (`local/`, `subsonic/`, `qobuz/`, `radio/`, `youtube/`, `queue/`), audio viz (`audio/`), Lua scripting (`scripting/`), AI DJ + MCP (`dj/`, `mcp/`), OS integrations (Discord RPC, MPRIS, macOS/Windows media) | | `tui/` | Terminal UI: the event/render loop (`runner.rs`), key plumbing (`event/`), per-block input handlers (`handlers/`), immutable draw fns (`ui/`) | | `cli/` | clap subcommands: playback control, listening history, self-update, MCP relay, plugin management | -| `runtime/` | `mod.rs::run_cli` (entry point + CLI dispatch), `bootstrap.rs::boot` (frontend-neutral config/auth/`App` construction, `run_cli` its sole caller), `cli.rs` (clap assembly + self-update), `pump.rs::start_tokio` (the IoEvent pump), `streaming/` (native-streaming startup every frontend shares: the pure saved-device decision in `mod.rs`, the librespot bring-up in `launch.rs`, gated on `streaming`), `startup.rs` (the UI-launch half, gated on `tui`) | +| `runtime/` | `mod.rs::run_cli` (entry point + CLI dispatch), `bootstrap.rs::boot` (frontend-neutral config/auth/`App` construction, `run_cli` its sole caller, plus the boot auth rule `spotify_auth_mode`: interactive only right after the client wizard or `--reconfigure-auth`, a subcommand needs a cached token, a UI launch tolerates no session), `cli.rs` (clap assembly + self-update), `pump.rs::start_tokio` (the IoEvent pump), `streaming/` (native-streaming startup every frontend shares: the pure saved-device decision in `mod.rs`, the librespot bring-up in `launch.rs`, gated on `streaming`), `startup.rs` (the UI-launch half, gated on `tui`) | ### Data flow @@ -126,6 +126,11 @@ worth knowing before adding an event: (`qobuz:`) → `route_radio_event` (`radio:`) → `route_youtube_event` (`youtube:`) → `Network::handle_network_event`. This is what keeps `infra/network/` Spotify-only. +- **Claim gate**: before the routers, `start_playback_has_taker` drops a + `StartPlayback` whose URI scheme (`core::queue::queue_item_source`) names no + compiled-in source and that no Spotify session can take. The routers' + foreign-start teardown arms therefore only run for a real source-to-source + handoff. - **Service lane**: `Network::runs_on_service_lane` lists events that run on a detached task so slow, source-agnostic work cannot head-of-line-block the serial pump. The service lane's `Network` is built with **no Spotify client** - adding a @@ -189,7 +194,11 @@ fixtures are `pub(super) fn`s in `test_support.rs`, imported as Multiple players share one UI, and the predicate order is the #1 source of regressions. Check in this order: `queue_owns_playback()` / `queue_now_is_spotify()`, then `active_decoded_source()`, then -`is_native_streaming_active_for_playback()`. +`is_native_streaming_active_for_playback()`. `App::playback_owner()` folds them +into one `PlaybackOwner`, and the transport chains (play/pause, +next, previous, shuffle, repeat, volume) end on `dispatch_spotify_fallback`, +which answers "Nothing is playing" instead of a Spotify dispatch when no +session exists. - Starting a decoded source (Local/Subsonic/Qobuz/Radio/YouTube) only **pauses** librespot - the native flag stays true, so driving librespot directly resumes diff --git a/src/core/action/apply.rs b/src/core/action/apply.rs index 258d2d25..9e9443a1 100644 --- a/src/core/action/apply.rs +++ b/src/core/action/apply.rs @@ -53,7 +53,7 @@ impl App { RepeatSetting::Track => RepeatState::Track, RepeatSetting::Context => RepeatState::Context, }; - self.dispatch(IoEvent::Repeat(state)); + self.dispatch_spotify_fallback(IoEvent::Repeat(state)); } Action::PlayUris { uris, offset } => self.start_playback_uris(uris, offset), Action::PlayContext { uri, offset } => self.start_playback_context(uri, offset), @@ -226,12 +226,8 @@ impl App { Action::RecommendFromTrackId { id, name } => { self.load_recommendations_for_track_id(id, name); } - Action::StartParty => { - self.dispatch(IoEvent::StartParty( - crate::infra::network::sync::ControlMode::HostOnly, - )); - } - Action::JoinParty { code, name } => self.dispatch(IoEvent::JoinParty { code, name }), + Action::StartParty => self.start_party(), + Action::JoinParty { code, name } => self.join_party(code, name), Action::LeaveParty => self.dispatch(IoEvent::LeaveParty), Action::TogglePartyControlMode => self.toggle_party_control_mode(), Action::SetPlaybarSegment { plugin, text } => match text { diff --git a/src/core/action/tests.rs b/src/core/action/tests.rs index 64bce834..761b764b 100644 --- a/src/core/action/tests.rs +++ b/src/core/action/tests.rs @@ -11,7 +11,7 @@ use std::sync::mpsc::{channel, Receiver}; use std::time::SystemTime; use super::{Action, NavTarget, RepeatSetting}; -use crate::core::app::{App, RouteId, UserInfo}; +use crate::core::app::{App, RouteId, UserInfo, NOTHING_PLAYING_STATUS}; use crate::core::theme::{Color, Theme, ThemeField}; use crate::core::user_config::UserConfig; use crate::infra::network::IoEvent; @@ -22,6 +22,13 @@ fn app_with_channel() -> (App, Receiver) { (app, rx) } +/// The same fixture with no Spotify session (`spotify_connected == false`). +fn session_free_app_with_channel() -> (App, Receiver) { + let (tx, rx) = channel(); + let app = App::new(tx, UserConfig::new(), None); + (app, rx) +} + #[allow(deprecated)] fn playback_context( is_playing: bool, @@ -266,6 +273,62 @@ fn toggle_shuffle_flips_the_spotify_shuffle_state() { assert!(matches!(rx.try_recv(), Ok(IoEvent::Shuffle(true)))); } +// --- transport without a Spotify session --- + +#[test] +fn next_track_without_a_session_reports_nothing_playing() { + let (mut app, rx) = session_free_app_with_channel(); + + app.apply(Action::NextTrack); + + assert!(rx.try_recv().is_err()); + assert_eq!(app.status_message.as_deref(), Some(NOTHING_PLAYING_STATUS)); +} + +#[test] +fn toggle_playback_without_a_session_reports_nothing_playing() { + let (mut app, rx) = session_free_app_with_channel(); + + app.apply(Action::TogglePlayback); + + assert!(rx.try_recv().is_err()); + assert_eq!(app.status_message.as_deref(), Some(NOTHING_PLAYING_STATUS)); +} + +#[test] +fn volume_down_without_a_session_leaves_no_latch() { + // The fixture starts at 100%, so only a decrease reaches the API fallback. + let (mut app, rx) = session_free_app_with_channel(); + + app.apply(Action::VolumeDown); + + assert!(rx.try_recv().is_err()); + assert_eq!(app.status_message.as_deref(), Some(NOTHING_PLAYING_STATUS)); + assert!(!app.is_volume_change_in_flight); + assert!(app.pending_volume.is_none()); +} + +#[test] +fn set_repeat_without_a_session_reports_nothing_playing() { + let (mut app, rx) = session_free_app_with_channel(); + + app.apply(Action::SetRepeat(RepeatSetting::Track)); + + assert!(rx.try_recv().is_err()); + assert_eq!(app.status_message.as_deref(), Some(NOTHING_PLAYING_STATUS)); +} + +#[test] +fn flush_pending_volume_without_a_session_clears_the_pending_value() { + let (mut app, rx) = session_free_app_with_channel(); + app.pending_volume = Some(40); + + app.flush_pending_volume(); + + assert!(rx.try_recv().is_err()); + assert!(app.pending_volume.is_none()); +} + // --- jump-to navigation --- #[test] @@ -388,6 +451,18 @@ fn play_uris_carries_the_offset() { )); } +#[test] +fn play_uris_with_an_empty_list_dispatches_nothing() { + let (mut app, rx) = app_with_channel(); + + app.apply(Action::PlayUris { + uris: vec![], + offset: None, + }); + + assert!(rx.try_recv().is_err()); +} + #[test] fn play_context_dispatches_a_context_start() { let (mut app, rx) = app_with_channel(); @@ -2130,6 +2205,36 @@ fn select_source_spotify_fetches_no_sidebar() { assert!(rx.try_recv().is_err(), "expected no IoEvent dispatched"); } +#[test] +fn selecting_spotify_without_a_session_does_not_reach_disk() { + let dir = tempfile::tempdir().unwrap(); + let (mut app, _rx) = session_free_app_with_channel(); + app.state_path = Some(dir.path().join("state.yml")); + + app.apply(Action::SelectSource(Source::Spotify)); + + assert_eq!(app.active_source, Source::Spotify); + assert!(!dir.path().join("state.yml").exists()); + + app.spotify_connected = true; + app.persist_active_source(); + + let written = std::fs::read_to_string(dir.path().join("state.yml")).unwrap(); + assert!(written.contains("active_source") && written.contains("Spotify")); +} + +#[test] +fn selecting_a_free_source_without_a_session_still_persists() { + let dir = tempfile::tempdir().unwrap(); + let (mut app, _rx) = session_free_app_with_channel(); + app.state_path = Some(dir.path().join("state.yml")); + + app.apply(Action::SelectSource(Source::Local)); + + let written = std::fs::read_to_string(dir.path().join("state.yml")).unwrap(); + assert!(written.contains("active_source") && written.contains("Local")); +} + // --- the now-playing item family --- use super::CopyTarget; diff --git a/src/core/app/construction.rs b/src/core/app/construction.rs index 6b17795b..d255fffe 100644 --- a/src/core/app/construction.rs +++ b/src/core/app/construction.rs @@ -236,6 +236,16 @@ impl Default for App { } impl App { + /// `App::default()` with a Spotify session, for tests that need one without + /// an `IoEvent` channel. + #[cfg(all(test, feature = "tui"))] + pub(crate) fn default_connected() -> App { + App { + spotify_connected: true, + ..App::default() + } + } + #[cfg(test)] pub fn new( io_tx: Sender, diff --git a/src/core/app/mod.rs b/src/core/app/mod.rs index 806327e7..1348dcba 100644 --- a/src/core/app/mod.rs +++ b/src/core/app/mod.rs @@ -128,6 +128,7 @@ pub use native_backend::*; pub use native_recovery::*; #[cfg(feature = "streaming")] pub(crate) use native_shuffle::*; +pub use playback_routing::*; pub use playlist_folders::*; pub use playlists::*; pub use plugins::*; diff --git a/src/core/app/party.rs b/src/core/app/party.rs index f86fbb77..0439af33 100644 --- a/src/core/app/party.rs +++ b/src/core/app/party.rs @@ -1,6 +1,31 @@ use super::*; +const PARTY_NEEDS_SPOTIFY: &str = + "Listening Party needs Spotify. Press `d` and pick Spotify to log in."; + impl App { + /// Host a party; needs a Spotify session, the relay drives Spotify playback. + pub(crate) fn start_party(&mut self) { + if !self.spotify_connected { + self.set_status_message(PARTY_NEEDS_SPOTIFY, 6); + return; + } + self.dispatch(IoEvent::StartParty(ControlMode::HostOnly)); + } + + /// Join a party; same session requirement as `start_party`. + pub(crate) fn join_party(&mut self, code: String, name: String) { + if !self.spotify_connected { + self.set_status_message(PARTY_NEEDS_SPOTIFY, 6); + return; + } + self.dispatch(IoEvent::JoinParty { code, name }); + // The typed code and name are consumed only by a join that went out. + self.view.party_input.clear(); + self.view.party_input_idx = 0; + self.view.party_join_name.clear(); + } + /// The local write is optimistic on purpose: the relay handler never writes /// the session back, so the popup's "Control" label renders from it. pub(crate) fn toggle_party_control_mode(&mut self) { @@ -15,3 +40,58 @@ impl App { self.dispatch(IoEvent::SetPartyControlMode(updated_mode)); } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::core::action::Action; + use crate::core::app::test_support::*; + + #[test] + fn start_party_without_a_session_dispatches_nothing_and_says_why() { + let (mut app, rx) = session_free_app(); + + app.apply(Action::StartParty); + + assert!(rx.try_recv().is_err()); + assert_eq!(app.status_message.as_deref(), Some(PARTY_NEEDS_SPOTIFY)); + } + + #[test] + fn join_party_without_a_session_dispatches_nothing_and_keeps_the_input() { + let (mut app, rx) = session_free_app(); + app.view.party_input = "ABC123".chars().collect(); + app.view.party_input_idx = 6; + app.view.party_join_name = "Guest".chars().collect(); + + app.apply(Action::JoinParty { + code: "ABC123".to_string(), + name: "Guest".to_string(), + }); + + assert!(rx.try_recv().is_err()); + assert_eq!(app.status_message.as_deref(), Some(PARTY_NEEDS_SPOTIFY)); + assert_eq!(app.view.party_input.iter().collect::(), "ABC123"); + assert_eq!(app.view.party_input_idx, 6); + assert_eq!(app.view.party_join_name.iter().collect::(), "Guest"); + } + + #[test] + fn join_party_with_a_session_dispatches_and_clears_the_input() { + let (tx, rx) = channel(); + let mut app = App::new(tx, UserConfig::new(), Some(SystemTime::now())); + app.view.party_input = "ABC123".chars().collect(); + app.view.party_input_idx = 6; + app.view.party_join_name = "Guest".chars().collect(); + + app.apply(Action::JoinParty { + code: "ABC123".to_string(), + name: "Guest".to_string(), + }); + + assert!(matches!(rx.try_recv(), Ok(IoEvent::JoinParty { .. }))); + assert!(app.view.party_input.is_empty()); + assert_eq!(app.view.party_input_idx, 0); + assert!(app.view.party_join_name.is_empty()); + } +} diff --git a/src/core/app/playback_routing.rs b/src/core/app/playback_routing.rs index cd1d0f47..24f69295 100644 --- a/src/core/app/playback_routing.rs +++ b/src/core/app/playback_routing.rs @@ -1,6 +1,80 @@ use super::*; +pub(crate) const NOTHING_PLAYING_STATUS: &str = "Nothing is playing"; + +/// The status shown when a Spotify-bound request finds no session. +pub(crate) const SPOTIFY_NOT_CONNECTED_STATUS: &str = + "Spotify not connected. Press `d` and pick Spotify to log in."; + +/// Who owns the audio output, in the order the transport chains check. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub enum PlaybackOwner { + /// The native queue slot (a decoded track or a queued Spotify track). + Queue, + /// A decoded source's sink (Local, Subsonic, Qobuz, Radio, YouTube). + Decoded, + /// librespot as the active Connect device. + #[cfg_attr(not(feature = "streaming"), allow(dead_code))] + NativeSpotify, + /// A Spotify session with no local player: an external device or idle. + Spotify, + /// No player and no session. + None, +} + impl App { + pub(crate) fn playback_owner(&self) -> PlaybackOwner { + if self.queue_owns_playback() { + return PlaybackOwner::Queue; + } + if self.active_decoded_source() { + return PlaybackOwner::Decoded; + } + #[cfg(feature = "streaming")] + if self.is_native_streaming_active_for_playback() { + return PlaybackOwner::NativeSpotify; + } + if self.spotify_connected { + return PlaybackOwner::Spotify; + } + PlaybackOwner::None + } + + /// The last arm of a transport chain: the Web API when a session exists, + /// otherwise a source-neutral status instead of the "not connected" nag. + pub(crate) fn dispatch_spotify_fallback(&mut self, event: IoEvent) { + if self.playback_owner() == PlaybackOwner::None { + self.set_status_message(NOTHING_PLAYING_STATUS, 4); + return; + } + self.dispatch(event); + } + + /// `Some(true)` when a decoded source owns the sink and plays, `Some(false)` + /// when it owns the sink and is paused, `None` when none owns it. + pub(crate) fn decoded_playing_state(&self) -> Option { + #[cfg(any( + feature = "local-files", + feature = "subsonic", + feature = "qobuz", + feature = "internet-radio", + feature = "youtube" + ))] + { + self.active_decoded_player().map(|p| !p.is_paused()) + } + #[cfg(not(any( + feature = "local-files", + feature = "subsonic", + feature = "qobuz", + feature = "internet-radio", + feature = "youtube" + )))] + { + None + } + } + /// Check if native streaming is the active playback device /// Returns true while the player is connected or reconnecting and it is the /// currently active device. @@ -209,16 +283,6 @@ impl App { feature = "internet-radio", feature = "youtube" ))] - // Consumed only by the OS media integrations (MPRIS / macOS / Windows), so - // builds with decoded sources but none of those integrations leave it unused. - #[cfg_attr( - not(any( - all(feature = "mpris", target_os = "linux"), - all(feature = "macos-media", target_os = "macos"), - all(feature = "windows-media", target_os = "windows") - )), - allow(dead_code) - )] pub fn active_decoded_player(&self) -> Option<&std::sync::Arc> { #[cfg(any( feature = "local-files", @@ -299,7 +363,7 @@ impl App { } } -#[cfg(all(test, feature = "streaming"))] +#[cfg(test)] mod tests { use super::*; use crate::core::app::test_support::*; @@ -316,6 +380,7 @@ mod tests { assert!(!app.active_decoded_source()); assert!(app.active_source_position_ms().is_none()); + assert_eq!(app.playback_owner(), PlaybackOwner::Queue); } #[cfg(all(feature = "streaming", feature = "audio-decode"))] @@ -330,4 +395,36 @@ mod tests { assert!(app.active_decoded_player().is_none()); } + + #[test] + fn playback_owner_is_none_without_a_session() { + let (app, _rx) = session_free_app(); + + assert_eq!(app.playback_owner(), PlaybackOwner::None); + } + + #[test] + fn playback_owner_is_spotify_with_a_session_and_no_player() { + assert_eq!(make_app_simple().playback_owner(), PlaybackOwner::Spotify); + } + + #[test] + fn dispatch_spotify_fallback_reports_nothing_playing_without_a_session() { + let (mut app, rx) = session_free_app(); + + app.dispatch_spotify_fallback(IoEvent::NextTrack); + + assert!(rx.try_recv().is_err()); + assert_eq!(app.status_message.as_deref(), Some(NOTHING_PLAYING_STATUS)); + } + + #[test] + fn dispatch_spotify_fallback_dispatches_with_a_session() { + let (tx, rx) = channel(); + let mut app = App::new(tx, UserConfig::new(), Some(SystemTime::now())); + + app.dispatch_spotify_fallback(IoEvent::NextTrack); + + assert!(matches!(rx.try_recv(), Ok(IoEvent::NextTrack))); + } } diff --git a/src/core/app/route.rs b/src/core/app/route.rs index 6157de02..6882e9df 100644 --- a/src/core/app/route.rs +++ b/src/core/app/route.rs @@ -226,6 +226,17 @@ impl App { pub fn set_active_source(&mut self, source: Source) { self.active_source = source; self.runtime_state.active_source = source; + // The Spotify scope reaches disk when the login succeeds (see + // `persist_active_source`), so a cancelled login never forces a browser at + // the next boot. + if source == Source::Spotify && !self.spotify_connected { + return; + } + self.persist_active_source(); + } + + /// Write the current browse scope to `state.yml`. + pub(crate) fn persist_active_source(&mut self) { if let Err(e) = self.save_runtime_state( &crate::core::state::PersistedRuntimeState::active_source(self.runtime_state.active_source), ) { diff --git a/src/core/app/seek.rs b/src/core/app/seek.rs index 7acc4d7b..3cda77b6 100644 --- a/src/core/app/seek.rs +++ b/src/core/app/seek.rs @@ -31,16 +31,19 @@ impl App { "seeking forwards by {} ms", self.user_config.behavior.seek_milliseconds ); - // A seekable decoded source (local/subsonic/youtube) owns the session: seek - // relative to *its* live position, never from the stale/foreign Spotify - // progress. Radio returns None here, so its seek keys are correct no-ops. - // The source player clamps to the track duration internally, so no upper - // clamp is needed (and we must not read the stale Spotify context duration). - if let Some(pos) = self.active_source_position_ms() { - let new_progress = (pos as u32).saturating_add(self.user_config.behavior.seek_milliseconds); - self.song_progress_ms = new_progress as u128; - self.seek_ms = None; - self.dispatch(IoEvent::Seek(new_progress)); + // A decoded source owns the session: seek relative to *its* live position, + // never from the stale/foreign Spotify progress. The source player clamps + // to the track duration internally, so no upper clamp is needed (and we + // must not read the stale Spotify context duration). Radio has no position + // (not seekable): stop there instead of repositioning the paused Spotify + // player underneath it. + if self.active_decoded_source() { + if let Some(pos) = self.active_source_position_ms() { + let new_progress = (pos as u32).saturating_add(self.user_config.behavior.seek_milliseconds); + self.song_progress_ms = new_progress as u128; + self.seek_ms = None; + self.dispatch(IoEvent::Seek(new_progress)); + } return; } if let Some(CurrentPlaybackContext { @@ -97,14 +100,17 @@ impl App { "seeking backwards by {} ms", self.user_config.behavior.seek_milliseconds ); - // A seekable decoded source (local/subsonic/youtube) owns the session: seek - // relative to *its* live position, never from the stale/foreign Spotify - // progress. Radio returns None here, so its seek keys are correct no-ops. - if let Some(pos) = self.active_source_position_ms() { - let new_progress = (pos as u32).saturating_sub(self.user_config.behavior.seek_milliseconds); - self.song_progress_ms = new_progress as u128; - self.seek_ms = None; - self.dispatch(IoEvent::Seek(new_progress)); + // A decoded source owns the session: seek relative to *its* live position, + // never from the stale/foreign Spotify progress. Radio has no position (not + // seekable): stop there instead of repositioning the paused Spotify player + // underneath it. + if self.active_decoded_source() { + if let Some(pos) = self.active_source_position_ms() { + let new_progress = (pos as u32).saturating_sub(self.user_config.behavior.seek_milliseconds); + self.song_progress_ms = new_progress as u128; + self.seek_ms = None; + self.dispatch(IoEvent::Seek(new_progress)); + } return; } let old_progress = match self.seek_ms { @@ -145,15 +151,18 @@ impl App { /// dragging on the playbar progress line). The target is clamped to the track /// duration. Mirrors the dispatch logic of [`Self::seek_forwards`]. pub fn seek_to(&mut self, position_ms: u32) { - // A seekable decoded source (local/subsonic/youtube) owns the session: seek - // it to the absolute target directly (the source player clamps to the track - // duration internally). Radio returns None here, so its playbar drags are - // correct no-ops. Never read the stale Spotify context duration for a source. - if self.active_source_position_ms().is_some() { - // Decoded `.seek()` can re-decode forward for many codecs, so a mouse - // drag must not dispatch one seek per drag event; coalesce to the last - // target with the same throttle-and-flush pattern as the other backends. - self.queue_source_seek(position_ms); + // A decoded source owns the session: seek it to the absolute target directly + // (the source player clamps to the track duration internally). Never read + // the stale Spotify context duration for a source. Radio has no position + // (not seekable): stop there instead of repositioning the paused Spotify + // player underneath it. + if self.active_decoded_source() { + if self.active_source_position_ms().is_some() { + // Decoded `.seek()` can re-decode forward for many codecs, so a mouse + // drag must not dispatch one seek per drag event; coalesce to the last + // target with the same throttle-and-flush pattern as the other backends. + self.queue_source_seek(position_ms); + } return; } if let Some(CurrentPlaybackContext { diff --git a/src/core/app/shuffle_repeat.rs b/src/core/app/shuffle_repeat.rs index 7edae10d..adf2a06f 100644 --- a/src/core/app/shuffle_repeat.rs +++ b/src/core/app/shuffle_repeat.rs @@ -244,7 +244,7 @@ impl App { } // Fallback to API-based shuffle for external devices - self.dispatch(IoEvent::Shuffle(new_shuffle_state)); + self.dispatch_spotify_fallback(IoEvent::Shuffle(new_shuffle_state)); }; } @@ -314,7 +314,7 @@ impl App { } // Fallback to API-based repeat for external devices - self.dispatch(IoEvent::Repeat(current_repeat_state)); + self.dispatch_spotify_fallback(IoEvent::Repeat(current_repeat_state)); } } } diff --git a/src/core/app/test_support.rs b/src/core/app/test_support.rs index 306b0192..a6c2ecfa 100644 --- a/src/core/app/test_support.rs +++ b/src/core/app/test_support.rs @@ -106,3 +106,9 @@ pub(super) fn make_app_simple() -> App { let (tx, _rx) = channel(); App::new(tx, UserConfig::new(), Some(SystemTime::now())) } + +/// An `App` with no Spotify session; the receiver keeps `dispatch` observable. +pub(super) fn session_free_app() -> (App, std::sync::mpsc::Receiver) { + let (tx, rx) = channel(); + (App::new(tx, UserConfig::new(), None), rx) +} diff --git a/src/core/app/tick.rs b/src/core/app/tick.rs index 472ba250..9ec2f22c 100644 --- a/src/core/app/tick.rs +++ b/src/core/app/tick.rs @@ -11,6 +11,17 @@ const _: () = assert!( STALE_TICK_AFTER.as_millis() >= 2 * crate::core::user_config::MAX_TICK_RATE_MILLISECONDS as u128 ); +/// Whether the machine must stay awake. The audible player answers first: a +/// decoded source that owns the sink overrides the suspended librespot flag and +/// the stale Spotify context it left behind. +fn playing_for_keepawake( + decoded: Option, + native: Option, + spotify: Option, +) -> bool { + decoded.or(native).or(spotify).unwrap_or(false) +} + impl App { /// Milliseconds into the current track, or `None` when no tick has run for /// over [`STALE_TICK_AFTER`]. A frontend that stops driving @@ -202,10 +213,11 @@ impl App { self.poll_current_playback(); let playing_now = self.user_config.behavior.keepawake_enabled - && self - .native_is_playing - .or_else(|| self.current_playback_context.as_ref().map(|c| c.is_playing)) - .unwrap_or(false); + && playing_for_keepawake( + self.decoded_playing_state(), + self.native_is_playing, + self.current_playback_context.as_ref().map(|c| c.is_playing), + ); match (playing_now, self.keepawake.is_some()) { (true, false) => { self.keepawake = keepawake::Builder::default() @@ -375,4 +387,33 @@ mod tests { assert!(matches!(rx.try_recv(), Ok(IoEvent::GetCurrentPlayback))); assert!(app.is_fetching_current_playback); } + + #[test] + fn a_paused_decoded_source_overrides_the_suspended_spotify_state() { + // The Spotify-to-decoded handoff only pauses librespot, so both the native + // flag and the context it left behind can still read as playing. + assert!(!playing_for_keepawake(Some(false), Some(true), Some(true))); + } + + #[test] + fn a_playing_decoded_source_keeps_the_machine_awake() { + assert!(playing_for_keepawake(Some(true), Some(false), Some(false))); + } + + #[test] + fn without_a_decoded_owner_the_native_flag_decides() { + assert!(playing_for_keepawake(None, Some(true), Some(false))); + assert!(!playing_for_keepawake(None, Some(false), Some(true))); + } + + #[test] + fn without_a_decoded_owner_or_a_native_flag_spotify_decides() { + assert!(playing_for_keepawake(None, None, Some(true))); + assert!(!playing_for_keepawake(None, None, Some(false))); + } + + #[test] + fn nothing_playing_lets_the_machine_sleep() { + assert!(!playing_for_keepawake(None, None, None)); + } } diff --git a/src/core/app/transport.rs b/src/core/app/transport.rs index 0cf1a2b8..a56bd7e7 100644 --- a/src/core/app/transport.rs +++ b/src/core/app/transport.rs @@ -165,10 +165,10 @@ impl App { }; if is_playing { - self.dispatch(IoEvent::PausePlayback); + self.dispatch_spotify_fallback(IoEvent::PausePlayback); } else { // When no offset or uris are passed, spotify will resume current playback - self.dispatch(IoEvent::StartPlayback(None, None, None)); + self.dispatch_spotify_fallback(IoEvent::StartPlayback(None, None, None)); } } @@ -213,7 +213,7 @@ impl App { } // Fallback for external devices - self.dispatch(IoEvent::Seek(0)); + self.dispatch_spotify_fallback(IoEvent::Seek(0)); } else { // If less than 3 seconds in, go to previous track #[cfg(feature = "streaming")] @@ -238,7 +238,7 @@ impl App { } // Fallback for external devices - self.dispatch(IoEvent::PreviousTrack); + self.dispatch_spotify_fallback(IoEvent::PreviousTrack); } } @@ -287,7 +287,7 @@ impl App { } self.song_progress_ms = 0; - self.dispatch(IoEvent::ForcePreviousTrack); + self.dispatch_spotify_fallback(IoEvent::ForcePreviousTrack); } pub fn next_track(&mut self) { @@ -359,7 +359,7 @@ impl App { } // Fallback for external devices - self.dispatch(IoEvent::NextTrack); + self.dispatch_spotify_fallback(IoEvent::NextTrack); } /// Start playback of an explicit list of playable URIs, optionally from an @@ -368,6 +368,10 @@ impl App { /// pump's source routers, exactly as for the equivalent keybinding. #[cfg_attr(not(feature = "scripting"), allow(dead_code))] pub(crate) fn start_playback_uris(&mut self, uris: Vec, offset: Option) { + // No URIs: nothing to start, and an empty start would only tear the current player down. + if uris.is_empty() { + return; + } self.dispatch(IoEvent::StartPlayback(None, Some(uris), offset)); } diff --git a/src/core/app/volume.rs b/src/core/app/volume.rs index 0ce2472b..eb5a8a09 100644 --- a/src/core/app/volume.rs +++ b/src/core/app/volume.rs @@ -1,7 +1,30 @@ use super::*; impl App { + /// Coalesced Web API volume for an external device. + fn queue_api_volume(&mut self, volume: u8) { + if self.playback_owner() == PlaybackOwner::None { + self.set_status_message(NOTHING_PLAYING_STATUS, 4); + return; + } + self.pending_volume = Some(volume); + if !self.is_volume_change_in_flight { + self.is_volume_change_in_flight = true; + self.dispatch(IoEvent::ChangeVolume(volume)); + } + } + pub fn flush_pending_volume(&mut self) { + if self.pending_volume.is_some() && self.playback_owner() == PlaybackOwner::None { + self.pending_volume = None; + return; + } + // A decoded source took the value in its own branch; a Web API call here + // would only latch `is_volume_change_in_flight` with no Spotify reply to + // clear it. + if self.active_decoded_source() { + return; + } if self.is_volume_change_in_flight { return; // previous request still processing } @@ -80,13 +103,7 @@ impl App { } } - // Fallback to API-based volume control for external devices - // Coalesce: only dispatch if no request is already in flight - self.pending_volume = Some(next_volume); - if !self.is_volume_change_in_flight { - self.is_volume_change_in_flight = true; - self.dispatch(IoEvent::ChangeVolume(next_volume)); - } + self.queue_api_volume(next_volume); } } @@ -136,13 +153,7 @@ impl App { } } - // Fallback to API-based volume control for external devices - // Coalesce: only dispatch if no request is already in flight - self.pending_volume = Some(next_volume); - if !self.is_volume_change_in_flight { - self.is_volume_change_in_flight = true; - self.dispatch(IoEvent::ChangeVolume(next_volume)); - } + self.queue_api_volume(next_volume); } } @@ -197,13 +208,7 @@ impl App { } } - // Fallback to API-based volume control for external devices - // Coalesce: only dispatch if no request is already in flight - self.pending_volume = Some(next_volume_u8); - if !self.is_volume_change_in_flight { - self.is_volume_change_in_flight = true; - self.dispatch(IoEvent::ChangeVolume(next_volume_u8)); - } + self.queue_api_volume(next_volume_u8); } } } diff --git a/src/core/auth.rs b/src/core/auth.rs index f80ca66c..97faf6ff 100644 --- a/src/core/auth.rs +++ b/src/core/auth.rs @@ -456,6 +456,15 @@ pub async fn authenticate_with_fallback( authenticate_candidates(client_config, config_paths, true, onboarding).await } +/// Load a cached token (with refresh); fails instead of opening a browser. +pub async fn authenticate_cached( + client_config: &mut ClientConfig, + config_paths: &ConfigPaths, + onboarding: &dyn Onboarding, +) -> Result { + authenticate_candidates(client_config, config_paths, false, onboarding).await +} + /// Best-effort silent Spotify load for a free-source launch: returns `Some` only /// when a cached token is present and usable, `None` otherwise (never prompts). pub async fn try_load_spotify_silently( @@ -463,7 +472,7 @@ pub async fn try_load_spotify_silently( config_paths: &ConfigPaths, onboarding: &dyn Onboarding, ) -> Option { - authenticate_candidates(client_config, config_paths, false, onboarding) + authenticate_cached(client_config, config_paths, onboarding) .await .ok() } diff --git a/src/core/config.rs b/src/core/config.rs index abb61eb1..c6886545 100644 --- a/src/core/config.rs +++ b/src/core/config.rs @@ -138,7 +138,9 @@ impl ClientConfig { Ok(()) } - pub fn load_config(&mut self, onboarding: &dyn Onboarding) -> Result<()> { + /// Load `client.yml`, or run the auth wizard when the file is absent. + /// Returns `true` when the wizard ran. + pub fn load_config(&mut self, onboarding: &dyn Onboarding) -> Result { let paths = self.get_or_build_paths()?; if paths.config_file_path.exists() { let config_string = fs::read_to_string(&paths.config_file_path)?; @@ -155,7 +157,7 @@ impl ClientConfig { self.streaming_bitrate = config_yml.streaming_bitrate; self.streaming_audio_cache = config_yml.streaming_audio_cache; - Ok(()) + Ok(false) } else { onboarding.info(BANNER); @@ -164,7 +166,8 @@ impl ClientConfig { paths.config_file_path.display() )); - self.run_auth_setup_wizard(onboarding) + self.run_auth_setup_wizard(onboarding)?; + Ok(true) } } diff --git a/src/infra/network/mod.rs b/src/infra/network/mod.rs index c9fb1847..91930b07 100644 --- a/src/infra/network/mod.rs +++ b/src/infra/network/mod.rs @@ -15,7 +15,7 @@ pub mod sync; pub mod user; pub mod utils; -use crate::core::app::App; +use crate::core::app::{App, SPOTIFY_NOT_CONNECTED_STATUS}; use crate::core::auth; use crate::core::config::ClientConfig; use crate::core::plugin_api::{ShowInfo, TrackInfo}; @@ -593,13 +593,11 @@ impl Network { if !bypass_auth { if self.spotify.is_none() { self - .show_status_message( - "Spotify not connected. Press `d` and pick Spotify to log in.".to_string(), - 6, - ) + .show_status_message(SPOTIFY_NOT_CONNECTED_STATUS.to_string(), 6) .await; let mut app = self.app.lock().await; app.is_loading = false; + app.is_volume_change_in_flight = false; if pending_playlist_id .as_deref() .is_some_and(|id| app.pending_playlist_open.as_deref() == Some(id)) @@ -1275,6 +1273,9 @@ impl Network { let mut app = self.app.lock().await; app.spotify_token_expiry = expiry; app.spotify_connected = true; + if app.active_source == crate::core::source::Source::Spotify { + app.persist_active_source(); + } // Load Spotify data now that a session exists. app.dispatch(IoEvent::GetUser); app.dispatch(IoEvent::GetPlaylists); @@ -1302,6 +1303,15 @@ impl Network { } async fn start_party(&mut self, control_mode: sync::ControlMode) { + // The event bypasses the auth gate, so the handler carries the requirement + // itself: the relay drives Spotify playback, and opening the socket first + // would leave a live party the drain has to close again. + if self.spotify.is_none() { + self + .show_status_message(SPOTIFY_NOT_CONNECTED_STATUS.to_string(), 6) + .await; + return; + } { let mut app = self.app.lock().await; app.party_status = sync::PartyStatus::Connecting; @@ -1343,6 +1353,13 @@ impl Network { } async fn join_party(&mut self, code: String, name: String) { + // Same requirement as `start_party`. + if self.spotify.is_none() { + self + .show_status_message(SPOTIFY_NOT_CONNECTED_STATUS.to_string(), 6) + .await; + return; + } { let mut app = self.app.lock().await; app.party_status = sync::PartyStatus::Connecting; @@ -1455,6 +1472,22 @@ impl Network { } pub async fn process_party_messages(&mut self) { + // Every relay handler below drives the Spotify client; without a session + // there is nothing to sync and `spotify()` would panic the pump. Close the + // party instead of returning, so an unread receiver cannot grow without a + // bound. The guard keeps the common no-party drain to one comparison. + if self.spotify.is_none() { + if self.party_connection.is_some() || self.party_incoming_rx.is_some() { + self.leave_party().await; + self + .show_status_message( + "Listening Party ended: Spotify is not connected.".to_string(), + 6, + ) + .await; + } + return; + } let messages: Vec = { match &mut self.party_incoming_rx { Some(rx) => { @@ -1857,4 +1890,75 @@ mod tests { assert!(app.lock().await.pending_playlist_open.is_none()); } + + fn session_free_network(app: &Arc>) -> Network { + Network::new(None, ClientConfig::new(), app, temp_token_cache_path()) + } + + fn app_without_a_session() -> Arc> { + let (io_tx, _io_rx) = std::sync::mpsc::channel(); + Arc::new(Mutex::new(App::new(io_tx, UserConfig::new(), None))) + } + + #[tokio::test] + async fn start_party_without_a_session_opens_no_relay() { + let app = app_without_a_session(); + let mut network = session_free_network(&app); + + network.start_party(sync::ControlMode::HostOnly).await; + + assert!(network.party_connection.is_none()); + assert!(network.party_incoming_rx.is_none()); + let app = app.lock().await; + assert_eq!(app.party_status, sync::PartyStatus::Disconnected); + assert_eq!( + app.status_message.as_deref(), + Some(SPOTIFY_NOT_CONNECTED_STATUS) + ); + } + + #[tokio::test] + async fn join_party_without_a_session_opens_no_relay() { + let app = app_without_a_session(); + let mut network = session_free_network(&app); + + network + .join_party("ABC123".to_string(), "Guest".to_string()) + .await; + + assert!(network.party_connection.is_none()); + assert!(network.party_incoming_rx.is_none()); + let app = app.lock().await; + assert_eq!(app.party_status, sync::PartyStatus::Disconnected); + assert_eq!( + app.status_message.as_deref(), + Some(SPOTIFY_NOT_CONNECTED_STATUS) + ); + } + + #[tokio::test] + async fn process_party_messages_closes_a_party_that_outlived_its_session() { + let app = app_without_a_session(); + let mut network = session_free_network(&app); + let (_tx, rx) = tokio::sync::mpsc::unbounded_channel(); + network.party_incoming_rx = Some(rx); + { + let mut app = app.lock().await; + app.party_status = sync::PartyStatus::Hosting; + app.party_session = Some(sync::PartySession { + role: sync::PartyRole::Host, + code: "ABC123".to_string(), + guests: Vec::new(), + control_mode: sync::ControlMode::HostOnly, + host_name: "Host".to_string(), + }); + } + + network.process_party_messages().await; + + assert!(network.party_incoming_rx.is_none()); + let app = app.lock().await; + assert_eq!(app.party_status, sync::PartyStatus::Disconnected); + assert!(app.party_session.is_none()); + } } diff --git a/src/runtime/bootstrap.rs b/src/runtime/bootstrap.rs index 6974d799..05c23da5 100644 --- a/src/runtime/bootstrap.rs +++ b/src/runtime/bootstrap.rs @@ -17,7 +17,7 @@ use crate::core::user_config::{ validate_tick_rate_milliseconds, BehaviorConfig, StartupBehavior, UserConfig, UserConfigPaths, }; use crate::infra::network::IoEvent; -use anyhow::Result; +use anyhow::{Context, Result}; use backtrace::Backtrace; use clap::ArgMatches; use log::info; @@ -271,6 +271,34 @@ fn should_prompt_global_song_count(client_yml_exists: bool, config_has_answer: b !client_yml_exists || !config_has_answer } +/// How boot obtains a Spotify session. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +enum SpotifyAuthMode { + /// Open the browser when no usable token is cached. + Interactive, + /// A cached token or a failed boot with a clear message. + CachedOrFail, + /// A cached token or no session; the frontend offers the login. + CachedOrNone, +} + +/// Interactive only right after the client wizard (fresh install, +/// `--reconfigure-auth`, or the auth-setup migration): the user just asked for +/// Spotify. A subcommand needs a session; a UI launch never blocks on a browser. +fn spotify_auth_mode( + subcommand: bool, + reconfigure_auth: bool, + wizard_ran: bool, +) -> SpotifyAuthMode { + if reconfigure_auth || wizard_ran { + SpotifyAuthMode::Interactive + } else if subcommand { + SpotifyAuthMode::CachedOrFail + } else { + SpotifyAuthMode::CachedOrNone + } +} + fn global_song_counter_prompt() -> OnboardingPrompt { OnboardingPrompt::Confirm { title: "Global Song Counter".to_string(), @@ -549,7 +577,7 @@ pub(super) async fn boot(matches: &ArgMatches, onboarding: Arc) ) .await?; } - client_config.load_config(onboarding.as_ref())?; + let mut wizard_ran = client_config.load_config(onboarding.as_ref())?; info!("client authentication config loaded"); let reconfigure_auth = matches.get_flag("reconfigure-auth"); @@ -561,6 +589,7 @@ pub(super) async fn boot(matches: &ArgMatches, onboarding: Arc) } else if matches.subcommand_name().is_none() && client_config.needs_auth_setup_migration() { if ask_auth_setup_migration(onboarding.as_ref())? { client_config.reconfigure_auth(onboarding.as_ref())?; + wizard_ran = true; onboarding.info("Client authentication setup updated.\n"); } else { client_config.mark_auth_setup_migrated()?; @@ -570,12 +599,11 @@ pub(super) async fn boot(matches: &ArgMatches, onboarding: Arc) let config_paths = client_config.get_or_build_paths()?; - // Spotify is only mandatory when the active source IS Spotify, or when running - // a CLI subcommand (every subcommand is Spotify-only and should fail cleanly - // when unauthenticated). A free-source TUI launch tries a silent token load and - // tolerates its absence; the user can add Spotify later via in-TUI login. - let spotify_required = matches.subcommand_name().is_some() - || runtime_state.active_source == crate::core::source::Source::Spotify; + let auth_mode = spotify_auth_mode( + matches.subcommand_name().is_some(), + reconfigure_auth, + wizard_ran, + ); // The GitHub update check runs concurrently with authentication: both are // network round trips and neither depends on the other, so the check no @@ -583,15 +611,24 @@ pub(super) async fn boot(matches: &ArgMatches, onboarding: Arc) // still restarts the process, exactly as before.) let (authenticated, installed_update) = tokio::join!( async { - if spotify_required { - auth::authenticate_with_fallback(&mut client_config, &config_paths, onboarding.as_ref()) - .await - .map(Some) - } else { - Ok( + match auth_mode { + SpotifyAuthMode::Interactive => { + auth::authenticate_with_fallback(&mut client_config, &config_paths, onboarding.as_ref()) + .await + .map(Some) + } + SpotifyAuthMode::CachedOrFail => { + auth::authenticate_cached(&mut client_config, &config_paths, onboarding.as_ref()) + .await + .map(Some) + .context( + "Spotify is not connected. Start `spotatui`, press `d` and pick Spotify to log in.", + ) + } + SpotifyAuthMode::CachedOrNone => Ok( auth::try_load_spotify_silently(&mut client_config, &config_paths, onboarding.as_ref()) .await, - ) + ), } }, super::cli::run_auto_update(matches, &user_config) @@ -707,7 +744,7 @@ mod tests { use super::{ apply_configured_runtime_defaults, ask_auth_setup_migration, auth_setup_migration_prompt, global_song_counter_prompt, persist_global_song_count, prompt_global_song_count_opt_in, - should_prompt_global_song_count, + should_prompt_global_song_count, spotify_auth_mode, SpotifyAuthMode, }; use crate::core::limits::MAX_PLAYBAR_ROWS; use crate::core::onboarding::OnboardingPrompt; @@ -905,4 +942,24 @@ mod tests { assert!(!ask_auth_setup_migration(&onboarding).unwrap()); assert!(onboarding.saw("Would you like to run the new auth setup wizard now? (Y/n): ")); } + + #[test] + fn the_boot_auth_mode_follows_the_wizard_and_the_subcommand() { + let cases = [ + (false, true, false, SpotifyAuthMode::Interactive), + (true, true, false, SpotifyAuthMode::Interactive), + (false, false, true, SpotifyAuthMode::Interactive), + (true, false, true, SpotifyAuthMode::Interactive), + (true, false, false, SpotifyAuthMode::CachedOrFail), + (false, false, false, SpotifyAuthMode::CachedOrNone), + ]; + + for (subcommand, reconfigure_auth, wizard_ran, expected) in cases { + assert_eq!( + spotify_auth_mode(subcommand, reconfigure_auth, wizard_ran), + expected, + "subcommand={subcommand} reconfigure_auth={reconfigure_auth} wizard_ran={wizard_ran}" + ); + } + } } diff --git a/src/runtime/pump.rs b/src/runtime/pump.rs index a62faa81..42867ff1 100644 --- a/src/runtime/pump.rs +++ b/src/runtime/pump.rs @@ -1,8 +1,61 @@ //! The serial IoEvent pump: source routing by URI scheme, the service lane, //! and the party-relay drain. Every frontend drives `App` through this. +use crate::core::app::SPOTIFY_NOT_CONNECTED_STATUS; +use crate::core::queue::{ + missing_source_feature, queue_item_source, source_available, QueueItemSource, +}; use crate::infra::network::{IoEvent, Network}; +/// The URI a `StartPlayback` addresses: the context, or the head of the list. +#[cfg_attr(not(feature = "tui"), allow(dead_code))] +fn start_playback_uri(event: &IoEvent) -> Option<&str> { + match event { + IoEvent::StartPlayback(Some(uri), _, _) => Some(uri), + IoEvent::StartPlayback(None, Some(uris), _) => uris.first().map(String::as_str), + _ => None, + } +} + +/// Whether some router or the Spotify session will start `event`; a start +/// nobody takes must not reach the routers, whose foreign-start arms tear the +/// audible player down. +#[cfg_attr(not(feature = "tui"), allow(dead_code))] +fn start_playback_has_taker(event: &IoEvent, spotify_session: bool) -> bool { + match event { + IoEvent::StartPlayback(None, None, None) => true, + IoEvent::StartPlayback(..) => match start_playback_uri(event) { + // Radio is never queued, so `queue_item_source` does not know its scheme. + Some(uri) if uri.starts_with("radio:") => cfg!(feature = "internet-radio"), + Some(uri) => match queue_item_source(uri) { + QueueItemSource::Spotify => spotify_session, + source => source_available(source), + }, + None => spotify_session, + }, + _ => true, + } +} + +/// Why the gate dropped a start: the source feature this build lacks, else +/// the missing Spotify session. +#[cfg_attr(not(feature = "tui"), allow(dead_code))] +fn dropped_start_status(event: &IoEvent) -> String { + let missing_feature = match start_playback_uri(event) { + Some(uri) if uri.starts_with("radio:") => { + (!cfg!(feature = "internet-radio")).then_some("internet-radio") + } + Some(uri) => missing_source_feature(uri), + None => None, + }; + match missing_feature { + Some(feature) => { + format!("This spotatui was built without the `{feature}` feature, so nothing can play that.") + } + None => SPOTIFY_NOT_CONNECTED_STATUS.to_string(), + } +} + // CLI mode never starts the pump; only a frontend launch does. #[cfg_attr(not(feature = "tui"), allow(dead_code))] pub(super) async fn start_tokio(io_rx: std::sync::mpsc::Receiver, network: &mut Network) { @@ -55,6 +108,12 @@ pub(super) async fn start_tokio(io_rx: std::sync::mpsc::Receiver, netwo } { + if !start_playback_has_taker(&io_event, network.spotify.is_some()) { + let mut app = network.app.lock().await; + app.set_status_message(dropped_start_status(&io_event), 6); + app.is_loading = false; + continue; + } // The native queue router runs first: it owns `AdvanceNativeQueue` and // the queue slot's transport controls, and relinquishes the slot on an // unrelated `StartPlayback` (returning false so the per-source @@ -135,3 +194,70 @@ pub(super) async fn start_tokio(io_rx: std::sync::mpsc::Receiver, netwo network.process_party_messages().await; } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn a_bare_resume_always_has_a_taker() { + let resume = IoEvent::StartPlayback(None, None, None); + + assert!(start_playback_has_taker(&resume, false)); + assert!(start_playback_has_taker(&resume, true)); + } + + #[test] + fn a_spotify_start_needs_a_session() { + let start = IoEvent::StartPlayback(None, Some(vec!["spotify:track:x".to_string()]), Some(0)); + + assert!(start_playback_has_taker(&start, true)); + assert!(!start_playback_has_taker(&start, false)); + } + + #[test] + fn an_empty_start_has_no_taker_without_a_session() { + let empty = IoEvent::StartPlayback(None, Some(vec![]), Some(0)); + + assert!(!start_playback_has_taker(&empty, false)); + } + + #[cfg(feature = "local-files")] + #[test] + fn a_local_start_has_a_taker_without_a_session() { + let list = IoEvent::StartPlayback(None, Some(vec!["file:///a.mp3".to_string()]), Some(0)); + let context = IoEvent::StartPlayback(Some("file:///album".to_string()), None, None); + + assert!(start_playback_has_taker(&list, false)); + assert!(start_playback_has_taker(&context, false)); + } + + #[cfg(feature = "internet-radio")] + #[test] + fn a_radio_start_has_a_taker_without_a_session() { + let start = IoEvent::StartPlayback(Some("radio:https://x.example/s".to_string()), None, None); + + assert!(start_playback_has_taker(&start, false)); + } + + #[cfg(not(feature = "local-files"))] + #[test] + fn a_compiled_out_source_has_no_taker_and_names_the_feature() { + let start = IoEvent::StartPlayback(None, Some(vec!["file:///a.mp3".to_string()]), Some(0)); + + assert!(!start_playback_has_taker(&start, true)); + assert!(dropped_start_status(&start).contains("`local-files`")); + } + + #[test] + fn a_spotify_start_without_a_session_reports_the_missing_session() { + let start = IoEvent::StartPlayback(None, Some(vec!["spotify:track:x".to_string()]), Some(0)); + + assert_eq!(dropped_start_status(&start), SPOTIFY_NOT_CONNECTED_STATUS); + } + + #[test] + fn a_non_start_event_always_has_a_taker() { + assert!(start_playback_has_taker(&IoEvent::NextTrack, false)); + } +} diff --git a/src/runtime/startup.rs b/src/runtime/startup.rs index 7a209ba3..b93bdf3d 100644 --- a/src/runtime/startup.rs +++ b/src/runtime/startup.rs @@ -152,12 +152,13 @@ fn update_macos_metadata( } } +/// Returns the snapshot's play state and position, `None` with no snapshot. #[cfg(all(feature = "windows-media", target_os = "windows"))] fn update_windows_metadata( manager: &smtc_tokio::WindowsMediaManager, last_metadata: &mut Option, app: &App, -) { +) -> Option<(bool, u64)> { if let Some(snapshot) = crate::infra::media_metadata::current_playback_snapshot(app) { let new_metadata = WindowsMetadata { title: snapshot.metadata.title.clone(), @@ -177,8 +178,12 @@ fn update_windows_metadata( ); *last_metadata = Some(new_metadata); } - } else if last_metadata.is_some() { - *last_metadata = None; + Some((snapshot.is_playing, snapshot.progress_ms as u64)) + } else { + if last_metadata.is_some() { + *last_metadata = None; + } + None } } @@ -221,10 +226,16 @@ pub(super) async fn launch_ui(boot: Boot) -> Result<()> { // flow above can block on a browser round trip, which would burn the // message's TTL before the first frame ever renders. Never displaces a // message something else just set (they are more urgent than this notice). - if let Some(message) = client_id_notice_message { + // A Spotify browse scope with no session comes second: say so once, before + // the first key press reaches the auth gate. + { let mut app_mut = app.lock().await; if app_mut.status_message.is_none() { - app_mut.set_status_message(message, 15); + if let Some(message) = client_id_notice_message { + app_mut.set_status_message(message, 15); + } else if spotify.is_none() && app_mut.active_source == crate::core::source::Source::Spotify { + app_mut.set_status_message(crate::core::app::SPOTIFY_NOT_CONNECTED_STATUS, 8); + } } } @@ -331,6 +342,8 @@ pub(super) async fn launch_ui(boot: Boot) -> Result<()> { // Gated on whether streaming will be attempted (the player itself now // initializes in the background): registering media keys for a session // whose native init later fails is harmless — the handlers just no-op. + // macOS plays no decoded source, so without native streaming the keys would + // only be taken away from the other players. #[cfg(all(feature = "macos-media", target_os = "macos"))] let macos_media_manager: Option> = if streaming_attempted { match macos_media::MacMediaManager::new() { @@ -350,8 +363,9 @@ pub(super) async fn launch_ui(boot: Boot) -> Result<()> { None }; + // Registered without a Spotify session, like MPRIS, so decoded sources get media keys too. #[cfg(all(feature = "windows-media", target_os = "windows"))] - let windows_media_manager: Option> = if streaming_attempted { + let windows_media_manager: Option> = match smtc_tokio::WindowsMediaManager::new() { Ok(mgr) => { info!("windows smtc com registered - media keys enabled"); @@ -364,10 +378,7 @@ pub(super) async fn launch_ui(boot: Boot) -> Result<()> { ); None } - } - } else { - None - }; + }; #[cfg(feature = "discord-rpc")] let discord_rpc_manager: DiscordRpcHandle = if user_config.behavior.enable_discord_rpc { @@ -461,25 +472,21 @@ pub(super) async fn launch_ui(boot: Boot) -> Result<()> { loop { interval.tick().await; if let Ok(app) = app_for_windows_metadata.try_lock() { - update_windows_metadata(&windows_media_for_metadata, &mut last_metadata, &app); - let is_playing = if app.native_track_info.is_some() { - app.native_is_playing.unwrap_or(false) - } else { - app - .current_playback_context - .as_ref() - .map(|c| c.is_playing) - .unwrap_or(false) - }; - - if app.native_track_info.is_none() { + let snapshot_state = + update_windows_metadata(&windows_media_for_metadata, &mut last_metadata, &app); + // Native playback pushes its own state from the player events; an + // external device or a decoded source (over paused librespot) is + // polled here from the snapshot. + if app.native_track_info.is_none() || app.active_decoded_source() { + let (is_playing, position_ms) = + snapshot_state.unwrap_or((false, app.song_progress_ms as u64)); if last_playing != Some(is_playing) { windows_media_for_metadata.set_playback_status(is_playing); last_playing = Some(is_playing); } - windows_media_for_metadata.set_position(app.song_progress_ms as u64); + windows_media_for_metadata.set_position(position_ms); } else { - last_playing = Some(is_playing); + last_playing = Some(app.native_is_playing.unwrap_or(false)); } } } @@ -913,9 +920,9 @@ async fn handle_mpris_events( .unwrap_or(false) }); if is_playing { - app_lock.dispatch(IoEvent::PausePlayback); + app_lock.dispatch_spotify_fallback(IoEvent::PausePlayback); } else { - app_lock.dispatch(IoEvent::StartPlayback(None, None, None)); + app_lock.dispatch_spotify_fallback(IoEvent::StartPlayback(None, None, None)); } } MprisEvent::Play => { @@ -926,7 +933,7 @@ async fn handle_mpris_events( continue; } let mut app_lock = app.lock().await; - app_lock.dispatch(IoEvent::StartPlayback(None, None, None)); + app_lock.dispatch_spotify_fallback(IoEvent::StartPlayback(None, None, None)); } MprisEvent::Pause => { #[cfg(feature = "streaming")] @@ -936,27 +943,13 @@ async fn handle_mpris_events( continue; } let mut app_lock = app.lock().await; - app_lock.dispatch(IoEvent::PausePlayback); + app_lock.dispatch_spotify_fallback(IoEvent::PausePlayback); } MprisEvent::Next => { - #[cfg(feature = "streaming")] - if let Some(ref player) = current_player { - let _ = player; - app.lock().await.next_track(); - continue; - } - let mut app_lock = app.lock().await; - app_lock.dispatch(IoEvent::NextTrack); + app.lock().await.next_track(); } MprisEvent::Previous => { - #[cfg(feature = "streaming")] - if let Some(ref player) = current_player { - let _ = player; - app.lock().await.previous_track(); - continue; - } - let mut app_lock = app.lock().await; - app_lock.dispatch(IoEvent::PreviousTrack); + app.lock().await.previous_track(); } MprisEvent::Stop => { #[cfg(feature = "streaming")] @@ -966,7 +959,7 @@ async fn handle_mpris_events( continue; } let mut app_lock = app.lock().await; - app_lock.dispatch(IoEvent::PausePlayback); + app_lock.dispatch_spotify_fallback(IoEvent::PausePlayback); } MprisEvent::Seek(offset_micros) => { // MPRIS sends relative offset in microseconds (can be negative for rewind) @@ -990,7 +983,7 @@ async fn handle_mpris_events( let offset_ms = offset_micros / 1000; let new_position_ms = (current_ms + offset_ms).max(0) as u32; app_lock.song_progress_ms = new_position_ms as u128; - app_lock.dispatch(IoEvent::Seek(new_position_ms)); + app_lock.dispatch_spotify_fallback(IoEvent::Seek(new_position_ms)); drop(app_lock); mpris_manager.emit_seeked(new_position_ms as u64); } @@ -1011,7 +1004,7 @@ async fn handle_mpris_events( // Fallback: dispatch Seek IoEvent let mut app_lock = app.lock().await; app_lock.song_progress_ms = new_position_ms as u128; - app_lock.dispatch(IoEvent::Seek(new_position_ms)); + app_lock.dispatch_spotify_fallback(IoEvent::Seek(new_position_ms)); drop(app_lock); mpris_manager.emit_seeked(new_position_ms as u64); } @@ -1044,7 +1037,7 @@ async fn handle_mpris_events( app_lock.schedule_state_save(crate::core::state::PersistedRuntimeState::shuffle_enabled( shuffle, )); - app_lock.dispatch(IoEvent::Shuffle(shuffle)); + app_lock.dispatch_spotify_fallback(IoEvent::Shuffle(shuffle)); } MprisEvent::SetLoopStatus(loop_status) => { use mpris::LoopStatusEvent; @@ -1075,7 +1068,7 @@ async fn handle_mpris_events( if let Some(ref mut ctx) = app_lock.current_playback_context { ctx.repeat_state = repeat_state; } - app_lock.dispatch(IoEvent::Repeat(repeat_state)); + app_lock.dispatch_spotify_fallback(IoEvent::Repeat(repeat_state)); } MprisEvent::SetVolume(volume_percent) => { let mut app_lock = app.lock().await; @@ -1405,7 +1398,7 @@ async fn handle_windows_media_events( app .lock() .await - .dispatch(IoEvent::StartPlayback(None, None, None)); + .dispatch_spotify_fallback(IoEvent::StartPlayback(None, None, None)); } WindowsMediaEvent::Pause => { if let Some(player) = &player_opt { @@ -1414,29 +1407,25 @@ async fn handle_windows_media_events( continue; } } - app.lock().await.dispatch(IoEvent::PausePlayback); + app + .lock() + .await + .dispatch_spotify_fallback(IoEvent::PausePlayback); } WindowsMediaEvent::Next => { - if let Some(player) = &player_opt { - let _ = player; - app.lock().await.next_track(); - } else { - app.lock().await.dispatch(IoEvent::NextTrack); - } + app.lock().await.next_track(); } WindowsMediaEvent::Previous => { - if let Some(player) = &player_opt { - let _ = player; - app.lock().await.previous_track(); - } else { - app.lock().await.dispatch(IoEvent::PreviousTrack); - } + app.lock().await.previous_track(); } WindowsMediaEvent::Stop => { if let Some(player) = &player_opt { player.stop(); } else { - app.lock().await.dispatch(IoEvent::PausePlayback); + app + .lock() + .await + .dispatch_spotify_fallback(IoEvent::PausePlayback); } } WindowsMediaEvent::SetPosition(pos) => { @@ -1448,7 +1437,7 @@ async fn handle_windows_media_events( } let mut app_lock = app.lock().await; app_lock.song_progress_ms = pos as u128; - app_lock.dispatch(IoEvent::Seek(pos as u32)); + app_lock.dispatch_spotify_fallback(IoEvent::Seek(pos as u32)); } } } diff --git a/src/tui/handlers/mouse.rs b/src/tui/handlers/mouse.rs index 2b6901cd..abc2824c 100644 --- a/src/tui/handlers/mouse.rs +++ b/src/tui/handlers/mouse.rs @@ -1199,7 +1199,7 @@ mod tests { #[test] fn click_main_layout_playbar_control_triggers_action() { - let mut app = App::default(); + let mut app = App::default_connected(); app.view.size = Viewport { width: 160, height: 50, @@ -1224,7 +1224,7 @@ mod tests { #[test] fn click_main_layout_playbar_progress_seeks() { - let mut app = App::default(); + let mut app = App::default_connected(); app.view.size = Viewport { width: 160, height: 50, @@ -1270,7 +1270,7 @@ mod tests { #[test] fn click_playbar_time_label_does_not_seek() { - let mut app = App::default(); + let mut app = App::default_connected(); app.view.size = Viewport { width: 160, height: 50, @@ -1293,7 +1293,7 @@ mod tests { #[test] fn drag_playbar_progress_seeks() { - let mut app = App::default(); + let mut app = App::default_connected(); app.view.size = Viewport { width: 160, height: 50, @@ -1326,7 +1326,7 @@ mod tests { use crate::tui::ui::player::draw_playbar; use ratatui::{backend::TestBackend, Terminal}; - let mut app = App::default(); + let mut app = App::default_connected(); app.view.size = Viewport { width: 160, height: 50, @@ -1367,7 +1367,7 @@ mod tests { #[test] fn click_lyrics_view_playbar_control_triggers_action() { - let mut app = App::default(); + let mut app = App::default_connected(); app.view.size = Viewport { width: 160, height: 50, @@ -1393,7 +1393,7 @@ mod tests { #[test] fn click_miniplayer_control_triggers_action_and_keeps_miniplayer_focus() { - let mut app = App::default(); + let mut app = App::default_connected(); app.view.size = Viewport { width: 160, height: 50, @@ -1446,7 +1446,7 @@ mod tests { #[test] fn click_hidden_lyrics_view_playbar_area_does_nothing() { - let mut app = App::default(); + let mut app = App::default_connected(); app.view.size = Viewport { width: 160, height: 50, @@ -1474,7 +1474,7 @@ mod tests { #[test] fn resized_playbar_control_click_still_maps_correctly() { - let mut app = App::default(); + let mut app = App::default_connected(); app.view.size = Viewport { width: 160, height: 50, @@ -1498,7 +1498,7 @@ mod tests { #[test] fn smaller_resized_playbar_control_click_still_maps_correctly() { - let mut app = App::default(); + let mut app = App::default_connected(); app.view.size = Viewport { width: 160, height: 50, @@ -1522,7 +1522,7 @@ mod tests { #[test] fn click_playbar_outside_controls_does_nothing() { - let mut app = App::default(); + let mut app = App::default_connected(); app.view.size = Viewport { width: 160, height: 50, diff --git a/src/tui/handlers/party.rs b/src/tui/handlers/party.rs index c435269a..80ac68f0 100644 --- a/src/tui/handlers/party.rs +++ b/src/tui/handlers/party.rs @@ -74,9 +74,6 @@ fn handle_code_input(key: Key, app: &mut App) { let name = normalized_guest_name(&app.view.party_join_name); if code.len() == PARTY_CODE_LEN && !name.is_empty() { app.apply(Action::JoinParty { code, name }); - app.view.party_input.clear(); - app.view.party_input_idx = 0; - app.view.party_join_name.clear(); } } Key::Backspace => { diff --git a/tools/gates.count b/tools/gates.count index d7f2c140..7535f968 100644 --- a/tools/gates.count +++ b/tools/gates.count @@ -13,4 +13,4 @@ synthetic_keys_in_mouse_handler = 3 # target 0 (the content-table re-entries wildcard_arms_in_action_tree = 0 # target 0, must stay 0 view_writes_outside_tui = 12 # target 0 (producers outside tui/ and core/app/ writing App::view) action_refs_in_tui_handlers = 173 # adoption: may only rise -test_attribute_total = 1721 # adoption: may only rise +test_attribute_total = 1753 # adoption: may only rise