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 16 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

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.

The loom has the title of the chest set by default.

I also get some other errors while testing
for the enchantment menu: https://pastes.dev/ZgBP7TjW9Y
for the merchant menu: https://pastes.dev/whPBf9wNNc

@Y2Kwastaken
Copy link
Contributor Author

Y2Kwastaken commented Feb 2, 2025

The loom has the title of the chest set by default.

I also get some other errors while testing for the enchantment menu: pastes.dev/ZgBP7TjW9Y for the merchant menu: pastes.dev/whPBf9wNNc

Hey can you show the code you're using when you get the second exception, thanks.
I was able to replicate this eventually

@Lulu13022002
Copy link
Contributor

I get another error when i tried to open a merchant menu without villager but with a title set: https://pastes.dev/hLGHRQQSSt

And the loom is still disguised as a chest.

@lynxplay lynxplay force-pushed the feature/invview-default-titles branch from c333083 to 16aba18 Compare February 8, 2025 21:30
Copy link
Member

@Warriorrrr Warriorrrr left a comment

Choose a reason for hiding this comment

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

Two small comments but otherwise all looks good to me

@@ -140,16 +142,16 @@ public static <V extends InventoryView, B extends InventoryViewBuilder<V>> MenuT
return asType(new MenuTypeData<>(InventoryView.class, () -> new CraftBlockEntityInventoryViewBuilder<>(handle, Blocks.SHULKER_BOX, ShulkerBoxBlockEntity::new)));
Copy link
Member

Choose a reason for hiding this comment

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

If opening noises should be prevented (as is done with chest using useFakeBlockEntity), then perhaps that could be done here too with the shulker.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah The whole shulker thing is a bit more nuanced due to the need for it to be distinctly different than a chest with GENERIC_9x3. In the interest of trying to keep this PR focused on adding default titles, this will need to be addressed in a PR where I rewrite the opening logic that Bukkit does. Long story short getNotchInventoryType isn't actually smart enough to identify the difference between a shulker box and GENERIC_9x3 as they'd be created with this API. The way around this is to define and force the use of the BlockEntity for now* The chest is okay to avoid this because it is a GENERIC_9x3.

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.

4 participants