elomatreb/mpd_client

Patch: Added shuffle command and expanded Queue command

Closed this issue · 5 comments

kholthaus commented

Hi,

I added a few things in the attached patch:

  • Expanded the queue command to allow a song ID, a song position and a range as optional parameters (will generate a playlistid or playlistinfo command with the appropriate arguments)
  • Added a shuffle command with an optional song range

I tried to create a pull request, but am stuck not being able to publish my branch (happy to create a branch and pull request, if you prefer - but will need a few pointers on how to do that).

Thanks for your work on this crate - it's very helpful to me. :)
shuffle_playlist.zip

elomatreb commented

Hi, I will take a look at this but it will take some time since I currently have to prepare for some work exams.

elomatreb commented

@kholthaus To submit your patch as a PR, follow these steps:

  1. Fork my repository (when viewing the code, there is "Fork" button at the top right)
  2. Clone that fork locally
  3. Create a new branch in that local repository (e.g. git checkout -b add_shuffle_and_expand_queue).
  4. Apply your patch and commit the change
  5. Push the new branch to your fork on Github (e.g. git push --set-upstream origin add_shuffle_and_expand_queue).
  6. After the changes are pushed, Github will show you a banner allowing you to create pull request when you view your fork.

Some notes just from looking at the patch: Sadly changing the Queue command to have a field would be a breaking API change (i.e. require a 2.0). Adding a separate type for this would be a better choice.

kholthaus commented

@elomatreb Thanks for the feedback. I'll follow the instructions above and also change the patch to introduce a new type for the expanded queue command.
Do you have a preference for the naming of the new type?

elomatreb commented

I think I would add some associated functions to the Queue type, called something like id and range, that don't return a new type (e.g. called QueueByRange). That way, you still get the discoverability of having it all grouped on the Queue command.

kholthaus commented

Pull request created using your ideas regarding new type in previous comment.