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

[Update] Add new Features & Fixes (see desc) #4

Merged

Conversation

JustAHuman-xD
Copy link
Contributor

@JustAHuman-xD JustAHuman-xD commented Feb 16, 2024

This PR does a LARGE amount of things, I will do my best to cover them all here.

To Do

  • Update the Readme (I need yall's feedback cause idk how to format it with the flags system)
  • More extensive testing? (I've done quite a lot but maybe I should do more)

Changes (Small ones)

  • Refactored the main class
    • Jeff wanted me to clean up this class so I delegated most of the things to relevant classes
    • Just makes it cleaner and such
  • Added a Selection wand
    • Added Items where any slimefun items can be registered on Items#init, currently only used for the wand
    • The wand and the item group are hidden
    • obtainable with /wesf wand
    • Added WandListener used to detect when a player uses the wand
  • Added PositionManager
    • this class was added in the refactor of the main class
    • just tracks players and their stored positions
  • Added Utils
    • Also added in the refactor of the main class
    • Just has misc things used in the plugin

Command Changes

There was some major changes made here

WorldEditSlimefunCommands Changes

  • New annotations
    • Added a suppressor for unused warnings (because of how commands work)
    • Added the @CommandPermission("wesf.admin") annotation, as it will automatically apply to all commands instead of manually adding to each subcommand
  • Added WorldEditSlimefunCommands#init
    • Registers any completions, contexts, and commands to ACF
  • Added a wand sub command
  • Refactored to use the new PositionManager
  • Added WorldEditSlimefunCommands#loopThroughSelection
    • This method is used to reduce duplicated code, it loops through the selection and calls the runnable provided
    • updated the paste and clear command to use this method
  • Slightly tweaked clear command parameters
    • default callEvent to false
    • instead of registering a completion for booleans just supply true and false directly
  • Largely tweaked the paste command
    • added some more warnings when invalid pastes are attempted
    • added command flags (see next section)
    • clear any blockstorage on the blocks being pasted (fixes some weird behavior)

CommandFlags

At the request of Jeff I have made a Command flag system, this is modular and can be easily updated in the future to add more features

  • Added CommandFlag this abstract class accepts a type parameter for its value, ie, EnergyFlag stores a boolean, true to add, false to not
  • Added CommandFlags this class stores all actual flags & is the "brains" behind most of the operation
    • Any CommandFlag implementations are stored in the FLAG_TYPES map, where the flag is mapped to the default instance of a flag
    • Currently the 5 types are "--energy", "--inputs", "--refill_inputs_task", "--void_outputs_task", and "--timeout"
  • Added a command completion "@command_flags" this completion is responsible for displaying the flags and helping complete the values for the player in the command line

Other

if you have any specific questions about implementation lmk

Copy link
Contributor

@J3fftw1 J3fftw1 left a comment

Choose a reason for hiding this comment

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

idk if i like the mess in the main class now.
but i also dont want to be a burden in this pr

@JustAHuman-xD
Copy link
Contributor Author

idk if i like the mess in the main class now.

but i also dont want to be a burden in this pr

Nah you're good, I can move stuff around

@JustAHuman-xD
Copy link
Contributor Author

Clean it up

README.md Outdated Show resolved Hide resolved
@J3fftw1
Copy link
Contributor

J3fftw1 commented Feb 16, 2024

can we change the commands to bee --energy and --inputslots
so in the future we can have --scripts for android
--text for holograms etc etc

@JustAHuman-xD JustAHuman-xD changed the title [Fix] Shade Dependencies Properly [Feature] Add Wand [Update] Add new Features & Fixes (see desc) Mar 5, 2024
@JustAHuman-xD
Copy link
Contributor Author

One thing worth noting is that I'd like to abstract this command flags system at some point, not sure how or to what but I could see it being useful in other projects

Copy link
Contributor

@J3fftw1 J3fftw1 left a comment

Choose a reason for hiding this comment

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

quick review.
Need to do a proper review later.

README.md Outdated Show resolved Hide resolved
Comment on lines +55 to +63

if (args.isEmpty()) {
return availableFlags;
}

String currentArg = args.remove(args.size() - 1);
if (args.isEmpty()) {
return availableFlags;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we return the same thing twice?
we both check the same thing here
should line 61 maybe be currentArg.isEmpty()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

line 60 calls .remove, so args changes and the check is made again.

because if it's empty after removing one arg (the arg it's typing actively) it means it's the first argument, which would be a --flag

return availableFlags;
}

String lastArg = args.get(args.size() - 1);
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldnt this use currentArg?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no....?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh I guess I never actually use currentArg do I, lol.

I can probably remove the variable declaration then? But it might make it even less readable doing so

currentArg is the argument actively being typed. acf will handle filtering the options based on the current argument so leaving it alone is all we need to do. What's really important is knowing what the last full argument was.

Co-authored-by: J3fftw <[email protected]>
@JustAHuman-xD
Copy link
Contributor Author

Btw do you plan on adding this to blob builds? Or just purely GitHub releases

@JustAHuman-xD JustAHuman-xD deleted the fix/bstats-and-wand-feature branch March 5, 2024 14:50
@JustAHuman-xD JustAHuman-xD restored the fix/bstats-and-wand-feature branch March 5, 2024 14:52
@JustAHuman-xD JustAHuman-xD reopened this Mar 5, 2024
@JustAHuman-xD
Copy link
Contributor Author

I am apparently not allowed to rename the branch, whoops lol

@JustAHuman-xD
Copy link
Contributor Author

I'll rebase and update the PR name & desc later today

@JustAHuman-xD
Copy link
Contributor Author

I've tested the new task flags, all now work as intended!

@JustAHuman-xD
Copy link
Contributor Author

One thing I still need to do is add those new flags to the readme

@J3fftw1
Copy link
Contributor

J3fftw1 commented Mar 15, 2024

have you tested this?

@J3fftw1 J3fftw1 merged commit b682812 into Slimefun-Addon-Community:main Mar 15, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants