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

Command build rewrite #602

Draft
wants to merge 42 commits into
base: dev/dev
Choose a base branch
from
Draft

Command build rewrite #602

wants to merge 42 commits into from

Commits on Sep 1, 2024

  1. Initial rewrite of command building system

    Precursor work for #483
    
    Notable changes:
    - No more flattening everything into a CommandAPICommand
      - CommandTrees are not flattened
        - CommandMetaData and Execution removed (only used to flatten CommandTrees)
      - MultiLiteralArgument and subcommands are no longer converted into LiteralArguments
      - Optional arguments are not chopped up into multiple arrays
      - In general, a single call to `ExecutableCommand#register` now creates the entire Brigadier tree for that command instance
        - Should only be converting Arguments to Brigadier nodes once instead of multiple times
    
    - CommandAPIHandler no longer creates Brigadier nodes
      - Delegated to AbstractCommandAPICommand, AbstractCommandTree, AbstractArgumentTree.java, and AbstractArgument
    
    - Problems that prevent a command from being registered have been unified under CommandRegistrationException
      - Exception messages tweaked
    
    - Bukkit-specific features removed from `commandapi-core`
      - Removed `isConverted` property of CommandAPICommand
      - Moved logic for flattening EntitySelectorArgument from `CommandAPIHandler#generateBrigadierCommand` to Converter
      - Moved special Bukkit command permission stuff into CommandAPIBukkit/SpigotCommandRegistration
    
    - Taking advantage of Brigadier redirects
      - Command alias nodes redirect to main command name
      - MultiLiteralArgument literal nodes use redirects
      - Instead of duplicating the argument node structure after these arguments, they can reference the same path
    
    TODO:
    - The checks performed by `CommandAPIHandler#hasCommandConflict` have not been reimplemented (I'm not sure if they did anything?)
    - MultiLiteralArgument is currently broken. See Mojang/brigadier#137
    - Many changes not covered by tests (could be sneaky behavioral differences)
    willkroboth committed Sep 1, 2024
    Configuration menu
    Copy the full SHA
    6517ca5 View commit details
    Browse the repository at this point in the history
  2. Disable broken test: `OptionalArgumentTests.kt#executionTestWithComma…

    …ndTreeAndOptionalArgumentMethod`
    
    TODO: Figure out what to do about this
    willkroboth committed Sep 1, 2024
    Configuration menu
    Copy the full SHA
    a321c9f View commit details
    Browse the repository at this point in the history
  3. Initial rewrite of optional Arguments

    Part of `dev/command-build-rewrite`
    Solves the problem with 1803e4e where optional arguments in CommandTrees didn't work
    This commit implements option 3 from 193f237#r126326791
    
    Basically, Arguments can no longer be optional by themselves. Instead, they are optional if they exist in a special list. This restricts the freedom of optional arguments in CommandTrees to avoid ambiguous cases, while CommandAPICommands work essentially the same via the `withOptionalArguments` method.
    
    Changes:
    AbstractArgument
     - Removed `boolean isOptional`
     - Removed method `setOptional`
    AbstractCommandAPICommand, AbstractCommandTree, AbstractArgumentTree
     - Added `List<Argument> optionalArguments`
     - Tweaked command-building logic to include `optionalArguments` list
    ExecutableCommand
     - Merged some registration logic shared between CommandAPICommand and CommandTree
    Kotlin DSL
     - `optional` boolean now chooses between calling `withArguments` or `withOptionalArgument`, instead of calling `setOptional`
    Tests
     - Added tests for making sure `RegisteredCommand` information is correct
     - Added tests for optional Arguments in CommandTree
     - Re-enabled test disabled by 193f237
    willkroboth committed Sep 1, 2024
    Configuration menu
    Copy the full SHA
    96e7338 View commit details
    Browse the repository at this point in the history
  4. Configuration menu
    Copy the full SHA
    0c9bea6 View commit details
    Browse the repository at this point in the history
  5. Configuration menu
    Copy the full SHA
    365580a View commit details
    Browse the repository at this point in the history
  6. Configuration menu
    Copy the full SHA
    5c41d50 View commit details
    Browse the repository at this point in the history
  7. Configuration menu
    Copy the full SHA
    9bcbb2a View commit details
    Browse the repository at this point in the history
  8. Configuration menu
    Copy the full SHA
    84b3887 View commit details
    Browse the repository at this point in the history
  9. Address some SonarCloud code smells

    Whoops, these actually are some silly mistakes I missed :P, thanks SonarCloud
    willkroboth committed Sep 1, 2024
    Configuration menu
    Copy the full SHA
    8957047 View commit details
    Browse the repository at this point in the history
  10. Configuration menu
    Copy the full SHA
    e176f2a View commit details
    Browse the repository at this point in the history
  11. Configuration menu
    Copy the full SHA
    100c8bb View commit details
    Browse the repository at this point in the history
  12. Configuration menu
    Copy the full SHA
    bd0cbdb View commit details
    Browse the repository at this point in the history
  13. Configuration menu
    Copy the full SHA
    963a5cf View commit details
    Browse the repository at this point in the history
  14. Fix MultiLiteralArguments - give nodes multiple parents instead of us…

    …ing redirects
    
    The Brigadier CommandDispatcher CommandNode structure does not need to be a tree. This means one node can have multiple parents. In this case, the node after a MultiLiteralArgument has parent node for each literal in the MultiLiteral.
    
    This works around the issue documented in Mojang/brigadier#137, where arguments disappear from the CommandContext used when executing an argument when the command encounters a redirect. This way, we can remember which literal was chosen no matter which branch was taken, yet still converge on the same path.
    willkroboth committed Sep 1, 2024
    Configuration menu
    Copy the full SHA
    d6e8a5d View commit details
    Browse the repository at this point in the history
  15. Initial implementation of FlagsArgument (and other changes)

    Changes:
    - Implemented `FlagsArgument` (#483)
      - Moved var handles for `CommandNode` `children`,`literals`, and `arguments` to `CommandAPIHandler`
      - Added `FlagsArgumentCommon` FlagsArgumentRootNode` and `FlagsArgumentEndingNode` to handle the special node structure and parsing required
    - Updated `CustomArgument`
      - All `AbstractArgument` builder methods are delegated to the base argument
      - Replaced `CommandAPIExecutor` parameter of `AbstractArgument#addArgumentNodes` to a `Function` to allow objects that hold arguments to better control how those arguments are executed
    - Added package `dev.jorel.commandapi.commandnodes` for class that extend `CommandNode` and related classes
    - Tweaked some exceptions
      - `GreedyArgumentException`
        - Changed because the `FlagsArgument` is sometimes greedy - only greedy iff it has no terminal branches
        - Greedy arguments are now detected when `AbstractArgument#addArgumentNodes` returns an empty list
        - Tweaked the exception message
      - `DuplicateNodeNameException`
        - Changed because literal arguments can conflict with other nodes if they are listed
        - Now thrown when two listed arguments have the same node name
        - Added `UnnamedArgumentCommandNode` to make sure unlisted arguments do not conflict
        - Renamed `MultiLiteralCommandNode` to `NamedLiteralCommandNode` for use by listed `Literal` arguments
        - Tweaked the exception message
    
    TODO:
    - Clean up code
    - Add tests
    - Remove test commands in CommandAPIMain
    - Add javadocs and documentation
    - Hope Mojang/brigadier#144 is resolved, otherwise be annoyed :(
    willkroboth committed Sep 1, 2024
    Configuration menu
    Copy the full SHA
    f464df1 View commit details
    Browse the repository at this point in the history
  16. Fix CommandNamespaceTests

    I messed this up when rebasing changes from #509 into `dev/command-build-rewrite`
    willkroboth committed Sep 1, 2024
    Configuration menu
    Copy the full SHA
    f36c43a View commit details
    Browse the repository at this point in the history
  17. Configuration menu
    Copy the full SHA
    91e1023 View commit details
    Browse the repository at this point in the history
  18. Configuration menu
    Copy the full SHA
    aa5947c View commit details
    Browse the repository at this point in the history
  19. Add tests for CommandConflictException

    Also fix typo in exception message
    willkroboth committed Sep 1, 2024
    Configuration menu
    Copy the full SHA
    a862694 View commit details
    Browse the repository at this point in the history
  20. Configuration menu
    Copy the full SHA
    5219af2 View commit details
    Browse the repository at this point in the history
  21. Configuration menu
    Copy the full SHA
    7b24440 View commit details
    Browse the repository at this point in the history
  22. Rewrite RegisteredCommand

    - Reordered constructor arguments to more closely match the order in `ExecutableCommand`
      - This branch is now definitely backward incompatible (though not majorly)
    - Replaced `List<String> argsAsStr` with `Node rootNode`
      - Added `RegisteredCommand.Node` class
        - Has an `argsAsStr` method as a replacement for the parameter of `RegisteredCommand`
        - Simplified representation of the Brigadier's `CommandNode` structure for the command
      - Removed `AbstractArgument#getHelpString` and `RegisteredCommand#arguments` (Added by #537)
        - The `RegisteredCommand.Node` structure is used to generate usage
        - `Literal` and `MultiLiteral` have separate logic for creating thier `RegisteredCommand.Node, wherein they define a different help string
        - TODO: #537 didn't have any tests, so I'm only guessing this new implementation works the same. In general, add more tests for usage generation.
    - Removed `ExecutableCommand#getArgumentsAsStrings` and `AbstractArgument#appendToCommandPaths`
      - Added `AbstractArgument.NodeInformation` to help pass around enough information to generate Brigadier nodes and `RegisteredCommand.Node`s in one `AbstractArgument` tree traversal.
      - Modified the signatures of a few methods to facilitate this, including overriding methods
      - TODO: This broke `Previewable` arguments, so I'll have to tweak those
    - Changed `CommandAPIHandler#registeredCommands` from an `ArrayList` to a `LinkedHashMap`
      - Instead of creating one `RegisteredCommand` object for each command path, `RegisteredCommand`s are merged when they share thier command name and namespace
      - One call to `ExecutableCommand#register` creates one `RegisteredCommand` object (if it has a namespace, an unnamespaced copy is created too)
      - `CommandAPIPlatform#postCommandRegistration` was reverted to original signature
      - NOTE: `CommandAPI#getRegisteredCommands` still returns a `List<RegisteredCommand>`
    - Updated `CommandAPICommandRegisteredCommandTests` and `CommandTreeRegisteredCommandTests`
      - Added `RegisteredCommandTestBase` to handle common utility methods
    willkroboth committed Sep 1, 2024
    Configuration menu
    Copy the full SHA
    4565eb9 View commit details
    Browse the repository at this point in the history
  23. Fix & update Previewable arguments

    - Removed `CommandAPIHandler#previewableArguments` and related methods
    - Added `PreviewableCommandNode` for storing `Previewable` information directly in Brigadier's tree
    - Tweak `NMS_1_19_Common_ChatPreviewHandler` to build previews from the node tree rather than by the node path
    
    TODO: Should probably test these changes on a real server to verify. Also, another example of Mojang/brigadier#144 being annoying.
    willkroboth committed Sep 1, 2024
    Configuration menu
    Copy the full SHA
    6f8b66c View commit details
    Browse the repository at this point in the history
  24. Help topic rewrite

    Revises github.com/JorelAli/CommandAPI/commit/ac8c06086f2e05015416b939028e1301ff95a87b
    
    Notable changes:
    - `RegisteredCommand` and `RegisteredCommand.Node` now have generic `<CommandSender>` parameter
      - `RegisteredCommand.Node` includes the `CommandPermission permission` and `Predicate<CommandSender> requirements`, copied from the argument/command the node represents
      - `RegisteredCommand#permission` is now acessibile as the permission of the `Node rootNode`
    
    - Created `CommandAPIHelpTopic` in `dev.jorel.commandapi.help` package
      - Replaced `shortDescription`, `fullDescription`, `usageDescription`, and `Object helpTopic` fields in `ExecutableCommand` and `RegisteredCommand` with `CommandAPIHelpTopic helpTopic`
      - Extended by `EditableHelpTopic`
        - Help builder methods of `ExecutableCommand` are delegated to its `CommandAPIHelpTopic` if it is editable
        - More general API created for #528: short description, full description, and usage can be provided separately to take advantage of the formatting the CommandAPI uses for static Strings
      - Extended by `BukkitHelpTopicWrapper` in `commandapi-bukkit-core`
        - Wraps Bukkit's `HelpTopic` as a `CommandAPIHelpTopic` so full `HelpTopic` customization can still be used
    
    - Created `CustomCommandAPIHelpTopic` in `commandapi-bukkit`
      - Converts a `CommandAPIHelpTopic` into a Bukkit `HelpTopic` for adding to the help map
      - Replaces usage of `NMS#generateHelpTopic`
      - Help formatting code extracted from `CommandAPIBukkit#updateHelpForCommands`
      - Resolves #470: `CommandSender` permissions and requirements are now checked when generating usage
    
    - Changed the treatement of namespaced help topics (#546). Namespaced help topics are now created. In the case where the same command name is registered with different namespaces, this allows the user to see the unmerged help using `/help namespace:commandName` (see `CommandHelpTests#testRegisterMergeNamespaces`).
    
    - Updated tests to fully cover changes
    
    TODO: Update and write documentation
    willkroboth committed Sep 1, 2024
    Configuration menu
    Copy the full SHA
    8d7a315 View commit details
    Browse the repository at this point in the history
  25. Configuration menu
    Copy the full SHA
    2d1187a View commit details
    Browse the repository at this point in the history
  26. Executor Rewrite

    Note: This commit makes this branch backwards incompatible, especially for non-DSL Kotlin code (see `commandapi-documentation-code/.../Examples.kt`). "Standard" Java and DSL API code usage looks the same, but I'm pretty sure plugins will need to recompile due to changes to the `FunctionalInterface`s. There are also some smaller public API changes, mentioned below.
    
    Notable changes:
    - Removed `AbstractCommandSender` and all its implemenations (i.e. the `dev.jorel.commandapi.commandsenders` package is completely gone)
      - Logic in `CommandAPIHandler#generateBrigadierRequirements` for checking if a sender satisfies a `CommandPermission` was moved to `CommandAPIPlatform#getPermissionCheck`
        - Previously, methods in `AbstractCommandSender` provided access to `hasPermission` and `isOp`. `CommandAPIBukkit` and `CommandAPIVelocity` now handle these definitions.
        -  `CommandPermission.TRUE()` and `CommandPermission.FALSE()` added for computing short circuits when combining permissions and requirements
      - `PreviewInfo` now has the generic parameter `Player` rather than an `AbstractPlayer`
        - Backwards-incompatible (`(Player) player.getSource()` becomes `player`)
        - Generic parameter propogates to `PreviewableFunction` and `Previewable`
      - `CommandAPIPlatform` methods for convert Brigadier Source to CommandSender simplified
        - `getSenderForCommand` removed
          - just pass `CommandContext#getSource` into `getCommandSenderFromCommandSource`
          - `forceNative` parameter was only used on Bukkit, which is now handled by `NMS#getNativeProxyCommandSender` (more on that below)
        - `wrapCommandSender` removed
          - Logic from #580 removed, since wrapping the NullCommandSender no longer needs to be handled as a special case
        - Wrapping the sender no longer necessary for `getBrigadierSourceFromCommandSender`
        - `CommandAPIVelocity` now does nothing in these methods :P
    
    - `CommandAPIExecutor` reworked
      - `ExecutorType` moved to platform modules (so Velocity can no longer try to define sender types that don't exist)
      - Detecting whether a certain executor can be run by the given class delegated to `BukkitTypedExecutor` and `VelocityTypedExecutor`
      - Priority of executors now depends on order of calling the `executes` methods (i.e the priority listed here https://commandapi.jorel.dev/9.4.1/normalexecutors.html#multiple-command-executor-implementations no longer applies)
      - Normal executors are no longer ignored if resulting executors exist (not sure if this was a bug or intended)
    
    - Tweaked `ExecutionInfo`
      - Added `CommandContext<Source> cmdCtx` to `ExecutionInfo`
        - This allows passing the information needed for a `NATIVE` executor on Bukkit to create a `NativeProxyCommandSender`
          - Uses `NMS#getNativeProxyCommandSender`, which was adapted from the removed `getSenderForCommand` method
          - Note: conflicts with #478, though should be easily resolved
        - Note: Velocity can define the `CommandSource` object, though Bukkit has it as `?`. This makes it hard for non DSL Kotlin to infer the type parameters for some reason, which is annoying because you now need to specify the type parameter. I don't know if there's a better way to handle that.
        - TODO: Make sure depdendents can still use `ExecutionInfo` without including Brigadier as a dependency
      - `ExecutionInfo` is now a record (`BukkitExecutionInfo` and `VelocityExecutionInfo` removed)
    
    - Simplified `dev.jorel.commandapi.executors` package
      - Platform and sender specific executor classes (e.g. `PlayerCommandExecutor` and `EntityResultingExecutionInfo`) removed
      - Just the functional interfaces `NormalExecutorInfo`, `NormalExecutor`, `ResultingExecutorInfo`, and `ResultingExecutor` now
      - `BukkitExecutable` and `VelocityExecutable` link different command sender classes as type parameters to the `ExecutorType` enum values
    
    TODO:
    - Add executor tests to `dev/dev` to ensure same behavior
      - Especially for `PROXY` and `NATIVE` senders
      - Especially for non-DSL Kotlin to see how its lamdba type inference works
    - Update documentation
    willkroboth committed Sep 1, 2024
    Configuration menu
    Copy the full SHA
    c874ab5 View commit details
    Browse the repository at this point in the history
  27. Configuration menu
    Copy the full SHA
    d390ccb View commit details
    Browse the repository at this point in the history
  28. Fully split converted arguments

    Resolves #557 concerns
    
    Expand CommandConvertedTests and ArgumentEntitySelectorTests to cover argument flattening
    
    Miscellaneous changes to testing framework to make entity selectors work better
    willkroboth committed Sep 1, 2024
    Configuration menu
    Copy the full SHA
    b27e7d4 View commit details
    Browse the repository at this point in the history
  29. Configuration menu
    Copy the full SHA
    2438daf View commit details
    Browse the repository at this point in the history
  30. Configuration menu
    Copy the full SHA
    288d6ef View commit details
    Browse the repository at this point in the history
  31. Implement DynamicMultiLiteralArgument

    Acts like `MultiLiteralArgument`, but literals can be changed and differ for each CommandSender
    
    Resolves #513
    
    Uses Paper's `AsyncPlayerSendCommandsEvent` and Velocity's `PlayerAvailableCommandsEvent` to change the client's view of the command tree. Note that a similar event does not exist on Spigot, so the suggestions are not dynamic, though the parsing still is.
    
    `DifferentClientNode` class added to handle creating client-server command tree de-syncs (I believe this can be used to make `FlagsArgument` platform-agnostic)
    willkroboth committed Sep 1, 2024
    Configuration menu
    Copy the full SHA
    3649509 View commit details
    Browse the repository at this point in the history
  32. Add FlagsArgument to Velocity (and other changes)

    This was not made trivial by DifferentClientNode
    
    DifferentClientNode changes:
    - DifferentClientNode now an interface
      - Split into Argument and Literal subclasses
    - Node rewriting happens in two stages, `onRegister = true`, then `onRegister = false`
      - Each platform uses this differently b/c they're all special
      - Spigot/Paper run `onRegister = true` when registering to get rid of FlagsArgument loops, which otherwise cause a StackOverflow
      - Spigot runs `onRegister = false` when registering since it doesn't have a command send event (and so DynamicMultiLiteral can't display suggestions correctly)
      - Velocity runs `onRegister = true` during the command send event because `CommandNode.literals` doesn't exist
    
    FlagsArgument changes:
    - Generalized `executorCreator` from AbstractArgument node building with TerminalNodeModifier FunctionalInterface
      - Allows redirecting and wrapping nodes without reflection
    - FlagsArgumentEndingNode extends DifferentClientNode
      - Rewriting turns server-side children loop into redirects for the client (Mojang/brigadier#137)
      - Reference to root for setting the redirect is sneakily stored in HiddenRedirect command node, which gets properly updated when Velocity copies a node tree
    - Children loops are built by FlagsArgumentRootNode by calling `addChild`, rather than sharing Map instances with reflection, since that could de-sync the `hasLiterals` boolean on Velocity
    - FlagsArgument now only uses reflection indirectly through DifferentClientNode :O
    
    CommandAPIHandler#writeDispatcherToFile changes:
    - All platforms now use custom algorithm for serializing CommandNode to JSON
      - Does not StackOverflow if children loop
      - References duplicate instances of nodes in a tree by their shortest path
      - Serializes orphaned redirects
      - Supports serializing non-Brigadier CommandNode classes through NodeTypeSerializer
    - CommandAPIPlatform#createDispatcherFile replaced by #getArgumentTypeProperties
      - On Bukkit, SafeStaticMethodHandle used to access private static nms method ArgumentUtils#serializeArgumentToJson
      - On Velocity, ArgumentTypeSerializer added to extract ArgumentType properties
    - Slight updates to tests that verify dispatcher JSON
    
    Other changes:
    - Forgot to insert `<>` on PaperImplementations constructors in last commit
    - Removed deprecated `MultiLiteralArgument(String[] literals)` constructor because the null node name caused a NPE when hashing its command nodes
    - Tweaked DynamicMultiLiteralCommandNode to make it work when wrapped inside a FlagsArgumentEndingNode
    - Removed warning when using empty namespace on Velocity
    - Removed warning when `CommandNode.literals` field not found on Velocity
    - Switched commandapi-velocity-core to depend on Velocity's custom Brigadier fork
    
    TODO:
    - DynamicMultiLiteral still needs tests
    - Test NMS ArgumentUtils reflection on relevant Bukkit versions
    - Remove test commands
    - Implement custom NodeTypeSerializers
    willkroboth committed Sep 1, 2024
    Configuration menu
    Copy the full SHA
    9007237 View commit details
    Browse the repository at this point in the history
  33. Configuration menu
    Copy the full SHA
    f77ee00 View commit details
    Browse the repository at this point in the history
  34. Clean up files (unused imports and spaces when other lines have tabs)

    Removed subcommand execution test from ArgumentMultiLiteralTests since subcommands are no longer converted to MultiLiterals
    willkroboth committed Sep 1, 2024
    Configuration menu
    Copy the full SHA
    dee00f1 View commit details
    Browse the repository at this point in the history
  35. Configuration menu
    Copy the full SHA
    3dfa38f View commit details
    Browse the repository at this point in the history
  36. Add serializers for custom CommandNodes

    Add NodeTypeSerializerTests
    willkroboth committed Sep 1, 2024
    Configuration menu
    Copy the full SHA
    8c20d4e View commit details
    Browse the repository at this point in the history
  37. Make CommandAPI#unregister remove commands from `CommandAPIHandler.…

    …registeredCommands`
    
    Also made unregistering on Velocity respect the `unregisterNamespaces` parameter and update the dispatcher file
    willkroboth committed Sep 1, 2024
    Configuration menu
    Copy the full SHA
    0e66ac8 View commit details
    Browse the repository at this point in the history
  38. Configuration menu
    Copy the full SHA
    1bc6e7a View commit details
    Browse the repository at this point in the history
  39. Remove test commands

    willkroboth committed Sep 1, 2024
    Configuration menu
    Copy the full SHA
    ad5f426 View commit details
    Browse the repository at this point in the history
  40. Fix proxy senders

    The logic for detecting when CommandSourceStacks have a target Entity different from the sender (e.g. when using `execute as`) was incorrectly removed, causing `executesProxy` executors to never be taken on a real server
    willkroboth committed Sep 1, 2024
    Configuration menu
    Copy the full SHA
    2d7ec35 View commit details
    Browse the repository at this point in the history

Commits on Sep 2, 2024

  1. Fix Brigadier node serialization on Bukkit 1.16.5 and 1.17

    For some reason, on those specific versions, `JsonArray#deepCopy` has protected visibility ¯\_(ツ)_/¯. So, just don't use that method ig.
    willkroboth committed Sep 2, 2024
    Configuration menu
    Copy the full SHA
    c02421f View commit details
    Browse the repository at this point in the history
  2. Resolve #310

    willkroboth committed Sep 2, 2024
    Configuration menu
    Copy the full SHA
    4844a0e View commit details
    Browse the repository at this point in the history