Skip to content

Conversation

rye761
Copy link
Contributor

@rye761 rye761 commented Oct 13, 2019

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.

@pop4959
Copy link
Member

pop4959 commented Oct 13, 2019

Build passes now due to merging #2818. I'll defer the review to md since he probably has a good idea of whether this is an acceptable solution to #2736.

@pop4959 pop4959 requested a review from mdcfe October 13, 2019 21:59
return addItems(fakeInventory, items);
}

public static Map<Integer, ItemStack> addAllOversizedItems(final Inventory inventory, final int oversizedStacks, final ItemStack... items) {
Copy link
Contributor

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(...).

Copy link
Contributor Author

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.

@Ichbinjoe Ichbinjoe added status: ready type: enhancement Features and feature requests. labels Dec 29, 2019
Copy link
Member

@mdcfe mdcfe left a 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?

@rye761
Copy link
Contributor Author

rye761 commented Dec 31, 2019

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.

@Ichbinjoe Ichbinjoe merged commit 77338d6 into EssentialsX:2.x Mar 31, 2020
ressidell pushed a commit to ressidell/Essentials that referenced this pull request Apr 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status: ready type: enhancement Features and feature requests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make drop-items-if-full work on Essentials kits

5 participants