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

Fix playlist loading #690

Closed
wants to merge 6 commits into from
Closed

Conversation

QPixel
Copy link

@QPixel QPixel commented Jan 5, 2024

This PR seeks to fix Spot's ability to load playlists.

Sometime around December 16th, Spotify broke the query string fields argument (#689). It now seems to be impossible to filter specific fields from larger objects. E.G. the artists field and albums field. Current behavior sees Spotify not returning the objects requested and depending on the order of the artists and albums field one will get returned while the other is not.

I spent a few hours messing around with the query string arguments and found a solution by not filtering the albums field
which is seen here: 56cf436

--- id,name,images,owner,tracks(total,items(is_local,track(name,id,uri,duration_ms,artists(name,id),album(name,id,images,artists)))) 
+++ id,name,images,owner,tracks(total,items(is_local,track(name,id,uri,duration_ms,album,artists(name,id))))

While this did fix the loading bug, it causes a lot more data to be sent than needed... So instead, it calls get_playlist_tracks in the initial loading of a playlist and separately add the song batch from that API call to the playlists song state.

Other Fixes

Spot crashing if a user tries to play a playlist with no songs
Spot erroring if a user loads a playlist with no image

Note: I am more than happy to split these other fixes into multiple PRs if needed.

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...
@QPixel
Copy link
Author

QPixel commented Jan 8, 2024

Tagging: @xou816 @Diegovsky @knokelmaat for a review :)

@QPixel QPixel closed this Jan 8, 2024
@QPixel QPixel reopened this Jan 8, 2024
@Diegovsky
Copy link
Collaborator

sup! I can't review exactly at this moment but I'll gladly do so sometime this week :)

thank you very much for your fix! I noticed spot not being able to load playlists for sometime now

@Diegovsky
Copy link
Collaborator

Btw, I thought your comments were very thoughtful and helpful to whoever sees the code later. Thank you for taking the time to contribute :)

@coveres
Copy link

coveres commented Jan 11, 2024

Can confirm this works, thank you 👍

@jacksongoode
Copy link

Hopefully this can get merged soon!

@Diegovsky
Copy link
Collaborator

@QPixel I'm looking forward to you addressing my questions. It's looking 99% as-is, just waiting for this :)

@Diegovsky
Copy link
Collaborator

Merged this in my maintained fork of Spot here

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