Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade rspotify to 0.11.5 #1079

Merged
merged 9 commits into from
Sep 6, 2022
Merged

Conversation

eladyn
Copy link
Member

@eladyn eladyn commented Apr 16, 2022

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 Err(Unauthorized) when fetching metadata #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

  • improve clunky OpenURI implementation (see Upgrading to v0.11: questions and support ramsayleung/rspotify#218 (comment))
    There is currently not much, we can do about that ugly implementation. Will probably be resolved in a future version of rspotify.
  • handle Episode cases properly
  • 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.
  • clean up the code
  • strip down requested token scopes

fixes #1069

@NNEU-1
Copy link
Contributor

NNEU-1 commented Apr 16, 2022

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 ?

@eladyn
Copy link
Member Author

eladyn commented Apr 16, 2022

@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.)

@NNEU-1
Copy link
Contributor

NNEU-1 commented Apr 17, 2022

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

@NNEU-1
Copy link
Contributor

NNEU-1 commented Apr 18, 2022

@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.

@eladyn
Copy link
Member Author

eladyn commented Apr 18, 2022

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

Should be added now. ☺️

@NNEU-1
Copy link
Contributor

NNEU-1 commented Apr 19, 2022

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.

@eladyn
Copy link
Member Author

eladyn commented Apr 19, 2022

@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.

@NNEU-1
Copy link
Contributor

NNEU-1 commented Apr 19, 2022

@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
Copy link
Contributor

NNEU-1 commented May 2, 2022

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.

@eladyn
Copy link
Member Author

eladyn commented May 6, 2022

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 eladyn marked this pull request as ready for review May 28, 2022 23:38
@eladyn eladyn mentioned this pull request Aug 15, 2022
1 task
@SimonTeixidor SimonTeixidor merged commit d430360 into Spotifyd:master Sep 6, 2022
@SimonTeixidor
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Err(Unauthorized) when fetching metadata DBUS/MPRIS-interface broken? Unable to fetch metadata
3 participants