spotifyd icon indicating copy to clipboard operation
spotifyd copied to clipboard

Upgrade rspotify to 0.11.5

Open eladyn opened this issue 2 years ago • 10 comments

This mainly upgrades the rspotify crate to 0.11.5. Since large parts of the API surface changed between (the previous) 0.8.0 and 0.11.5, the changes are a bit more complex in some cases.

Other changes

  • no longer clones the token many times on the creation of the dbus server and instead passes an Arc to the API client; this facilitates requesting a new token after expiry (fixes #1074)
  • avoids cloning in the .insert_metadata() as much as possible
  • makes the rspotify dependency optional (only needed with dbus_mpris)
  • fix device_name matching in some dbus methods, as the device.name is not (or no longer?) percent encoded
  • cargo update, since otherwise spotifyd would not compile with the dependency updates

TODO

  • [x] ~~improve clunky OpenURI implementation (see https://github.com/ramsayleung/rspotify/issues/218#issuecomment-1097240398)~~ There is currently not much, we can do about that ugly implementation. Will probably be resolved in a future version of rspotify.
  • [X] handle Episode cases properly
  • [X] ~~consider using the async variant of the rspotify crate (reqwest client)~~ This is not easily feasible as it would require many changes that would make this PR quite difficult to review, better suited for a follow-up.
  • [x] clean up the code
  • [x] strip down requested token scopes

fixes #1069

eladyn avatar Apr 16 '22 11:04 eladyn

i'm getting this error when compiling:

| error[E0658]: arbitrary expressions in key-value attributes are unstable | --> /usr/src/debug/spotifyd/0.3.3.2.AUTOINC+f0ee51cb1a-r0/cargo_home/bitbake/rspotify-model-0.11.5/src/idtypes.rs:284:21 | | | 284 | #[doc = concat!( | | _____________________^ | 285 | | "ID of type [Type::", stringify!($type), "], made up of only \ | 286 | | alphanumeric characters. Refer to the [module-level \ | 287 | | docs][crate::idtypes] for more information.") | | |_______________________________________________________________^ | ... | 413 | / define_idtypes!( | 414 | | Artist => ArtistId, | 415 | | Album => AlbumId, | 416 | | Track => TrackId, | ... | | 419 | | Episode => EpisodeId | 420 | | ); | | |__- in this macro invocation | | | = note: see issue #78835 <https://github.com/rust-lang/rust/issues/78835> for more information | = note: this error originates in a macro (in Nightly builds, run with -Z macro-backtrace for more info)

Any ideas on how to fix it ?

NNEU-1 avatar Apr 16 '22 20:04 NNEU-1

@NNEU-1 This originates in the rspotify crate and it seems like it might be because of an old rust version. Which version are you trying to compile this with? (It works for me on 1.60.0.)

eladyn avatar Apr 16 '22 21:04 eladyn

... Of course you are spot on :P It works with a new cargo version.

NNEU-1 avatar Apr 17 '22 06:04 NNEU-1

@eladyn, could you please consider merging this with your pr_collection branch ?

I just realised I have to go back to hardcoding the dbus type to "system" with this.

NNEU-1 avatar Apr 18 '22 16:04 NNEU-1

@eladyn, could you please consider merging this with your pr_collection branch ?

Should be added now. :relaxed:

eladyn avatar Apr 18 '22 19:04 eladyn

Just tried it. One thing I noticed, is that playback status messages on dbus stop after some time. However, if I change the volume, a properties changes event is fired with the correct metadata and playback status.

So it seems like polling the status & metadata works fine, but metadata & status changes are not properly registered to trigger a properties changed event.

NNEU-1 avatar Apr 19 '22 11:04 NNEU-1

@NNEU-1 Huh, that is strange. Has never happened to me yet (I think).

In theory, this still uses the same code that #1025 introduced. Is this a regular time interval that it stops working, does it start working again after manually polling the properties? Is there something in the logs that indicates a potential problem? If you turn on --debug mode, you should also be able to see the requests that rspotify sends (Making request ...).

Also, is this something that happens only on the pr_collection branch or also with this PR? I will try running it for a while and see if it happens to me too.

eladyn avatar Apr 19 '22 12:04 eladyn

@eladyn I only tried pr_collection so far.

I'll dig a little deeper on both version and let you know my findings.

NNEU-1 avatar Apr 19 '22 12:04 NNEU-1

Just a quick preliminary update: I think that my previous comment was a false alarm. I haven't experienced this problem ever since.

I was / am using a custom software to monitor the dbus for player events, which I also used to check the functionality of Spotifyds dbus. I believe I had two instances of this software running at once, one of them as a systemd service, the other one in the foreground for checking. So that might be why some events had gone missing.

NNEU-1 avatar May 02 '22 06:05 NNEU-1

Good to hear that this hasn't happened again. However, thanks to your comment, I discovered a bug: In combination with #1059 (which makes spotifyd reusable without restarting, among other things), it becomes apparent that the DBus “server” isn't properly shut down after the connection terminates. So spawning the new DBus server fails, and we end up having the orphaned server still running. We'll probably need to cancel the asynchronous task that is spawned on startup.

This might have been the reason for some weird behaviour that you describe, but I'm not sure. Anyway, I'll probably fix this in #1059, since this isn't really related to this PR.

Thank you for the help with testing!

eladyn avatar May 06 '22 23:05 eladyn

Thanks!

SimonTeixidor avatar Sep 06 '22 16:09 SimonTeixidor