Whitelist Squeezebox for Shuffle and LoopStatus props

[mavit]

• The shuffle and loop statuses toggle back so quickly that a change of value is never signalled, meaning that these capabilities are not detected.
• Silently attempting to toggle the shuffle status is potentially disruptive here, since Squeezeboxes support two kinds of shuffle (by track and by album), but MPRIS doesn’t distinguish between the two.

[JasonLG1979]

Maybe create a SHUFFLE_LOOP_WHITELIST array const below METADATA_KEYS with 'Headset', 'Squeezebox remote control' in it.

I really hate white lists though it just encourages poor behavior from player.

The shuffle and loop statuses toggle back so quickly that a change of value is never signalled, meaning that these capabilities are not detected.

That's a bug in their MPRIS implementation. It doesn't matter how fast a prop is changed. A prop change should always be followed by a prop change signal. 99.9% of players do not have a problem with this.

Silently attempting to toggle the shuffle status is potentially disruptive here, since Squeezeboxes support two kinds of shuffle (by track and by album), but MPRIS doesn’t distinguish between the two.

Well then it's up to their MPRIS implementation to decide if they want to support shuffle at all and how they map it.

You should bring these issues up with whoever is in charge of their MPRIS implementation before putting in a PR to workaround their bugs.

[mavit]

Maybe create a SHUFFLE_LOOP_WHITELIST array const below METADATA_KEYS with 'Headset', 'Squeezebox remote control' in it.

Done.

I really hate white lists though it just encourages poor behavior from apps.

Fair.

I wonder, is the test that this whitelist bypasses actually needed? If a player exists with read-only Shuffle and LoopStatus, wouldn’t it be better to show the buttons in that case? Clicking the buttons will be ineffective, but at least the current status will be shown. To (partially) indicate this, perhaps these buttons could be made insensitive on click and sensitive on receiving a change of shuffle/loop status.

The shuffle and loop statuses toggle back so quickly that a change of value is never signalled, meaning that these capabilities are not detected.

That's a bug in their MPRIS implementation. It doesn't matter how fast a prop is signaled. A prop change should always be followed by a prop change signal. 99.9% of players do not have a problem with this.

A complication is that the player does not implement MPRIS natively (and cannot, since it runs on a remote host). Here, MPRIS is being bridged to its native remote control protocol.

Silently attempting to toggle the shuffle status is potentially disruptive here, since Squeezeboxes support two kinds of shuffle (by track and by album), but MPRIS doesn’t distinguish between the two.

Well then it's up to their MPRIS implementation to decide if they want to support shuffle at all and how they map it.

You should bring these issues up with whoever is in charge of their MPRIS implementation

That’s wound up being me.

[JasonLG1979]

I wonder, is the test that this whitelist bypasses actually needed? If a player exists with read-only Shuffle and LoopStatus, wouldn’t it be better to show the buttons in that case? Clicking the buttons will be ineffective, but at least the current status will be shown. To (partially) indicate this, perhaps these buttons could be made insensitive on click and sensitive on receiving a change of shuffle/loop status.

Shuffle and LoopStatus are read/write according to the MPRIS spec. If a player's Shuffle or LoopStatus is read-only I would expect them to either not expose them over MPRIS or if it is exposed they need to handle clients trying to change it and still send a prop changed signal. Poking properties is correct way to tell if they are implemented and implemented correctly according to how DBus works and how MPRIS works according to the authors of the spec.

A complication is that the player does not implement MPRIS natively (and cannot, since it runs on a remote host). Here, MPRIS is being bridged to its native remote control protocol.

Well, the interface should not even be brought up yet unless it's fully functional and completely hooked into the player. Where the player is and how you do that is not a concern of MPRIS.

It should be Bring up player > hook into player & gather all states/props/whatever > *Bring up MPRIS interface.

You shouldn't bring up a non-fully functional MPRIS interface. When that interface show up it should be ready to rock'n roll.

That’s wound up being me.

Well then fix your implementation before you put in PR's here to work around your bug.

[mavit]

Shuffle and LoopStatus are read/write according to the MPRIS spec.

So what I'm trying to say is, would there be any problem with testing for their existence by reading them, rather than writing them? I'm perhaps missing something obvious.

[JasonLG1979]

So what I'm trying to say is, would there be any problem with testing for their existence by reading them, rather than writing them? I'm perhaps missing something obvious.

Testing if the props exist and testing if they're functional are 2 different things. The point of poking the props is to make sure they work. Like I said that's the proper way to make sure things work in DBus. Shuffle and LoopStatus are optional props so in the effort to not expose functionality that is possibly non-existent or doesn't work I poke them to make sure they elicit a response. No response, no work, no show.

Again the answer is to fix your implementation. So far you've offered no valid reason why I should accept this PR to workaround bugs in your implementation. It's up to you to figure out how to map Squeezebox's functionality to MPRIS not the other way around. Make your bridge spec compliant and it will work fine.

[JasonLG1979]

My fear is that I'll accept this PR and you'll never fix your implementation.

[mavit]

Another thought: there's a short window between toggling and untoggling these settings during which the currently playing track could end. That will occasionally cause the player to go on to play the wrong next track.

[mavit]

You shouldn't bring up a non-fully functional MPRIS interface.

It functions, but the APIs don't exactly map to each other, and I think a bit of minor wonkyness is tolerable. Nobody’s paying for any of this, after all. Perfect is the enemy of good.

Of course, if you don’t want your code clogging up with what could be perceived as ugly workarounds then I’m not going to be offended.

[JasonLG1979]

Another thought: there's a short window between toggling and untoggling these settings during which the currently playing track could end. That will occasionally cause the player to go on to play the wrong next track.

It functions, but the APIs don't exactly map to each other, and I think a bit of minor wonkyness is tolerable. Nobody’s paying for any of this, after all. Perfect is the enemy of good.

Again that's up to you to deal with. If anything would put Squeezebox is some sort of undesired state you need to handle it. The default timeout of DBus is pretty generous. It's not like your responses have to be instantaneous you just have to respond eventually.

It functions, but the APIs don't exactly map to each other, and I think a bit of minor wonkyness is tolerable. Nobody’s paying for any of this, after all. Perfect is the enemy of good.

Of course, if you don’t want your code clogging up with what could be perceived as ugly workarounds then I’m not going to be offended.

If it doesn't map 1 to 1 then it's up to you to figure out if you want to expose it and how. It's not about being perfect. It's about not papering over a poor implementation.

I'm not trying to be a a-hole but if I worked around every players bugs the extension wouldn't be functional and it would just encourage poor implementation. I would assume you would want software you write to function correctly. The spec is public, just follow it.