New DBUS method: TransferPlayback

[dzubybb]

This is a new TransferPlayback method available for DBUS. It's useful in my setup where spotifyd is part of audio system based on dietpi micro computer. It's used to transfer playback from other active spotify device.

Also i discovered weird error when setting my spotifyd device type to S_T_B. It was returned from device() method of spotify client as STB (all capitals) and was throwing error:

json parse error: unknown variant `STB`, expected one of `Computer`, `Tablet`, `Smartphone`, `Speaker`, `Tv`, `Avr`, `Stb`, `AudioDongle`, `GameConsole`, `CastVideo`, `CastAudio`, `Automobile`, `Unknown` at line 16 column 18

I didn't find STB anywhere in code so i assume that spotify changes that somehow ?

[eladyn]

Thank you for the contribution!

In general, looks like a good idea. I only just had a quick look and didn't look at the code properly yet, but one concern that I have, is that the interface we'd be adding that method to is org.mpris.MediaPlayer2.Player, which has a formal specification which doesn't include that method. We already differ from that spec with the VolumeUp and VolumeDown methods, but IMO we should try to stick to that as much as possible. Maybe you could add a separate interface specific to spotifyd to the dbus server which would then include your new method and maybe the two others that also don't fit into the spec.
Open to your opinion on this, though!

---

Regarding the error you're receiving, I'm quite sure that this comes from raspotify failing to parse the Web API answer. The reason is probably a missing alias here. If you want you could either contribute a fix to them directly or report it over there so that they can fix it. :)

[dzubybb]

Maybe you could add a separate interface specific to spotifyd to the dbus server which would then include your new method and maybe the two others that also don't fit into the spec.

Yes, i think this is good idea. Currently there are two DBUS interfaces org.mpris.MediaPlayer2 and org.mpris.MediaPlayer2.Player, both on /org/mpris/MediaPlayer2 path.
Maybe something like interface org.spotifyd.Controls on path /org/spotifyd/Controls ? Or do You have better idea ?

Regarding the error you're receiving, I'm quite sure that this comes from raspotify failing to parse the Web API answer. The reason is probably a missing alias here.

I will try to fix it.

Also i found another error, when trying to call my new method after longer time (about 20 hours, service was running this whole time) i got Get devices error: http error: status code 401 on call sp_client.device(), looks like token had expired. Shouldn't it be refreshed automatically ?

[dzubybb]

If you want you could either contribute a fix to them directly or report it over there so that they can fix it. :)

Issue ramsayleung/rspotify#383 pull request ramsayleung/rspotify#384

[eladyn]

Yes, i think this is good idea. Currently there are two DBUS interfaces org.mpris.MediaPlayer2 and org.mpris.MediaPlayer2.Player, both on /org/mpris/MediaPlayer2 path. Maybe something like interface org.spotifyd.Controls on path /org/spotifyd/Controls ? Or do You have better idea ?

Sounds good. To stick to a domain that the project actually "owns", maybe we could use io.github.spotifyd.Controls?

Also i found another error, when trying to call my new method after longer time (about 20 hours, service was running this whole time) i got Get devices error: http error: status code 401 on call sp_client.device(), looks like token had expired. Shouldn't it be refreshed automatically ?

Oh, that's weird! I think I implemented auto-refresh for the tokens some time ago and the D-Bus things should only be called, when a non-expired token is available? No idea, what's happening there. Did you notice anything special logged in the output?

[dzubybb]

Sounds good. To stick to a domain that the project actually "owns", maybe we could use io.github.spotifyd.Controls?

I've pushed changes, for now i copied VolumeUp and VolumeDown methods, instead of moving them, to avoid breaking changes. I've also marked those methods on org.mpris.MediaPlayer2.Player interface as deprecated.

Oh, that's weird! I think I implemented auto-refresh for the tokens some time ago and the D-Bus things should only be called, when a non-expired token is available? No idea, what's happening there. Did you notice anything special logged in the output?

I didn't notice anything else in logs, but i added logging of token expiration time, so i will know more when error repeats itself.

[eladyn]

Thank you. Some comments, but apart from that: looks good!

[slondr]

To stick to a domain that the project actually "owns", maybe we could use io.github.spotifyd.Controls?

Do we want to grab an actual domain name for the project? Seems useful in general, and for this specifically. I'd be happy to set that up if so.

[dzubybb]

@eladyn It looks like token is not always refreshed. I added code to log token expiration time an got those logs :

Jan 26 15:38:24 DietPi spotifyd[774]: Token expires at: 2023-01-26 14:20:53.258021114 UTC
Jan 26 15:38:24 DietPi spotifyd[774]: Get devices error: http error: status code 401
Jan 26 15:38:24 DietPi spotifyd[774]: Could not find device with name unitra_dietpi
Jan 26 15:38:38 DietPi spotifyd[774]: Token expires at: 2023-01-26 14:20:53.258021114 UTC
Jan 26 15:38:38 DietPi spotifyd[774]: Get devices error: http error: status code 401
Jan 26 15:38:38 DietPi spotifyd[774]: Could not find device with name unitra_dietpi
Jan 26 15:39:32 DietPi spotifyd[774]: Token expires at: 2023-01-26 14:20:53.258021114 UTC
Jan 26 15:39:32 DietPi spotifyd[774]: Get devices error: http error: status code 401
Jan 26 15:39:32 DietPi spotifyd[774]: Could not find device with name unitra_dietpi

Log time is UTC +1, so it's about 18 minutes past token expiration.

[eladyn]

@dzubybb Oh, right. This took me a bit, to figure out! What's happening here, is that we spawn our dbus handling logic on a separate tokio task. This way, our main logic isn't blocked by those sync http requests (maybe there are other reasons as well, I don't know), but this also means that our token refresh logic is not run, when there's activity in the dbus server, but only, when there are new events for our PropertiesChanged signal. That is, why it was always working for me and for you it regularly failed: I only tested the behaviour with spotifyd continuously running while you had longer periods without any playback which caused the token to expire. And since you aren't able to re-enable playback, no activity occurs and the token is still not refreshed.

I'm not yet sure, what an elegant solution to this might look like, and I'm likely not going to get to it in the next few days, but now we have at least identified the root cause.

Apart from that, I hope that get around to taking a look at your updated code in the following week.

[eladyn]

@slondr Sorry for not replying earlier! In general, I like the idea of having a proper domain for spotifyd. I'm not sure, however, if the benefits weigh up against the increased maintenance and cost burden on the domain holders (e.g. you).

My concern in that regard would be, that as soon as such a domain is established, dependencies on that domain will be popping up (e.g. usage of it as DBus name, linking to it in README, issues and other resources). Some of these are reverted more easily than others, so such a decision shouldn't be made light-heartedly, and the ones taking responsibility for the domain should be aware of the possible implications. But if you'd like to take up that challenge, I won't be stopping you. :)

[eladyn]

Some minor comments, other than that, this looks good to me. And it's working nicely as well!

[slondr]

@slondr Sorry for not replying earlier! In general, I like the idea of having a proper domain for spotifyd. I'm not sure, however, if the benefits weigh up against the increased maintenance and cost burden on the domain holders (e.g. you).

My concern in that regard would be, that as soon as such a domain is established, dependencies on that domain will be popping up (e.g. usage of it as DBus name, linking to it in README, issues and other resources). Some of these are reverted more easily than others, so such a decision shouldn't be made light-heartedly, and the ones taking responsibility for the domain should be aware of the possible implications. But if you'd like to take up that challenge, I won't be stopping you. :)

Oh it's no big deal for me, I have close to a dozen domains already that I don't even do anything with (but definitely will one day! I swear!), so adding 1 to the pile is fine.

Looks like I can grab spotifyd.rs for like $25 a year!

[dzubybb]

Code fixes done. I also added setter for Shuffle property. Should i commit it here, or open new pull request for that ?

[eladyn]

Code fixes done.

Thanks, it only seems that you missed one item? Or was this intentional / doesn't work like that?

I also added setter for Shuffle property. Should i commit it here, or open new pull request for that ?

Oh, that's great! I think, opening a new pull request should make it easier to review / allow us to merge this one earlier. It would be totally fine by me, as well, if you just based that second PR on the changes in this one. This way, you don't have to go through the hassle and rebase them on master.

[dzubybb]

Thanks, it only seems that you missed one item? Or was this intentional / doesn't work like that?

Right, forgot about that, pushed.

[dzubybb]

Damn, pushed too much :)

[dzubybb]

It would be totally fine by me, as well, if you just based that second PR on the changes in this one.

So... how do i do that ? Just create another PR from same master branch in my fork ?

[eladyn]

So... how do i do that ? Just create another PR from same master branch in my fork ?

You can create a new branch from the current commit on your master branch, add your changes and push that branch to your forked repo. Then, you should be able to create a new pull request from this second branch into this repo's master.

[eladyn]

Two little last things that I missed previously. After that, I think we're good to go!

[eladyn]

Thank you! LGTM and works fine.

[slondr]

epic