-
Notifications
You must be signed in to change notification settings - Fork 126
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
Use Oauth2 login with spotify #731
Conversation
This fix is extremely scuffed, as Spotify's API will not return filtered fields when multiple complex objects are in the same query. This seems to be a regression from previous behavior... fix: playlist not loading if the image field is null
update Spanish translations
…ifyId::from_base62` is setting this to unkown which causes playback to fail with ` Unable to load audio item: Error { kind: Unavailable, error: NonPlayable } / unable to load track <SpotifyId("spotify:unknown:1iI5J72TQxYdQkKnkRwWCn")>`
…e URL in the default browser
That flatpak builder failure looks like clippy complaining about pre-existing code. Is this a new thing? |
tested it on my pinephone too and I have tunes :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but it should work with librespot 0.6.0 too which is also what I run on my OS currently
but thanks for the fix it works now :))))
You must have commented 0.6.0 within days of it's release :') I'll give it a go and see how it works. I was actually looking at the login5 stuff on librespot too as it may fix the other authentication issue.... |
I also tested it locally and it seems it's working quite well. I would remove the old login button. @xou816 Could you review? |
Looks like a lot of great work, I'll try to review that tomorrow, thanks! |
Looks good overall! I would have a few things to nitpick but nothing major/no reason to delay this. Just hesitant about the client id change, is it needed? |
hi! :) no yeah you're right I had not thought of this initally, you had no choice in order to test! but yeah it can be reverted, the client is now configured with the expected redirect uris as for the rest dont worry about it too much, that can all be adressed later, i dont mind too much, your MR brings a lot of long needed fixes so thats the priority! |
looks like the refresh token is not being used when re-opening Spot at a later time, I'll look into that |
…page, fix token refresh
I pushed a few modifications hope you don't mind! |
…ifyId::from_base62` is setting this to unkown which causes playback to fail with ` Unable to load audio item: Error { kind: Unavailable, error: NonPlayable } / unable to load track <SpotifyId("spotify:unknown:1iI5J72TQxYdQkKnkRwWCn")>`
…e URL in the default browser
…page, fix token refresh
66
not at all 😄 I'll fix up the oauth2 stuff this morning and bump librespot to 0.6.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice that its bumped now to 0.6.0
is the problem with the "you close the app and get logged out" fixed? just curious |
I think xou816 fixed that with his push |
merged!! thanks for fixing a major issue! |
i might still need to fix a few things with reopening the app after while -- the token needs to be refreshed we need to store the refresh token and use that to get a fresh access token after a while |
Thanks @xou816 if you want me to look at anything, feel free to @ me on an issue. I can make some time for spot ❤️ |
fixes #729, fixes #728 and fixes #726
works for me locally, I preserved the original login method, added a new one for login with spotify and fixed an issue that stopped tracks playing. I'm not massively familiar with this code (I pulled it yesterday because I wanted it to work on my pinephone) I'm happy to do a bit more to get this mergeable if needed but please bear in mind I may need a bit of guidance on what files to look at to solve issues