TheBusyBiscuit
commented
3 years ago As I mentioned before, I only have a working knowledge of Java right now, so I haven't dove into the particulars on annotations quite yet. Since you mentioned the differences in NonNull versus Nonnull being due to the method being private, I reverted those back to NonNull for the public version; hopefully that was the right move. If you'd rather keep InvUtils as is, putItem could check with the public fits instead, however since there would be redundant code involved with doing that, I figured these changes would be the better option.
Normally, annotations are only visual guidance. Meaning that @Nonnull for example is only a sort-of "notice" to developers that this should never be null. Similarly, @Nullable indicates: "Developers, be aware that this can sometimes be null".
Your IDE or any code scanner you use often respects these guidances and will prompt you a visual warning when they are misused. But in a nutshell: They are just visual prompts.
However, we are using lombok in this project and lombok (you can look it up) gets compiled into java code upon compilation. So the @NonNull annotation from lombok will actually result in a null check and an exception being thrown if it is null.
The reason I said that we don't need the lombok for our private method is that the private access level means that we have full control over what goes into it, so the visual indicance should be enough. When it's public though, we may want to strongly enforce this precondition, that's why lombok is useful here. Though, these two annotations being named the same is quite awful.
Anyway, thanks for this change! :)
Previously, InvUtils was updated to respect the Inventory's maxStackSize when checking for space. ChestMenu, however, used the same, patched-out code when it would put an item in the Inventory. This meant that although checking for free space did abide by the limits, ChestMenu still behaved as before and would stack the items regardless of the limits. This commit changes the limit-ignoring comparison to use the comparison method
isValidStackSizein InvUtils, and makes that method public.This was missed in the first pull request, my apologies.
For a concrete example, assume an AContainer has an Inventory with a maxStackSize of 1 and one item in its first output slot. The recipe check (with the previous changes) would correctly identify that the second output slot is open and begin processing the recipe. When the recipe finished processing, the BlockMenu (-> DirtyChestMenu -> ChestMenu) would attempt to add the item to inventory. Because it used the old comparison, it would still find the first output slot to be a valid output and add the recipe's output item to it despite the maxStackSize. If the AContainer produced the same recipe output every time, this would likely be fine since the items would stack in the player inventory anyway. However, if the AContainer outputs the same base item with varying metadata values that are not checked for by ItemStackWrapper, then the newly output item becomes a copy of the item in the first output slot when it is put in the Inventory instead of going in the open second slot as intended.
As I mentioned before, I only have a working knowledge of Java right now, so I haven't dove into the particulars on annotations quite yet. Since you mentioned the differences in
NonNullversusNonnullbeing due to the method being private, I reverted those back toNonNullfor the public version; hopefully that was the right move. If you'd rather keep InvUtils as is,putItemcould check with the publicfitsinstead, however since there would be redundant code involved with doing that, I figured these changes would be the better option.