Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inventory -> ItemContainer #1107

Closed
Prospector opened this issue Feb 9, 2020 · 7 comments
Closed

Inventory -> ItemContainer #1107

Prospector opened this issue Feb 9, 2020 · 7 comments
Labels
discussion vote A vote on a name refactor

Comments

@Prospector
Copy link
Contributor

Prospector commented Feb 9, 2020

This issue is assuming that #1106 is merged. Discuss in this issue as if it has been.

The word "inventory" in the game, both internally and externally, is used to refer to the player's inventory only. Many strings in the game refer to this Inventory class as a container, and many places in yarn already call this a container, in fact (see LootableContainerBlockEntity, LockableContainerBlockEntity, etc.).

I am proposing adding an Item prefix to it, for multiple reasons. Firstly, Container itself is too vague to be used alone for any class name. There are numerous classes already suffixed with Container that bear no relation to this class. Adding an Item prefix to it clarifies that it is something that contains items. Secondly, prefixing with Item leaves ample room for modders to make FluidContainers or EnergyContainers or whatever and not having confusing terminology with the item one being simply, Container.

@ghost
Copy link

ghost commented Feb 9, 2020

I like the reasoning behind this but with Container being named Container I feel like it might get confusing.
If Container does get renamed to Menu then yes.
However maybe we should use a synomyn such as ItemHolder or ItemStorage.
For now I give you hooray ( yes with change ).

Just re-read the first sentance. Yes. I agree.

@Prospector
Copy link
Contributor Author

Prospector commented Feb 9, 2020

Calling a chest, for example, an inventory doesn't sound right. While Mojang doesn't use it this way, I could see an argument that a chest does indeed have an inventory. However, a chest itself isn't an inventory.

A chest cannot be an inventory, but it can have an inventory. Using the inventory terminology, a chest would be an inventory in Minecraft, which doesn't make sense. On the other hand, a chest could be a container or have a container, which means it works out fine with the container terminology.

The player's inventory, on the other hand, makes sense to be an inventory because a player isn't an instance of a container, it has a container as a field. Therefore, the container can be called an inventory because it is something the player has, and not something it is.

@kvverti
Copy link
Contributor

kvverti commented Mar 15, 2020

I also agree with ItemContainer. The MVC trifecta of class hierarchies would then be

  • ItemContainer - model
  • Screen - view
  • ScreenHandler - controller

@Prospector
Copy link
Contributor Author

can someone else PR this please because I did ScreenHandler :P

@modmuss50
Copy link
Member

modmuss50 commented Mar 15, 2020

Im not a fan of this change? Almost seems like its being cached for the sake of it.

@Prospector
Copy link
Contributor Author

Prospector commented Mar 15, 2020

¯\(ツ)/¯ it's your opinion, but I've explained my reasoning pretty thoroughly. it's not just for the sake of it.

Juuxel added a commit to AntiquityMC/pomf that referenced this issue Mar 17, 2020
@liach liach added javadoc A PR that adds or refactors javadoc. vote A vote on a name refactor and removed javadoc A PR that adds or refactors javadoc. labels Mar 17, 2020
@Prospector
Copy link
Contributor Author

The arguments for Inventory have compelled me to believe it is fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion vote A vote on a name refactor
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants