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 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
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
98 changes: 61 additions & 37 deletions src/main/java/ch/njol/skript/expressions/ExprNamed.java
Original file line number Diff line number Diff line change
Expand Up @@ -35,60 +35,84 @@
import ch.njol.skript.expressions.base.PropertyExpression;
import ch.njol.skript.lang.Expression;
import ch.njol.skript.lang.ExpressionType;
import ch.njol.skript.lang.Literal;
import ch.njol.skript.lang.SkriptParser.ParseResult;
import ch.njol.skript.util.Getter;
import ch.njol.util.Kleenean;

/**
* @author Peter Güttinger
*/
@Name("Named Item/Inventory")
@Description("Directly names an item/inventory, useful for defining a named item/inventory in a script. " +
"If you want to (re)name existing items/inventories you can either use this expression or use <code>set <a href='#ExprName'>name of &lt;item/inventory&gt;</a> to &lt;text&gt;</code>.")
@Examples({"give a diamond sword of sharpness 100 named \"&lt;gold&gt;Excalibur\" to the player",
"set tool of player to the player's tool named \"&lt;gold&gt;Wand\"",
"set the name of the player's tool to \"&lt;gold&gt;Wand\"",
"open hopper inventory named \"Magic Hopper\" to player"})
@Description({
"Directly names an item/inventory, useful for defining a named item/inventory in a script.",
"If you want to (re)name existing items/inventories you can either use this expression " +
"or use <code>set <a href='#ExprName'>name of &lt;item/inventory&gt;</a> to &lt;text&gt;</code>."
})
@Examples({
"give a diamond sword of sharpness 100 named \"&lt;gold&gt;Excalibur\" to the player",
"set tool of player to the player's tool named \"&lt;gold&gt;Wand\"",
"set the name of the player's tool to \"&lt;gold&gt;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


static {
Skript.registerExpression(ExprNamed.class, Object.class, ExpressionType.PROPERTY,
Skript.registerExpression(ExprNamed.class, Object.class, ExpressionType.COMBINED,
"%itemtype/inventorytype% (named|with name[s]) %string%");
}

@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) {
// 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]);
}
Comment on lines +68 to +73
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.

name = (Expression<String>) exprs[1];
if (getExpr() instanceof Literal) {
Literal<?> literal = (Literal<?>) getExpr();
Object object = literal.getSingle();
if (object instanceof InventoryType && !((InventoryType) object).isCreatable()) {
// 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!");
TheLimeGlass marked this conversation as resolved.
Show resolved Hide resolved
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.");

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

return false;
}
}
return true;
}

@Override
protected Object[] get(final Event e, final Object[] source) {
String name = this.name.getSingle(e);
protected Object[] get(Event event, 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) {
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

return Bukkit.createInventory(null, (InventoryType) object);
} catch (NullPointerException | IllegalArgumentException e) {
return null;
}
}
return object; // Return the same ItemType they passed without applying a name.
});
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;
stack = stack.clone();
ItemMeta meta = stack.getItemMeta();
if (meta != null) {
meta.setDisplayName(name);
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

return Bukkit.createInventory(null, (InventoryType) object, name);
} catch (NullPointerException | IllegalArgumentException e) {
return null;
}
return new ItemType(stack);
}
ItemType item = (ItemType) obj;
assert object instanceof ItemType;
ItemType item = (ItemType) object;
item = item.clone();
ItemMeta meta = item.getItemMeta();
meta.setDisplayName(name);
Expand All @@ -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 :)

}

@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.toString(event, debug);
}

}
Loading