FlashyReese / CommandAliases

Alternate short commands for complex commands
MIT License
25 stars 6 forks source link

fix: Register aliases after default command registration phase #35

Closed John-Paul-R closed 2 years ago

John-Paul-R commented 2 years ago

Immediately register on server start. Re-register on CommandRegistrationCallback.EVENT

Resolves #34


Not sure I'm a fan of this approach. Having to keep track of isServerStarted state is annoying, but I don't think I have a cleaner approach atm.


I've not tested this yet, because the current state of the 1.19.x/dev branch seems to just never register commands for me. Looking into that locally...

FlashyReese commented 2 years ago

I think an alternative option is to make our own registration callback and inject after Fabric's using mixin priorities.

I've not tested this yet, because the current state of the 1.19.x/dev branch seems to just never register commands for me. Looking into that locally...

Format change and loader change it loads from .minecraft/config/commandaliases directory individual files. .minecraft/config/commandaliases/home.toml

schemaVersion = 1
commandMode = "COMMAND_REDIRECT"
command = "h"
redirectTo = "essentialcommands home tp"
John-Paul-R commented 2 years ago

Aha, removing the old configs and adding the new toml allows things to work. Thanks for the explanation.


Your comment reminded me that Fabric's event system has the ability to add phases with relative ordering. I think we can use that to our advantage here. (the latest commit makes use of this).

In my local testing, this resolves the registration order issue. Additionally, command aliases continue to work after /reload (though I do not have any datapacks installed, not sure if that matters for this repro)

FlashyReese commented 2 years ago

Your comment reminded me that Fabric's event system has the ability to add phases with relative ordering. I think we can use that to our advantage here. (the latest commit makes use of this).

I'm aware that some APIs have this but for command registration wise I'm fairly certain there aren't phases. I just saw the latest commit, does this exist for older versions of the API?

John-Paul-R commented 2 years ago

Seems to have been added with event phases. First release is 0.42.0+1.18

(Event file history)

John-Paul-R commented 2 years ago

Oh, I see you support as far back as 1.16.5 actively. Will put together something compatible with the older versions if that's preferable.

FlashyReese commented 2 years ago

Yes, although looking at 0.42.0-1.16 does mention the addition of event phases.

John-Paul-R commented 2 years ago

Oh, that's perfect. I wonder why that wasn't showing up on the tag list for that file. Looking at the changes in the event phases commit associated with that release, it looks like it includes the addPhaseOrdering method that we want.

John-Paul-R commented 2 years ago

I wonder why that wasn't showing up on the tag list for that file.

Aha, seemingly same changes, different commit:

FlashyReese commented 2 years ago

Looks good to me. :D

John-Paul-R commented 2 years ago

Thanks for the quick assist & communication!

FlashyReese commented 2 years ago

Thank you for the PR!