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

Add ItemStack#effectiveName #11469

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

masmc05
Copy link
Contributor

@masmc05 masmc05 commented Oct 5, 2024

Adds a new method which returns the name of an itemstack which the player sees in their inventory. Takes into account any data components exactly as vanilla.

Unlike ItemStack#displayName, does not add additional brackets etc

@masmc05 masmc05 requested a review from a team as a code owner October 5, 2024 01:30
electronicboy
electronicboy previously approved these changes Oct 9, 2024
@lynxplay
Copy link
Contributor

lynxplay commented Oct 9, 2024

Duplicate of #9050

@lynxplay lynxplay marked this as a duplicate of #9050 Oct 9, 2024
@lynxplay lynxplay dismissed electronicboy’s stale review October 9, 2024 21:38

Not so fast buckaroo

@masmc05
Copy link
Contributor Author

masmc05 commented Nov 24, 2024

Rebased on 1.21.3

@kennytv
Copy link
Member

kennytv commented Nov 24, 2024

Something else that was brought up internally is the whole "display name" naming being outdated/confusing in general now. effectiveName is good for the new method on ItemStack, but we'd like this to go alongside improving ItemStack#displayName docs and replacing ItemMeta displayName methods with customName (since that's what it returns, but other data components can change or be the effective display name). Replace in this case means marking the old as obsolete and linking to the new

@masmc05
Copy link
Contributor Author

masmc05 commented Nov 24, 2024

Can I move the ItemStack#displayName from the adventure patch to my patch? It ends up a bit uglier without that

@kennytv
Copy link
Member

kennytv commented Nov 24, 2024

Yeah sure

@masmc05
Copy link
Contributor Author

masmc05 commented Nov 24, 2024

Applied the deprecation and made the replacement for ItemStack#displayName

@jpenilla
Copy link
Member

jpenilla commented Nov 26, 2024

I'm confused with what's going on in this diff, what kenny asked for was:

  • Improve docs for ItemStack#displayName()
  • Deprecate/obsolete ItemMeta#displayName(Component) and ItemMeta#displayName() in favor of new customName methods (also update the deprecation notice on the legacy methods to point to customName)
  • Add ItemStack#effectiveName() method

@masmc05
Copy link
Contributor Author

masmc05 commented Nov 26, 2024

ItemMeta#displayName(Component) and ItemMeta#displayName()

Oh, missed that he mentioned ItemMeta and didn't even think of it as this pr didn't touch item meta. Maybe I could just rollback the diff and this pr would just add the new api, and dealing with displayName methods in a separate pr? As personally I disagree with deprecation of a highly used method (I'm sure it's used in almost every plugin) in favor of making an unused by almost every plugin method reflect the mojang naming

@Malfrador
Copy link
Member

Malfrador commented Nov 26, 2024

Not a fan of deprecating the ItemMeta method either.
Yes its objectively wrong since two versions, but that thing has been called "display name" in vanilla too for ten years now and just spamming more deprecation will make more people simply ignore them.

I would just deprecate the ItemStack one because its borderline useless. And add a little note to the JD that this works slightly differently in vanilla nowadays.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Awaiting final testing
Development

Successfully merging this pull request may close these issues.

7 participants