-
-
Notifications
You must be signed in to change notification settings - Fork 1k
Make drop-items-if-full work on Essentials kits #2820
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
Conversation
return addItems(fakeInventory, items); | ||
} | ||
|
||
public static Map<Integer, ItemStack> addAllOversizedItems(final Inventory inventory, final int oversizedStacks, final ItemStack... items) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can't this just be a Collection<ItemStack>
? That way, you can simply pass in the ArrayList from above without needing to do a cumbersome toArray(...)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be, but then we should be changing addAllItems and any other usages of it as well. I'm not sure if such a change is appropriate for this pull request or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't this just leave the player with the items that did fit in their inventory, or is there something I'm missing here?
The addAllItems function uses a copy of the inventory first to check if everything will fit, and then only adds the items if they all fit in the inventory copy. So no, the items won't be added if only some of them fit. |
Fixes #2736
This changes the way the kit command works by creating an ArrayList of items and making sure all can be added before giving the kit if drop-items-if-full is false. Works with oversized stacks.
More testing is recommended. I believe I have included proper economy support, but haven't actually tested it because I don't have an economy plugin on my test server right now. This is also a fairly significant change to the way this command works, so more testing is always better.