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

Migrate to cloud framework #1080

Merged
merged 6 commits into from
Nov 4, 2022
Merged

Migrate to cloud framework #1080

merged 6 commits into from
Nov 4, 2022

Conversation

Pablete1234
Copy link
Member

@Pablete1234 Pablete1234 commented Oct 14, 2022

This is still WIP (a couple command classes still comented out) but already very functional and usable version of pgm using the cloud command framework (https://github.com/Incendo/cloud).

The main if not only regression is that flags are not (yet) supported in the middle of the command, so they expect to always be at the end. However i've already made a proof-of-concept fix for this in Incendo/cloud@430beb7, so i'm hoping a fix can either be upstream soon, or we'll have to run a fork until it gets merged.

The benefits are several, from better autocompletion, to support for "MatchPlayer" as both injection (context/sender) and argument (ie: targeting someone else), cleaner commands, better tab-completion, and greedy strings (like /g, /msg etc) no longer replace quotes or prevent -f being typed as it's detected as a flag.

Example of greedy text being auto completed by fuzzy-matching (note how entered text is gold not golden):

javaw_9eu7wkrr2M.mp4

@Pablete1234 Pablete1234 added the refactor Code needs to be redesigned label Oct 14, 2022
flags = "f",
perms = Permissions.START)
@CommandMethod("recycle|rematch [duration]")
@CommandDescription("Reload (cycle to) the current map")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Always wanted to see if we can make these translatable. Do you think it’s possible with cloud?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the framework is flexible enough that i'm certain it can be done, but it requires using our own annotation

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want them to be globally translatable or per sender? There are different approaches depending on what you need :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We handle all translations in PGM per-sender, via adventure. We have our own systems for that here. Ideally we'd simply make the descriptions be an adventure component created from a translatable with a key specified in the @CommandDescription annotation, then in MinecraftHelp make sure it uses the components

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With builders you can set a component in the command meta directly (see MinecraftExtrasMetaKeys), with annotations you probably need a command builder modifier to retrieve component instances by key or something. Can either use the CommandDescription itself to store the intermediary key, or could add your own annotation and meta value

@Pablete1234 Pablete1234 marked this pull request as ready for review October 18, 2022 14:35
@Pablete1234
Copy link
Member Author

This has moved over to ready for review. The migration is complete, although some things no longer work as they used to. There's an upstream PR for cloud to allow flag parsing in any location (Incendo/cloud#395) that is required for things like /cycle 0 -f map to work (currently only /cycle 0 map -f works).

Besides that one issue, everything else is smoother than it used to, and map autocompletion has gotten alot of work, now very wild things auto-complete nicely and predictably. Things like /sn gdIII will result in Golden Drought III being selected.

@Pablete1234 Pablete1234 force-pushed the cloud-cmd branch 2 times, most recently from f4dbb8d to e9b88f9 Compare October 19, 2022 04:47
@KingOfSquares
Copy link
Contributor

Am i understanding correctly that this closes #66 ?

@Pablete1234
Copy link
Member Author

Am i understanding correctly that this closes #66 ?

it does

@Pablete1234
Copy link
Member Author

Pablete1234 commented Nov 1, 2022

Pushed a few changes, made parsers for PlayerClass & SettingKey. Setting keys will suggest & parse based on the previous argument for setting, so if you /toggle chat it will suggest admin/global/team while if you /toggle effects it suggests off/on.

Additionally since Incendo/cloud#395 doesn't seem to be getting any progress, i'm considering making PGM run a cloud fork until the changes get upstreamed so that this PR doesn't keed to keep waitning.

Electroid
Electroid previously approved these changes Nov 4, 2022
Copy link
Member

@Electroid Electroid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Pablete1234 If you resolve the conflict, this is good to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Code needs to be redesigned
Development

Successfully merging this pull request may close these issues.

5 participants