Added possibility to turn on/off shuffle using MPRIS

[Bettehem]

Hello, I added shuffle support for MPRIS.
Solves #317.

For example using playerctl:
playerctl -p ncspot shuffle on -> Turns shuffle on
playerctl -p ncspot shuffle off -> Turns shuffle off

However I'm facing one problem that I can't figure out.
After adding these changes, running playerctl -p ncspot shuffle always returns "Off" regardless of the actual shuffle state.
Without these changes it functions as expected, and returns "On" or "Off" depending on the shuffle state.
I'd imagine it's probably a simple fix but I'm still a Rust newbie so I can't quite figure it out on my own :/

[r4v3n6101]

You could do it better using unwrap_or

[Bettehem]

Thanks for the suggestion @r4v3n6101! I have changed it to use unwrap_or now.

[hrkfdn]

Thanks for the suggestion @r4v3n6101! I have changed it to use unwrap_or now.

Previously an on_set without parameters would toggle the shuffle. Now it just sets the current shuffle setting. Is this intended, or should the default value in unwrap_or be flipped?

[r4v3n6101]

Also if get_spotify is very time consuming operation, you could do it with unwrap_or_else. Previously you have used variable so I didn't pay attention for lazy version of this method.

[Bettehem]

I was reading the MPRIS specification about the Shuffle property and there was no mention about toggling.
Playerctl has it's own implementation for toggling the shuffle state, which gets the shuffle state of the player and then inverts it.
I'm not quite sure what on_set without parameters should return. Does it ever even get called without parameters because if I understood correctly, on_set only gets called if you use either "On" or "Off"? Perhaps it could be enough to just use unwrap instead of unwrap_or

[r4v3n6101]

Using unwrap may cause application's crash, so it should be backed by default value in this case. You could use either true or false to provide default behaviour. As for me, unwrap isn't such bad way because it crashes with an error if something went wrong.

[Bettehem]

Which do you think would be a better default, true or false? Or just the current state but inverted

[r4v3n6101]

I think the best one is not using shuffle cause I'm assuming there's no shuffle by default. Also you can ask @hrkfdn as he defines code design.

[Bettehem]

Yeah. Defaulting to no shuffle is probably the best. @hrkfdn what do you think?

[hrkfdn]

The specifications state that clients should handle the no parameter case gracefully, so I'd suggest to do nothing if no setting is supplied, i.e. return.

…

On Sat, Nov 14, 2020, 7:48 AM Bettehem ***@***.***> wrote: Yeah. Defaulting to no shuffle is probably the best. @hrkfdn <https://github.com/hrkfdn> what do you think? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#318 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAE3LD7CNDHZGFFQ6ODVRLSPYR5FANCNFSM4TS3H2OA> .

[Bettehem]

So in the case that no setting is supplied, do you mean it would be good to best unwrap_or_default? If just using unwrap would result in a crash.
Oh and a quick question, in this case, does unwrap_or_default work the same way as unwrap_or_else(false)?

[hrkfdn]

I think unwrap_or with the current shuffle setting as the default is a solid approach. unwrap_or_default will fall back to the default value of a datatype, e.g. 0 for integers or false for booleans, so in this case it'd turn shuffle off by default. unwrap_or_else executes a closure to set the default value instead.

[hrkfdn]

I have pushed a minor change to implement this behavior. Could you confirm that it works for your use-case?

[Bettehem]

Yes! It works as it should. Thank you.

[Bettehem]

Now there is the next problem though.
For some reason getting the shuffle state always returns "Off" or false but without the addition of .on_set it works normally

[hrkfdn]

Ah, right. Sorry, I didn't have access to a machine running DBus to test it. Now it should work as expected.

[hrkfdn]

Merged, thanks!