21joakim / jockie-utils Goto Github PK

Introduction

When extending the CommandImpl class to make a custom command class you may run into an unexpected problem, for this problem to occur you need to

1. Override a method from CommandImpl such as setDescription
2. Use a variable defined in the extending class in the method
3. Use the command annotation to define a command @Command

What will happen when all these conditions are met is that the variable which was defined in the extending class is going to be null (if it had a default value) at the time setDescription is called or if the variable is set inside of setDescription it will be reset to null or any default value it may have had once the command has been initialized.

Example

Consider the following example

public class ExtendedCommand extends CommandImpl {
	
	private Map<Locale, String> translations = new HashMap<>();
	
	public ExtendedCommand(String name) {
		super(name, true);
	}
	
	public ExtendedCommand(String name, Method method, Object invoker) {
		super(name, method, invoker);
	}
	
	public ExtendedCommand setDescription(Locale locale, String description) {
		this.translations.put(locale, description);
		
		return this;
	}
	
	public ExtendedCommand setDescription(String description) {
		return this.setDescription(Locale.ENGLISH, description);
	}
}

In this example, at the time setDescription is called, translations would be null and therefore setDescription would throw a NullPointerException.

Why does this happen?

The problem may initially look like it doesn't make a lot of sense but this happens because of how Java initializes classes.

This would be the execution process of the example

1. The command would be instantiated, through something like new ExtendedCommand(name, method, invoker)
2. CommandImpl's constructor would be run, because of super
3. setDescription is called in CommandImpl's constructor because it is setting the value defined in the @Command annotation

At this point, ExtendedCommand has not been initialized and translations is null, which causes setDescription to throw a NullPointerException.

The only real solution I can find to this problem would be to move the command creation code of the constructor which I don't really like doing but I can't think of any alternative ways of fixing this issue.

The IArgumentParser should be extended to allow for custom parsers, such as a JSON parser. This would allow for better argument handling as you would for instance not need to have a JSON argument as "endless" just because it could contain spaces {"hello": "world"}.

Currently, the only problem I can see with implementing such a feature would be how to handle argument annotations. With the current implementation, you can specify the endless, quoted and empty properties per argument which will be used when parsing and a custom parser would most likely not need these properties, one possibility would be to have a separate annotation for String arguments as they are mostly what these properties are used for.

Due to how optional arguments work with "dummy commands" having two or more optional arguments, of the same type, could result in them being in an inconsistent order to how they were defined.

Example:

@Command
public void example(CommandEvent event, @Argument(nullDefault=true) String firstOptional, @Argument(nullDefault=true) String secondOptional) {
	if(firstOptional == null && secondOptional != null) {
		throw new IllegalStateException();
	}
}

Executing this example with any single argument will now result in an IllegalStateException

• The definition of an endless argument is an argument which allows for the user to specify as many of that type of argument as they want or within a specified limit, these arguments are implemented by the EndlessArgumentImpl class and @Endless annotation. An example of an endless argument could be !add all 1 2 3 4 5.

The arguments within the endless argument currently do not support the ArgumentTrimType which was introduced in fde1ddb.

This includes the new custom quotes feature as well

The Timeoutable class wasn't made in a very good way, it was just a class rushed together to get everything working.

Not quite sure how I want it to be right now, it would seem like having it as an interface would be a good idea but there would most likely be a bunch of duplicate code from the implementation of it. I think we should only have the parts that change in the Timeoutable interface and then have a TimeoutManager which manages everything else.

A sub-command system (Like a tree implementation or something) where a command for instance named "kick" can have a sub-command called "all" which would be triggered when the message is "kick all", not sure how this would be implemented with the current setup. I think it would definitively be a challenge fitting it in with all the already existing components of the command and argument checking and verification process.

A problem with a sub-command system using this implementation is that each sub-command would need to be ordered just the way they are in CommandStore for everything to work and for there not to be any edge cases.

Problem
Commands in most forms use Class#getMethods and due to the fact that it does not return the methods in the order they were specified nor any consistent order at all it can cause problems.

Let's consider the following problem,

public void onCommand(int argument) {}

public void onCommand(String argument) {}

We have two different implementations of the same command, an Integer and a String variant, now let's say that you want the first implementation with an Integer argument to always be checked first because String will always just accept the content given to it. If the second argument was to be registered before the first one, in this case, it would result in the Integer implementation never being called.

Of course, this does not apply to all situations, for instance, if both of them have different amounts of arguments it will be sorted correctly.

Possible solutions

• Have a new annotation @Order where you can specify what order they should be registered in, for instance @Order(0)
• Have a new priority property for the arguments where you can specify what priority an argument type should have when you register them
• Find a new way to get them which returns them in the order they were specified in (?)

PagedResult should be remade to be more modular.

For instance we can seperate each part of the PagedResult in to different interfaces such as Timeoutable (see #2), Cancelable, Selectable and the general IPagedResult.

I think using a PagedResultBuilder might be good, opinion?

When the bot fails to execute the command due to an argument not being correct it currently just responds with the help for that command, this could cause some confusion as to what is wrong. The bot should instead respond with information about what argument failed and the reason to it failing. It might be hard to get this working correctly with commands which has optional arguments since it internally creates a DummyCommand that works like a separate command. This might cause there to be multiple reasons for a command failing. A question would be whether we show all of the reasons or just one of them.

When the library parses a url with a param for arguments it throws a internal error
Version: jda-v4-SNAPSHOT

Screenshot of myself testing this

https://cdn.discordapp.com/attachments/588014518530080794/611116397015334923/unknown.png

Example code

class ExampleCommand : CommandImpl("example") { fun onCommand(event: CommandEvent, @Argument query: String) { event.channel.sendMessage(query).queue() } }

Error thrown

[20:37:50:305] [JDA [0 / 1] Main] [ ERROR] [ net.dv8tion.jda.api.JDA] :: One of the EventListeners had an uncaught exception java.lang.StringIndexOutOfBoundsException: String index out of range: 0 at java.base/java.lang.StringLatin1.charAt(StringLatin1.java:47) at java.base/java.lang.String.charAt(String.java:693) at com.jockie.bot.core.utility.StringUtility.parseWrapped(StringUtility.java:50) at com.jockie.bot.core.utility.StringUtility.parseWrapped(StringUtility.java:46) at com.jockie.bot.core.utility.StringUtility.asMap(StringUtility.java:84) at com.jockie.bot.core.command.parser.impl.CommandParserImpl.parse(CommandParserImpl.java:304) at com.jockie.bot.core.command.impl.CommandListener.parse(CommandListener.java:1406) at com.jockie.bot.core.command.impl.CommandListener.onEvent(CommandListener.java:1261) at net.dv8tion.jda.api.hooks.InterfacedEventManager.handle(InterfacedEventManager.java:96) at net.dv8tion.jda.internal.hooks.EventManagerProxy.handle(EventManagerProxy.java:64) at net.dv8tion.jda.internal.JDAImpl.handleEvent(JDAImpl.java:148) at net.dv8tion.jda.internal.handle.MessageCreateHandler.handleInternally(MessageCreateHandler.java:122) at net.dv8tion.jda.internal.handle.SocketHandler.handle(SocketHandler.java:37) at net.dv8tion.jda.internal.requests.WebSocketClient.onDispatch(WebSocketClient.java:838) at net.dv8tion.jda.internal.requests.WebSocketClient.onEvent(WebSocketClient.java:737) at net.dv8tion.jda.internal.requests.WebSocketClient.handleEvent(WebSocketClient.java:716) at net.dv8tion.jda.internal.requests.WebSocketClient.onBinaryMessage(WebSocketClient.java:876) at com.neovisionaries.ws.client.ListenerManager.callOnBinaryMessage(ListenerManager.java:368) at com.neovisionaries.ws.client.ReadingThread.callOnBinaryMessage(ReadingThread.java:270) at com.neovisionaries.ws.client.ReadingThread.handleBinaryFrame(ReadingThread.java:990) at com.neovisionaries.ws.client.ReadingThread.handleFrame(ReadingThread.java:749) at com.neovisionaries.ws.client.ReadingThread.main(ReadingThread.java:108) at com.neovisionaries.ws.client.ReadingThread.runMain(ReadingThread.java:64) at com.neovisionaries.ws.client.WebSocketThread.run(WebSocketThread.java:45)

Steps to reproduce

run a command with any argument as a link with a param string, like a youtube video

You should be able to add multiple onCommand methods to a command class each with different arguments.

For instance:

public class CommandCopy extends CommandImpl {

	public CommandCopy() {
		super("copy");
	}
	
	public void onCommand(MessageReceivedEvent event, TextChannel channel) {
		channel.createCopy().queue(copy -> {
			event.getChannel().sendMessage("Created copy of channel " + channel.getName()).queue();
		});
	}

	public void onCommand(MessageReceivedEvent event, VoiceChannel channel) {
		channel.createCopy().queue(copy -> {
			event.getChannel().sendMessage("Created copy of channel " + channel.getName()).queue();
		});
	}

	public void onCommand(MessageReceivedEvent event, Role role) {
		role.createCopy().queue(copy -> {
			event.getChannel().sendMessage("Created copy of role " + role.getName()).queue();
		});
	}
}

Problems with this is that the command class does currently not support any way of doing this or anything similar, possible solution to it is to use DummyCommands as done for optional arguments.

For instance if you had a command named create role with the arguments name, hoisted, mentionable, color and permissions you should be able to write the command as
<prefix>create role name="a role" color=#00FFFF permissions=8 hoisted=true
where an argument's value is defined by its name/key and not index.

A question would be whether we allow them to specify some with the key and the other with the index or if all of them have to be with a key, for instance
<prefix>create role color=#00FFFF "a role" permissions=8 hoisted=true
because name is on the first index and it would have ignored the first entry color as an index entry it would know that it is the name. If we were to allow this there would be a problem where we do not know whether the argument has a key or if the user just wanted an argument that looks like one with a key.

I am also not sure how we would implement it with the current setup, I don't think we will be able to find anywhere we can fit it in without having to remake the way we handle everything.

Add command categories which can be used to have a collection of commands, the category should be able to have a name and should be displayed in the help (Or you can have the ability to get the commands for just that category). Additionally you should be able to override certain command properties if the command is in a category.

Not quite sure how to implement this with the current CommandStore class, a possibility would be repurpose it for a Category class. Another possibility would be for the CommandStore to allow for adding categories and if you add a command without a category it internally stores it as an UNKNOWN category or something similar.