Add support for Playlists, etc (#656) #758
Conversation
skiphansen
changed the title
PromyLOPh
left a comment
PromyLOPh
left a comment
There was a problem hiding this comment.
Sorry for the months of silence :( I finally had a look at the code and tried your branch. My problem as a maintainer is that your changes are pretty invasive and I cannot test alot of them myself due to the lack of a premium subscription (and not living inside the US). I was hoping that – on the pianobar UI side – we would not have to differentiate between stations, playlists, podcasts, … and that we could hide this complexity in libpiano. I’m going to add a few review comments, but I’m unsure how to proceed.
| case PIANO_TYPE_PLAYLIST: { | ||
| json_object *a = json_object_new_object (); | ||
| json_object_object_add(a,"pandoraId",json_object_new_string(reqData->station->id)); | ||
| json_object_object_add(a,"limit",json_object_new_int(100)); |
There was a problem hiding this comment.
We’d probably need a very large number here or some kind of pagingation aka PIANO_RET_CONTINUE_REQUEST.
| json_object *a = json_object_new_object (); | ||
| json_object_object_add(j,"request",a); | ||
| #if 0 | ||
| // test ability to continue after hitting limit |
There was a problem hiding this comment.
So, we also need a continue return value here?
| if(!ret) { | ||
| break; | ||
| } | ||
| if(app->ph.user.IsPremiumUser) { |
There was a problem hiding this comment.
I’d remove these kind of checks unless the API calls always return an error for non-premium users (apparently they don’t).
| * @param auto-select if only one station remains after filtering | ||
| * @return pointer to selected station or NULL | ||
| */ | ||
| void BarUiSelectFilter(BarApp_t *app) |
There was a problem hiding this comment.
I don’t like this UI, to be honest. It requires me to go to a different menu to filter the station selection by type. The idea of station selection was that you can just type name/id of the station and filter through that. It’d be more fitting if we could add some operator(?) to BarUiSelectStation.
| break; | ||
| } | ||
|
|
||
| case PIANO_TYPE_STATION: |
There was a problem hiding this comment.
We could easily implement this for ordinary stations.
|
As another thought: We already have a song “history” and “upcoming“ songs. If we add support for playlists, we might as well merge all of that into a single, big “playlist“ functionality, allowing to move freely between past and future songs (only if we can fetch a new song URL, obviously). |
Thanks for the feedback! I was surprised by how invasive the changes became, but once I got the hang of reverse engineering the protocol I got a bit carried away. I'm an old fart who typically plays "albums" from beginning to end while focused on listening, otherwise the "station" concept works for me for background music. Personally I'm not that interested in playlists, tracks or podcasts, but since they were there ... I'll look into the detailed code comments in the next few days. |
Yes, but I don't think you can fetch a new song URL with a non-paid account. I'm not sure, I haven't done much testing with a free account. |
I've addressed several of your comments and updated the PR. The rest will require some work which I'm fine with if our eventual goal is to merge this PR. As far as testing goes is it possible for you to create a throwaway account, start a premium subscription trial and then cancel before the end of the trial period? If it's just not worth your effort and/or not practical I'm fine with that but I probably won't make additional changes since the current code is filling my needs. |
…lists requests. Apparenly not needed and meaning is unknown.
2 participants
This Wiki page describes these changes from the user's viewpoint.
I use pianobar for playback but I use the Pandora App to manage my "collection" so I have not added any support for creating or editing playlists or collecting tracks.
I have however added support for deleting podcasts, albums, playlists and tracks to the delete command.
Most of the new changes are only of any value to those with a premium account, If it's not worth your effort to review/merge the changes I understand.