Implement PropertiesChanged event for dbus_mpris

[MDeiml]

This should fix #457.

[eladyn]

Thanks for implementing this!

I assume that you are aware of #575? I personally think that your code looks much cleaner, as it reuses most of the existing logic, although I have not yet tested it.

(I'm not a maintainer, just wanted to add my thoughts on this.)

[MDeiml]

To be more specific: this now implements the PropertiesChanged signal for the Metadata and PlaybackStatus properties as well as the Seeked signal.

[eladyn]

I tested this PR for some time now and it works really well! (tested with the native GNOME indicator as well as with playerctl)

It should probably also fix #910.

[dave-tucker]

Tested on my setup (i3status-rust with spotifyd and spotify-tui) and I can also confirm that this works like a charm!
Thank you!!!

[PaulFinch]

Thank you ! It's a great feature
Waiting for this to be implemented, it will prevent me to poll the PlayingStatus (which sometimes returns 'Stopped' when Playing, awkwardly).

[NNEU-1]

Great to see this PR.
The DBus implementation has been the subject of many issues reported on this package.
It is sad however, that now that someone has put in the work, it doesn't get merged.
Should have been approved by the maintainers by now, or at least had feedback on why it isn't.

[eladyn]

As per this comment, I'll just try and ping the maintainers of this project. @mainrs, @JojiiOfficial, @slondr, @simonpersson: It would be great, if someone "official" could review this PR as it really solves several longstanding problems!

Sidenote, unrelated to this specific PR: I really appreciate the time and energy that you put into this project, to keep it going. Still, it would definitely help, if there was some guideline (e.g. in the CONTRIBUTING.md) on what people, who want to make contributions to spotifyd, should do, if there is no reaction at all (except for those from the stale bot) on their PRs. At the very least, it could set the right expectations for potential contributors.

[JojiiOfficial]

Please rather add review requests than pings. I appreciate your effort but it feels wrong for me to approve bigger PRs, as I barely have knowledge of the existing code nor the code style of spotifyd. Also I don't have the time to check on that everything is bugfree and works as intended.

[slondr]

To be honest, we should probably turn off stalebot. It made sense at the time, but development has slowed down since 2020 (and I certainly have less free time now to review issues and PRs).

[eladyn]

Thank you both for the immediate answers!

Unfortunately it seems that "normal" people (non-members) can't request reviews (I definitely can't do that on my own PRs).

[NNEU-1]

@MDeiml
Can you confirm that the Volume property is not working ?
I can see it being implemented, but it doesn't seem to emit any signals.

[MDeiml]

You are right, this is not implemented as of now. My implementation only listens to PlayerEvent::Stopped, PlayerEvent::Playing and PlayerEvent::Paused events. Volume could be implemented in a similar way by using the PlayerEvent::VolumeSet event. (see PlayerEvent)

[NNEU-1]

Ah I see..

I was looking at:
b.property("Volume").emits_changed_false().get(move |_, _| { let sp = create_spotify_api(&mv_api_token); let vol = sp .current_playback(None) .ok() .flatten() .map_or(0.0, |p| p.device.volume_percent as f64); Ok(vol) });

If I see this correctly, you haven't actually changed much of the existing code, but instead added the properties changed events separately by using the DBus_crossroads framework.

So the old code is still handling commands or requests towards spotifyd.
At least some of them - Volume is a read only property which it shouldn't be.

[eladyn]

@NNEU-1 There was a short discussion on the Volume property being readonly here (even if it was not really related to the actual issue). Changing the function to be read and write shouldn't be too difficult as there is a function in rspotify which does exactly that. (The librespot functions allow only in- or decreasing the volume and not setting a specific value, IIRC)

[NNEU-1]

@eladyn
Now that you're pointing it out, I recall having investigated this myself some time ago...
#962

Anyways, back to the issue..
I made a PR on this branch to implement volume changed events according to @MDeiml s proposal.
Tested and works.
I have no background or experience in rust, so i trust you'll let me know if this is good enough.

MDeiml#2 (comment)

[Kerwood]

Thank you for the PR.
This was much needed. Compiled and tested, works like a charm. 👍

[robinvd]

looks very good! sad that the other pr never got merged, but i think this one is more complete and up to date.

if you can fix the lints i think this is ready!

also sorry for the delays and again thx for the PR!

[robinvd]

Please rather add review requests than pings. I appreciate your effort but it feels wrong for me to approve bigger PRs, as I barely have knowledge of the existing code nor the code style of spotifyd. Also I don't have the time to check on that everything is bugfree and works as intended.

@JojiiOfficial maybe just test it and approve?