From 87cd0a2dde7ddfab9a7cd275bcba1dafafa368df Mon Sep 17 00:00:00 2001 From: haruInDisguise Date: Wed, 11 Sep 2024 01:18:27 +0200 Subject: [PATCH 1/4] MPRIS: Restructured existing implementation My pr addresses some of the inconsistencies in ncspot's mpris implementation. While the previous logic was technically good enough, it was inflexible and reported redundant information. This will make it easier for software, that processes mpris events in some way, to accurately react to player updates. - 'Metadata' and 'Playback' updates have been separated into there own command - Mpris commands are only emitted from within spotify.rs - Some parts of the application creation logic has been restructured to allow for mpris events to be emitted upon startup - The initial song loading code has been moved from 'Queue::new' into 'Application::new'. --- src/application.rs | 26 ++++++++++++++++++++++---- src/mpris.rs | 6 +++++- src/queue.rs | 24 +++--------------------- src/spotify.rs | 24 +++++++++++++++++++----- 4 files changed, 49 insertions(+), 31 deletions(-) diff --git a/src/application.rs b/src/application.rs index 52899e20..dcfca674 100644 --- a/src/application.rs +++ b/src/application.rs @@ -12,7 +12,7 @@ use signal_hook::{consts::SIGHUP, consts::SIGTERM, iterator::Signals}; use crate::command::Command; use crate::commands::CommandManager; -use crate::config::Config; +use crate::config::{Config, PlaybackState}; use crate::events::{Event, EventManager}; use crate::library::Library; use crate::queue::Queue; @@ -140,6 +140,27 @@ impl Application { #[cfg(feature = "mpris")] spotify.set_mpris(mpris_manager.clone()); + // Load the last played track into the player + let playback_state = configuration.state().playback_state.clone(); + let queue_state = configuration.state().queuestate.clone(); + + if let Some(playable) = queue.get_current() { + spotify.load( + &playable, + playback_state == PlaybackState::Playing, + queue_state.track_progress.as_millis() as u32, + ); + spotify.update_track(); + match playback_state { + PlaybackState::Stopped => { + spotify.stop(); + } + PlaybackState::Paused | PlaybackState::Playing | PlaybackState::Default => { + spotify.pause(); + } + } + } + #[cfg(unix)] let ipc = if let Ok(runtime_directory) = utils::create_runtime_directory() { Some( @@ -240,9 +261,6 @@ impl Application { trace!("event received: {:?}", state); self.spotify.update_status(state.clone()); - #[cfg(feature = "mpris")] - self.mpris_manager.send(MprisCommand::NotifyPlaybackUpdate); - #[cfg(unix)] if let Some(ref ipc) = self.ipc { ipc.publish(&state, self.queue.get_current()); diff --git a/src/mpris.rs b/src/mpris.rs index 4a66ac4d..2c8dcbd9 100644 --- a/src/mpris.rs +++ b/src/mpris.rs @@ -463,11 +463,13 @@ impl MprisPlayer { } /// Commands to control the [MprisManager] worker thread. +#[derive(Debug)] pub enum MprisCommand { /// Notify about playback status and metadata updates. NotifyPlaybackUpdate, /// Notify about volume updates. NotifyVolumeUpdate, + NotifyMetadataUpdate, } /// An MPRIS server that internally manager a thread which can be sent commands. This is internally @@ -527,12 +529,14 @@ impl MprisManager { match rx.next().await { Some(MprisCommand::NotifyPlaybackUpdate) => { player_iface.playback_status_changed(ctx).await?; - player_iface.metadata_changed(ctx).await?; } Some(MprisCommand::NotifyVolumeUpdate) => { info!("sending MPRIS volume update signal"); player_iface.volume_changed(ctx).await?; } + Some(MprisCommand::NotifyMetadataUpdate) => { + player_iface.metadata_changed(ctx).await?; + } None => break, } } diff --git a/src/queue.rs b/src/queue.rs index dce7b131..94f48e4a 100644 --- a/src/queue.rs +++ b/src/queue.rs @@ -11,6 +11,7 @@ use strum_macros::Display; use crate::config::{Config, PlaybackState}; use crate::library::Library; use crate::model::playable::Playable; +use crate::mpris::MprisCommand; use crate::spotify::PlayerEvent; use crate::spotify::Spotify; @@ -50,34 +51,15 @@ pub struct Queue { impl Queue { pub fn new(spotify: Spotify, cfg: Arc, library: Arc) -> Self { let queue_state = cfg.state().queuestate.clone(); - let playback_state = cfg.state().playback_state.clone(); - let queue = Self { + + Self { queue: Arc::new(RwLock::new(queue_state.queue)), spotify: spotify.clone(), current_track: RwLock::new(queue_state.current_track), random_order: RwLock::new(queue_state.random_order), cfg, library, - }; - - if let Some(playable) = queue.get_current() { - spotify.load( - &playable, - playback_state == PlaybackState::Playing, - queue_state.track_progress.as_millis() as u32, - ); - spotify.update_track(); - match playback_state { - PlaybackState::Stopped => { - spotify.stop(); - } - PlaybackState::Paused | PlaybackState::Playing | PlaybackState::Default => { - spotify.pause(); - } - } } - - queue } /// The index of the next item in `self.queue` that should be played. None diff --git a/src/spotify.rs b/src/spotify.rs index 36d754e1..1b999b21 100644 --- a/src/spotify.rs +++ b/src/spotify.rs @@ -17,7 +17,7 @@ use librespot_playback::config::PlayerConfig; use librespot_playback::mixer::softmixer::SoftMixer; use librespot_playback::mixer::MixerConfig; use librespot_playback::player::Player; -use log::{debug, error, info}; +use log::{debug, error, info, warn}; use tokio::sync::mpsc; use url::Url; @@ -315,6 +315,9 @@ impl Spotify { start_playing, position_ms, )); + + #[cfg(feature = "mpris")] + self.send_mpris(MprisCommand::NotifyMetadataUpdate); } /// Update the cached status of the [Player]. This makes sure the status @@ -338,6 +341,9 @@ impl Spotify { let mut status = self.status.write().unwrap(); *status = new_status; + + #[cfg(feature ="mpris")] + self.send_mpris(MprisCommand::NotifyPlaybackUpdate); } /// Reset the time tracking stats for the current song. This should be called when a new song is @@ -362,6 +368,17 @@ impl Spotify { } } + /// Send an [MprisCommand] to the mpris thread. + #[cfg(feature ="mpris")] + fn send_mpris(&self, cmd: MprisCommand) { + debug!("Sending mpris command: {:?}", cmd); + if let Some(mpris_manager) = self.mpris.lock().unwrap().as_ref() { + mpris_manager.send(cmd); + } else { + warn!("mpris context is unitialized"); + } + } + /// Send a [WorkerCommand] to the worker thread. fn send_worker(&self, cmd: WorkerCommand) { info!("sending command to worker: {:?}", cmd); @@ -418,10 +435,7 @@ impl Spotify { // MPRIS implementation. if notify { #[cfg(feature = "mpris")] - if let Some(mpris_manager) = self.mpris.lock().unwrap().as_ref() { - info!("updating MPRIS volume"); - mpris_manager.send(MprisCommand::NotifyVolumeUpdate); - } + self.send_mpris(MprisCommand::NotifyVolumeUpdate) } } From 2cc6adc0bbe54f5aa11cda4fbbd76eb66f4698c6 Mon Sep 17 00:00:00 2001 From: haruInDisguise Date: Thu, 12 Sep 2024 00:04:56 +0200 Subject: [PATCH 2/4] MPRIS: implemented most clippy suggestions --- src/application.rs | 2 +- src/mpris.rs | 12 ++++++------ src/queue.rs | 3 +-- src/spotify.rs | 6 +++--- 4 files changed, 11 insertions(+), 12 deletions(-) diff --git a/src/application.rs b/src/application.rs index dcfca674..f64130f4 100644 --- a/src/application.rs +++ b/src/application.rs @@ -22,7 +22,7 @@ use crate::{authentication, ui, utils}; use crate::{command, queue, spotify}; #[cfg(feature = "mpris")] -use crate::mpris::{self, MprisCommand, MprisManager}; +use crate::mpris::{self, MprisManager}; #[cfg(unix)] use crate::ipc::{self, IpcSocket}; diff --git a/src/mpris.rs b/src/mpris.rs index 2c8dcbd9..0a998a31 100644 --- a/src/mpris.rs +++ b/src/mpris.rs @@ -466,10 +466,10 @@ impl MprisPlayer { #[derive(Debug)] pub enum MprisCommand { /// Notify about playback status and metadata updates. - NotifyPlaybackUpdate, + PlaybackUpdate, /// Notify about volume updates. - NotifyVolumeUpdate, - NotifyMetadataUpdate, + VolumeUpdate, + MetadataUpdate, } /// An MPRIS server that internally manager a thread which can be sent commands. This is internally @@ -527,14 +527,14 @@ impl MprisManager { loop { let ctx = player_iface_ref.signal_context(); match rx.next().await { - Some(MprisCommand::NotifyPlaybackUpdate) => { + Some(MprisCommand::PlaybackUpdate) => { player_iface.playback_status_changed(ctx).await?; } - Some(MprisCommand::NotifyVolumeUpdate) => { + Some(MprisCommand::VolumeUpdate) => { info!("sending MPRIS volume update signal"); player_iface.volume_changed(ctx).await?; } - Some(MprisCommand::NotifyMetadataUpdate) => { + Some(MprisCommand::MetadataUpdate) => { player_iface.metadata_changed(ctx).await?; } None => break, diff --git a/src/queue.rs b/src/queue.rs index 94f48e4a..69f6ba17 100644 --- a/src/queue.rs +++ b/src/queue.rs @@ -8,10 +8,9 @@ use notify_rust::Notification; use rand::prelude::*; use strum_macros::Display; -use crate::config::{Config, PlaybackState}; +use crate::config::{Config}; use crate::library::Library; use crate::model::playable::Playable; -use crate::mpris::MprisCommand; use crate::spotify::PlayerEvent; use crate::spotify::Spotify; diff --git a/src/spotify.rs b/src/spotify.rs index 1b999b21..3feaa578 100644 --- a/src/spotify.rs +++ b/src/spotify.rs @@ -317,7 +317,7 @@ impl Spotify { )); #[cfg(feature = "mpris")] - self.send_mpris(MprisCommand::NotifyMetadataUpdate); + self.send_mpris(MprisCommand::MetadataUpdate); } /// Update the cached status of the [Player]. This makes sure the status @@ -343,7 +343,7 @@ impl Spotify { *status = new_status; #[cfg(feature ="mpris")] - self.send_mpris(MprisCommand::NotifyPlaybackUpdate); + self.send_mpris(MprisCommand::PlaybackUpdate); } /// Reset the time tracking stats for the current song. This should be called when a new song is @@ -435,7 +435,7 @@ impl Spotify { // MPRIS implementation. if notify { #[cfg(feature = "mpris")] - self.send_mpris(MprisCommand::NotifyVolumeUpdate) + self.send_mpris(MprisCommand::VolumeUpdate) } } From 29474eb9b4981ec7fc257a0d789c7cfa764fb81f Mon Sep 17 00:00:00 2001 From: haruInDisguise <--help> Date: Mon, 16 Sep 2024 17:23:02 +0200 Subject: [PATCH 3/4] Fix: applied clippy suggestions + format --- src/application.rs | 9 ++------- src/mpris.rs | 17 +++++++++-------- src/queue.rs | 2 +- src/spotify.rs | 10 +++++----- 4 files changed, 17 insertions(+), 21 deletions(-) diff --git a/src/application.rs b/src/application.rs index f64130f4..fb76d901 100644 --- a/src/application.rs +++ b/src/application.rs @@ -22,7 +22,7 @@ use crate::{authentication, ui, utils}; use crate::{command, queue, spotify}; #[cfg(feature = "mpris")] -use crate::mpris::{self, MprisManager}; +use crate::mpris::MprisManager; #[cfg(unix)] use crate::ipc::{self, IpcSocket}; @@ -68,9 +68,6 @@ pub struct Application { /// Internally shared event_manager: EventManager, /// An IPC implementation using the D-Bus MPRIS protocol, used to control and inspect ncspot. - #[cfg(feature = "mpris")] - mpris_manager: MprisManager, - /// An IPC implementation using a Unix domain socket, used to control and inspect ncspot. #[cfg(unix)] ipc: Option, /// The object to render to the terminal. @@ -130,7 +127,7 @@ impl Application { )); #[cfg(feature = "mpris")] - let mpris_manager = mpris::MprisManager::new( + let mpris_manager = MprisManager::new( event_manager.clone(), queue.clone(), library.clone(), @@ -229,8 +226,6 @@ impl Application { queue, spotify, event_manager, - #[cfg(feature = "mpris")] - mpris_manager, #[cfg(unix)] ipc, cursive, diff --git a/src/mpris.rs b/src/mpris.rs index 0a998a31..44cbaf5c 100644 --- a/src/mpris.rs +++ b/src/mpris.rs @@ -465,11 +465,12 @@ impl MprisPlayer { /// Commands to control the [MprisManager] worker thread. #[derive(Debug)] pub enum MprisCommand { - /// Notify about playback status and metadata updates. - PlaybackUpdate, - /// Notify about volume updates. - VolumeUpdate, - MetadataUpdate, + /// Emit playback status + Playback, + /// Emit volume + Volume, + /// Emit metadata + Metadata, } /// An MPRIS server that internally manager a thread which can be sent commands. This is internally @@ -527,14 +528,14 @@ impl MprisManager { loop { let ctx = player_iface_ref.signal_context(); match rx.next().await { - Some(MprisCommand::PlaybackUpdate) => { + Some(MprisCommand::Playback) => { player_iface.playback_status_changed(ctx).await?; } - Some(MprisCommand::VolumeUpdate) => { + Some(MprisCommand::Volume) => { info!("sending MPRIS volume update signal"); player_iface.volume_changed(ctx).await?; } - Some(MprisCommand::MetadataUpdate) => { + Some(MprisCommand::Metadata) => { player_iface.metadata_changed(ctx).await?; } None => break, diff --git a/src/queue.rs b/src/queue.rs index 69f6ba17..ced8ad4b 100644 --- a/src/queue.rs +++ b/src/queue.rs @@ -8,7 +8,7 @@ use notify_rust::Notification; use rand::prelude::*; use strum_macros::Display; -use crate::config::{Config}; +use crate::config::Config; use crate::library::Library; use crate::model::playable::Playable; use crate::spotify::PlayerEvent; diff --git a/src/spotify.rs b/src/spotify.rs index 3feaa578..aeaf1770 100644 --- a/src/spotify.rs +++ b/src/spotify.rs @@ -317,7 +317,7 @@ impl Spotify { )); #[cfg(feature = "mpris")] - self.send_mpris(MprisCommand::MetadataUpdate); + self.send_mpris(MprisCommand::Metadata); } /// Update the cached status of the [Player]. This makes sure the status @@ -342,8 +342,8 @@ impl Spotify { let mut status = self.status.write().unwrap(); *status = new_status; - #[cfg(feature ="mpris")] - self.send_mpris(MprisCommand::PlaybackUpdate); + #[cfg(feature = "mpris")] + self.send_mpris(MprisCommand::Playback); } /// Reset the time tracking stats for the current song. This should be called when a new song is @@ -369,7 +369,7 @@ impl Spotify { } /// Send an [MprisCommand] to the mpris thread. - #[cfg(feature ="mpris")] + #[cfg(feature = "mpris")] fn send_mpris(&self, cmd: MprisCommand) { debug!("Sending mpris command: {:?}", cmd); if let Some(mpris_manager) = self.mpris.lock().unwrap().as_ref() { @@ -435,7 +435,7 @@ impl Spotify { // MPRIS implementation. if notify { #[cfg(feature = "mpris")] - self.send_mpris(MprisCommand::VolumeUpdate) + self.send_mpris(MprisCommand::Volume) } } From 32372905f38d70a43c4242d84e5d993d7edbd216 Mon Sep 17 00:00:00 2001 From: haruInDisguise Date: Thu, 19 Sep 2024 23:08:02 +0200 Subject: [PATCH 4/4] MPRIS: Renamed MprisCommands to be more resonable I've added a clippy exception so it does not complain to us about enum variants starting with the same prefix. --- src/mpris.rs | 13 +++++++------ src/spotify.rs | 6 +++--- 2 files changed, 10 insertions(+), 9 deletions(-) diff --git a/src/mpris.rs b/src/mpris.rs index 44cbaf5c..29fde7d6 100644 --- a/src/mpris.rs +++ b/src/mpris.rs @@ -464,13 +464,14 @@ impl MprisPlayer { /// Commands to control the [MprisManager] worker thread. #[derive(Debug)] +#[allow(clippy::enum_variant_names)] pub enum MprisCommand { /// Emit playback status - Playback, + EmitPlaybackStatus, /// Emit volume - Volume, + EmitVolumeStatus, /// Emit metadata - Metadata, + EmitMetadataStatus, } /// An MPRIS server that internally manager a thread which can be sent commands. This is internally @@ -528,14 +529,14 @@ impl MprisManager { loop { let ctx = player_iface_ref.signal_context(); match rx.next().await { - Some(MprisCommand::Playback) => { + Some(MprisCommand::EmitPlaybackStatus) => { player_iface.playback_status_changed(ctx).await?; } - Some(MprisCommand::Volume) => { + Some(MprisCommand::EmitVolumeStatus) => { info!("sending MPRIS volume update signal"); player_iface.volume_changed(ctx).await?; } - Some(MprisCommand::Metadata) => { + Some(MprisCommand::EmitMetadataStatus) => { player_iface.metadata_changed(ctx).await?; } None => break, diff --git a/src/spotify.rs b/src/spotify.rs index aeaf1770..41273b16 100644 --- a/src/spotify.rs +++ b/src/spotify.rs @@ -317,7 +317,7 @@ impl Spotify { )); #[cfg(feature = "mpris")] - self.send_mpris(MprisCommand::Metadata); + self.send_mpris(MprisCommand::EmitMetadataStatus); } /// Update the cached status of the [Player]. This makes sure the status @@ -343,7 +343,7 @@ impl Spotify { *status = new_status; #[cfg(feature = "mpris")] - self.send_mpris(MprisCommand::Playback); + self.send_mpris(MprisCommand::EmitPlaybackStatus); } /// Reset the time tracking stats for the current song. This should be called when a new song is @@ -435,7 +435,7 @@ impl Spotify { // MPRIS implementation. if notify { #[cfg(feature = "mpris")] - self.send_mpris(MprisCommand::Volume) + self.send_mpris(MprisCommand::EmitVolumeStatus) } }