java version "10" 2018-03-20
Java(TM) SE Runtime Environment 18.3 (build 10+46)
Java HotSpot(TM) 64-Bit Server VM 18.3 (build 10+46, mixed mode)

@md_5

I would like to propose another fix to this (I don't know if this has been proposed before, I tried searching but found nothing). Since the InventoryHolder API makes working with inventories very easy, but the API is misused and being treated like a context for inventories, what about adding a supported way for plugins to specify contexts for inventories?

Thus, I propose adding an empty interface public interface InventoryContext which would be implemented by plugins and replacing the InventoryHolder parameter in createInventory(...) methods with InventoryContext (the old methods will obviously stay as deprecated).

Also, the Inventory interface will now have a @Nullable InventoryContext getContext() method, which would return the context passed at creation time or null if not provided.

The main benefits of the InventoryContext API are:

1. It doesn't depend on the world state
2. The context instance provided when creating the inventory cannot be changed (it's final in the Java sense)
3. It allows plugin to distinguish their inventories from the ones of other plugins without needing Maps
4. Using it should be faster than using maps (the context would be in a (final) field, so no lookup inside a map is needed)
5. Given how popular the (misusage of) InventoryHolder API is, it should be fairly easy for developers to migrate their code
6. The implementation should be all on the CraftBukkit level (or at least I hope, I haven't tried to implement this yet)
7. InventoryContext doesn't require to implement any methods (i.e. getInventory())

When it comes to the UIs like anvils yeah I know about that but it should be doable theoretically even if its a lot of work.

Anyway that's not my main issue right now, so this issue is about how the InventoryHolder is used and that some blocks (basically ones that have "special" inventories) can't have a custom InventoryHolder so my main question was if it would make sense to add support for those blocks. (Inventory types in question: DISPENSER, DROPPER, FURNACE, BREWING, HOPPER, BLAST_FURNACE, LECTERN, SMOKER)

Small summary of everything that was discussed here:

Basically you (md_5) thought that the InventoryHolder api as it is now should never been exposed to the public api for custom use as it was probably never intended that way.
Many people use this api though as it makes working with custom UIs very easy especially when it comes to differentiating between inventories.
Most of the discussion was about making Inventories distinguishable from another and deprecating the InventoryHolder api or implementing support for the previously mentioned blocks.

It was also discussed to possibly wrap around the actual tile entities / inventories and provide the custom InventoryHolder that way.
The custom InventoryHolder would take priority if it is set otherwise it would return the actual TileEntity as the InventoryHolder.

Without reading the comments, the main issue is these blocks rely on world state to function 

@md_5

Since this issue is years old and I didn't want to start to work on something that was declined before I wanted to ask if this is an issue that might be merged if it is implemented "correctly"?

I would love to see a fix for this and would want to give it a try because right now I'm facing exactly this issue.
And while I would be working on this I also might address the problem with custom anvil inventories (you can't actually use them as they cause exceptions when doing so).

When I do 

Inventory inventory = Bukkit.createInventory(new MainGUIHolder(), 54, ChatColor.RED + "Jobs")

the InventoryHolder also isn't the right type
If I call this:

if(inventory.getHolder() instanceof GUIHolder)

{
    return  (GUIHolder) inventory.getHolder();
} else {
    throw new IllegalStateException("the inventoryHolder is no GUIHolder!!");
}

it throws the IllegalStateException
The inventory is the Inventory I just created above

Please fix this, as I am using this a lot

PR: https://hub.spigotmc.org/stash/projects/SPIGOT/repos/craftbukkit/pull-requests/482/overview

Agreed since InvetoryHolder can also be used for lets say protections plugins or some other data value

I would rather see a system that returns the correct type of inventory than one that allows you to have an InventoryHolder if we had to choose between the two.

However, as it is now there's no nice way to reconcile these problems. If you create a FurnaceInventory you would reasonably expect the holder to be the furnace. On the other hand, I could understand that passing in a custom InventoryHolder would override that as well. I think if we were going to have a solution it'd be adding another constructor to the CraftInventories that accepts an InventoryHolder so we can specify the InventoryHolder that should have priority over the TileEntity.

Yeah so you're making your own InventoryHolder and using it as context.
I don't think that's what whoever made the API intended, but I'm also not sure what they did intend for it to be.

The point of passing a holder is obvious to me, it allows plugins to 'provide' their own data along with the inventory. I've used patterns like the one in the attachment to create inventory menus and other stuff. Inventory holders provide a solid way for a plugin to check whether the inventory was created by the plugin using a simple instanceof. I personally haven't used a furnace inventory for this, just regular chest inventories and dispenser inventories, but one use I can think of is a plugin that lets you preview recipes and displays the recipe in a furnace inventory. Then there is a listener on InventoryClickEvent that cancels the clicks when the holder was an instance of a RecipePreviewInventoryHolder. Of course such a plugin is now obsolete because recipes in 1.13 are synced to the client but you get the point. There could be other reasons why plugin devs may want to use a furnace inventory.

I agree that we can't just wholesale copy and paste code.

The previous code was much more correct from an API perspective; but I'm not even sure what exactly the point of passing in a holder is when the inventory is custom. It makes very little sense to me to have an API that is "create a custom inventory using this specific furnace, but actually its not really part of the furnace in any way".

I'm not a big fan of duplicating code if we don't have to. It just makes more work later on for someone else. I'd rather just create a wrapper for tile entities that we can pass the inventory holder into and delegate all inventory related methods to the ITileInventory.

I might be able to drop a few cents in as well here.

A year a go I reported that shiftclicking in custom non-chest inventories could caused an infinite loop: https://hub.spigotmc.org/jira/browse/SPIGOT-3500

The shiftclicking algorithm resides in net.minecraft.Container and its subclasses. Think of it as the NMS equivalent of InventoryView. The container used by CraftInventoryCustom/MinecraftInventory was a simple CraftContainer that didn't specialize for shiftclicking in nonchest inventories. The bug was patched by delegating to the vanilla container implementations. I thought this would be a good solution because if minecraft ever updates their inventories again (as they did when requiring lapis lazuli for enchanting and blaze powder for brewing) in the future, bukkit's shift-click algorithm would be up to date automatically.

However this is what caused this issue: https://hub.spigotmc.org/jira/browse/SPIGOT-3981 as the net.minecraft.server.ContainerFurnace expects that the inventory is a TileEntityFurnace, which the MinecraftInventory wrapped by CraftInventoryCustom obviously isn't.

Now this issue was patched too by using the actual NMS TileEntity inventories when Server#createInventory is called - however the inventory holder is never passed along to the tile entity. I guess we could patch the NMS again and create a setter for the inventory holder - or just pass it to a secondary constructor of the tile entity class. or should we extend classes such as TileEntityFurnace and override the getOwner method?

All these changes to nms make me wonder whether delegating to nms classes was the right decision. What if bukkit/craftbukkit has its own Containers that emulate the vanilla click behaviour. Sure it'll be more classes for craftbukkit but weird interactions with NMS code such as the bugs mentioned above should not occur anymore. If this approach is taken in the future (I'm just speculating now) the Inventory/InventoryView api could be extended to such that shiftclicking is represented as a method just like it is in net.minecraft.server.Container - that way plugin developers can implement their own shifclick algorithms by overriding a method and associating a certain view with a certain inventory type.

So yeah bascially I'm asking for opinions here. What is good design in this case?

Senmori