DBus PropertiesChanged event not fired

[mainrs]

Description

Metadata changes do not raise the org.freedesktop.DBus.Properties.PropertiesChanged event

Compilation flags

• [x] dbus_mpris
• [ ] dbus_keyring
• [x] alsa_backend
• [ ] portaudio_backend
• [ ] pulseaudio_backend
• [ ] rodio_backend

Versions (please complete the following information):

• OS:
• Spotifyd:
  • cargo: <!-- cargo --version -->

See https://github.com/Spotifyd/spotifyd/issues/455

[justin-gerhardt]

I've tried implementing this a bit but I'm over my head with rust. The dbus library has native support for the propertiesChanged event. The properties have to have tagged .emits_changed(dbus::tree::EmitsChangedSignal::True)(invalidate doesn't work with playerctl)

Normally the signal is sent by the library automatically on property modification from dbus. In this case since the change is internal it has to be manually sent. A relevant github issue https://github.com/diwic/dbus-rs/issues/66 The only result on github I could find that uses the API You should also note that the signal will not appear on the introspection API. It is however still sent.

Ideally the signal would be raised for the metadata, playback status and position properties.

[mainrs]

Would you mind trying out if the version over at https://github.com/Spotifyd/spotifyd/tree/fix_propertieschanged_event ? This might work but I can't really try it because I don't have access to a linux env right now @justin-gerhardt

It should specifically only fix the metadata problem.

[justin-gerhardt]

Setting emits_changed alone only raises the signal if the property change was caused by a dbus message setting the metadata property (which can't happen, it's read only). dbus-rs has no way of knowing that fetching the underlying metadata would result in a new result.

You will have to somehow feed metadata change events into the dbus server and then run something along the lines of

let mut chg = Vec::new();
property_metadata.clone().add_propertieschanged(&mut chg, media_player2_player_interface.get_name(), ||{
     Box::new(// new metadata)
});
let msg = chg.first().unwrap().to_emit_message(object_path.clone().get_name());
connection.clone().send(msg).unwrap();

every time the metadata changes Note: I've wrapped the property in an Arc

[JasonLG1979]

Ideally the signal would be raised for the metadata, playback status and position properties.

DO NOT emit a property changed signal for position. That would result in a crazy amount of signals.

The spec clearly says not to.

"The org.freedesktop.DBus.Properties.PropertiesChanged signal is not emitted when this property changes."

https://specifications.freedesktop.org/mpris-spec/2.2/Player_Interface.html#Property:Position

[JasonLG1979]

My advice on properties are this:

1. Read the spec of the interfaces you implement. It will tell you if a property changed signal is expected. (most of the time it is with a couple exceptions)

2. Include ALL properties for the interfaces you implement even if you do not plan on changing them.

3. TrackId in the metadata is important, especially if you plan on implementing the TrackList interface.

4. Do not put up an empty interface. make sure that all properties will return at least a default value before you expose the interface.

5. When going from track to track you do not need to update the playback status unless it actually changes. (I see a lot of players that send redundant playback status prop change signals.)

6. You don't have to worry about batching signals, Send the signals when something changes don't wait. The flip side to that is also don't machine gun a bunch of redundant signals. (I see that a lot too.)

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[real-or-random]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

I don't think that this issue has been solved.

[MDeiml]

I have a fix for this coming, but it depends on #977 which is about to get merged.

[capnfabs]

977 apparently needs a second review before I can merge it :(

[capnfabs]

Also @MDeiml, don't know how far along your fix is, but this branch (https://github.com/Spotifyd/spotifyd/compare/master...capnfabs:main) has a working implementation of MPRIS with change notifications in case you want to borrow things from it!

It, however, also relies on underlying changes to librespot so that it doesn't have to make requests to the Spotify API; I got approval for that from the librespot team here (https://github.com/librespot-org/librespot/discussions/834) and then life got in the way and I never did anything about it 😅

Just in case any of that is helpful :)

[MDeiml]

@capnfabs Oh, my fix is only a few lines, I mostly just leverage the player event channel. It still uses the web API so it's probably a bit slower and less reliable than what you tried to achieve.

Unrelated to that I also looked at if we could reduce usage of the web API by instead getting info from librespot since that seems to be the cause of some delays like in #891, but I ran into the same problems as you did. librespot does not expose all the data that the web API does.