diff --git a/src-tauri/src/commands/player/mod.rs b/src-tauri/src/commands/player/mod.rs index 92f1ed62..eccda70b 100644 --- a/src-tauri/src/commands/player/mod.rs +++ b/src-tauri/src/commands/player/mod.rs @@ -1003,6 +1003,16 @@ pub async fn player_stop( .clone() }; client.send_session_command(session_id, "Stop").await?; + + // Stopping the remote session ends the cast, so the manager returns to + // Idle — same as a local stop. This is also what hands OS volume control + // back to this device: set_mode releases the Android remote volume + // provider on any exit from remote mode. Without it the mode stayed + // Remote and the system volume slider remained stuck on the remote + // session with no way back to the local speaker. + playback_mode + .0 + .set_mode(crate::playback_mode::PlaybackMode::Idle); } else { // Local playback let controller = player.0.lock().await; diff --git a/src-tauri/src/playback_mode/mod.rs b/src-tauri/src/playback_mode/mod.rs index e15fb947..4fa91960 100644 --- a/src-tauri/src/playback_mode/mod.rs +++ b/src-tauri/src/playback_mode/mod.rs @@ -27,6 +27,10 @@ const TICKS_PER_SECOND: f64 = 10_000_000.0; /// send a resume position, so a fresh track casts from 0 rather than ~0. const RESUME_THRESHOLD_SECONDS: f64 = 0.5; +/// Volume level (0-100) the remote volume slider starts at. The real level is +/// corrected by the session poller once the remote session reports its volume. +const DEFAULT_REMOTE_VOLUME: i32 = 50; + /// Convert a live playback position (seconds) into the `StartPositionTicks` to /// hand to a remote session, or `None` if we're effectively at the start. /// @@ -42,6 +46,50 @@ fn start_position_ticks_from_seconds(position_seconds: f64) -> Option { } } +/// Platform hook for attaching/detaching the OS remote-volume control. +/// +/// On Android, entering remote mode hands the `MediaSession` a +/// `VolumeProviderCompat` so hardware volume buttons and the system slider drive +/// the *remote* session; leaving remote mode must hand it back to the local +/// media stream. Behind a trait so the routing rule (see +/// [`PlaybackModeManager::set_mode`]) is unit-testable off-device — the real +/// implementation is JNI and only exists on Android. +pub trait RemoteVolumeControl: Send + Sync { + /// Attach remote-volume control (and, on Android, start the playback service). + fn enable(&self, initial_volume: i32); + /// Return volume control to the local device speaker. + fn disable(&self); +} + +/// Production hook: forwards to the Android JNI bridge; no-op elsewhere. +struct PlatformRemoteVolumeControl; + +impl RemoteVolumeControl for PlatformRemoteVolumeControl { + #[allow(unused_variables)] + fn enable(&self, initial_volume: i32) { + #[cfg(target_os = "android")] + { + if let Err(e) = crate::player::enable_remote_volume(initial_volume) { + log::warn!( + "[PlaybackMode] Failed to enable remote volume/service: {}", + e + ); + // Non-fatal - continue; the next poll tick will retry metadata. + } + } + } + + fn disable(&self) { + #[cfg(target_os = "android")] + { + if let Err(e) = crate::player::disable_remote_volume() { + log::warn!("[PlaybackMode] Failed to disable remote volume: {}", e); + // Non-fatal - the mode change itself has already happened. + } + } + } +} + /// Manages playback mode transfers between local and remote sessions pub struct PlaybackModeManager { jellyfin_client: Arc>>, @@ -51,6 +99,8 @@ pub struct PlaybackModeManager { /// Optional emitter used to notify the frontend when the mode changes, so its /// mirror store stays in sync with this authoritative one. `None` in tests. event_emitter: Arc>>>, + /// Platform hook for OS-level remote volume routing (swapped in tests). + remote_volume: Arc, } impl PlaybackModeManager { @@ -65,6 +115,24 @@ impl PlaybackModeManager { current_mode: Arc::new(RwLock::new(PlaybackMode::Idle)), is_transferring: Arc::new(AtomicBool::new(false)), event_emitter: Arc::new(Mutex::new(None)), + remote_volume: Arc::new(PlatformRemoteVolumeControl), + } + } + + /// Construct with a custom remote-volume hook (tests). + #[cfg(test)] + fn with_remote_volume( + jellyfin_client: Arc>>, + player_controller: Arc>, + remote_volume: Arc, + ) -> Self { + Self { + jellyfin_client, + player_controller, + current_mode: Arc::new(RwLock::new(PlaybackMode::Idle)), + is_transferring: Arc::new(AtomicBool::new(false)), + event_emitter: Arc::new(Mutex::new(None)), + remote_volume, } } @@ -86,19 +154,39 @@ impl PlaybackModeManager { /// the frontend's mirror store reconciles to this authoritative value. The /// write lock is released before emitting to avoid holding it across the /// emitter call. + /// + /// Also owns **OS volume routing**, which is derived from the transition + /// rather than from each call site: entering remote mode attaches the remote + /// volume control, and *any* exit from remote mode hands it back to the local + /// speaker. Doing this per-call-site is what caused the bug where stopping a + /// remote session (`player_stop` → Idle) left Android stuck on the remote + /// volume slider — only the transfer-to-local path tore it down. + /// + /// TRACES: UR-010 | DR-059, IR-021 pub fn set_mode(&self, mode: PlaybackMode) { log::info!("[PlaybackMode] Setting mode to: {:?}", mode); - let changed = { + let (changed, was_remote) = { let mut current = self.current_mode.write_safe(); let changed = *current != mode; + let was_remote = matches!(*current, PlaybackMode::Remote { .. }); *current = mode.clone(); - changed + (changed, was_remote) }; if !changed { return; } + // Volume routing follows the transition. Note remote->remote (switching + // target session) re-arms rather than releasing control. + let is_remote = matches!(mode, PlaybackMode::Remote { .. }); + if is_remote { + self.remote_volume.enable(DEFAULT_REMOTE_VOLUME); + } else if was_remote { + log::info!("[PlaybackMode] Leaving remote mode - restoring local volume control"); + self.remote_volume.disable(); + } + let (mode_str, session_id) = match &mode { PlaybackMode::Local => ("local".to_string(), None), PlaybackMode::Idle => ("idle".to_string(), None), @@ -122,18 +210,13 @@ impl PlaybackModeManager { /// Both symptoms share this one cause, so this must not be skipped on any /// remote-entry path (notably the empty-queue early return in /// `transfer_to_remote_inner`). No-op / non-Android builds do nothing. - #[allow(unused_variables)] + /// + /// [`set_mode`](Self::set_mode) already arms this on entry into remote mode; + /// calling it again is harmless (the service start is idempotent) and keeps + /// the guarantee when the mode was already remote, which `set_mode` skips as + /// a no-op transition. fn enable_remote_control(&self) { - #[cfg(target_os = "android")] - { - if let Err(e) = crate::player::enable_remote_volume(50) { - log::warn!( - "[PlaybackMode] Failed to enable remote volume/service: {}", - e - ); - // Non-fatal - continue; the next poll tick will retry metadata. - } - } + self.remote_volume.enable(DEFAULT_REMOTE_VOLUME); } /// Check if currently transferring @@ -766,18 +849,10 @@ impl PlaybackModeManager { // This will be improved in Phase 3 when repository is migrated to Rust. log::debug!("[PlaybackMode] Cannot load media item in Rust yet - frontend handled it"); - // Update mode to local + // Update mode to local. This also returns volume control to the local + // device speaker — set_mode owns that for every exit from remote mode. self.set_mode(PlaybackMode::Local); - // Disable remote volume control on Android (return to system volume) - #[cfg(target_os = "android")] - { - if let Err(e) = crate::player::disable_remote_volume() { - log::warn!("[PlaybackMode] Failed to disable remote volume: {}", e); - // Non-fatal - continue with transfer - } - } - log::info!("[PlaybackMode] Successfully transferred to local"); Ok(()) } @@ -893,6 +968,118 @@ mod tests { } } + /// Records enable/disable calls so tests can assert volume routing. + struct RecordingVolumeControl { + calls: Mutex>, + } + + impl RemoteVolumeControl for RecordingVolumeControl { + fn enable(&self, _initial_volume: i32) { + self.calls.lock().unwrap().push("enable"); + } + fn disable(&self) { + self.calls.lock().unwrap().push("disable"); + } + } + + fn manager_with_volume_control() -> (PlaybackModeManager, Arc) { + let volume = Arc::new(RecordingVolumeControl { + calls: Mutex::new(Vec::new()), + }); + let manager = PlaybackModeManager::with_remote_volume( + Arc::new(Mutex::new(None)), + Arc::new(TokioMutex::new(crate::player::PlayerController::default())), + volume.clone(), + ); + (manager, volume) + } + + /// Leaving remote mode must hand volume control back to the local device. + /// + /// Stopping a remote session (`player_stop`) drives the manager + /// Remote -> Idle without going through `transfer_to_local`. Before this was + /// centralised in `set_mode`, only the transfer path tore the Android + /// `VolumeProviderCompat` down, so a plain stop left the system stuck on the + /// remote volume slider with no way back to the phone speaker. + /// + /// @req-test: UR-010 - Control playback of Jellyfin remote sessions + #[test] + fn test_leaving_remote_mode_restores_local_volume() { + let (manager, volume) = manager_with_volume_control(); + + manager.set_mode(PlaybackMode::Remote { + session_id: "sess-1".to_string(), + }); + // The stop path: remote -> idle, no transfer involved. + manager.set_mode(PlaybackMode::Idle); + + assert_eq!( + *volume.calls.lock().unwrap(), + vec!["enable", "disable"], + "remote->idle must return volume control to the local speaker" + ); + } + + /// The same must hold for remote -> local (transfer back to this device). + /// + /// @req-test: UR-010 - Control playback of Jellyfin remote sessions + #[test] + fn test_remote_to_local_restores_local_volume() { + let (manager, volume) = manager_with_volume_control(); + + manager.set_mode(PlaybackMode::Remote { + session_id: "sess-1".to_string(), + }); + manager.set_mode(PlaybackMode::Local); + + assert_eq!( + *volume.calls.lock().unwrap(), + vec!["enable", "disable"], + "remote->local must return volume control to the local speaker" + ); + } + + /// Volume routing must not be touched by transitions that never involve + /// remote mode — an idle->local start would otherwise issue a pointless + /// `setPlaybackToLocal` on every playback start. + /// + /// @req-test: UR-010 - Control playback of Jellyfin remote sessions + #[test] + fn test_non_remote_transitions_leave_volume_routing_alone() { + let (manager, volume) = manager_with_volume_control(); + + manager.set_mode(PlaybackMode::Local); + manager.set_mode(PlaybackMode::Idle); + manager.set_mode(PlaybackMode::Local); + + assert!( + volume.calls.lock().unwrap().is_empty(), + "local/idle transitions must not touch remote volume routing" + ); + } + + /// Switching directly between two remote sessions stays remote: control must + /// remain attached (re-armed for the new session), never handed back local. + /// + /// @req-test: UR-010 - Control playback of Jellyfin remote sessions + #[test] + fn test_remote_to_remote_keeps_remote_volume() { + let (manager, volume) = manager_with_volume_control(); + + manager.set_mode(PlaybackMode::Remote { + session_id: "sess-1".to_string(), + }); + manager.set_mode(PlaybackMode::Remote { + session_id: "sess-2".to_string(), + }); + + assert_eq!( + *volume.calls.lock().unwrap(), + vec!["enable", "enable"], + "remote->remote re-arms control without releasing it to local" + ); + } + /// Setting the same mode twice must not re-emit — the frontend reconciler /// (and the event channel) shouldn't be spammed on no-op transitions. #[test]