search

xou816 / spot

Native Spotify client for the GNOME desktop
MIT License
2.24k stars 118 forks source link

Fix playlist loading #690

Closed QPixel closed 5 months ago

QPixel commented 6 months ago

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: https://github.com/xou816/spot/commit/56cf43625a1dc281daef3f74924d21d4a4a324bd

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

QPixel commented 6 months ago

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

Diegovsky commented 6 months ago

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 commented 6 months ago

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 commented 5 months ago

Can confirm this works, thank you :+1:

jacksongoode commented 5 months ago

Hopefully this can get merged soon!

Diegovsky commented 5 months ago

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

Diegovsky commented 5 months ago

Merged this in my maintained fork of Spot here