fix(mpris): missing PropertyChanged signal for volume

CHANGELOG.md

@@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 - Crash on Android (Termux) due to unknown user runtime directory
 - Crash due to misconfigured or unavailable audio backend
+- Missing MPRIS signal for volume changes when volume is changed from inside `ncspot`
 
 ## [1.0.0] - 2023-12-16
 

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::{self, MprisCommand, MprisManager};
 
 #[cfg(unix)]
 use crate::ipc::{self, IpcSocket};
@@ -115,7 +115,7 @@ impl Application {
 
         let event_manager = EventManager::new(cursive.cb_sink().clone());
 
-        let spotify =
+        let mut spotify =
             spotify::Spotify::new(event_manager.clone(), credentials, configuration.clone())?;
 
         let library = Arc::new(Library::new(
@@ -138,6 +138,9 @@ impl Application {
             spotify.clone(),
         );
 
+        #[cfg(feature = "mpris")]
+        spotify.set_mpris(mpris_manager.clone());
+
         #[cfg(unix)]
         let ipc = if let Ok(runtime_directory) = utils::create_runtime_directory() {
             Some(
@@ -239,7 +242,7 @@ impl Application {
                         self.spotify.update_status(state.clone());
 
                         #[cfg(feature = "mpris")]
-                        self.mpris_manager.update();
+                        self.mpris_manager.send(MprisCommand::NotifyPlaybackUpdate);
 
                         #[cfg(unix)]
                         if let Some(ref ipc) = self.ipc {

src/commands.rs

@@ -196,7 +196,7 @@ impl CommandManager {
                     .spotify
                     .volume()
                     .saturating_add(VOLUME_PERCENT * amount);
-                self.spotify.set_volume(volume);
+                self.spotify.set_volume(volume, true);
                 Ok(None)
             }
             Command::VolumeDown(amount) => {
@@ -205,7 +205,7 @@ impl CommandManager {
                     .volume()
                     .saturating_sub(VOLUME_PERCENT * amount);
                 debug!("vol {}", volume);
-                self.spotify.set_volume(volume);
+                self.spotify.set_volume(volume, true);
                 Ok(None)
             }
             Command::Help => {

src/mpris.rs

@@ -1,3 +1,4 @@
+use log::info;
 use std::collections::HashMap;
 use std::error::Error;
 use std::sync::Arc;
@@ -274,7 +275,7 @@ impl MprisPlayer {
         log::info!("set volume: {volume}");
         volume = volume.clamp(0.0, 1.0);
         let vol = (VOLUME_PERCENT as f64) * volume * 100.0;
-        self.spotify.set_volume(vol as u16);
+        self.spotify.set_volume(vol as u16, false);
         self.event.trigger();
     }
 
@@ -461,8 +462,19 @@ impl MprisPlayer {
     }
 }
 
+/// Commands to control the [MprisManager] worker thread.
+pub enum MprisCommand {
+    /// Notify about playback status and metadata updates.
+    NotifyPlaybackUpdate,
+    /// Notify about volume updates.
+    NotifyVolumeUpdate,
+}
+
+/// An MPRIS server that internally manager a thread which can be sent commands. This is internally
+/// shared and cloning it will yield a reference to the same server.
+#[derive(Clone)]
 pub struct MprisManager {
-    tx: mpsc::UnboundedSender<()>,
+    tx: mpsc::UnboundedSender<MprisCommand>,
 }
 
 impl MprisManager {
@@ -480,7 +492,7 @@ impl MprisManager {
             spotify,
         };
 
-        let (tx, rx) = mpsc::unbounded_channel::<()>();
+        let (tx, rx) = mpsc::unbounded_channel::<MprisCommand>();
 
         ASYNC_RUNTIME.get().unwrap().spawn(async {
             let result = Self::serve(UnboundedReceiverStream::new(rx), root, player).await;
@@ -493,7 +505,7 @@ impl MprisManager {
     }
 
     async fn serve(
-        mut rx: UnboundedReceiverStream<()>,
+        mut rx: UnboundedReceiverStream<MprisCommand>,
         root: MprisRoot,
         player: MprisPlayer,
     ) -> Result<(), Box<dyn Error + Sync + Send>> {
@@ -511,15 +523,24 @@ impl MprisManager {
         let player_iface = player_iface_ref.get().await;
 
         loop {
-            rx.next().await;
             let ctx = player_iface_ref.signal_context();
-            player_iface.playback_status_changed(ctx).await?;
-            player_iface.metadata_changed(ctx).await?;
+            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?;
+                }
+                None => break,
+            }
         }
+        Err("MPRIS server command channel closed".into())
     }
 
-    pub fn update(&self) {
-        if let Err(e) = self.tx.send(()) {
+    pub fn send(&self, command: MprisCommand) {
+        if let Err(e) = self.tx.send(command) {
             log::warn!("Could not update MPRIS state: {e}");
         }
     }

src/spotify.rs

@@ -25,6 +25,8 @@ use crate::application::ASYNC_RUNTIME;
 use crate::config;
 use crate::events::{Event, EventManager};
 use crate::model::playable::Playable;
+#[cfg(feature = "mpris")]
+use crate::mpris::{MprisCommand, MprisManager};
 use crate::spotify_api::WebApi;
 use crate::spotify_worker::{Worker, WorkerCommand};
 
@@ -46,6 +48,8 @@ pub enum PlayerEvent {
 pub struct Spotify {
     events: EventManager,
     /// The credentials for the currently logged in user, used to authenticate to the Spotify API.
+    #[cfg(feature = "mpris")]
+    mpris: Arc<std::sync::Mutex<Option<MprisManager>>>,
     credentials: Credentials,
     cfg: Arc<config::Config>,
     /// Playback status of the [Player] owned by the worker thread.
@@ -67,6 +71,8 @@ impl Spotify {
     ) -> Result<Self, Box<dyn Error>> {
         let mut spotify = Self {
             events,
+            #[cfg(feature = "mpris")]
+            mpris: Default::default(),
             credentials,
             cfg: cfg.clone(),
             status: Arc::new(RwLock::new(PlayerEvent::Stopped)),
@@ -80,7 +86,7 @@ impl Spotify {
         spotify.start_worker(Some(user_tx))?;
         let user = ASYNC_RUNTIME.get().unwrap().block_on(user_rx).ok();
         let volume = cfg.state().volume;
-        spotify.set_volume(volume);
+        spotify.set_volume(volume, true);
 
         spotify.api.set_worker_channel(spotify.channel.clone());
         spotify.api.update_token();
@@ -393,11 +399,21 @@ impl Spotify {
         self.cfg.state().volume
     }
 
-    /// Set the current volume of the [Player].
-    pub fn set_volume(&self, volume: u16) {
+    /// Set the current volume of the [Player]. If `notify` is true, also notify MPRIS clients about
+    /// the update.
+    pub fn set_volume(&self, volume: u16, notify: bool) {
         info!("setting volume to {}", volume);
         self.cfg.with_state_mut(|s| s.volume = volume);
         self.send_worker(WorkerCommand::SetVolume(volume));
+        // HACK: This is a bit of a hack to prevent duplicate update signals when updating from the
+        // 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);
+            }
+        }
     }
 
     /// Preload the given [Playable] in the [Player]. This makes sure it can be played immediately
@@ -410,6 +426,11 @@ impl Spotify {
     pub fn shutdown(&self) {
         self.send_worker(WorkerCommand::Shutdown);
     }
+
+    #[cfg(feature = "mpris")]
+    pub fn set_mpris(&mut self, mpris: MprisManager) {
+        *self.mpris.lock().unwrap() = Some(mpris);
+    }
 }
 
 /// A type of Spotify URI.

src/ui/statusbar.rs

@@ -238,7 +238,7 @@ impl View for StatusBar {
                         .volume()
                         .saturating_add(crate::spotify::VOLUME_PERCENT);
 
-                    self.spotify.set_volume(volume);
+                    self.spotify.set_volume(volume, true);
                 }
 
                 if event == MouseEvent::WheelDown {
@@ -247,7 +247,7 @@ impl View for StatusBar {
                         .volume()
                         .saturating_sub(crate::spotify::VOLUME_PERCENT);
 
-                    self.spotify.set_volume(volume);
+                    self.spotify.set_volume(volume, true);
                 }
             } else if event == MouseEvent::Press(MouseButton::Left) {
                 self.queue.toggleplayback();