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 to librespot 0.5 #1309

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

davidgauch
Copy link

There are a number of breaking changes in the Librespot 0.4 to 0.5 release.
There are 3 TODO comments that correspond to future feature work. I tried to do the minimal changes to get this working.

@eladyn
Copy link
Member

eladyn commented Oct 30, 2024

Thank you for tackling this major task! Looks good so far.

I actually started working on this as well, and definitely should've opened a draft PR for this to avoid duplicating the work. :/ If you want to have a look, my changes are here. Like you, I'm also still missing the necessary changes to dbus_mpris.rs. I wanted to get rid of rspotify there, so started replacing all the calls to the web API with respective librespot calls, but there's still some stuff missing.

So I'm not sure what should be done now. If you have the time to get MPRIS support working, we could just merge your version and I would rework the MPRIS logic at some later point. Otherwise, I could of course also try to finish my version and look through your stuff, if there are parts that I missed.

@Exceen
Copy link

Exceen commented Oct 30, 2024

thanks for all the work you put into this! I tried to compile and run it without credentials today and I gotta say that upon connecting to it, it just says
failed to create spirc: Service unavailable { Session is not connected }

Using credentials results in
failed to connect to spotify: Permission denied { Login failed with reason: Bad credentials }

... and I'm 100% sure that the credentials are correct btw

any information about this?

@Lasz
Copy link

Lasz commented Oct 31, 2024

Compilation fails at:

src/dbus_mpris.rs:18:5 could not find `keymaster` in `librespot_core`
src/dbus_mpris.rs:21:5 no `SpotifyAudioType` in `spotify_id`
src/dbus_mpris.rs:595:26 variant not found in `PlayerEvent`

Haven't done anything in Rust before, but might give it a try later.

On the other hand, librespot v0.6.0 just released a few hours ago.

@davidgauch
Copy link
Author

Thank you for tackling this major task! Looks good so far.

I actually started working on this as well, and definitely should've opened a draft PR for this to avoid duplicating the work. :/ If you want to have a look, my changes are here. Like you, I'm also still missing the necessary changes to dbus_mpris.rs. I wanted to get rid of rspotify there, so started replacing all the calls to the web API with respective librespot calls, but there's still some stuff missing.

So I'm not sure what should be done now. If you have the time to get MPRIS support working, we could just merge your version and I would rework the MPRIS logic at some later point. Otherwise, I could of course also try to finish my version and look through your stuff, if there are parts that I missed.

No worries, I did most this work before librespot released (I was trying to debug an auth issue). I wouldn't mind you closing this.

If you'd like though I can take a stab at mpris later today in about 10 hours.

I would also be willing to bump librespot again to 0.6.0 which was released 18 hours ago.

@davidgauch
Copy link
Author

I messed with the codebase for a while this evening. I have fixed the compiler bugs but I can't seem to get it to run properly. spotifyd starts, shows up in the web client as a device, claims an authentication success, but then crashes.

I tried incorporating the changes you made to main_loop.rs but that didn't seem to resolve the issue, just change how the crash error I get.

I think you're probably better-equipped to actually debug this issue @eladyn

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.

4 participants