Purchased items may appear in armor slots (Spigot 1.9)

[ORelio]

Hello,

We've found another bug after more testing. A minor one. When inventory is full, items purchased from the shop will be placed in armor slots:

Expected behavior would be an "inventory full" error message and the purchase being cancelled. We're runing some recent Spigot 1.9-R0.1-SNAPSHOT and QuickShop v0.9.8.

Please note that it might be a Spigot bug depending on the implementation as 1.9 builds still contain many issues, but I'm reporting just so that you can confirm whether it is Spigot or QuickShop related ;)

[KaiKikuchi]

Thank you for reporting this, but I think this isn't something that really needs a fix!

PS: you're using an old version of QuickShop.

[ORelio]

Hello, This allow players to have the item as a hat, which isn't intended behavior. I've read all your commits since v0.9.8, nothing seems likely to fix that hat issue :/

[KaiKikuchi]

There was an API change. Now PlayerInventory.getContents() includes armor slots too. Instead, Inventory.getStorageContents() has been added.

Please download v.0.9.13, it should fix the issue, hopefully.

[ORelio]

OK, apparently Spigot fixed it on their side too in newer builds, now the given item will drop on the floor instead of reaching armor slots. However, getting an error message seems better indeed. Thanks!

[ORelio]

After testing, for some weird reason I'm getting the following crash:

[21:42:31 WARN]: [QuickShop] Task #239 for QuickShop v0.9.13 generated an exception
java.lang.Error: Unresolved compilation problem:
        The method getStorageContents() is undefined for the type Inventory
        at org.maxgamer.quickshop.Util.Util.countSpace(Util.java:944) ~[?:?]
        at org.maxgamer.quickshop.Shop.ShopManager$1.run(ShopManager.java:441) ~[?:?]
        at org.bukkit.craftbukkit.v1_9_R1.scheduler.CraftTask.run(CraftTask.java:71)
//[...]

Since getStorageContents() is new, in order to retain backward compatibility with Spigot 1.8 and older, maybe a bit of reflection would be interesting?

    public static int countSpace(Inventory inv, ItemStack item) {
        int space = 0;
        ItemStack[] contents;
        try { // Bukkit API MC 1.9+ : Get actual storage slots (not armor...)
            Method storageContents = Inventory.class.getMethod("getStorageContents");
            contents = (ItemStack[])storageContents.invoke(inv);
        } catch (Exception e) {
            contents = inv.getContents(); // Bukkit API MC 1.8- fallback
        }
        for (ItemStack iStack : contents) {
            if (iStack == null || iStack.getType() == Material.AIR) {
                space += item.getMaxStackSize();
            } else if (matches(item, iStack)) {
                space += item.getMaxStackSize() - iStack.getAmount();
            }
        }
        return space;
    }

The above code will automatically pick getStorageContents() if available, or fall back to getContents() in case of unavailability or other error. The contents of the for loop is the same as before.

After a quick test on spigot-1.9.0-R0.1-SNAPSHOT.jar:git-Spigot-b1c1b55-7d73fbb, it seems to perform properly, I'm getting the red error message if my inventory is full with empty armor slots, whereas QuickShop v1.9.12 or lower will give the items anyway (resulting in item drop or using armor slot) :)

[KaiKikuchi]

I know how reflection works. If you're a developer, you can make a pull request instead.

[ORelio]

Sorry, I wasn't trying to look pedantic or anything, that was merely a suggestion for getting your opinion about the "reflection" approach. Since you're asking for a pull request, here it is ;)

[KaiKikuchi]

No, I'm sorry. I'm not in a good shape, and I tried to make a fix without thinking about it too much. I didn't even test it. And it's completely wrong, I noticed that too late.

You tried to help, no need to be sorry.

Anyway I wonder if this fix is necessary.

[ORelio]

That's fine, and thanks for the fix! Indeed that wasn't really important, but since Spigot can do weird things when trying to move items to a full inventory, that's an additional safety.