Improve remove index's tooltip and parameter type

[rafaeldamasceno]

Rationale

Currently, when selecting a track to be removed from the queue, it isn't immediately obvious how it is going to work.

Description

The UX of selecting the track to be removed could be improved with a tooltip that better clarifies how the queue indexing works. Additionally, the index parameter could be converted to an INTEGER option.

Reference

Application Command Option Type

[rafaeldamasceno]

@aquelemiguel @joao-conde Do you think this is feasible within the current codebase? I haven't really looked at the code besides changing some aliases in the past, so I have no idea if it would be easy for me to pick this issue up 😁

[aquelemiguel]

The UX of selecting the track to be removed could be improved with a tooltip that better clarifies how the queue indexing works.

We're using Position of the track (0 is currently playing) as a tooltip for the index argument. I'd agree this is counter-intuitive (would make you believe skipping 0 is valid). Open to suggestions for clearer phrasing here. 👍

Additionally, the index parameter could be converted to an INTEGER option.

Already is. 🙂

parrot/src/events/serenity_handler.rs

Lines 77 to 88 in fd2a0c0

	.create_application_command(|command| {
	command
	.name("remove")
	.description("Removes a track from the queue")
	.create_option(|option| {
	option
	.name("index")
	.description("Position of the track (0 is currently playing)")
	.kind(ApplicationCommandOptionType::Integer)
	.required(true)
	})
	})

[afonsojramos]

As I said on #90, I think that we should change the tool tip to indicate that the next track is 1, instead of saying that the current one is 0.

[rafaeldamasceno]

As I said on #90, I think that we should change the tool tip to indicate that the next track is 1, instead of saying that the current one is 0.

This is what I'm aiming for, in addition to making it explicit that it removes the tracks from the queue.

Already is. 🙂

Oops @aquelemiguel, you're absolutely correct! I thought that it wasn't because Discord let me input free text, but it validates for the integer only when you try to send it.

What I can actually improve is the validation for out-of-bound integers, because inputing a negative integer currently shows this:

This suggests that somehow the input wasn't received, but that isn't the case.

[afonsojramos]

Taking this issue, and already found a few bugs on the remove_index caused by all the unwraps in line 25:

let remove_index = args
        .first()
        .unwrap()
        .value
        .as_ref()
        .unwrap()
        .as_u64()
        .unwrap() as usize;

[joao-conde]

@afonsojramos It's specifically the line 32 one, where we had cast it to u64. So your input was not convertible to u64? Or you are on a 32-bit machine which I doubt

[aquelemiguel]

@afonsojramos Interesting, what command did you try? Probably negative?

[afonsojramos]

Yep, good guess, it's because I tried with a negative number, already making changes to handle this ;)

[rafaeldamasceno]

As for the tooltip for the index parameter, I suggest Position of the track in the queue (1 is the next track to be played). This clarifies that remove only interacts with the queue, and not with the currently playing track. Additionally, it clarifies that the queue is 1-indexed.

[afonsojramos]

I had made the changes before seeing your comment @rafaeldamasceno, and I used a more "barebones" description, but yours does seem more eloquent.