Make commands identifyable

[vicky5124]

https://5124.16-b.it/ss/23:53:39_15-10-2021.png
Makes a command with different names (like context and normal command names) share the same ID, this can be used to determinate a specific command being run.

[kangalio]

Do I see correctly that subcommands will have the exact same name as their parents by default, and their ID will be zero (Default::default())? That's not great I think, because it means neither command name nor ID are unique when subcommands are involved

[vicky5124]

Do I see correctly that subcommands will have the exact same name as their parents by default, and their ID will be zero (Default::default())? That's not great I think, because it means neither command name nor ID are unique when subcommands are involved

Command IDs are set when registering the command, if it's done on the macro, if a command is registered more than once (for example, in a subcommand and as a normal command), the IDs would be the same; doing it on register makes the command have a different ID on the different places it may be.

[vicky5124]

fdcd6d1 fixes subcommand IDs, and replaces the ids_count field with command_ids, which includes subcommand IDs, unlike the return value of options.command()

You can see how i use it here:
https://gitlab.com/vicky5124/slash-rash/-/commit/cc864a7cff0adf2e30d98bbdaabf7e7e5e405f51

[kangalio]

Sorry for being quiet on this.

I'm very unsure if this is a good approach, though for the most part I can't articulate why I feel like this. Maybe it's irrational.

---

One concrete concern I do have is in relation to #13. To show slash-only commands in the help menu, the help menu also needs some way of identifying commands across types (e.g. slash or prefix). So I tried to implement #13 with this PR as a base.

I noticed that many fields that currently reside in PrefixCommandOptions would need to be shared across command types (i.e. SlashCommand, ContextMenuCommand, PrefixCommand):

• inline_help
• multiline_help
• hide_in_help

Should those things go in CommandId as well? They would be completely duplicated three times... idk

---

Maybe the commands' command_id field should store an Arc<CommandId>, and to compare two commands for ID equality you do Arc::ptr_eq.

But maybe the way poise currently splits up commands into completely separate structs (PrefixCommand, SlashCommand, ContextMenuCommand) is wrong altogether, and it would be better to store a Vec<Command> at top level. The Command struct would contain the command-type-agnostic data directly and house the different types as optional fields.

So many ways to do this and they all are really not great :( I'll be away for vacation for the next two weeks so this change will probably need to stall some more, sorry

[vicky5124]

Here's my idea: combine everything, including CommandBuilder into a single framework structure, that will look something like this:

// Sorry in advance if i make you have to scroll sideways :P

// All structures and fields are public, took too much to write so many `pub`s.
// Some fields should be Options if that makes implementing them easier, that's on you; i just kept the ones that were already options as options, and made some of the new fields options if they absolutely required it.

struct CommandGroup<U, E> {
    name: String,
    description: String,
    commands: Vec<Command<U, E>>, // maybe a HashMap with an Arc of the CommandId can be used here for fast indexing?
    sub_groups: Vec<CommandGroup<U, E>>,
}

struct Command<U, E> {
    prefix: Option<PrefixCommand<U, E>>,
    application: Option<ApplicationCommand<U, E>>,
    id: CommandId,
    invoke_parameters: InvokeParameters,
}

/// Command information
struct CommandId {
    /// A unique `usize` ID for the command, that will be shared if the same command has multiple invoke levels.
    /// This is defined when inserted to the framework.
    id: Option<UniqueId>,
    /// The name of the command.
    name: String,
    /// A customizable name of the command.
    /// It will default as being equal to `name`, but can be overwritten with `.custom_name()` when registering the command to the framework.
    custom_name: String,
    /// Short description for the command.
    /// This is also used as the slash command description.
    short_help: String,
    /// Full description for the command.
    long_help: String,
    /// A function to make a dynamic help description.
    dynamic_help: Option<fn(...) -> BoxFuture<'_, String>>,
    /// Should the command be hidden from help? (hit, no!)
    hide_in_help: bool,
}

// Shared command fields
// these should have the same functionality as before, but take Context instead of being specific.
struct InvokeParameters {
    on_error: Option<fn(...) -> BoxFuture<...>>,
    check: Option<fn(...) -> BoxFuture<bool>>,
    required_permissions: Permissions,
    owners_only: bool,
}

struct PrefixCommand<U, E> {
    aliases: Vec<String>,
    track_edits: bool,
    action: for<'a> fn(_: PrefixContext<'a, U, E>, args: &'a str) -> BoxFuture<'a, Result<(), E>>,
}

struct ApplicationCommand<U, E> {
    context_menu_name: Option<String> // if not overwritten, it will be None, and use the same name as the function or the overwrite name.
    ephemeral: bool,
    parameters: Vec<fn(_: &mut CreateApplicationCommandOption) -> &mut CreateApplicationCommandOption>,
    // ContextMenuCommandAction enum is the same as currently.
    context_menu_action: Option<ContextMenuCommandAction<U, E>>,
    slash_action: Option<for<'a> fn(_: ApplicationContext<'a, U, E>, _: &'a [ApplicationCommandInteractionDataOption]) -> BoxFuture<'a, Result<(), E>>>,
}

// ---------------------

struct FrameworkOptions<U, E> {
    on_error: fn(_: E, _: ErrorContext<'_, U, E>) -> BoxFuture<'_, ()>,
    
    // these 2 could be combined, since command_check is always ran now, and have effectively the same structure.
    pre_command: fn(_: Context<'_, U, E>) -> BoxFuture<'_, ()>,
    command_check: Option<fn(_: Context<'_, U, E>) -> BoxFuture<'_, Result<bool, E>>>,
    
    missing_permissions_handler: fn(_: Context<'_, U, E>) -> BoxFuture<'_, ()>,
    allowed_mentions: Option<CreateAllowedMentions>,
    // listener -> events
    // i would love for this to work as it does with serenity rn, with a trait that's implemented and a structure is passed, much more convinient IMO than leaving the matching to the user. 
    event_handler: for<'a> fn(_: &'a Context, _: &'a Event<'a>, _: &'a Framework<U, E>, _: &'a U) -> BoxFuture<'a, Result<(), E>>,
    owners: HashSet<UserId>,
    /// Aditional options for prefix commands.
    // this removes ApplicationOptions, not needed.
    prefix_options: PrefixOptions<U, E>,
    // All the commands
    // Commands without a group will be at index 0.
    command_groups: Vec<CommandGroup<U, E>>,
}

// Most of these only work for prefix commands
struct PrefixOptions<U, E> {
    prefix: Option<String>,
    additional_prefixes: Vec<Prefix>,
    dynamic_prefix: Option<for<'a> fn(_: &'a Context, _: &'a Message, _: &'a U) -> BoxFuture<'a, Option<String>>,
    stripped_dynamic_prefix: Option<for<'a> fn(_: &'a Context, _: &'a Message, _: &'a U) -> BoxFuture<'a, Option<&'a str>>>,
    mention_as_prefix: bool,
    edit_tracker: Option<RwLock<EditTracker>>,
    ignore_edit_tracker_cache: bool,
    execute_self_messages: bool,
    case_insensitive_commands: bool,
}

This design is in my mind really doable, I can think of ways to implement everything, even with my limited macro knowledge; this also unifies way more all the command types, and since context menus and slash commands are effectively the same, combining them seems like a way better option than having them separate; it also fixes parity issues between the command types, like for example the permission check, only being available to application commands, and not prefix commands, or having different implementations or duplicate data of on_error, check, required_permissions and the owners_only flag.

This PR should be merged, because it fixes the issue with being unable to identify command invocations as the same when they have a different name (for example, context menus), or called as part of a group, or when the same command is registered in different places, or when there are 2 commands with the same name in different groups, but different functionality.
In a later date, the CommandId could be used to store additional command information, as shown by the new framework structure above.

Have fun in your holiday! Hope it clears your mind up to make implementing this when you come back easier!
I'll be patient, and likely learning macros and proc macros to help with more stuff in poise :D

[kangalio]

I had a go at implementing the design and ran into the problem that you can no longer just pass PrefixCommand or ApplicationCommand (for example in [Prefix, Application]ErrorContext, [Prefix, Application]Context or in the command dispatch code internals): most of the important info is stored outside those structs. One solution would be to pass both [Prefix, Application]Command and Command everywhere but that seems suboptimal.

I'm continuing to think about this, just so you know I haven't forgotten about the PR ;)

[kangalio]

I'll close this PR, since the PR as is will probably not be merged. Thank you for investing the time and effort into the code and discussion!

The discussion has shifted to be about a potential new library architecture, which is probably better suited to its own issue: #26