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

Fix inventory renaming error. #6159

Closed
wants to merge 15 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 58 additions & 16 deletions src/main/java/ch/njol/skript/expressions/ExprNamed.java
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
*/
package ch.njol.skript.expressions;

import ch.njol.skript.lang.Literal;
import ch.njol.skript.util.Utils;
import org.bukkit.Bukkit;
import org.bukkit.event.Event;
import org.bukkit.event.inventory.InventoryType;
Expand Down Expand Up @@ -59,27 +61,51 @@ public class ExprNamed extends PropertyExpression<Object, Object> {
@SuppressWarnings("null")
private Expression<String> name;

@SuppressWarnings({"unchecked", "null"})
@Override
public boolean init(final Expression<?>[] exprs, final int matchedPattern, final Kleenean isDelayed, final ParseResult parseResult) {
setExpr(exprs[0]);
@SuppressWarnings("unchecked")
public boolean init(Expression<?>[] exprs, int matchedPattern, Kleenean isDelayed, ParseResult parseResult) {
if (exprs[0].getReturnType().equals(ItemStack.class)) {
setExpr(exprs[0].getConvertedExpression(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.

Is this needed? Can you add a comment explaining why it is if so

Copy link
Member Author

Choose a reason for hiding this comment

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

I have no idea what it is, what it does or why it's there, I didn't write it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Existing code had casting to ensure the type was not an ItemStack. For some reason in the past an ItemType could have been an ItemStack. This enforces that and removes the need for casting in runtime. This was discussed on the skriptlang Discord a long time ago to do it this way.

} else {
setExpr(exprs[0]);
}
name = (Expression<String>) exprs[1];
if (getExpr() instanceof Literal) {
Literal<?> literal = (Literal<?>) getExpr();
Object object = literal.getSingle();
if (object instanceof InventoryType && !isCreatable((InventoryType) object)) {
Skript.error(Utils.A(literal.toString()) + " cannot be created.");
return false;
}
}
return true;
}

@Override
protected Object[] get(final Event e, final Object[] source) {
String name = this.name.getSingle(e);
protected Object[] get(final Event event, final Object[] source) {
String name = this.name.getSingle(event);
if (name == null)
return get(source, obj -> obj); // No name provided, do nothing
return get(source, object -> {
if (object instanceof InventoryType) {
InventoryType type = (InventoryType) object;
if (!isCreatable(type))
return null;
return Bukkit.createInventory(null, type);
}
return object; // Return the same ItemType they passed without applying a name.
Copy link
Member

Choose a reason for hiding this comment

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

The other get below handles converting ItemStacks to ItemTypes. Do we need to handle that here too (to avoid an ArrayStoreException)? I'm not actually sure that ItemStacks could even be passed into this method though.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know, TheLimeGlass didn't do this in the original PR but there was another change made to this class since that added that converter outside the getter, so I merged the changes together but since I don't know what/why that's there I can't say whether it's needed.

});
return get(source, new Getter<Object, Object>() {
@Override
@Nullable
public Object get(Object obj) {
if (obj instanceof InventoryType)
return Bukkit.createInventory(null, (InventoryType) obj, name);
if (obj instanceof ItemStack) {
ItemStack stack = (ItemStack) obj;
public Object get(Object object) {
if (object instanceof InventoryType) {
InventoryType type = (InventoryType) object;
if (!isCreatable(type))
return null;
return Bukkit.createInventory(null, type, name);
}
if (object instanceof ItemStack) {
ItemStack stack = (ItemStack) object;
stack = stack.clone();
ItemMeta meta = stack.getItemMeta();
if (meta != null) {
Expand All @@ -88,7 +114,7 @@ public Object get(Object obj) {
}
return new ItemType(stack);
}
ItemType item = (ItemType) obj;
ItemType item = (ItemType) object;
item = item.clone();
ItemMeta meta = item.getItemMeta();
meta.setDisplayName(name);
Expand All @@ -99,13 +125,29 @@ public Object get(Object obj) {
}

@Override
public Class<? extends Object> getReturnType() {
return getExpr().getReturnType() == InventoryType.class ? Inventory.class : ItemType.class;
public Class<?> getReturnType() {
Class<?> returnType = getExpr().getReturnType();
if (returnType == InventoryType.class)
return Inventory.class;
if (returnType == ItemStack.class || returnType == ItemType.class)
return ItemType.class;
return Object.class;
}

@Override
public String toString(final @Nullable Event e, final boolean debug) {
return getExpr().toString(e, debug) + " named " + name;
public String toString(@Nullable Event event, boolean debug) {
return getExpr().toString(event, debug) + " named " + name;
}

private boolean isCreatable(InventoryType type) {
// Spigot forgot to label some InventoryTypes as non-creatable in some versions < 1.19.4
// So this throws NullPointerException as well as an IllegalArgumentException.
// See https://hub.spigotmc.org/jira/browse/SPIGOT-7301
if (Skript.isRunningMinecraft(1, 14) && type == InventoryType.COMPOSTER)
return false;
if (Skript.isRunningMinecraft(1, 20) && type == InventoryType.CHISELED_BOOKSHELF)
return false;
Comment on lines +146 to +149
Copy link
Member

Choose a reason for hiding this comment

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

caching still maybe a good idea ;)

return type.isCreatable();
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
test "inventory type ExprNamed name null" when running minecraft "1.14.4": # 1.13 makes the inventory name return "Shulker Box"
set {_inventory} to shulker box inventory named name of tool of random player out of all players
assert {_inventory} is set with "Failed to set new inventory to local variable"
assert name of {_inventory} is not set with "The name of the inventory was set"
clear {_inventory}
set {_inventory} to shulker box inventory named {_null}
assert name of {_inventory} is not set with "The name of the inventory was set (2)"
9 changes: 9 additions & 0 deletions src/test/skript/tests/syntaxes/expressions/ExprNamed.sk
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
test "named":
set {_i} to an iron sword
assert name of {_i} is not set with "default item name failed: %name of {_i}%"
set {_i} to {_i} named "blob"
assert name of {_i} is "blob" with "item name failed: %name of {_i}%"
set {_inventory} to a hopper inventory
assert name of {_inventory} is not set with "default inventory name failed: %name of {_inventory}%"
Moderocky marked this conversation as resolved.
Show resolved Hide resolved
set {_inventory} to a hopper inventory named "test"
assert {_inventory} is set with "inventory set failed" # we can't check inventory names any more