-
Notifications
You must be signed in to change notification settings - Fork 31
Command Lists #73
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
base: master
Are you sure you want to change the base?
Command Lists #73
Conversation
|
Includes support for new playlist commands in the command list. These methods accept either a playlist name or a playlist as the identifier. |
| end | ||
| when MPD::Song | ||
| quotable_param param.file | ||
| when MPD::Playlist |
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.
Playlists can be passed as params in command lists methods where a playlist name is required.
|
@archseer Any thoughts on this? |
|
@archseer ping? |
|
Sorry, I've been traveling and it's a big feature to look over; I'll On Saturday, 27 February 2016, Gavin Kistner notifications@github.com
|
|
|
||
| # List all of the playlists in the database | ||
| def playlists | ||
| send_command(:listplaylists) |
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.
Hmm, I'm not too thrilled about having another implementation of the playlists just for the command list. Do you think it would be possible to reuse the original MPD::Playlist objects? It won't be possible to fetch it and then reuse it in subsequent commands in the list, but that's not possible with these plain method calls either
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.
All the playlist methods currently use @mpd.send_command. MPD#send_command includes handle_server_response and parse_response which we can't have called immediately with a command list. I can get around this for all the other MPD methods because they just call send_command (without an explicit receiver) and I provide my own send_command implementation for CommandList::Commands.
The only way that I can think to re-use the playlist objects would be to use an instance variable set as a flag when the command list was started and mutate the way send_command works everywhere under such circumstances. (I don't think I need to worry about tracking this state per thread, since multiple threads trying to use the same MPD instance would cause problems with the socket communication.)
I'd also have to modify MPD::Playlist#songs to handle the case when no result is returned from send_command, but I've already done something similar in a few other places as part of this patch.
Would you prefer that approach? Having MPD#send_command modify its behavior based on an instance variable tracking whether we're in a command list or not? Does this feel better to you?
|
Short summary of all the above: I will create an alternate pull request for consideration that modifies |
Command List support. In short: