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

Allow For Default Titles in InventoryView Builders #12013

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

Y2Kwastaken
Copy link
Contributor

This PR addresses a previous critique of no default titles for the InventoryView builders PR. The goal of this PR is to integrate default titles and minimize the explicit declaration of translatable keys to ease on maintenance burden of this API.

I have bundled the test plugin I used along with this PR to ease of testing code if needed.

Below examplifies the change from an API perspective

// Previous Disallowed
MenuType.CHEST.builder()
 .title(null); // the title can be null now
 .build(player);
// Now Permitted
MenuType.CHEST.builder()
 .build(player);

Within this PR I took a few different approaches to accessing titles. The first two of, which are off of the BlockEntity and Block menu provider respectively. However, there are two cases where I directly declare a Translatable component within this API, once in CraftStandardInventoryViewBuilder and once in CraftDoubleChestInventoryViewBuilder.

  • CraftStandardInventoryViewBuilder: Within this class I directly referenced the "container.chest" translatable key because, first, it is not necessary to create a BlockEntity for these "standard" menus as well as the fact that the possibility that the "container.chest" translation key will change is slim to none.
  • CraftDoubleChestInventoryViewBuilder: This key was explicitly declared simply because its not easily accessible and writing around this lack of accessibility leads to less than cohesive implementation. This translation key is not as accessible because it is declared inside of ChestBlock#MENU_PROVIDER_COMBINER, while this field itself is accessible, the MenuProvider DoubleChest uses is nested with the "acceptDouble(ChestBlockEntity, ChestBlockEntity" method, which then nests its MenuProvider in the then Bukkit wrapper called "DoubleInventory", doing this for both location and non location backed containers, was in my opinion not beneficial enough to warrant the implementation.

@Y2Kwastaken Y2Kwastaken requested a review from a team as a code owner January 26, 2025 04:50
@lynxplay lynxplay added type: feature Request for a new Feature. scope: api labels Jan 26, 2025
Copy link
Contributor

@Lulu13022002 Lulu13022002 left a comment

Choose a reason for hiding this comment

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

Maybe MenuType#create could have a new method without title and a nullable title.
Also i get this error while testing the lectern: https://pastes.dev/KgmpW6mqCE

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: Changes required
Development

Successfully merging this pull request may close these issues.

3 participants