Skip to content

Make GuiItem constructor with NamespacedKey param public#2494

Open
ThomasWega wants to merge 1 commit into
stefvanschie:masterfrom
ThomasWega:public-constructor-gui-item
Open

Make GuiItem constructor with NamespacedKey param public#2494
ThomasWega wants to merge 1 commit into
stefvanschie:masterfrom
ThomasWega:public-constructor-gui-item

Conversation

@ThomasWega
Copy link
Copy Markdown

Hey.

Could we make this constructor public?

I have a pretty good use in my project to change the keys of the namespaced keys. It works completely fine with custom defined keys.

I do not see any reason for this constructor to be private

@stefvanschie
Copy link
Copy Markdown
Owner

Could you elaborate on why you'd want to use custom keys? Since the keys are internal data, I'd prefer to keep related methods internal as much as possible as well.

@ThomasWega
Copy link
Copy Markdown
Author

In my specific case I'm modifying packets to add certain things to item lores. Like enchantment bonuses, item worth etc. When opening the GUI, the packet will all items is sent and I've used two different implementations of GuiItem class with different keys to differentiate between player items in a GUI and pre-made navigation buttons in the GUI. I might be able to just add another Key to the item, but that just felt like adding new unnecessary data to the item

@stefvanschie
Copy link
Copy Markdown
Owner

I think adding an additional key is the more appropriate solution to this. My main reason for this is because using the key for both identifying the click handler (which is what IF uses it for) and for identifying different types of items is overextending its responsibility to two domains. Additionally, you'd also be using the key for identifying an attribute, whereas an attribute should generally be identified by the value instead, leaving the key itself designated only for the presence/absence of the attribute.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants