Skip to content

Add a method on InventoryView to get the MenuType#12193

Merged
Owen1212055 merged 3 commits into
PaperMC:mainfrom
GliczDev:inventory-view-menu-type
May 1, 2025
Merged

Add a method on InventoryView to get the MenuType#12193
Owen1212055 merged 3 commits into
PaperMC:mainfrom
GliczDev:inventory-view-menu-type

Conversation

@GliczDev
Copy link
Copy Markdown
Contributor

It is possible to get the InventoryType, but not MenuType.
Since there is a new (better) way to create views for players using MenuType, it would be nice to also be able to get it back from InventoryView after creating.

@GliczDev GliczDev requested a review from a team as a code owner February 26, 2025 18:46
@github-project-automation github-project-automation Bot moved this to Awaiting review in Paper PR Queue Feb 26, 2025
Copy link
Copy Markdown
Contributor

@Y2Kwastaken Y2Kwastaken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to ensure the API functions properly, some adjustments are needed. I’ve tested it, and currently, the API throws an error for any menu that doesn’t have a MenuType. This is an issue in scenarios like the player’s inventory or with animals, where a MenuType is not applicable. In these cases, it results in an UnsupportedOperationException. If you’re aiming for this to be merged, I’d recommend modifying the behavior to return null instead of throwing the exception. Otherwise, everything else seems fine. I'm not in love with the CraftContainer impl, but I think its pretty much unavoidable at this point sadly.

Comment thread paper-api/src/main/java/org/bukkit/inventory/InventoryView.java Outdated
@github-project-automation github-project-automation Bot moved this from Awaiting review to Changes required in Paper PR Queue Feb 26, 2025
@GliczDev
Copy link
Copy Markdown
Contributor Author

GliczDev commented Feb 26, 2025

In these cases, it results in an UnsupportedOperationException.

@Y2Kwastaken does it also throw in CraftContainer implementation? It seems that CraftContainer.getNotchInventoryType never returns null, so there should be no issues, but I want to verify it.

@Y2Kwastaken
Copy link
Copy Markdown
Contributor

Y2Kwastaken commented Feb 26, 2025

In these cases, it results in an UnsupportedOperationException.

@Y2Kwastaken does it also throw in CraftContainer implementation? It seems that CraftContainer.getNotchInventoryType never returns null, so there should be no issues, but I want to verify it.

The naive conversion for Inventory's is fine and won't throw that exception to see why its thrown see AbstractContainerMenu#getType. This goes back to what I explained earlier where not every Menu has a MenuType sadly.

Comment thread paper-api/src/main/java/org/bukkit/inventory/InventoryView.java
@GliczDev GliczDev requested a review from Y2Kwastaken March 9, 2025 14:53
Copy link
Copy Markdown
Contributor

@Y2Kwastaken Y2Kwastaken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a lot better thank you, From my testing it works as you'd expect.

@Warriorrrr Warriorrrr added type: feature Request for a new Feature. scope: api labels Mar 17, 2025
@Warriorrrr Warriorrrr moved this from Changes required to Awaiting review in Paper PR Queue Mar 17, 2025
@github-project-automation github-project-automation Bot moved this from Awaiting review to Awaiting final testing in Paper PR Queue Mar 17, 2025
@Owen1212055 Owen1212055 merged commit 835b955 into PaperMC:main May 1, 2025
@github-project-automation github-project-automation Bot moved this from Awaiting final testing to Merged in Paper PR Queue May 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

scope: api type: feature Request for a new Feature.

Projects

Status: Merged

Development

Successfully merging this pull request may close these issues.

5 participants