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.

Sign up for GitHub

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

Jump to bottom

Fix an issue with creating a non creatable inventory from ExprNamed and fix array store exception #5943

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

Fix an issue with creating a non creatable inventory from ExprNamed and fix array store exception #5943

wants to merge 4 commits into from

Conversation

TheLimeGlass
Copy link
Collaborator

Description

Fix an issue with creating a non creatable inventory from ExprNamed and fix array store exception.

  • See Composter Inventory #5495 and solution pull request for it at Fix and enhance EffOpenInventory #5531 about the issue with creating non creatable inventories. This also affects ExprNamed which is what this pull request fixes, where as the referenced pull request is for EffOpenInventory.
  • ExprNamed takes in an inventory type or an item type, and the array that PropertyExpression builds is of the type the implementing property expression class returns. So ExprNamed was returning Inventory.class from it's getReturnType method making the required array for the converters to be an Inventory[]. If the String of the ExprNamed was null, the get method would just return the getExpr(), so it was return an InventoryType, and you cannot store that in an Inventory[] array, so an exception was being thrown.
  • The solution for the for mentioned was to make it return a new Bukkit#createInventory without a name.
  • Changed the ExpressionType to a COMBINED as it combines multiple expressions to make one, but utilizes the PropertyExpression class.
  • Cleaned up the examples and the class to follow modern code standards.
  • Added event and debug boolean reference to the name expression for the toString method of ExprNamed.

Target Minecraft Versions: any
Requirements: none
Related Issues: #5495 and #5923

@TheLimeGlass TheLimeGlass added the bug An issue that needs to be fixed. Alternatively, a PR fixing an issue. label Aug 27, 2023
Moderocky
Moderocky approved these changes Sep 8, 2023
View reviewed changes
Copy link
Member

@Moderocky Moderocky left a comment

Choose a reason for hiding this comment

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

Looks ok, I'm not a fan of the try/catch but it could future-proof it.

src/main/java/ch/njol/skript/expressions/ExprNamed.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/expressions/ExprNamed.java Show resolved Hide resolved
@Moderocky Moderocky changed the base branch from master to dev/feature September 17, 2023 07:45
@sovdeeth sovdeeth added patch-ready A PR/issue that has been approved and is ready to be merged/closed for the next patch version. feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version. labels Oct 1, 2023
@TheLimeGlass TheLimeGlass removed the feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version. label Oct 5, 2023
@Moderocky Moderocky mentioned this pull request Oct 31, 2023
Closed
@TheLimeGlass TheLimeGlass force-pushed the fix/exprnamed-invalid-array-storage branch from 949b345 to c51dc41 Compare November 1, 2023 21:16
@TheLimeGlass TheLimeGlass changed the base branch from dev/feature to dev/patch November 1, 2023 21:16
@TheLimeGlass TheLimeGlass changed the base branch from dev/patch to dev/feature December 15, 2023 02:54
@TheLimeGlass TheLimeGlass changed the base branch from dev/feature to dev/patch December 15, 2023 02:54
@TheLimeGlass TheLimeGlass force-pushed the fix/exprnamed-invalid-array-storage branch from 83ca24c to 1b7b3a5 Compare January 17, 2024 19:13
@TheLimeGlass TheLimeGlass changed the base branch from dev/patch to dev/feature January 17, 2024 19:13
@TheLimeGlass TheLimeGlass changed the base branch from dev/feature to dev/patch February 1, 2024 20:28
sovdeeth
sovdeeth approved these changes Feb 1, 2024
View reviewed changes
Copy link
Member

@sovdeeth sovdeeth left a comment

Choose a reason for hiding this comment

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

did some quick manual tests, seems good.

APickledWalrus
APickledWalrus requested changes Feb 1, 2024
View reviewed changes
src/main/java/ch/njol/skript/expressions/ExprNamed.java
"set tool of player to the player's tool named \"<gold>Wand\"",
"set the name of the player's tool to \"<gold>Wand\"",
"open hopper inventory named \"Magic Hopper\" to player"
})
@Since("2.0, 2.2-dev34 (inventories)")
public class ExprNamed extends PropertyExpression<Object, Object> {
Copy link
Member

Choose a reason for hiding this comment

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

IMO some tests would be good. You'd be fine to incorporate the tests from #6159

src/main/java/ch/njol/skript/expressions/ExprNamed.java
// Spigot forgot to label some InventoryType's as non creatable in some versions < 1.19.4
// So this throws NullPointerException aswell ontop of the IllegalArgumentException.
// See https://hub.spigotmc.org/jira/browse/SPIGOT-7301
Skript.error("You can't create a '" + literal.toString() + "' inventory. It's not creatable!");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Skript.error("You can't create a '" + literal.toString() + "' inventory. It's not creatable!");
Skript.error(Utils.a(literal.toString) + "inventory cannot be created.");

Skript/src/main/java/ch/njol/skript/util/Utils.java

Line 333 in b9f89c1

public static String a(final String s) {
will need imported

src/main/java/ch/njol/skript/expressions/ExprNamed.java
return get(source, obj -> obj); // No name provided, do nothing
return get(source, object -> {
if (object instanceof InventoryType) {
try {
Copy link
Member

Choose a reason for hiding this comment

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

I think we should at least attempt to check if the inventory type is creatable here

src/main/java/ch/njol/skript/expressions/ExprNamed.java
@@ -97,15 +121,15 @@ public Object get(Object obj) {
}
});
}

@Override
public Class<? extends Object> getReturnType() {
return getExpr().getReturnType() == InventoryType.class ? Inventory.class : ItemType.class;
Copy link
Member

Choose a reason for hiding this comment

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

I don't believe this is safe for variables, or any other expression that would have Object.class as the return type. We might have an InventoryType type in a variable but would be returning ItemType as the return type here. Likely another issue we can fix while we're here :)

src/main/java/ch/njol/skript/expressions/ExprNamed.java
stack.setItemMeta(meta);
public Object get(Object object) {
if (object instanceof InventoryType) {
try {
Copy link
Member

Choose a reason for hiding this comment

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

Same here, I think we'd be best off at least checking if it is creatable first. Since we have to do this in two places, and have the try (maybe could be replaced with checking incorrectly marked types? not sure), it might be worth moving to a private method

src/main/java/ch/njol/skript/expressions/ExprNamed.java
Comment on lines +68 to +73
// See issue #1822 as to why this convert is required.
if (exprs[0].getReturnType().equals(ItemStack.class)) {
setExpr(exprs[0].getConvertedExpression(ItemType.class));
} else {
setExpr(exprs[0]);
}
Copy link
Member

Choose a reason for hiding this comment

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

I can't imagine an ItemStack would be passed here, and if one were to be, that is a separate issue to be fixed elsewhere. I don't think we should check this.

@sovdeeth sovdeeth removed the patch-ready A PR/issue that has been approved and is ready to be merged/closed for the next patch version. label May 1, 2024
Base automatically changed from dev/patch to master June 1, 2024 19:38
@sovdeeth
Copy link
Member

closing due to inactivity

@sovdeeth sovdeeth closed this Sep 15, 2024
@TheLimeGlass
Copy link
Collaborator Author

February requested changes is a crazy inactivity time

@TheLimeGlass TheLimeGlass reopened this Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@APickledWalrus APickledWalrus APickledWalrus requested changes

@sovdeeth sovdeeth sovdeeth approved these changes

@Moderocky Moderocky Moderocky approved these changes

@moraedu moraedu Awaiting requested review from moraedu

Assignees
No one assigned
Labels
bug An issue that needs to be fixed. Alternatively, a PR fixing an issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@TheLimeGlass @sovdeeth @Moderocky @APickledWalrus @moraedu