JorelAli / CommandAPI

A Bukkit/Spigot API for the command UI introduced in Minecraft 1.13
https://commandapi.jorel.dev
MIT License
504 stars 60 forks source link

Delegate Bukkit command registering logic to `CommandRegistrationStrategy` #560

Closed willkroboth closed 1 month ago

willkroboth commented 1 month ago

This PR moves a lot of the current logic for registering commands on Bukkit into the class SpigotCommandRegistration. This allows an alternate implementation of the new CommandRegistrationStrategy interface to be loaded when internal changes from https://github.com/PaperMC/Paper/pull/8235 (Paper-1.20.6-65 and later) are present. The CommandRegistrationStrategy is created by the method NMS#createCommandRegistrationStrategy.

This is a more permanent fix than https://github.com/JorelAli/CommandAPI/pull/555 and resolves these issues when running on Paper-1.20.65 and later:

The implementation can still probably be refined by https://github.com/JorelAli/CommandAPI/pull/517 (e.g. CommandAPI-Spigot should never need PaperCommandRegistration, and later Paper versions should never need SpigotCommandReigistration)

The NMS methods getResourcesDispatcher, isVanillaCommandWrapper, wrapToVanillaCommandWrapper, and isBukkitCommandWrapper were moved to SpigotCommandRegistration because they only make sense if Paper changes are not present. These methods are only used in the CommandAPI by SpigotCommandRegistration anyway, but developers can still access them using CommandAPIBukkit#getCommandRegistrationStrategy. PaperCommandRegistration provides a similar method: isBukkitCommand.

TODO:

Before merging I want to double check that the following works

On the following versions:

And of course~ code review is greatly appreciated!

JorelAli commented 1 month ago

What's our standpoint on those * imports? Do we want them?

We don't care. They don't change the output of the compiled code, the only downside to them is it's harder for humans to statically analyse code by staring at it in something other than an IDE, but it's 2024 and that's basically irrelevant.