Incorrect Implementations of the Can* properties of the MPRIS Specification

[N-Yuki]

CanGoNext/CanGoPrevious:

• Specification: https://specifications.freedesktop.org/mpris-spec/latest/Player_Interface.html#Property:CanGoNext
• Code locations:
  1. https://github.com/pragha-music-player/pragha/blob/master/plugins/mpris2/pragha-mpris2-plugin.c#L704
  2. https://github.com/pragha-music-player/pragha/blob/master/plugins/mpris2/pragha-mpris2-plugin.c#L711
• Current behaviour: Always return true.
• Consequence: MPRIS UIs cannot determine whether the Next/Previous button should be clickable.
• Expected behaviour: Return pragha_playlist_get_next/prev_track (playlist) != NULL.
• Notes:
  1. Pragha's UI never disables the Next/Previous, so this would give extra capabilities to MPRIS UIs over the default UI.
  2. The spec says to return true when it is unknown (for example, when streaming tracks), but this shouldn't be a potential case in Pragha.

CanPlay:

• Specification: https://specifications.freedesktop.org/mpris-spec/latest/Player_Interface.html#Property:CanPlay
• Code locations:
  1. https://github.com/pragha-music-player/pragha/blob/master/plugins/mpris2/pragha-mpris2-plugin.c#L718
• Current behaviour: Return true if pragha_backend_get_state (backend) == ST_PAUSED.
• Consequence: MPRIS UIs are misinformed on whether there is a track available to play. Quoting the spec:

  this is related to whether there is a "current track": the value should not depend on whether the track is currently paused or playing

• Possible behaviours:
  1. Return true if the playlist is non-empty. There is logic for starting a track from the Stopped state when Play is clicked, so it can be considered to mean there is always a "current track" whenever there is a playlist. This is consistent with with the Play/Pause button in Pragha (and other music players).
  2. Return pragha_backend_get_state (backend) != ST_STOPPED.

CanPause:

• Specification: https://specifications.freedesktop.org/mpris-spec/latest/Player_Interface.html#Property:CanPause
• Code locations:
  1. https://github.com/pragha-music-player/pragha/blob/master/plugins/mpris2/pragha-mpris2-plugin.c#L726
• Current behaviour: Return true if pragha_backend_get_state (backend) == ST_PLAYING.
• Consequence: MPRIS UIs are misinformed on whether the current track is pause-able. Quoting the spec:

  this is an intrinsic property of the current track: its value should not depend on whether the track is currently paused or playing

• Note: There does not seem to be the concept of an unpause-able track in Pragha(?).
• Possible behaviours:
  1. Match CanPlay, so this is true whenever there is a "current track".
  2. Always return true.

All of the above and CanSeek:

• Current behaviour: org.freedesktop.DBus.Properties.PropertiesChanged is not triggered for any of these properties.
• Consequence: MPRIS UIs using GDBusProxy will not recognise property changes (since they are cached and only updated when the appropriate signal is sent).
• Expected behaviour: Emit signal with appropriate parameters for each of these properties.

---

There is a $5 open bounty on this issue. Add to the bounty at Bountysource.

[matiasdelellis]

Hi @N-Yuki
Thanks for showing me all this .. 😉
The implementation tries to be the minimum so that it works in the current MPRIS controlers, and can be improved.. I take note!. 👍

[N-Yuki]

@matiasdelellis No problem, but it's not really about being minimal. CanPlay and CanPause is outright wrong. I initially looked into since the mpris controller in gnome shell wouldn't work with pragha, so I was debugging on both sides to see if it was an issue on the shell side, but I realised that the implementation in pragha was just wrong, causing the controller to not work. Having a broken implementation is quite misleading. If the goal is minimal implementation, CanPlay and CanPause should just always return true, rather than the current incorrect logic.

…

On Wed, 12 Jul 2017, 10:09 PM matiasdelellis ***@***.***> wrote: Hi @N-Yuki <https://github.com/n-yuki> Thanks for showing me all this .. 😉 The implementation tries to be the minimum so that it works in the current MPRIS controlers, and can be improved.. I take note!. 👍 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#123 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAecY0kBbuir3lJxH4_C8usQF_IeJCWEks5sNNOhgaJpZM4OUeek> .

[matiasdelellis]

Ohh.. You're right!.
After merging the WIP/Ampache brach I will fix... Thanks again..

[matiasdelellis]

Hi @N-Yuki
Please.. See last commit.. 😉

Thanks for all..

[N-Yuki]

Hi @matiasdelellis
Looks better!

However, the last point about org.freedesktop.DBus.Properties.PropertiesChanged not being triggered for the above properties could still mean issues with GNOME Shell's MPRIS controller.
GNOME Shell checks the CanPlay property before showing the interface, and this property is only checked on application start-up and when signals to org.freedesktop.DBus.Properties.PropertiesChanged for CanPlay are made (due to how GDBusProxy works). This however may also warrant a bug report with GNOME Shell since they should ideally check CanControl, not CanPlay, to decide whether the interface is visible.

Anyway, for Pragha, I remember looking at its MPRIS implementation, and I noticed that it may not be trivial to make this change (maybe keep track of the last returned value for the properties, and see if they've change when there is a track change or something to decide whether a signal should be sent), but if there is no org.freedesktop.DBus.Properties.PropertiesChanged signal sent for CanGoNext/CanGoPrev/CanPlay/CanPause state changes, then it reduces the purpose of having any sort of dynamic logic for these properties. From the perspective of any application that uses GDBusProxy to communicate over DBus, they will see these properties as static since they will see the properties for CanGoNext/CanGoPrev/CanPlay/CanPause as they were on application start-up.

[matiasdelellis]

Hi again,
Now it emits all the properties that change, and emits all the properties when initiates a new song...
Also each time the playlist changes --Ie. Add new songs--, it emits the properties that change accordingly -- CanNext, etc-.

Please, try it, I think it's functional. 😄

P.s: Theoretically you could apply these patches in 1.3.3..

[matiasdelellis]

See coments on first commit.. I continue tomorrow.. 😉

[N-Yuki]

I agree with the comment on the first commit. It is not consistent at the moment with pragha_backend_get_state (backend) != ST_STOPPED.

As for the signal emitting, looks good!
I haven't tested it, but I can see it working with GNOME Shell and other MPRIS controllers that use GDBusProxy now.