-
Bug
-
Resolution: Fixed
-
Major
-
None
-
None
-
Any; I've traced the bug in the code.
I was rewriting a Virtual Chest Plugin when I noticed this bug.
The method CraftHumanEntity.openInventory(Inventory inventory), which of course implements HumanEntity.openInventory(Inventory inventory), does something which in my opinion (explained below) is very nasty: it casts the received Inventory to CraftInventory (without even checking, which is something that IMO should be corrected as soon as possible).
What this results in is that a plugin writer that has written a perfectly fine implementation of Inventory, with a different storage mechanism than CraftInventory, to fit his purpose, can't use it at all because it will cause an exception to be thrown when the plugin tries to finally send it to the client.
Here you can find a shrinked version of my Inventory implementation, to see what I'm talking about: http://pastebin.com/uAFgxTdX
Here's my complete opinion on the matter:
One could argue that is it legitimate for CraftBukkit to internally use and work with CraftInventory instances. And that is completely true! I understand that CraftInventory internally uses a Minecraft IInventory, and that it makes the code very convenient as all it has to be done then is to call Minecraft's EntityHuman.openContainer on that IInventory, and then that IInventory would be directly updated by the Minecraft server.
But it should also provide a fallback for classes other than CraftInventory that don't use a IInventory as the internal storage method OR the interface of Inventory should be changed to have a getInventory method which returns a IInventory, which is of course something not to do because then Bukkit would depend on Minecraft (and I think that now even the most skeptical will start to see my point).
I discussed this on the Bukkit forums a lot of time ago, and people told me that I should just extend CraftInventory, and I hope that everyone will see why that's not something to do when developing a plugin with the Bukkit API.
CraftBukkit should glue Bukkit-implementing plugins to Minecraft. It is its exact and only purpose. It should do the work of creating a Minecraft IInventory as the Inventory is sent to the client, and of course place a hook into that IInventory to update the corresponding Inventory as the player manipulates the items in it.
And, again, it can of course check for and use CraftInventory instances the way it does now to optimize, but it should work with any Bukkit Inventory.
Note 1: I created this as major because in my opinion is goes against the purpose of CraftBukkit and also because while this is being fixed there should be at least a check in the code that will throw an appropriate exception (and not a ClassCastException).
Note 2: If it is of any help, I had already reported the bug here, on the old Bukkit tracker: https://bukkit.atlassian.net/browse/BUKKIT-5750