feat: /play subcommands (next, all, reverse, shuffle)

[StaticRocket]

Setup basic play flags leveraging current PlayFlag enum system. Hopefully a good starting point for #51

This adds the flags for:

• PLAYALL which overrides the URL logic to force the playlist to unravel for any URL that ytdl can interpret as a playlist
• PLAYTOP which replaces the current /playtop command

Please let me know if anything looks odd. This is my first real attempt at rust.

[aquelemiguel]

@StaticRocket Thank you for your submission. ☺️

Functionality-wise, it seems great. I just requested a few changes regarding subcommands, but they're open for discussion if anyone wants to chime in with an even better idea.

However, please be aware that #51 is already assigned to @rafaeldamasceno.

Before proceeding with this, let's understand whether these changes are compatible with the work Rafael might've already done or whether it makes sense to transfer the issue. Let's respect the assignments. 🙂

[StaticRocket]

Well this all works now, but there's an issue with the way the playlist spooling system works right now. Queuing an instant mix will result in an infinite queue, adding results until the bot is restarted. This continues even if the bot is forced to leave, or at least the yt-dlp thread continues after the bot is forced to leave. The queue seems to stop growing after the /leave command is issued.

This isn't really a bug, but it is an interesting edge case. How should we identify and handle instant mix playlists? They are technically unending.

[joao-conde]

@StaticRocket I am unfamiliar with what an instant mix is. If you can fill me in I can propose some solution.

I know that for live streams we check the track metadata for the duration of the track.

In any case, I will review once there are no known bugs/issues with the PR.

[aquelemiguel]

@StaticRocket @joao-conde I'm also unfamiliar with what instant mixes are, but I'm assuming it's like those infinitely generating playlists Spotify has? Could you please provide an example link?

[StaticRocket]

Sure, here's a link to a YouTube instant mix: https://youtu.be/1V_xRb0x9aw?list=RD1V_xRb0x9aw

And what they look like:

The only reason we haven't had to deal with this issue before now is because these don't quite work like a playlist. They cannot be accessed by the usual playlist formatted URL.

https://www.youtube.com/playlist?list=RD1V_xRb0x9aw

But you'll find that when you reach the last song in the playlist the size will increase as it adds more videos.

This is probably something we should use those new ytdl parameters for, but I'm not sure what we should do. Having a hard cutoff for playlist lengths seems bad. Having something like a rate-limit that only applies to instant mixes seems alright, but we still have to deal with the runaway process, assuming that wasn't just my machine being weird.

I assume the ideal thing would be to wait until the current video is half way done before processing the next video in the mix (making this a new enqueuing method just for mixes and infinite length playlists), but I haven't looked to see how feasible that is yet.

[joao-conde]

So a mix is kind of youtube suggestions, in a playlist, but when you reach the end it just gives you new ones? Ok, if that is so, I would say we either:

a) don't allow queueing mixes
b) queue a frozen snapshot of the mix, meaning we take the first tracks and queue them, ignoring newly added ones

I will take a look at the source later to suggest such implementation but I believe it would be possible.

[afonsojramos]

If there is a way to programmatically distinguish these mixes from normal playlists, I would that your (@joao-conde) option b) is the most reasonable one. Or if it is not possible to properly create a frozen snapshot, we should at least put a limit to these playlists in specific.

[aquelemiguel]

@StaticRocket I just checked and yt-dlp seems to return 256 songs with an instant mix. I wouldn't worry much about this, TBH.

Alternatively, the simplest fix would be limiting the number of songs, either by trimming yt-dlp's output on the play command or by passing a custom yt-dlp argument (possibly --playlist-end). But again, I personally wouldn't bother, I don't know what the rest think.

[StaticRocket]

@aquelemiguel I'm comfortable with leaving it, but currently the /stop and /clear commands can't do anything to stop the queueing until all 256 entries have been added. Or the /leave command is issued, in which case the ytdl process keeps requesting video info from the entries in the playlist, but it isn't queued.

Not sure if people will like that.

This could be avoided if the leave/stop/clear commands also kill the ytdl subprocess, but I'm not sure how good of a solution that is.

[aquelemiguel]

This is working pretty well! ✨
Before merging, don't forget to run cargo fmt so our pipeline passes.

[aquelemiguel]

Currently the /stop and /clear commands can't do anything to stop the queueing until all 256 entries have been added.

@StaticRocket This is true (for any playlist really) and not ideal. Because AFAIK communication between commands can only be accomplished through context.data, I can only really see a solution that would involve reading some sort of "stop flag". Set it to true in /stop and have /play break the queue loop if it's set.

I don't think this is super relevant to this issue in particular but definitely something to think about in the future in its own issue.

[rafaeldamasceno]

@StaticRocket Thank you for your great work on these commands!

As for the feature and as a discussion topic for everyone, I have some thoughts I want to add, even though I have probably missed this train. When I first suggested the flags feature, I had in mind the possibility of mixing and matching flags (for example, shuffling a playlist and playing it next with -sn, playing all the videos in a video with playlist instantly with -aj, etc.).

Is there any plan for mixing and matching flags that doesn't result in a subcommand explosion?

[aquelemiguel]

Is there any plan for mixing and matching flags that doesn't result in a subcommand explosion?

I thought about that too, but I believe right now this isn't possible with slash commands. The optional flags approach was finicky even with only one flag and with subcommands you'd have to cover all possible flags combinations.

I think here we should try to cover the most popular cases and think of this less like flags and more like subcommands. It's better to have a few stable, intuitive subcommands instead of a dodgy fully flexible flag implementation is what I mean. It does suck to lose some functionality, but following Discord standards benefits us in the long run.

[afonsojramos]

I think here we should try to cover the most popular cases and think of this less like flags and more like subcommands. It's better to have a few stable, intuitive subcommands instead of a dodgy fully flexible flag implementation is what I mean. It does suck to lose some functionality, but following Discord standards benefits us in the long run.

Agree with @aquelemiguel here.

[rafaeldamasceno]

It's better to have a few stable, intuitive subcommands instead of a dodgy fully flexible flag implementation is what I mean.

While I undestand what you mean, I don't believe the latter option would be dodgy or unintuitive. If manually parsing a flags string parameter is undesirable (and I can agree with that), having the flags as optional booleans in the play command would still allowed users to know what they were used for and for them to activate only the ones they wanted to use.

The main disadvantage I see with the subcommands approach is that it doesn't lose some functionality, but instead most of the flexibility that the flags system would provide. Currenly, I can do all of the behavior of these commands by manually doing the work after queueing something, and since they only provide one functionality, it's not that much work (i.e. I'm not sure how much additional value these subcommands provide, since they only remove the need of calling a single command afterwards). The purpose of the flags would be to have any of the desired behaviors to be done automatically when enqueueing someting. I understand that more subcommands can be added in the future (some for shuffling and playing the playlist next, or any other combinations that are deemed common or useful), but IMHO it's only a matter of time before a combination that wasn't implemented is requested because it's used a lot by a specific group or person.

[aquelemiguel]

If manually parsing a flags string parameter is undesirable

@rafaeldamasceno Something like /play query flags -a -r where flags is a string option?
It mixes application commands and CLI interaction and you probably lose helper texts but I don't think it's atrocious. 🤷

having the flags as optional booleans in the play command

This would look terrible, no? For example, /play query all true random true. Options must always have a value.

Currently, I can do all of the behavior of these commands by manually doing the work after queueing something, and since they only provide one functionality, it's not that much work

Sort of, play with the random flag is similar to playing and then shuffling, but if you had other tracks in the queue, they'd also be mixed with the playlist. But I see what you mean, they're not fantastic shortcuts.

I understand that more subcommands can be added in the future (some for shuffling and playing the playlist next, or any other combinations that are deemed common or useful), but IMHO it's only a matter of time before a combination that wasn't implemented is requested because it's used a lot by a specific group or person.

I agree, but it's a little difficult for me to picture how useful these combinations are because I never really used flags on other bots.
Let's ensure here on #140 that all the flags work well separately via subcommands. Then I suggest opening another issue for discussing how we'd go about mixing and matching flags in an intuitive way. Does this sound fair?

[afonsojramos]

@aquelemiguel @rafaeldamasceno please move that discussion to the appropriate issue 😅 Let's restrict ourselves to what this PR is related to.

[rafaeldamasceno]

@afonsojramos For sure, it's just that this PR isn't really aligned with what I was expecting in #51 and the discussion then started here. I will continue the discussion there.

In any case, @aquelemiguel the next steps you proposed sounds great. What I then recommend is that #51 isn't closed by this PR (although very advanced by it) and that this development takes into account ways to make it easy to invoke internal functions with different flags (e.g enqueue_song was changed to receive a single PlayMode and matches only on one PlayMode when there could be multiple, PlayMode could indeed just be called PlayFlag again, etc.). Otherwise the current rework of the playing/enqueing will need to be undone almost straight away when implementing multiple flags.

[afonsojramos]

Approving since the PR is already in a mergeable state. We can update the Shuffle and Reverse changes in another PR or in this one, up to you @StaticRocket, in my opinion.

[aquelemiguel]

Approving since the PR is already in a mergeable state. We can update the Shuffle and Reverse changes in another PR or in this one, up to you @StaticRocket, in my opinion.

Sure, that and the jump subcommand is missing but can also be tackled in a different one.

[StaticRocket]

the jump subcommand is missing

Absolutely going to leave that to someone else. It's not a bad feature, but I don't see myself using it that much. Implementation wise it should be easy; just make a call to /stop before queuing anything.

[joao-conde]

LGTM, make the small changes requested and I approve it.

[aquelemiguel]

Reviewed with @joao-conde, LGTM as well. ☺️

[afonsojramos]

Awesome PR, and awesome contributions @StaticRocket!! 🚀

[joao-conde]

LGTM