mpris: report trackid as an object path, drop artUrl, track loop status

An empty artUrl is not a valid URI and makes some clients render a broken
image, and the repeat button snapped back to "None" because LoopStatus was
not stored.

1 files changed, 203 insertions, 22 deletions

diff --git a/src/mpris.rs b/src/mpris.rs
index 39630b0..f156f7f 100644
--- a/src/mpris.rs
+++ b/src/mpris.rs
@@ -25,7 +25,7 @@ use std::thread;
 use std::time::Duration;
 use zbus::blocking::Connection;
 use zbus::interface;
-use zbus::zvariant::Value;
+use zbus::zvariant::{ObjectPath, Value};
 
 // ---------------------------------------------------------------------------
 // org.mpris.MediaPlayer2 — root interface
@@ -96,6 +96,9 @@ pub struct MprisPlayer {
     queue: Arc<Mutex<Queue>>,
     scrobbler: Arc<RwLock<Scrobbler>>,
     scrobble_state: Arc<Mutex<ScrobbleState>>,
+    /// Shared with the state watcher in [`start_mpris_server`] so changes made over D-Bus are
+    /// announced back out as `PropertiesChanged`.
+    loop_status: Arc<Mutex<String>>,
 }
 
 /// Build the `Metadata` map (a{sv}) required by MPRIS, using only owned values
@@ -103,17 +106,18 @@ pub struct MprisPlayer {
 fn build_metadata(track: &Track) -> HashMap<String, Value<'static>> {
     let mut map: HashMap<String, Value<'static>> = HashMap::new();
 
-    map.insert(
-        "mpris:trackid".into(),
-        Value::new("/org/mpris/MediaPlayer2/TrackList/CurrentTrack"),
-    );
+    // `mpris:trackid` is typed as an object path (`o`) in the spec, not as a string.
+    let trackid = ObjectPath::try_from("/org/mpris/MediaPlayer2/TrackList/CurrentTrack")
+        .expect("static object path");
+    map.insert("mpris:trackid".into(), Value::new(trackid));
 
     let length_us = track
         .duration_secs
         .map(|d| (d * 1_000_000.0) as i64)
         .unwrap_or(0);
     map.insert("mpris:length".into(), Value::new(length_us));
-    map.insert("mpris:artUrl".into(), Value::new(""));
+    // NOTE: `mpris:artUrl` is intentionally omitted. We have no artwork to offer, and an empty
+    // string is not a valid URI, which makes some clients render a broken image placeholder.
 
     map.insert(
         "xesam:title".into(),
@@ -138,13 +142,39 @@ fn build_metadata(track: &Track) -> HashMap<String, Value<'static>> {
 
 fn empty_metadata() -> HashMap<String, Value<'static>> {
     let mut map: HashMap<String, Value<'static>> = HashMap::new();
-    map.insert(
-        "mpris:trackid".into(),
-        Value::new("/org/mpris/MediaPlayer2/TrackList/NoTrack"),
-    );
+    let trackid = ObjectPath::try_from("/org/mpris/MediaPlayer2/TrackList/NoTrack")
+        .expect("static object path");
+    map.insert("mpris:trackid".into(), Value::new(trackid));
     map
 }
 
+/// Path of the track MPRIS should be reporting, if there is one.
+fn current_track_path(audio: &AudioEngine, queue: &Arc<Mutex<Queue>>) -> Option<PathBuf> {
+    if let Some(path) = audio.current_track() {
+        return Some(path);
+    }
+    queue
+        .lock()
+        .ok()
+        .and_then(|q| q.current().map(|t| t.path.clone()))
+}
+
+/// Build the `Metadata` map for whatever is loaded right now.
+fn current_metadata(
+    audio: &AudioEngine,
+    queue: &Arc<Mutex<Queue>>,
+) -> HashMap<String, Value<'static>> {
+    if let Some(track_path) = audio.current_track() {
+        build_metadata(&Track::from_path(track_path))
+    } else if let Ok(q) = queue.lock()
+        && let Some(track) = q.current()
+    {
+        build_metadata(track)
+    } else {
+        empty_metadata()
+    }
+}
+
 fn playback_status(audio: &AudioEngine) -> String {
     if audio.is_playing() {
         "Playing"
@@ -303,7 +333,29 @@ impl MprisPlayer {
 
     #[zbus(property)]
     fn loop_status(&self) -> String {
-        "None".to_string()
+        self.loop_status
+            .lock()
+            .map(|s| s.clone())
+            .unwrap_or_else(|_| "None".to_string())
+    }
+
+    /// NOTE: repeat behavior is not implemented in the queue yet. This only records what a
+    /// client asked for so controls such as the Plasma widget's repeat button round-trip
+    /// instead of snapping straight back to "None".
+    #[zbus(property)]
+    #[zbus(name = "LoopStatus")]
+    fn set_loop_status(&self, value: String) -> zbus::fdo::Result<()> {
+        match value.as_str() {
+            "None" | "Playlist" | "Track" => {
+                if let Ok(mut s) = self.loop_status.lock() {
+                    *s = value;
+                }
+                Ok(())
+            }
+            other => Err(zbus::fdo::Error::InvalidArgs(format!(
+                "invalid LoopStatus: {other}"
+            ))),
+        }
     }
 
     #[zbus(property)]
@@ -319,6 +371,28 @@ impl MprisPlayer {
             .unwrap_or(false)
     }
 
+    /// MPRIS only knows on/off, so turning shuffle on maps to [`ShuffleMode::All`] — unless a
+    /// more specific mode (album shuffle) is already active, which this deliberately preserves
+    /// instead of silently downgrading it.
+    #[zbus(property)]
+    #[zbus(name = "Shuffle")]
+    fn set_shuffle(&self, value: bool) {
+        if let Ok(mut q) = self.queue.lock() {
+            let current = q.shuffle_mode().clone();
+            let mode = if !value {
+                ShuffleMode::None
+            } else if current == ShuffleMode::None {
+                ShuffleMode::All
+            } else {
+                current.clone()
+            };
+
+            if mode != current {
+                q.set_shuffle(mode);
+            }
+        }
+    }
+
     #[zbus(property)]
     fn volume(&self) -> f64 {
         self.audio.get_volume() as f64 / 100.0
@@ -382,16 +456,7 @@ impl MprisPlayer {
 
     #[zbus(property)]
     fn metadata(&self) -> HashMap<String, Value<'static>> {
-        if let Some(track_path) = self.audio.current_track() {
-            let track = Track::from_path(track_path);
-            build_metadata(&track)
-        } else if let Ok(q) = self.queue.lock()
-            && let Some(track) = q.current()
-        {
-            build_metadata(track)
-        } else {
-            empty_metadata()
-        }
+        current_metadata(&self.audio, &self.queue)
     }
 }
 
@@ -420,12 +485,20 @@ pub fn start_mpris_server(
                 }
             };
 
+            // Clones for the state watcher below; the interface takes the originals.
+            let poll_audio = Arc::clone(&audio);
+            let poll_queue = Arc::clone(&queue);
+
+            let loop_status = Arc::new(Mutex::new("None".to_string()));
+            let poll_loop_status = Arc::clone(&loop_status);
+
             // Register both MPRIS interfaces on the same object path
             let player = MprisPlayer {
                 audio,
                 queue,
                 scrobbler,
                 scrobble_state,
+                loop_status,
             };
 
             if let Err(e) = conn.object_server().at("/org/mpris/MediaPlayer2", Mpris2) {
@@ -453,8 +526,116 @@ pub fn start_mpris_server(
                 "mpris: serving on org.mpris.MediaPlayer2.rsplayer"
             ));
 
-            // Keep the connection alive until shutdown.
+            // Keep the connection alive until shutdown, and announce our state changes.
+            //
+            // Most MPRIS clients do not poll: they read the properties once when our bus name
+            // appears and afterwards rely exclusively on `PropertiesChanged` signals. The KDE
+            // Plasma "Media Player" widget is one of them, which is why it used to stay frozen
+            // on the values from startup — or stay hidden entirely when we happened to be
+            // stopped at that moment. (playerctl keeps working because it polls on demand.)
+            //
+            // So we watch our own state here and push every change out: playback status,
+            // metadata, volume, shuffle and loop status. The steady tick of the playhead is
+            // deliberately not pushed — the spec tells clients to interpolate while we report
+            // `Playing` — but jumps in it are announced through the `Seeked` signal.
+            let mut last_status: Option<String> = None;
+            let mut last_track: Option<PathBuf> = None;
+            let mut last_volume: Option<f64> = None;
+            let mut last_shuffle: Option<ShuffleMode> = None;
+            let mut last_loop: Option<String> = None;
+
             while !shutdown_flag.load(std::sync::atomic::Ordering::SeqCst) {
+                let status = playback_status(&poll_audio);
+                let track = current_track_path(&poll_audio, &poll_queue);
+                let volume = poll_audio.get_volume() as f64 / 100.0;
+                let shuffle = poll_queue
+                    .lock()
+                    .map(|q| q.shuffle_mode().clone())
+                    .unwrap_or(ShuffleMode::None);
+                let loop_status_value = poll_loop_status
+                    .lock()
+                    .map(|s| s.clone())
+                    .unwrap_or_else(|_| "None".to_string());
+
+                let status_changed = last_status.as_deref() != Some(status.as_str());
+                let track_changed = last_track != track;
+                let volume_changed = last_volume != Some(volume);
+                let shuffle_changed = last_shuffle.as_ref() != Some(&shuffle);
+                let loop_changed = last_loop.as_deref() != Some(loop_status_value.as_str());
+
+                if status_changed
+                    || track_changed
+                    || volume_changed
+                    || shuffle_changed
+                    || loop_changed
+                {
+                    let mut changed: HashMap<&str, Value<'static>> = HashMap::new();
+                    if status_changed {
+                        changed.insert("PlaybackStatus", Value::new(status.clone()));
+                    }
+                    if track_changed {
+                        // Building metadata reads tags from disk, so only do it when the
+                        // current track actually changed.
+                        changed.insert(
+                            "Metadata",
+                            Value::new(current_metadata(&poll_audio, &poll_queue)),
+                        );
+                    }
+                    if volume_changed {
+                        changed.insert("Volume", Value::new(volume));
+                    }
+                    if shuffle_changed {
+                        changed.insert("Shuffle", Value::new(shuffle != ShuffleMode::None));
+                    }
+                    if loop_changed {
+                        changed.insert("LoopStatus", Value::new(loop_status_value.clone()));
+                    }
+
+                    let body = (
+                        "org.mpris.MediaPlayer2.Player",
+                        &changed,
+                        Vec::<&str>::new(),
+                    );
+                    if let Err(e) = conn.emit_signal(
+                        None::<&str>,
+                        "/org/mpris/MediaPlayer2",
+                        "org.freedesktop.DBus.Properties",
+                        "PropertiesChanged",
+                        &body,
+                    ) {
+                        crate::debug_eprintln(format_args!(
+                            "mpris: failed to emit PropertiesChanged: {}",
+                            e
+                        ));
+                    }
+                }
+
+                // A different track (or none at all) means the playhead jumped, which is what
+                // `Seeked` exists for. Clients use it to reset their progress bar without
+                // having to notice that two consecutive tracks happen to share a length.
+                if track_changed {
+                    let position_us = if track.is_some() {
+                        (poll_audio.position() * 1_000_000.0) as i64
+                    } else {
+                        0
+                    };
+                    if let Err(e) = conn.emit_signal(
+                        None::<&str>,
+                        "/org/mpris/MediaPlayer2",
+                        "org.mpris.MediaPlayer2.Player",
+                        "Seeked",
+                        &(position_us,),
+                    ) {
+                        crate::debug_eprintln(format_args!("mpris: failed to emit Seeked: {}", e));
+                    }
+                }
+
+                last_status = Some(status);
+                last_track = track;
+                last_volume = Some(volume);
+                last_shuffle = Some(shuffle);
+                last_loop = Some(loop_status_value);
+
                 thread::sleep(Duration::from_millis(500));
             }