Command system fixes based on TEEESTS

- Fixed unregistering commands
- Fixed command registration
- Fixed command argument handling
- Fixed command handling async task not implemented

Chroma-Core/pom.xml

@@ -255,6 +255,12 @@
 			<version>2.29.0</version>
 			<scope>test</scope>
 		</dependency>
+		<!-- https://mvnrepository.com/artifact/org.jetbrains.kotlin/kotlin-test -->
+		<dependency>
+			<groupId>org.jetbrains.kotlin</groupId>
+			<artifactId>kotlin-test-junit</artifactId>
+			<version>1.9.0</version>
+		</dependency>
 	</dependencies>
 	<organization>
 		<name>TBMCPlugins</name>

Chroma-Core/src/main/java/buttondevteam/lib/chat/Command2.kt

@@ -1,6 +1,7 @@
 package buttondevteam.lib.chat
 
 import buttondevteam.core.MainPlugin
+import buttondevteam.lib.ChromaUtils
 import buttondevteam.lib.TBMCCoreAPI
 import buttondevteam.lib.chat.commands.*
 import buttondevteam.lib.chat.commands.CommandUtils.coreCommand
@@ -101,20 +102,25 @@ abstract class Command2<TC : ICommand2<TP>, TP : Command2Sender>(
         if (results.reader.canRead()) {
             return false // Unknown command
         }
-        //Needed because permission checking may load the (perhaps offline) sender's file which is disallowed on the main thread
+        val executeCommand: () -> Unit = {
-        Bukkit.getScheduler().runTaskAsynchronously(MainPlugin.instance) { _ ->
             try {
                 dispatcher.execute(results)
             } catch (e: CommandSyntaxException) {
                 sender.sendMessage(e.message)
             } catch (e: Exception) {
                 TBMCCoreAPI.SendException(
-                    "Command execution failed for sender " + sender.name + "(" + sender.javaClass.canonicalName + ") and message " + commandline,
+                    "Command execution failed for sender ${sender.name}(${sender.javaClass.canonicalName}) and message $commandline",
                     e,
                     MainPlugin.instance
                 )
             }
         }
+        if (ChromaUtils.isTest) {
+            executeCommand()
+        } else {
+            //Needed because permission checking may load the (perhaps offline) sender's file which is disallowed on the main thread
+            Bukkit.getScheduler().runTaskAsynchronously(MainPlugin.instance, executeCommand)
+        }
         return true //We found a method
     }
 
@@ -145,8 +151,10 @@ abstract class Command2<TC : ICommand2<TP>, TP : Command2Sender>(
             val ann = meth.getAnnotation(Subcommand::class.java) ?: continue
             val fullPath = command.commandPath + CommandUtils.getCommandPath(meth.name, ' ')
             assert(fullPath.isNotBlank()) { "No path found for command class ${command.javaClass.name} and method ${meth.name}" }
-            val (lastNode, mainNode, remainingPath) = registerNodeFromPath(fullPath)
+            val (lastNode, mainNodeMaybe, remainingPath) = registerNodeFromPath(fullPath)
-            lastNode.addChild(getExecutableNode(meth, command, ann, remainingPath, CommandArgumentHelpManager(command), fullPath))
+            val execNode = getExecutableNode(meth, command, ann, remainingPath, CommandArgumentHelpManager(command), fullPath)
+            lastNode.addChild(execNode)
+            val mainNode = mainNodeMaybe ?: execNode
             if (mainCommandNode == null) mainCommandNode = mainNode
             else if (mainNode.name != mainCommandNode.name) {
                 MainPlugin.instance.logger.warning("Multiple commands are defined in the same class! This is not supported. Class: " + command.javaClass.simpleName)
@@ -169,8 +177,10 @@ abstract class Command2<TC : ICommand2<TP>, TP : Command2Sender>(
      * @param fullPath The full command path as registered
      * @return The executable node
      */
-    private fun getExecutableNode(method: Method, command: TC, ann: Subcommand, remainingPath: String,
+    private fun getExecutableNode(
-                                  argHelpManager: CommandArgumentHelpManager<TC, TP>, fullPath: String): LiteralCommandNode<TP> {
+        method: Method, command: TC, ann: Subcommand, remainingPath: String,
+        argHelpManager: CommandArgumentHelpManager<TC, TP>, fullPath: String
+    ): CoreExecutableNode<TP, TC> {
         val (params, senderType) = getCommandParametersAndSender(method, argHelpManager) // Param order is important
         val paramMap = HashMap<String, CommandArgument>()
         for (param in params) {
@@ -190,7 +200,7 @@ abstract class Command2<TC : ICommand2<TP>, TP : Command2Sender>(
             val argType = getArgumentType(param)
             parent.then(CoreArgumentBuilder.argument<TP, _>(param.name, argType, param.optional).also { parent = it })
         }
-        return node.build()
+        return node.build().coreExecutable() ?: throw IllegalStateException("Command node should be executable but isn't: $fullPath")
     }
 
     /**
@@ -200,11 +210,11 @@ abstract class Command2<TC : ICommand2<TP>, TP : Command2Sender>(
      * @return The last no-op node that can be used to register the executable node,
      * the main command node and the last part of the command path (that isn't registered yet)
      */
-    private fun registerNodeFromPath(path: String): Triple<CommandNode<TP>, CoreCommandNode<TP, *>, String> {
+    private fun registerNodeFromPath(path: String): Triple<CommandNode<TP>, CoreCommandNode<TP, *>?, String> {
         val split = path.split(" ")
         var parent: CommandNode<TP> = dispatcher.root
         var mainCommand: CoreCommandNode<TP, *>? = null
-        split.forEachIndexed { i, part ->
+        split.dropLast(1).forEachIndexed { i, part ->
             val child = parent.getChild(part)
             if (child == null) parent.addChild(CoreCommandBuilder.literalNoOp<TP, TC>(part, getSubcommandList())
                 .executes(::executeHelpText).build().also { parent = it })
@@ -212,7 +222,7 @@ abstract class Command2<TC : ICommand2<TP>, TP : Command2Sender>(
             if (i == 0) mainCommand =
                 parent as CoreCommandNode<TP, *> // Has to be our own literal node, if not, well, error
         }
-        return Triple(parent, mainCommand!!, split.last())
+        return Triple(parent, mainCommand, split.last())
     }
 
     private fun getSubcommandList(): (Any) -> Array<String> {
@@ -235,10 +245,10 @@ abstract class Command2<TC : ICommand2<TP>, TP : Command2Sender>(
     ): Pair<List<CommandArgument>, Class<*>> {
         val parameters = method.parameters
         if (parameters.isEmpty()) throw RuntimeException("No sender parameter for method '$method'")
-        val usage = argHelpManager.getParameterHelpForMethod(method)
+        val usage = argHelpManager.getParameterHelpForMethod(method)?.ifEmpty { null }
         val paramNames = usage?.split(" ")
         return Pair(
-            parameters.zip(paramNames ?: (1 until parameters.size).map { i -> "param$i" })
+            parameters.drop(1).zip(paramNames ?: (1 until parameters.size).map { i -> "param$i" })
                 .map { (param, name) ->
                     val numAnn = param.getAnnotation(NumberArg::class.java)
                     CommandArgument(
@@ -398,18 +408,22 @@ abstract class Command2<TC : ICommand2<TP>, TP : Command2Sender>(
      * @param condition The condition for removing a given command
      */
     fun unregisterCommandIf(condition: Predicate<CoreCommandNode<TP, SubcommandData<TC, TP>>>, nested: Boolean) {
-        dispatcher.root.children.removeIf { node -> node.coreExecutable<TP, TC>()?.let { condition.test(it) } ?: false }
+        unregisterCommandIf(condition, dispatcher.root, nested)
-        if (nested) for (child in dispatcher.root.children) child.coreCommand<_, NoOpSubcommandData>()?.let { unregisterCommandIf(condition, it) }
     }
 
     private fun unregisterCommandIf(
         condition: Predicate<CoreCommandNode<TP, SubcommandData<TC, TP>>>,
-        root: CoreCommandNode<TP, NoOpSubcommandData>
+        root: CommandNode<TP>,
+        nested: Boolean
     ) {
-        // TODO: Remvoe no-op nodes without children
         // Can't use getCoreChildren() here because the collection needs to be modifiable
-        root.children.removeIf { node -> node.coreExecutable<TP, TC>()?.let { condition.test(it) } ?: false }
+        if (nested) for (child in root.children)
-        for (child in root.children) child.coreCommand<_, NoOpSubcommandData>()?.let { unregisterCommandIf(condition, it) }
+            child.coreCommand<_, NoOpSubcommandData>()?.let { unregisterCommandIf(condition, it, true) }
+        root.children.removeIf { node ->
+            node.coreExecutable<TP, TC>()
+                ?.let { condition.test(it) }
+                ?: node.children.isEmpty()
+        }
     }
 
     /**

Chroma-Core/src/main/java/buttondevteam/lib/chat/commands/CommandUtils.kt

@@ -29,22 +29,45 @@ object CommandUtils {
 
     /**
      * Casts the node to whatever you say if it's a command node. Use responsibly. Returns null if an argument node.
+     *
+     * Command nodes are nodes that are subcommands. They may do something or just print their help text.
+     * They are definitely not argument nodes.
      */
     @Suppress("UNCHECKED_CAST")
     fun <TP : Command2Sender, TSD : NoOpSubcommandData> CommandNode<TP>.coreCommand(): CoreCommandNode<TP, TSD>? {
-        return if (this is CoreCommandNode<*, *>) this as CoreCommandNode<TP, TSD>
+        return if (this.isCommand()) this as CoreCommandNode<TP, TSD>
         else null
     }
 
     /**
      * Returns the node as an executable core command node or returns null if it's a no-op node.
+     *
+     * Executable nodes are valid command nodes that do something other than printing help text.
      */
     fun <TP : Command2Sender, TC : ICommand2<*>> CommandNode<TP>.coreExecutable(): CoreExecutableNode<TP, TC>? {
-        val ret = this.coreCommand<TP, NoOpSubcommandData>()
+        return if (isExecutable()) coreCommand() else null
-        return if (ret?.data is SubcommandData<*, *>) ret.coreCommand() else null
     }
 
+    /**
+     * Returns the node as an argument node or returns null if it's not one.
+     *
+     * Argument nodes are children of executable command nodes.
+     */
     fun <TP : Command2Sender> CommandNode<TP>.coreArgument(): CoreArgumentCommandNode<TP, *>? {
         return if (this is CoreArgumentCommandNode<*, *>) this as CoreArgumentCommandNode<TP, *> else null
     }
+
+    /**
+     * Returns whether the current node is an executable or help text command node.
+     */
+    fun <TP : Command2Sender> CommandNode<TP>.isCommand(): Boolean {
+        return this is CoreCommandNode<*, *>
+    }
+
+    /**
+     * Returns whether the current node is an executable command node.
+     */
+    fun <TP : Command2Sender> CommandNode<TP>.isExecutable(): Boolean {
+        return coreCommand<TP, NoOpSubcommandData>()?.data is SubcommandData<*, *>
+    }
 }

Chroma-Core/src/test/kotlin/buttondevteam/lib/chat/test/Command2MCTest.kt

@@ -8,6 +8,7 @@ import buttondevteam.lib.chat.Command2
 import buttondevteam.lib.chat.Command2MCSender
 import buttondevteam.lib.chat.CommandClass
 import buttondevteam.lib.chat.ICommand2MC
+import buttondevteam.lib.chat.commands.CommandUtils.coreExecutable
 import buttondevteam.lib.player.ChromaGamerBase
 import buttondevteam.lib.player.TBMCPlayer
 import org.junit.jupiter.api.MethodOrderer
@@ -15,6 +16,7 @@ import org.junit.jupiter.api.Order
 import org.junit.jupiter.api.Test
 import org.junit.jupiter.api.TestMethodOrder
 import java.util.*
+import kotlin.test.assertEquals
 
 @TestMethodOrder(MethodOrderer.OrderAnnotation::class)
 class Command2MCTest {
@@ -27,17 +29,22 @@ class Command2MCTest {
                 throw RuntimeException("Failed to init tests! Something in here fails to initialize. Check the first test case.", e)
             }
             MockBukkit.load(MainPlugin::class.java, true)
-            ButtonPlugin.command2MC.unregisterCommands(MainPlugin.instance) // FIXME should have the init code separate of the plugin init code
             initialized = true
         }
     }
 
     @Test
-    @Order(1)
+    @Order(2)
     fun testRegisterCommand() {
         MainPlugin.instance.registerCommand(TestCommand)
-        assert(ButtonPlugin.command2MC.commandNodes.size == 1)
+        val nodes = ButtonPlugin.command2MC.commandNodes
-        assert(ButtonPlugin.command2MC.commandNodes.first().literal == "test")
+        assert(nodes.size == 1)
+        assert(nodes.first().literal == "test")
+        val coreExecutable = nodes.first().coreExecutable<Command2MCSender, TestCommand>()
+        assertEquals(TestCommand::class.qualifiedName, coreExecutable?.data?.command?.let { it::class.qualifiedName }, "The command class name doesn't match or command is null")
+        assertEquals("test", coreExecutable?.data?.argumentsInOrder?.firstOrNull()?.name, "Failed to get correct argument name")
+        assertEquals(String::class.java, coreExecutable?.data?.arguments?.get("test")?.type, "The argument could not be found or type doesn't match")
+        assertEquals(Command2MCSender::class.java, coreExecutable?.data?.senderType, "The sender's type doesn't seem to be stored correctly")
     }
 
     @Test
@@ -49,11 +56,14 @@ class Command2MCTest {
     }
 
     @Test
+    @Order(1)
     fun testUnregisterCommands() {
+        ButtonPlugin.command2MC.unregisterCommands(MainPlugin.instance) // FIXME should have the init code separate of the plugin init code
+        assert(ButtonPlugin.command2MC.commandNodes.isEmpty())
     }
 
     @Test
-    @Order(2)
+    @Order(3)
     fun testHandleCommand() {
         val user = ChromaGamerBase.getUser(UUID.randomUUID().toString(), TBMCPlayer::class.java)
         assert(ButtonPlugin.command2MC.handleCommand(Command2MCSender(user, Channel.globalChat, user), "/test hmm"))

Chroma-Core/src/test/resources/commands.yml

@@ -6,7 +6,7 @@ buttondevteam:
           TestCommand:
             def:
               method: def()
-              params: ''
+              params: 'test'
   core:
     ComponentCommand:
       def: