Add Inventory Support for Some InventoryViewBuilders #12804
Conversation
paper-server/src/main/java/org/bukkit/craftbukkit/CraftServer.java
Outdated
Show resolved
Hide resolved
| * @param size the size | ||
| * @return this inventory | ||
| */ | ||
| Inventory createMenuInventory(@Nullable InventoryHolder owner, int size); |
There was a problem hiding this comment.
I want discussion on this unsure what to do here
There was a problem hiding this comment.
I do not like this method. Not one bit. Let me try and give some constructive thoughts...
Having to give major disclosures of how the created inventory CANNOT be used is a huge red flag to me in terms of the API design. The sucky part is... that if one wanted to make this a thing. The correct way to do it would be to change-up the top-level inventory related interfaces to introduce the concept of a virtual unopenable container which doesn't align with any of the Menu Types. But that delves into a level of refactoring that needs a lot more intention behind. Best be done by the core team.
I understand the intention behind re-using the Inventory interface to piggyback on the existing methods... It just is wrong here...
If someone wanted to force through this change... Seems like a route to take would be to add a parent interface to Inventory to hold some of those methods (I can't think of a name)... And then this support-menu-inventory concept could extend that parent interface instead of Inventory - if it isn't supposed to be used like a normal inventory.
So, my big sticking point, is that the Inventory interface is specifically designed currently to be tied to one specific InventoryType (Menu Type).
Final thought. Is that if a random developer would read the javadoc here... I don't think they would have any idea what it is supposed to do.
There was a problem hiding this comment.
I think that's a fair association. I'm largely dissatisfied with this method myself, but a few points for discussion.
"I understand the intention behind re-using the Inventory interface to piggyback on the existing methods... It just is wrong here..."
I don't know if it's necessarily wrong, but I do agree it could be confusing given the long-standing nature of the API. I’ll admit, the javadocs could definitely use some work, and that’s something I’ll try to improve, however, it's not my focus at the moment (my focus moreso on creating a easy to use and rational API). My rationale for using the Inventory here is that, conceptually, it’s not inherently flawed, though, as you pointed out, the getType() method does present issues about clarity and it muddles in the concepts of what is an Inventory, which a lot of people don't really understand to begin with seeing as the reality and the Bukkit concept are so far detatched.
The bigger issue is that the current Inventory system has been a hack for nearly a decade now, and I've been working to improve that over the last two years. So, I’m definitely open to alternative approaches, and I appreciate your feedback immensely. I've been considering similar ideas myself. Also, while I'm not personally a huge fan of adding inventory support to menus to begin with, there’s been a fair bit of interest from others, which is why I've been trying to make this to make it work.
"The correct way to do it would be to change-up the top-level inventory related interfaces to introduce the concept of a virtual unopenable container which doesn't align with any of the Menu Types."
Containers don’t align with MenuTypes to begin with, and that’s more of a Bukkit API concept. That said, I’m actually a fan of introducing a top-level ItemContainer interface that’s distinct from Inventory. I’ll bring it up in my conversations with team members to see if we can explore that further.
Like you said, I’m not entirely satisfied with this current API structure. I want to improve it, but I’m not sure how to fit everything into the existing framework just yet. I’ll keep experimenting with ideas, and I appreciate your input.
This PR will likely be a bit of a playground for a while so I'll see what I can cook up. And yeah this is a really bad method.
There was a problem hiding this comment.
This whole PR... I think what you would really want to do is create the necessary hooks for a plugin to implement some of this InventorySupport logic. But I also wonder to myself if that couldn't already be done within the existing API without such sweeping changes...
Idk, I guess a lot of it comes down to a dissatisfaction of the docs in InventorySupport to describe what is happening here. A regular plugin developer that opens that class and sees that InventorySupport "Adds inventory support to a {@link InventoryViewBuilder}." Are left wanting more...
There are some other things that need picked apart about this PR, but I am not convinced it is something I want.
| * @param size the size | ||
| * @return this inventory | ||
| */ | ||
| Inventory createMenuInventory(@Nullable InventoryHolder owner, int size); |
There was a problem hiding this comment.
I do not like this method. Not one bit. Let me try and give some constructive thoughts...
Having to give major disclosures of how the created inventory CANNOT be used is a huge red flag to me in terms of the API design. The sucky part is... that if one wanted to make this a thing. The correct way to do it would be to change-up the top-level inventory related interfaces to introduce the concept of a virtual unopenable container which doesn't align with any of the Menu Types. But that delves into a level of refactoring that needs a lot more intention behind. Best be done by the core team.
I understand the intention behind re-using the Inventory interface to piggyback on the existing methods... It just is wrong here...
If someone wanted to force through this change... Seems like a route to take would be to add a parent interface to Inventory to hold some of those methods (I can't think of a name)... And then this support-menu-inventory concept could extend that parent interface instead of Inventory - if it isn't supposed to be used like a normal inventory.
So, my big sticking point, is that the Inventory interface is specifically designed currently to be tied to one specific InventoryType (Menu Type).
Final thought. Is that if a random developer would read the javadoc here... I don't think they would have any idea what it is supposed to do.
This PR aims to add support to as many Menus as possible so long as it is reasonable to maintain and it makes sense to add. This PR in its original state supports all chest inventories (generic 1-6) and the dispesner and hopper. I'm up for discussion on other menu types.
This PR also needs to make another important decision regarding the future of Server#createInventory. This PR will aim to fully replace the old inventory api thus will aim to deprecate it completely.
My current plan on this front. Keep Server#createInventory(InventoryHolder, int). and deprecate the rest along with deprecated InventoryType. Keeping the mentioned method it will need to be expanded to support more arbitrary sizes as in the case with the hopper it needs to support atleast 5 slots. Though the arbitrary value may need to change depending on if any more menu support is added.
This PR also somewhat annoyingly so goes back and cleans up internals. I know its not necessarily what you would want to see in this PR, but it is somewhat necessary to ensure this works cleanly and becomes easier to maintain. Lots of comments were chopped things were reorganized etc. Testing should be done further to ensure there were no regressions. I'll take any questions on these "cleanups" as well.
This is the culmination of many iterations and suggestions on further design to make it cleaner to use is appreciated.