Skip to content
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

Skip current song on /remove 0 #90

Closed
afonsojramos opened this issue Jan 19, 2022 · 13 comments · Fixed by #95
Closed

Skip current song on /remove 0 #90

afonsojramos opened this issue Jan 19, 2022 · 13 comments · Fixed by #95
Assignees
Labels
💬 discussion Further information is requested

Comments

@afonsojramos
Copy link
Collaborator

Rationale

/remove 0 currently replies with an error message. Should this behaviour change?

Description

I propose that a /remove 0 skips the current song, as the tooltip even suggests that 0 is the currently playing song.

Reference

image

@afonsojramos afonsojramos added the 💬 discussion Further information is requested label Jan 19, 2022
@afonsojramos afonsojramos self-assigned this Jan 19, 2022
@aquelemiguel
Copy link
Owner

As you know, I'm sort of against this because I still support that users aiming to use /remove 0 probably got confused with indexes and weren't looking to skip the current song. For that, they would have used /skip.

However, if @joao-conde (or anyone else) pushes for this behaviour, I'll gladly approve it.

@joao-conde
Copy link
Collaborator

I like the idea of skipping

@aquelemiguel
Copy link
Owner

Then it's set.

@afonsojramos
Copy link
Collaborator Author

I think we need more opinions on this. Tagging @rafaeldamasceno @danrpinho, known users of Parrot 👀 🦜

@rafaeldamasceno
Copy link
Contributor

I can't think of a single instance where I would possibly want to use /remove 0 instead of /skip. If you think that /remove 0 is somewhat suggested from the current parameter description and error message, they should probably be reworded to make a clear distinction between the queue and the current playing track (i.e. remove messages that suggest that 0 is the current playing track and make it clear that queue starts at 1).

@joao-conde
Copy link
Collaborator

@rafaeldamasceno remove exists to remove a certain track (e.g. remove 2 removes the 3rd song in the queue)

The question here is that the user might input "0". What should we do? Currently we just print a message like "Invalid index". I think it makes sense to skip. Semantically, if I want to remove a track and I say remove the current one, I expect it to skip it.

But now that I think about it, the regular user thinks from 1..Infinity not 0-based indexing. So in reality I would say:

  • ignore remove 0 commands
  • remove 1 should remove the current playing track by skipping it
  • remove 2 should remove the next track

TLDR: switch from 0-based to 1-base indexing and skip first song when remove 1 is used

@afonsojramos
Copy link
Collaborator Author

Agree with @rafaeldamasceno that maybe the text should be clearer. As it was my first suggestion in #89, I'd say to simply ignore the command if the user sends a remove 0. Additionally, as @rafaeldamasceno suggests, in the remove tool tip, say that the next track is 1, instead of saying that the current one is 0.

Disagree with @joao-conde on the remove 1 skipping to the next track.

I'd say that, in the end, the best User Experience is for the remove to simply remove stuff from the queue, and if you want to skip, you use skip.

@aquelemiguel
Copy link
Owner

aquelemiguel commented Jan 20, 2022

TLDR: switch from 0-based to 1-base indexing and skip first song when remove 1 is used

For context, this is the output of our queue command:

image

I personally don't consider the currently playing track as being part of the queue (even if Songbird considers it so). For me (and I'd argue most people), the queue is only for tracks that are waiting to be played. And like a real life queue, once I start getting served, I leave the queue. If I'm riding in Space Mountain or eating at a restaurant, I'm most definitely not in the queue anymore.

So for me, 1-based index for the "up next" songs makes sense. 0 is out-of-bounds.

@rafaeldamasceno
Copy link
Contributor

rafaeldamasceno commented Jan 20, 2022

@joao-conde To me, your suggestion sounds a bit nuts 😅

I'm not going to comment on the internals of the code and on what a queue acually means there. However, from a user perspective, the queue does not include the currently playing track, as @afonsojramos and @aquelemiguel just mentioned. Additionally, remove should be used to remove queued tracks, not any track. For this reason, the index 1 should correspond to the first track in the queue (i.e. the track to be played next).

However, it should definitely not ignore the command when it's called with the index 0, but to reply it's an invalid track number (as it should for all negative integers). When the index is not out of bounds, but there is no track for that number, the error message should then say there's no track for that number. No error message isn't good UX :)

@aquelemiguel
Copy link
Owner

aquelemiguel commented Jan 20, 2022

I'd say to simply ignore the command if the user sends a remove 0

Careful as with slash commands, if you do not provide an interaction reply, the bot will fail with an ugly generic message. We should now always handle unexpected arguments or block invalid requests like such:

image

@joao-conde
Copy link
Collaborator

joao-conde commented Jan 20, 2022

@joao-conde To me, your suggestion sounds a bit nuts sweat_smile

I'm not going to comment on the internals of the code and on what a queue acually means there. However, from a user perspective, the queue does not include the currently playing track, as @afonsojramos and @aquelemiguel just mentioned. Additionally, remove should be used to remove queued tracks, not any track. For this reason, the index 1 should correspond to the first track in the queue (i.e. the track to be played next).

However, it should definitely not ignore the command when it's called with the index 0, but to reply it's an invalid track number (as it should for all negative integers). When the index is not out of bounds, but there is no track for that number, the error message should then say there's no track for that number. No error message isn't good UX :)

Was just my opinion :) Like you just gave yours. No error message is not necessarily bad UX. As a matter of fact, in terms of my user experience I would prefer what I described. So as you see, UX is relative, and commonly adopted practices are no more than opting by what MOST people would prefer :)

With that said, do whatever you guys prefer, all of the presented solutions are fine for me.

@afonsojramos
Copy link
Collaborator Author

As such, I will then close this ticket, since the current behaviour of allowing for a remove 0 with an error message still seems very much in line with a good UX.

@rafaeldamasceno
Copy link
Contributor

rafaeldamasceno commented Jan 20, 2022

Was just my opinion :) Like you just gave yours.

Yes, and I didn't mean any of it on an absolute way 😁

No error message is not necessarily bad UX.

I agree. For example, if I'm running terminal applications, this is what I'm expecting as well. However, in the context of slash commands, I do not believe this is the case.

So as you see, UX is relative, and commonly adopted practices are no more than opting by what MOST people would prefer :)

Absolutely. In fact, I would be totally fine with the behavior you described, but I was simply thinking of what the common user would be more comfortable with. 😉

I'll create a new issue with some small improvements for this command.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💬 discussion Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants